Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Unified Diff: src/plugin/PluginClass.cpp

Issue 29323561: Issue #3383 - Rewrite and simplify browser-site handling in CPluginClass (Closed)
Patch Set: change type of m_webBrowser2 Created Nov. 18, 2015, 6:08 p.m.
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View side-by-side diff with in-line comments
Download patch
« src/plugin/PluginClass.h ('K') | « src/plugin/PluginClass.h ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: src/plugin/PluginClass.cpp
===================================================================
--- a/src/plugin/PluginClass.cpp
+++ b/src/plugin/PluginClass.cpp
@@ -60,7 +60,6 @@
std::map<DWORD, CPluginClass*> CPluginClass::s_threadInstances;
CComAutoCriticalSection CPluginClass::s_criticalSectionLocal;
-CComAutoCriticalSection CPluginClass::s_criticalSectionBrowser;
CComAutoCriticalSection CPluginClass::s_criticalSectionWindow;
CComQIPtr<IWebBrowser2> CPluginClass::s_asyncWebBrowser2;
@@ -91,6 +90,7 @@
}
CPluginClass::CPluginClass()
+ : m_webBrowser2(nullptr)
sergei 2015/11/30 15:52:08 We don't need it because ATL::CComPtr default cons
Eric 2015/11/30 16:31:51 I'd rather have it explicit as a lightweight form
sergei 2015/12/07 10:49:08 ATL::CComPtr initializes it to nullptr in the cons
Eric 2015/12/07 14:04:12 The very-lightweight documentation is that every v
sergei 2015/12/08 09:41:59 I think it will go away in a future and I don't un
sergei 2015/12/08 09:41:59 It sounds not convincing. Why are we not doing it
{
//Use this line to debug memory leaks
// _CrtDumpMemoryLeaks();
@@ -115,56 +115,27 @@
delete m_tab;
}
-
-/////////////////////////////////////////////////////////////////////////////
-// Initialization
-
-HRESULT CPluginClass::FinalConstruct()
-{
- return S_OK;
-}
-
-void CPluginClass::FinalRelease()
-{
- s_criticalSectionBrowser.Lock();
- {
- m_webBrowser2.Release();
- }
- s_criticalSectionBrowser.Unlock();
-}
-
HWND CPluginClass::GetBrowserHWND() const
{
- SHANDLE_PTR hBrowserWndHandle = NULL;
-
- CComQIPtr<IWebBrowser2> browser = GetBrowser();
- if (browser)
+ if (!m_webBrowser2)
{
- HRESULT hr = browser->get_HWND(&hBrowserWndHandle);
- if (FAILED(hr))
- {
- DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_UI, PLUGIN_ERROR_UI_GET_BROWSER_WINDOW, "Class::GetBrowserHWND - failed")
- }
+ DEBUG_ERROR_LOG(0, 0, 0, "CPluginClass::GetBrowserHWND - Reached with m_webBrowser2 == nullptr");
+ return nullptr;
}
-
+ SHANDLE_PTR hBrowserWndHandle;
sergei 2015/11/30 15:52:09 it would be good to initialize it to NULL.
Eric 2015/11/30 16:31:51 Welcome to MicrosoftLand. SHANDLE_PTR is not a poi
Oleksandr 2015/12/03 11:51:58 We don't check its value now, but we might in futu
Eric 2015/12/03 14:24:54 Initialized it to zero, which matches its type. A
sergei 2015/12/07 10:49:08 Agree, except following some good practices, like
sergei 2015/12/07 10:49:08 Although zero is OK, NULL looks better here.
Eric 2015/12/07 14:04:12 I disagree. Sometimes NULL is defined as '(void*)0
Eric 2015/12/07 14:04:12 Almost always. But even saying that is saying too
+ HRESULT hr = m_webBrowser2->get_HWND(&hBrowserWndHandle);
+ if (FAILED(hr))
+ {
+ DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_UI, PLUGIN_ERROR_UI_GET_BROWSER_WINDOW, "Class::GetBrowserHWND - failed")
sergei 2015/11/30 15:52:08 Actually, I think we should return nullptr in case
Eric 2015/11/30 16:31:51 Done. That's a good idea.
+ }
return (HWND)hBrowserWndHandle;
}
-
-CComQIPtr<IWebBrowser2> CPluginClass::GetBrowser() const
+bool CPluginClass::IsRootBrowser(IWebBrowser2* otherBrowser)
{
- CComQIPtr<IWebBrowser2> browser;
-
- s_criticalSectionBrowser.Lock();
- {
- browser = m_webBrowser2;
- }
- s_criticalSectionBrowser.Unlock();
-
- return browser;
+ return m_webBrowser2.IsEqualObject(otherBrowser);
}
-
CComQIPtr<IWebBrowser2> CPluginClass::GetAsyncBrowser()
{
CComQIPtr<IWebBrowser2> browser;
@@ -181,11 +152,10 @@
std::wstring CPluginClass::GetBrowserUrl() const
{
std::wstring url;
- CComQIPtr<IWebBrowser2> browser = GetBrowser();
- if (browser)
+ if (m_webBrowser2)
{
CComBSTR bstrURL;
- if (SUCCEEDED(browser->get_LocationURL(&bstrURL)) && bstrURL)
+ if (SUCCEEDED(m_webBrowser2->get_LocationURL(&bstrURL)) && bstrURL)
{
url = std::wstring(bstrURL, SysStringLen(bstrURL));
UnescapeUrl(url);
@@ -193,6 +163,7 @@
}
else
{
+ DEBUG_ERROR_LOG(0, 0, 0, "CPluginClass::GetBrowserUrl - Reached with m_webBrowser2 == nullptr");
url = m_tab->GetDocumentUrl();
}
return url;
@@ -235,16 +206,18 @@
DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_COINIT, "Class::SetSite - CoInitialize");
}
- s_criticalSectionBrowser.Lock();
+ /*
+ * We were instantiated as a BHO, so our site is always of type IWebBrowser2.
+ */
+ m_webBrowser2 = ATL::CComQIPtr<IWebBrowser2>(unknownSite);
+ if (!m_webBrowser2)
{
- m_webBrowser2 = unknownSite;
+ throw std::logic_error("CPluginClass::SetSite - Unable to convert site pointer to IWebBrowser2*");
}
- s_criticalSectionBrowser.Unlock();
//register the mimefilter
//and only mimefilter
//on some few computers the mimefilter does not get properly registered when it is done on another thread
-
s_criticalSectionLocal.Lock();
{
// Always register on startup, then check if we need to unregister in a separate thread
@@ -256,30 +229,26 @@
try
{
- auto webBrowser = GetBrowser();
- if (webBrowser)
+ DEBUG_GENERAL("Loaded as BHO");
+ HRESULT hr = DispEventAdvise(m_webBrowser2);
+ if (SUCCEEDED(hr))
{
- DEBUG_GENERAL("Loaded as BHO");
- HRESULT hr = DispEventAdvise(webBrowser);
- if (SUCCEEDED(hr))
+ m_isAdvised = true;
+ try
{
- m_isAdvised = 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)
- {
- DEBUG_SYSTEM_EXCEPTION(ex, PLUGIN_ERROR_THREAD, PLUGIN_ERROR_MAIN_THREAD_CREATE_PROCESS,
- "Class::Thread - Failed to create StartInitObject thread");
- }
+ 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 - Advise");
+ DEBUG_SYSTEM_EXCEPTION(ex, PLUGIN_ERROR_THREAD, PLUGIN_ERROR_MAIN_THREAD_CREATE_PROCESS,
+ "Class::Thread - Failed to create StartInitObject thread");
}
}
+ else
+ {
+ DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_ADVICE, "Class::SetSite - Advise");
+ }
}
catch (const std::runtime_error& ex)
{
@@ -329,23 +298,16 @@
}
s_criticalSectionLocal.Unlock();
- // Release browser interface
- s_criticalSectionBrowser.Lock();
- {
- m_webBrowser2.Release();
sergei 2015/11/30 15:52:08 It's still good to call `m_webBrowser2.Release();`
Eric 2015/11/30 16:31:51 Basically right. The correct thing to do is to nul
- }
- s_criticalSectionBrowser.Unlock();
-
DEBUG_GENERAL("================================================================================\nNEW TAB UI - END\n================================================================================")
::CoUninitialize();
}
- IObjectWithSiteImpl<CPluginClass>::SetSite(unknownSite);
}
catch (...)
{
}
+ IObjectWithSiteImpl<CPluginClass>::SetSite(unknownSite);
return S_OK;
}
@@ -492,7 +454,7 @@
if (url.find(L"javascript") == 0)
{
}
- else if (GetBrowser().IsEqualObject(webBrowser))
+ else if (IsRootBrowser(webBrowser))
{
m_tab->OnNavigate(url);
DEBUG_GENERAL(
@@ -521,12 +483,13 @@
{
try
{
+ if (!m_webBrowser2)
+ {
+ DEBUG_ERROR_LOG(0, 0, 0, "CPluginClass::OnDownloadComplete - Reached with m_webBrowser2 == nullptr");
+ return;
+ }
DEBUG_NAVI(L"Navi::Download Complete")
- ATL::CComPtr<IWebBrowser2> browser = GetBrowser();
- if (browser)
- {
- m_tab->OnDownloadComplete(browser);
- }
+ m_tab->OnDownloadComplete(m_webBrowser2);
}
catch (...)
{
@@ -546,8 +509,7 @@
}
std::wstring frameSrc = GetLocationUrl(*webBrowser2);
UnescapeUrl(frameSrc);
- bool isRootPageBrowser = GetBrowser().IsEqualObject(webBrowser2);
- m_tab->OnDocumentComplete(webBrowser2, frameSrc, isRootPageBrowser);
+ m_tab->OnDocumentComplete(webBrowser2, frameSrc, IsRootBrowser(webBrowser2));
}
catch (...)
{
@@ -626,7 +588,7 @@
bool CPluginClass::InitObject()
{
- DEBUG_GENERAL("InitObject");
+ DEBUG_GENERAL("InitObject - begin");
CPluginSettings* settings = CPluginSettings::GetInstance();
if (!settings->GetPluginEnabled())
@@ -739,6 +701,8 @@
CPluginClient::GetInstance()->AddSubscription(aaUrl);
}
s_criticalSectionLocal.Unlock();
+
+ DEBUG_GENERAL("InitObject - end");
return true;
}
@@ -1628,11 +1592,16 @@
void CPluginClass::Unadvise()
{
+ if (!m_webBrowser2)
+ {
+ DEBUG_ERROR_LOG(0, 0, 0, "CPluginClass::Unadvise - Reached with m_webBrowser2 == nullptr");
+ return;
+ }
s_criticalSectionLocal.Lock();
{
if (m_isAdvised)
{
- HRESULT hr = DispEventUnadvise(GetBrowser());
+ HRESULT hr = DispEventUnadvise(m_webBrowser2);
if (FAILED(hr))
{
DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_UNADVISE, "Class::Unadvise - Unadvise");
@@ -1672,70 +1641,3 @@
return s_atomPaneClass;
}
-HWND CPluginClass::GetTabHWND() const
-{
- std::array<wchar_t, MAX_PATH> className;
- // Get browser window and url
- HWND hBrowserWnd = GetBrowserHWND();
- if (!hBrowserWnd)
- {
- DEBUG_ERROR_LOG(0, PLUGIN_ERROR_UI, PLUGIN_ERROR_UI_NO_STATUSBAR_BROWSER, "Class::GetTabWindow - No tab window")
- s_criticalSectionWindow.Unlock();
-
- return false;
- }
-
- // Looking for a TabWindowClass window in IE7
-
- HWND hTabWnd = ::GetWindow(hBrowserWnd, GW_CHILD);
- while (hTabWnd)
- {
- className[0] = L'\0';
- int classNameLength = GetClassNameW(hTabWnd, className.data(), className.size());
-
- if (classNameLength && (wcscmp(className.data(), L"TabWindowClass") == 0 || wcscmp(className.data(), L"Frame Tab") == 0))
- {
- // IE8 support
- HWND hTabWnd2 = hTabWnd;
- if (wcscmp(className.data(), L"Frame Tab") == 0)
- {
- hTabWnd2 = ::FindWindowEx(hTabWnd2, NULL, L"TabWindowClass", NULL);
- }
-
- if (hTabWnd2)
- {
- DWORD nProcessId;
- ::GetWindowThreadProcessId(hTabWnd2, &nProcessId);
- if (::GetCurrentProcessId() == nProcessId)
- {
- bool bExistingTab = false;
- s_criticalSectionLocal.Lock();
-
- {
- for (auto instance : s_instances)
- {
- if (instance->m_hTabWnd == hTabWnd2)
- {
- bExistingTab = true;
- break;
- }
- }
- }
-
- if (!bExistingTab)
- {
- hBrowserWnd = hTabWnd2;
- hTabWnd = hTabWnd2;
- s_criticalSectionLocal.Unlock();
- break;
- }
- s_criticalSectionLocal.Unlock();
-
- }
- }
- }
-
- hTabWnd = ::GetWindow(hTabWnd, GW_HWNDNEXT);
- }
- return hTabWnd;
-}
« src/plugin/PluginClass.h ('K') | « src/plugin/PluginClass.h ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld