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
+ * Copyright (C) 2006-2016 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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
@@ skipping 30 matching lines @@
 /**
  * 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

[sergei, 2016/01/06 08:41:58]

I doubt that we need two classes here, AtlEventRegistrationHolder and
AtlEventRegistration, only one would be completely enough. The code is correct
but it's overengineering.

[Eric, 2016/01/07 16:24:48]

It's not overengineering. You actually want two classes here. The underlying
class is the RAII class. The other one with Start() and Stop() also needs to
implicitly call Stop() in its destructor. If you combined them, you'd end up
with the equivalent of the RAII destructor code as a private method called from
two places. It could be made to work, but it's error-prone.

The present code is obviously exception-safe by design. That's why you split out
a resource class into its own class and, if it needs some control sequence other
than construction/destruction, why you write such a control class separately.

This is an example of the Pimpl idiom, here not done so much to hide the
implementation but more to get reliable destructor semantics.

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

Answered in the message.
It's not a Pimpl idiom here and we don't need it here.

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

Perhaps you are unclear about PImpl.

PImpl = Pointer to Implementation
PImpl class = AtlEventRegistrationHolder
pointer = AtlEventRegistrationHolder::impl (a smart pointer)
implementation = AtlEventRegistration

Most PImpl classes use facade methods to forward methods to the underlying
implementation. This PImpl class differs in that it has no methods to forward;
either the event connection exists or does not. It's also ordinary for the
lifetime of the PImpl class to coincide with that of the implementation. This
PImpl class differs because it explicitly manages that lifetime.

The most conventional reason to use PImpl is to get efficient pass-by-value.
Just because this class doesn't act that way doesn't mean it's not PImpl.

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

I don't want to debate here, but it's not PIMPL, I would consider it as merely a
composition. Patterns and idioms are not merely some, let's say, UML diagram.
They also include in particular the intention to use it and the way to organize
files and code.

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

I don't want to debate here either, so I'll forgo a response.

 {
   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)
     : 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");

[sergei, 2016/01/06 08:41:57]

I would prefer to make private constructor and have a static creator method
(e.g.
`static std::unique_ptr<AtlEventRegistration<Sink, Source>> Create(Sink*
consumer, Source* producer);`) instead of AtlEventRegistrationHolder and such
usage of exceptions.

In addition we can have a public typedef for
`std::unique_ptr<AtlEventRegistration<Sink, Source>>`.

[Eric, 2016/01/07 16:24:48]

I consider this suggestion an inferior design. See my previous comment for why.

Also see "Fear of adding classes".

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

I consider the current impl as overengineering and I don't like that exceptions
are used for the logic.

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

Yes, I know. You said that already.
If you don't like exceptions, then you can't like RAII. Exceptions are at the
core of how RAII works. If initialization fails, you throw an exception. If you
don't, you're falling outside of the RAII pattern.

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

I have not said whether I like exceptions or not, I said that I don't like when
they are used to build some logic. It does not matter whether somebody likes
exceptions or not, I'm trying to communicate that it's overengineering and we
are trying to avoid it.

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

You'll have to tell me what "logic" you're talking about then. The constructor
of this class throws when it fails to acquire a resource. If you have a problem
with that, you have a problem with RAII generally.

     }
   }
 
   ~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))
@@ skipping 19 matching lines @@
  * \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:
-  AtlEventRegistrationHolder()
-    : impl(nullptr)

[sergei, 2016/01/06 08:41:58]

This line as well as the empty constructor and destructor are not needed.

[Eric, 2016/01/07 16:24:48]

Done.

-  {}
-
-  ~AtlEventRegistrationHolder() // = default;
-  {}
-
   bool Start(Sink* consumer, Source* producer) // noexcept
   {
     try
     {
-      impl = new ImplType(consumer, producer);
+      impl.reset(new ImplType(consumer, producer));
     }
     catch (...)
     {
-      impl = nullptr;

[sergei, 2016/01/06 08:41:58]

This line is not needed.

[Eric, 2016/01/07 16:24:48]

It is. Start() has the semantics of stopping whatever was previously there.

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

Actually expecting that Start can be called several times looks redundant, just
one class with factory method as proposed would be better.

+      impl.reset();
       return false;
     }
     return true;
   }
 
-  Stop() // noexcept
-  {
-    impl = nullptr;

[sergei, 2016/01/06 08:41:58]

it's correct, but I find it better to use impl.reset();

[Eric, 2016/01/07 16:24:48]

Done.

+  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 24 matching lines @@
   ~CPluginClass();
 
   // IObjectWithSite
   STDMETHOD(SetSite)(IUnknown *pUnkSite);
 
   // IOleCommandTarget
   STDMETHOD(QueryStatus)(const GUID* pguidCmdGroup, ULONG cCmds, OLECMD prgCmds[], OLECMDTEXT* pCmdText);
   STDMETHOD(Exec)(const GUID*, DWORD nCmdID, DWORD, VARIANTARG*, VARIANTARG* pvaOut);
 
 
-  static CPluginTab* GetTab(DWORD dwThreadId);
+  static CPluginTab* GetTabForCurrentThread();
   CPluginTab* GetTab();
 
   void UpdateStatusBar();
 
 private:
 
   bool SetMenuBar(HMENU hMenu, const std::wstring& url);
   HMENU CreatePluginMenu(const std::wstring& url);
 
   void DisplayPluginMenu(HMENU hMenu, int nToolbarCmdID, POINT pt, UINT nMenuFlags);
@@ skipping 35 matching lines @@
   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.

[sergei, 2016/01/06 08:41:58]

Why do we need this comment?

[Eric, 2016/01/07 16:24:48]

Because we have multiple sources of events.

We have UI events, which should be and might eventually be split out. But we're
also anticipating having this class handle 'HTMLDocumentEvents2Listener' events,
which would use a second instance of this new class.

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

I mean, it's clear from line `AtlEventRegistrationHolder<CPluginClass,
IWebBrowser2> browserEvents;`, why do we need to comment it? The comment
explains neither why we do it nor any unevident trick.

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

It's a Doxygen comment.

    */
   AtlEventRegistrationHolder<CPluginClass, IWebBrowser2> browserEvents;
   bool detachedInitializationFailed;

[sergei, 2016/01/06 08:41:58]

The member naming is not consistent with other members because "m_" prefix is
missed.

[sergei, 2016/01/06 08:41:58]

Since it's accessed from different threads, there should be a proper access
methods, e.g. it can be std::atomic<bool> or protected by some synchronization
primitive if we actually need this member.

[Eric, 2016/01/07 16:24:48]

You're raising a valid issue, certainly, but I've already solved it elsewhere.
See https://codereview.adblockplus.org/29332660/, which contains exactly such
primitives, and also the review dependent upon it that uses those primitives. I
wrote that code first, and this member variable is essentially a placeholder for
"EnsureInitialized()" from a new detached initialization facility.

I'd prefer to get those two reviews done first, frankly, but I'm not holding out
any particular expectation for prompt and speedy reviewing. (Much less
converting the rest of the detached initialization.) The present change set
could be committed separately with the caveat that we need to kill off the
legacy detached initialization thread before any public release.

[Eric, 2016/01/07 16:24:48]

Right. It's not there because "m_" prefixes are against our style guidelines,
and this is new code.

 
   HWND m_hBrowserWnd;
   HWND m_hTabWnd;
   HWND m_hStatusBarWnd;
   HWND m_hPaneWnd;
   
   WNDPROC m_pWndProcStatus;
   int m_nPaneWidth;
   HANDLE m_hTheme;
 
@@ skipping 13 matching lines @@
   static DWORD s_hIconTypes[ICON_MAX];
 
   static HICON GetIcon(int type);
 
   // Main thread
   static HANDLE s_hMainThread;
   static bool s_isMainThreadDone;
 
   static HINSTANCE s_hUxtheme;
   static std::set<CPluginClass*> s_instances;
-  static std::map<DWORD,CPluginClass*> s_threadInstances;
   static CComAutoCriticalSection s_criticalSectionLocal;
   static CComAutoCriticalSection s_criticalSectionWindow;
 
   // Async browser
   static CComQIPtr<IWebBrowser2> s_asyncWebBrowser2;
   static CComQIPtr<IWebBrowser2> GetAsyncBrowser();
 };
 
 OBJECT_ENTRY_AUTO(__uuidof(PluginClass), CPluginClass)
 
-
 #endif // _PLUGIN_CLASS_H_