Issue #3432 - Manage COM events with a resource class

src/plugin/PluginClass.h

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 23 matching lines @@
 
 #define ICON_PLUGIN_DISABLED 0
 #define ICON_PLUGIN_ENABLED 1
 #define ICON_PLUGIN_DEACTIVATED 2
 #define ICON_MAX 3
 
 #define WM_LAUNCH_INFO                                  (WM_APP + 10)
 
 class CPluginMimeFilterClient;
 
+/**
+ * Resource class for COM events.
+ * If an instance exists, there's an active connection behind it.
+ *
+ * \tparam Sink
+ *   subclass of ATL::IDispEventImpl.
+ * \tparam Source
+ *   Type of event source. Implements IConnectionPoint
+ */
+template<class Sink, class Source>
+class AtlEventRegistration
+{
+  CComPtr<Sink> consumer;
+  CComPtr<Source> producer;
+public:
+  /**
+   * \throw std::runtime_error
+   *   If connection is not established
+   *
+   * \par Precondition
+   *   - consumer is not nullptr (not checked)
+   *   - producer is not nullptr (not checked)
+   */
+  AtlEventRegistration(Sink* consumer, Source* producer)

[sergei, 2016/02/04 13:45:50]

Just noticed, it would be better to call them as listener and source
respectively, because the domain is events.

[Eric, 2016/02/04 17:00:28]

Neither pair of words "source/sink" and "listener/source" is particularly great
English. "Consume/produce" is better. If you'd like to get into the fine points
of English connotation, I can expound.

[sergei, 2016/02/04 19:41:57]

I think we should not write the code how it would be written by Shakespeare.
There is a common terms in certain domains and we should stay with that terms to
have the code maintainable.

Terms "consumer" and "producer" better fit producer–consumer problem, which is
about transferring data from producer to consumer using different types of
shared buffers.

Terms "listener" and "source" or "sender" better fit to the events domain.
Compare the frequency of usage
https://github.com/search?l=cpp&q=event+producer+consumer&ref=searchresults&t...
https://github.com/search?l=cpp&q=event+listener+source&ref=searchresults&typ...
https://github.com/search?l=cpp&q=event+listener+sender&ref=searchresults&typ...

[Eric, 2016/02/04 20:13:25]

We've entered bikeshedding territory here. You have a personal preference that
differs from mine.

Suggestion rejected.

+    : consumer(consumer), producer(producer)
+  {
+    auto hr = consumer->DispEventAdvise(producer);
+    if (FAILED(hr))
+    {
+      DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_ADVICE, "AtlEventRegistrationImpl::<constructor> - Advise");
+      throw std::runtime_error("DispEventAdvise() failed");
+    }
+  }
+
+  ~AtlEventRegistration() // noexcept
+  {
+    try
+    {
+      // Smart pointer members ensure that this statement does not fail because of life span
+      auto hr = consumer->DispEventUnadvise(producer);
+      if (FAILED(hr))
+      {
+        DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_UNADVISE, "AtlEventRegistrationImpl::<destructor> - Unadvise");
+      }
+    }
+    catch (...)
+    {
+      // Suppress exceptions
+    }
+  }
+};
+
+/**
+ * Resource holder for COM events.
+ *
+ * Manages the life time of an 'AtlEventRegistration' instance.
+ * Allows events to be started and stopped at some time other than construction/destruction.
+ *
+ * \tparam Sink
+ *   subclass of ATL::IDispEventImpl.
+ * \tparam Source
+ *   Type of event source. Implements IConnectionPoint
+ */
+template<class Sink, class Source>
+class AtlEventRegistrationHolder
+{
+  typedef AtlEventRegistration<Sink, Source> ImplType;
+  std::unique_ptr<ImplType> impl;
+
+public:
+  bool Start(Sink* consumer, Source* producer) // noexcept
+  {
+    try
+    {
+      impl.reset(new ImplType(consumer, producer));
+    }
+    catch (...)
+    {
+      impl.reset();
+      return false;
+    }
+    return true;
+  }
+
+  void Stop() // noexcept
+  {
+    impl.reset();
+  }
+};
 
 class ATL_NO_VTABLE CPluginClass :
   public ATL::CComObjectRootEx<ATL::CComMultiThreadModel>,
   public ATL::CComCoClass<CPluginClass, &CLSID_PluginClass>,
   public ATL::IObjectWithSiteImpl<CPluginClass>,
   public IOleCommandTarget,
   public ATL::IDispEventImpl<1, CPluginClass, &DIID_DWebBrowserEvents2, &LIBID_SHDocVw, 1, 1>
 {
 
   friend class CPluginTab;
@@ skipping 12 matching lines @@
     COM_INTERFACE_ENTRY(IObjectWithSite)
     COM_INTERFACE_ENTRY(IOleCommandTarget)
   END_COM_MAP()
 
   BEGIN_SINK_MAP(CPluginClass)
     SINK_ENTRY_EX(1, DIID_DWebBrowserEvents2, DISPID_BEFORENAVIGATE2, OnBeforeNavigate2)
     SINK_ENTRY_EX(1, DIID_DWebBrowserEvents2, DISPID_DOWNLOADCOMPLETE, OnDownloadComplete)
     SINK_ENTRY_EX(1, DIID_DWebBrowserEvents2, DISPID_DOCUMENTCOMPLETE, OnDocumentComplete)
     SINK_ENTRY_EX(1, DIID_DWebBrowserEvents2, DISPID_WINDOWSTATECHANGED, OnWindowStateChanged)
     SINK_ENTRY_EX(1, DIID_DWebBrowserEvents2, DISPID_COMMANDSTATECHANGE, OnCommandStateChange)
-    SINK_ENTRY_EX(1, DIID_DWebBrowserEvents2, DISPID_ONQUIT, OnOnQuit)
   END_SINK_MAP()
 
   CPluginClass();
   ~CPluginClass();
 
   // IObjectWithSite
   STDMETHOD(SetSite)(IUnknown *pUnkSite);
 
   // IOleCommandTarget
   STDMETHOD(QueryStatus)(const GUID* pguidCmdGroup, ULONG cCmds, OLECMD prgCmds[], OLECMDTEXT* pCmdText);
@@ skipping 38 matching lines @@
                                            VARIANT* URL /**< [in] */,
                                            VARIANT* Flags /**< [in] */,
                                            VARIANT* TargetFrameName /**< [in] */,
                                            VARIANT* PostData /**< [in] */,
                                            VARIANT* Headers /**< [in] */,
                                            VARIANT_BOOL* Cancel /* [in, out] */);
   void STDMETHODCALLTYPE OnDownloadComplete();
   void STDMETHODCALLTYPE OnDocumentComplete(IDispatch* frameBrowserDisp, VARIANT* /*urlOrPidl*/);
   void STDMETHODCALLTYPE OnWindowStateChanged(unsigned long flags, unsigned long validFlagsMask);
   void STDMETHODCALLTYPE OnCommandStateChange(long command, VARIANT_BOOL enable);
