Issue 1672 - Subscribe and listen COM events using ATL::IDispEventImpl and BEGIN_SINK_MAP

src/plugin/PluginClass.cpp

Index: src/plugin/PluginClass.cpp
diff --git a/src/plugin/PluginClass.cpp b/src/plugin/PluginClass.cpp
index c97bf962172d2b9bf41151c3fc36d800d496982a..8d70330084d68fc77c6d61ae36d86954834f18aa 100644
--- a/src/plugin/PluginClass.cpp
+++ b/src/plugin/PluginClass.cpp
@@ -76,7 +76,6 @@ CPluginClass::CPluginClass()
   //	_CrtDumpMemoryLeaks();
 
   m_isAdviced = false;
-  m_nConnectionID = 0;
   m_hTabWnd = NULL;
   m_hStatusBarWnd = NULL;
   m_hPaneWnd = NULL;
@@ -114,28 +113,6 @@ void CPluginClass::FinalRelease()
   s_criticalSectionBrowser.Unlock();
 }
 
-
-// This method tries to get a 'connection point' from the stored browser, which can be
-// used to attach or detach from the stream of browser events
-CComPtr<IConnectionPoint> CPluginClass::GetConnectionPoint()
-{
-  CComQIPtr<IConnectionPointContainer, &IID_IConnectionPointContainer> pContainer(GetBrowser());
-  if (!pContainer)
-  {
-    return NULL;
-  }
-
-  CComPtr<IConnectionPoint> pPoint;
-  HRESULT hr = pContainer->FindConnectionPoint(DIID_DWebBrowserEvents2, &pPoint);
-  if (FAILED(hr))
-  {
-    DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_FIND_CONNECTION_POINT, "Class::GetConnectionPoint - FindConnectionPoint")
-      return NULL;
-  }
-
-  return pPoint;
-}
-
 HWND CPluginClass::GetBrowserHWND() const
 {
   SHANDLE_PTR hBrowserWndHandle = NULL;
@@ -262,34 +239,30 @@ STDMETHODIMP CPluginClass::SetSite(IUnknown* unknownSite)
     try
     {
       // Check if loaded as BHO
-      if (GetBrowser())
+      auto webBrowser = GetBrowser();
+      if (webBrowser)
       {
         DEBUG_GENERAL("Loaded as BHO");
-        CComPtr<IConnectionPoint> pPoint = GetConnectionPoint();
-        if (pPoint)
+        HRESULT hr = DispEventAdvise(webBrowser);
+        if (SUCCEEDED(hr))
         {
-          HRESULT hr = pPoint->Advise((IDispatch*)this, &m_nConnectionID);
-          if (SUCCEEDED(hr))
+          m_isAdviced = true;

[Oleksandr, 2015/01/09 00:02:17]

NIT: m_isAdvised. I know it was my spelling mistake to begin with, but now seems
like a good time to fix it. We can also change Unadvice to Unadvise as well,
since we are already touching it.

+          try
           {
-            m_isAdviced = true;
-
-            try
-            {
-              std::thread startInitObjectThread(StartInitObject, this);
-              startInitObjectThread.detach(); // TODO: but actually we should wait for the thread in the dtr.
-            }
-            catch (const std::system_error& ex)
-            {
-              auto errDescription = std::string("Class::Thread - Failed to create StartInitObject thread, ") +
-                ex.code().message() + ex.what();
-              DEBUG_ERROR_LOG(ex.code().value(), PLUGIN_ERROR_THREAD, PLUGIN_ERROR_MAIN_THREAD_CREATE_PROCESS, errDescription.c_str());
-            }
+            std::thread startInitObjectThread(StartInitObject, this);
+            startInitObjectThread.detach(); // TODO: but actually we should wait for the thread in the dtr.
           }
-          else
+          catch (const std::system_error& ex)
           {
-            DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_ADVICE, "Class::SetSite - Advice");
+            auto errDescription = std::string("Class::Thread - Failed to create StartInitObject thread, ") +
+              ex.code().message() + ex.what();
+            DEBUG_ERROR_LOG(ex.code().value(), PLUGIN_ERROR_THREAD, PLUGIN_ERROR_MAIN_THREAD_CREATE_PROCESS, errDescription.c_str());
           }
         }
+        else
+        {
+          DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_ADVICE, "Class::SetSite - Advice");
+        }
       }
       else // Check if loaded as toolbar handler
       {
@@ -501,83 +474,106 @@ void CPluginClass::ShowStatusBar()
   DEBUG_GENERAL("ShowStatusBar end");
 }
 
