Issue #3383 - Rewrite and simplify browser-site handling in CPluginClass

src/plugin/PluginClass.h

Index: src/plugin/PluginClass.h
===================================================================
--- a/src/plugin/PluginClass.h
+++ b/src/plugin/PluginClass.h
@@ -79,15 +79,10 @@
   CPluginClass();
   ~CPluginClass();
 
-  HRESULT FinalConstruct();
-  void FinalRelease();
-
   // 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);
 
@@ -96,7 +91,6 @@
   CPluginTab* GetTab();
 
   void UpdateStatusBar();
-  static DWORD WINAPI MainThreadProc(LPVOID pParam);
 
 private:
 
@@ -108,9 +102,7 @@
 
 public:
   HWND GetBrowserHWND() const;
-  HWND GetTabHWND() const;
-  CComQIPtr<IWebBrowser2> GetBrowser() const;
-
+  bool IsRootBrowser(IWebBrowser2*);
 
   static CPluginMimeFilterClient* s_mimeFilter;
 
@@ -146,9 +138,15 @@
   void ShowStatusBar();
   bool IsStatusBarEnabled();
 
-public:
-  CComQIPtr<IWebBrowser2> m_webBrowser2;
-private:
+  /**
+   * Our site object as a browser.
+   *
+   * It's values are set and reset solely in SetSite().

[sergei, 2015/11/30 15:52:09]

This part of the comment is not necessary, it's clear from the aim of
ATL::CComPtr.

[Eric, 2015/11/30 16:31:51]

I disagree. Nothing about this variable is either clear or obvious.

[Oleksandr, 2015/12/03 11:51:58]

I assume we're talking about the part of the comment below, not above. I agree
it looks redundant - CComPtr usage is always a defensive programming. But this
being just a comment I don't mind leaving it here.

[Eric, 2015/12/03 14:24:54]

It's no more defensive, at least ordinarily, than using 'unique_ptr' or
'shared_ptr' is.

In the present case it's compensating for a poor interface design in SetSite(),
which takes an 'IUnknown*' and contains no further type information, even though
it's always present in the caller.

I've edited down the comment.

+   * The declaration as CComPtr is defensive programming;
+   *   it ensures that the site keeps this interface alive for us in the
+   *   unlikely case that the site is different from a browser.
+   */
+  CComPtr<IWebBrowser2> m_webBrowser2;
   HWND m_hBrowserWnd;
   HWND m_hTabWnd;
   HWND m_hStatusBarWnd;
@@ -180,14 +178,10 @@
   static HANDLE s_hMainThread;
   static bool s_isMainThreadDone;
 
-  static HANDLE GetMainThreadHandle();
-  static bool IsMainThreadDone(HANDLE mainThread);
-
   static HINSTANCE s_hUxtheme;
   static std::set<CPluginClass*> s_instances;
   static std::map<DWORD,CPluginClass*> s_threadInstances;
   static CComAutoCriticalSection s_criticalSectionLocal;
-  static CComAutoCriticalSection s_criticalSectionBrowser;
   static CComAutoCriticalSection s_criticalSectionWindow;
 
   // Async browser