-  void STDMETHODCALLTYPE OnOnQuit();
-  void Unadvise();
-
+  
   void ShowStatusBar();
   bool IsStatusBarEnabled();
 
   /**
    * A browser interface pointer to our site object
    *
    * It's values are set and reset solely in SetSite().
    */
   CComPtr<IWebBrowser2> m_webBrowser2;
+  /**
+   * Event manager for events coming from our site object.
+   */
+  AtlEventRegistrationHolder<CPluginClass, IWebBrowser2> browserEvents;
+  bool detachedInitializationFailed;
+
   HWND m_hBrowserWnd;
   HWND m_hTabWnd;
   HWND m_hStatusBarWnd;
   HWND m_hPaneWnd;
   
   WNDPROC m_pWndProcStatus;
   int m_nPaneWidth;
   HANDLE m_hTheme;
 
   CriticalSection m_csStatusBar;
 
   NotificationMessage notificationMessage;
 
-  bool m_isAdvised;
   bool m_isInitializedOk;
 
   // Atom pane class
   static ATOM s_atomPaneClass;
 
   static ATOM GetAtomPaneClass();
 
   // Icons
   static HICON s_hIcons[ICON_MAX];
   static DWORD s_hIconTypes[ICON_MAX];
@@ skipping 12 matching lines @@
 
   // Async browser
   static CComQIPtr<IWebBrowser2> s_asyncWebBrowser2;
   static CComQIPtr<IWebBrowser2> GetAsyncBrowser();
 };
 
 OBJECT_ENTRY_AUTO(__uuidof(PluginClass), CPluginClass)
 
 
 #endif // _PLUGIN_CLASS_H_