Issue #3432 - Manage COM events with a resource class

src/plugin/PluginClass.h

Index: src/plugin/PluginClass.h
===================================================================
--- a/src/plugin/PluginClass.h
+++ b/src/plugin/PluginClass.h
@@ -41,6 +41,95 @@
 
 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>,
@@ -73,7 +162,6 @@
     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();
@@ -132,9 +220,7 @@
   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();
 
@@ -144,6 +230,12 @@
    * 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;
@@ -157,7 +249,6 @@
 
   NotificationMessage notificationMessage;
 
-  bool m_isAdvised;
   bool m_isInitializedOk;
 
   // Atom pane class