-void CPluginClass::BeforeNavigate2(DISPPARAMS* pDispParams)
+void STDMETHODCALLTYPE CPluginClass::OnBeforeNavigate2(
+                        /* [in] */ IDispatch* frameBrowserDisp,
+                        /* [in] */ VARIANT* urlVariant,
+                        /* [in] */ VARIANT* /*Flags*/,
+                        /* [in] */ VARIANT* /*TargetFrameName*/,
+                        /* [in] */ VARIANT* /*PostData*/,
+                        /* [in] */ VARIANT* /*Headers*/,
+                        /* [in, out] */ VARIANT_BOOL* /*Cancel*/)
 {
-
-  if (pDispParams->cArgs < 7)
+  ATL::CComQIPtr<IWebBrowser2> webBrowser = frameBrowserDisp;
+  if (!webBrowser)
   {
     return;
   }
-  //Register a mime filter if it's not registered yet
-  if (s_mimeFilter == NULL)
-  {
-    s_mimeFilter = CPluginClientFactory::GetMimeFilterClientInstance();
-  }
 
-  // Get the IWebBrowser2 interface
-  CComQIPtr<IWebBrowser2, &IID_IWebBrowser2> WebBrowser2Ptr;
-  VARTYPE vt = pDispParams->rgvarg[6].vt;
-  if (vt == VT_DISPATCH)
-  {
-    WebBrowser2Ptr = pDispParams->rgvarg[6].pdispVal;
-  }
-  else
+  if (!urlVariant || urlVariant->vt != VT_BSTR)
   {
-    // Wrong type, return.
     return;
   }
+  std::wstring url(V_BSTR(urlVariant), SysStringLen(V_BSTR(urlVariant)));

[Oleksandr, 2015/01/09 00:02:17]

I would prefer minimizing the amount of macros we use. How about
urlVariant.bstrVal?

[sergei, 2015/01/09 16:10:30]

Sure, fine for me, fixed.

+  UnescapeUrl(url);
 
-  // Get the URL
-  CString url;
-  vt = pDispParams->rgvarg[5].vt;
-  if (vt == VT_BYREF + VT_VARIANT)
-  {
-    url = pDispParams->rgvarg[5].pvarVal->bstrVal;
-
-    CPluginClient::UnescapeUrl(url);
-  }
-  else
+  //Register a mime filter if it's not registered yet
+  if (s_mimeFilter == NULL)
   {
-    // Wrong type, return.
-    return;
+    s_mimeFilter = CPluginClientFactory::GetMimeFilterClientInstance();
   }
 
+  CString urlCString = ToCString(url);
+
   // If webbrowser2 is equal to top level browser (as set in SetSite), we are navigating new page
   CPluginClient* client = CPluginClient::GetInstance();
 
-  if (url.Find(L"javascript") == 0)
+  if (urlCString.Find(L"javascript") == 0)
   {
   }
-  else if (GetBrowser().IsEqualObject(WebBrowser2Ptr))
+  else if (GetBrowser().IsEqualObject(webBrowser))
   {
-    m_tab->OnNavigate(url);
+    m_tab->OnNavigate(urlCString);
 
-    DEBUG_GENERAL(L"================================================================================\nBegin main navigation url:" + url + "\n================================================================================")
+    DEBUG_GENERAL(L"================================================================================\nBegin main navigation url:" + urlCString + "\n================================================================================")
 
 #ifdef ENABLE_DEBUG_RESULT
-      CPluginDebug::DebugResultDomain(url);
+      CPluginDebug::DebugResultDomain(urlCString);
 #endif
 
     UpdateStatusBar();
   }
   else
   {
-    DEBUG_NAVI(L"Navi::Begin navigation url:" + url)
-    m_tab->CacheFrame(url);
+    DEBUG_NAVI(L"Navi::Begin navigation url:" + urlCString)
+    m_tab->CacheFrame(urlCString);
   }
 }
-STDMETHODIMP CPluginClass::OnTabChanged(DISPPARAMS* pDispParams, WORD wFlags)
+
+void STDMETHODCALLTYPE CPluginClass::OnDownloadBegin()
+{
+  DEBUG_NAVI("Navi::Download Begin")
+}
+
+void STDMETHODCALLTYPE CPluginClass::OnDownloadComplete()
+{
+  DEBUG_NAVI("Navi::Download Complete")
+  ATL::CComPtr<IWebBrowser2> browser = GetBrowser();
+  if (browser)
+  {
+    m_tab->OnDownloadComplete(browser);
+  }
+}
+
+void STDMETHODCALLTYPE CPluginClass::OnDocumentComplete(IDispatch* frameBrowserDisp, VARIANT* /*urlOrPidl*/)
+{
+  ATL::CComQIPtr<IWebBrowser2> webBrowser2 = frameBrowserDisp;
+  if (!webBrowser2)
+  {
+    return;
+  }
+  ATL::CString frameSrc;
+  ATL::CComBSTR locationUrl;
+  if (FAILED(webBrowser2->get_LocationURL(&locationUrl)) && !!locationUrl)
+  {
+    return;
+  }
+  frameSrc = locationUrl;
+  CPluginClient::UnescapeUrl(frameSrc);
+  bool isRootPageBrowser = GetBrowser().IsEqualObject(webBrowser2);
+  m_tab->OnDocumentComplete(webBrowser2, frameSrc, isRootPageBrowser);
+}
+
+void STDMETHODCALLTYPE CPluginClass::OnWindowStateChanged(unsigned long flags, unsigned long validFlagsMask)
 {
   DEBUG_GENERAL("Tab changed");
-  bool newtabshown = pDispParams->rgvarg[1].intVal==3;
+  bool newtabshown = validFlagsMask == (OLECMDIDF_WINDOWSTATE_USERVISIBLE | OLECMDIDF_WINDOWSTATE_ENABLED)
+                  && flags == (OLECMDIDF_WINDOWSTATE_USERVISIBLE | OLECMDIDF_WINDOWSTATE_ENABLED);
   if (newtabshown)
   {
     std::map<DWORD,CPluginClass*>::const_iterator it = s_threadInstances.find(GetCurrentThreadId());
     if (it == s_threadInstances.end())
     {
       s_threadInstances[::GetCurrentThreadId()] = this;
-
-
       if (!m_isInitializedOk)
       {
         m_isInitializedOk = true;
@@ -591,148 +587,26 @@ STDMETHODIMP CPluginClass::OnTabChanged(DISPPARAMS* pDispParams, WORD wFlags)
   }
   notificationMessage.Hide();
   DEBUG_GENERAL("Tab change end");
-  return VARIANT_TRUE;
 }
 
-// This gets called whenever there's a browser event
-STDMETHODIMP CPluginClass::Invoke(DISPID dispidMember, REFIID riid, LCID lcid, WORD wFlags, DISPPARAMS* pDispParams, VARIANT* pvarResult, EXCEPINFO* pExcepInfo, UINT* puArgErr)
+void STDMETHODCALLTYPE CPluginClass::OnCommandStateChange(long /*command*/, VARIANT_BOOL /*enable*/)

[Oleksandr, 2015/01/09 00:02:17]

Is there any reason why parameter names are commented out?

 {
-  WCHAR tmp[256];
-  wsprintf(tmp, L"Invoke: %d\n", dispidMember);
-  DEBUG_GENERAL(tmp);
-  switch (dispidMember)
+  if (m_hPaneWnd == NULL)
   {
-
-  case DISPID_WINDOWSTATECHANGED:
-    return OnTabChanged(pDispParams, wFlags);
-    break;
-  case DISPID_HTMLDOCUMENTEVENTS2_ONBEFOREUPDATE:
-    return VARIANT_TRUE;
-    break;
-
-  case DISPID_HTMLDOCUMENTEVENTS2_ONCLICK:
-    return VARIANT_TRUE;
-    break;
-
-  case DISPID_EVMETH_ONLOAD:
-    DEBUG_NAVI("Navi::OnLoad")
-      return VARIANT_TRUE;
-    break;
-
-  case DISPID_EVMETH_ONCHANGE:
-    return VARIANT_TRUE;
-
-  case DISPID_EVMETH_ONMOUSEDOWN:
-    return VARIANT_TRUE;
-
-  case DISPID_EVMETH_ONMOUSEENTER:
-    return VARIANT_TRUE;
-
-  case DISPID_IHTMLIMGELEMENT_START:
-    return VARIANT_TRUE;
-
-  case STDDISPID_XOBJ_ERRORUPDATE:
-    return VARIANT_TRUE;
-
-  case STDDISPID_XOBJ_ONPROPERTYCHANGE:
-    return VARIANT_TRUE;
-
-  case DISPID_READYSTATECHANGE:
-    DEBUG_NAVI("Navi::ReadyStateChange")
-      return VARIANT_TRUE;
-
-  case DISPID_BEFORENAVIGATE:
-    DEBUG_NAVI("Navi::BeforeNavigate")
-      return VARIANT_TRUE;
-  case DISPID_COMMANDSTATECHANGE:
-    if (m_hPaneWnd == NULL)
-    {
-      CreateStatusBarPane();
-    }
-    else
-    {
-      if (CPluginClient::GetInstance()->GetIEVersion() > 6)
-      {
-        RECT rect;
-        BOOL rectRes = GetClientRect(m_hStatusBarWnd, &rect);
-        if (rectRes == TRUE)
-        {
-          MoveWindow(m_hPaneWnd, rect.right - 200, 0, m_nPaneWidth, rect.bottom - rect.top, TRUE);
-        }
-      }      
-    }
-    break;
-  case DISPID_STATUSTEXTCHANGE:
-    break;
-
-  case DISPID_BEFORENAVIGATE2:
-    BeforeNavigate2(pDispParams);
-    break;
-
-  case DISPID_DOWNLOADBEGIN:
-    {
-      DEBUG_NAVI("Navi::Download Begin")
-    }
-    break;
-
-  case DISPID_DOWNLOADCOMPLETE:
-    {
-      DEBUG_NAVI("Navi::Download Complete")
-
-        CComQIPtr<IWebBrowser2> browser = GetBrowser();
-      if (browser)
-      {
-        m_tab->OnDownloadComplete(browser);
-      }
-    }
-    break;
-
-  case DISPID_DOCUMENTCOMPLETE:
+    CreateStatusBarPane();
+  }
+  else
+  {
+    if (CPluginClient::GetInstance()->GetIEVersion() > 6)
     {
-      DEBUG_NAVI("Navi::Document Complete")
-
-        CComQIPtr<IWebBrowser2> browser = GetBrowser();
-
-      if (browser && pDispParams->cArgs >= 2 && pDispParams->rgvarg[1].vt == VT_DISPATCH)
+      RECT rect;
+      BOOL rectRes = GetClientRect(m_hStatusBarWnd, &rect);
+      if (rectRes == TRUE)
       {
-        CComQIPtr<IWebBrowser2> pBrowser = pDispParams->rgvarg[1].pdispVal;
-        if (pBrowser)
-        {
-          CString url;
-          CComBSTR bstrUrl;
-          if (SUCCEEDED(pBrowser->get_LocationURL(&bstrUrl)) && ::SysStringLen(bstrUrl) > 0)
-          {
-            url = bstrUrl;
-
-            CPluginClient::UnescapeUrl(url);
-
-            m_tab->OnDocumentComplete(browser, url, browser.IsEqualObject(pBrowser));
-          }
-        }
+        MoveWindow(m_hPaneWnd, rect.right - 200, 0, m_nPaneWidth, rect.bottom - rect.top, TRUE);
       }
     }
-    break;
-
-  case DISPID_ONQUIT:
-  case DISPID_QUIT:
-    {
-      Unadvice();
-    }
-    break;
-
-  default:
-    {
-      CString did;
-      did.Format(L"DispId:%u", dispidMember);
-
-      DEBUG_NAVI(L"Navi::Default " + did)
-    }
-
-    // do nothing
-    break;
   }
-
-  return VARIANT_TRUE;
 }
 
 bool CPluginClass::InitObject(bool bBHO)
@@ -1731,22 +1605,17 @@ void CPluginClass::UpdateStatusBar()
 }
 
 
-void CPluginClass::Unadvice()
+void STDMETHODCALLTYPE CPluginClass::Unadvice()
 {
   s_criticalSectionLocal.Lock();
   {
     if (m_isAdviced)
     {
-      CComPtr<IConnectionPoint> pPoint = GetConnectionPoint();
-      if (pPoint)
+      HRESULT hr = DispEventUnadvise(GetBrowser());
+      if (FAILED(hr))
       {
-        HRESULT hr = pPoint->Unadvise(m_nConnectionID);
-        if (FAILED(hr))
-        {
-          DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_UNADVICE, "Class::Unadvice - Unadvise");
-        }
+        DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_UNADVICE, "Class::Unadvice - Unadvise");
       }
-
       m_isAdviced = false;
     }
   }