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

Delta Between Two Patch Sets: src/plugin/PluginClass.cpp

Issue 29323561: Issue #3383 - Rewrite and simplify browser-site handling in CPluginClass (Closed)
Left Patch Set: Created Aug. 13, 2015, 4:52 p.m.
Right Patch Set: initialization; shorten comment Created Dec. 3, 2015, 2:23 p.m.
Left:
Right:
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
Left: Side by side diff | Download
Right: Side by side diff | Download
« no previous file with change/comment | « src/plugin/PluginClass.h ('k') | no next file » | no next file with change/comment »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
LEFTRIGHT
1 /* 1 /*
2 * This file is part of Adblock Plus <https://adblockplus.org/>, 2 * This file is part of Adblock Plus <https://adblockplus.org/>,
3 * Copyright (C) 2006-2015 Eyeo GmbH 3 * Copyright (C) 2006-2015 Eyeo GmbH
4 * 4 *
5 * Adblock Plus is free software: you can redistribute it and/or modify 5 * Adblock Plus is free software: you can redistribute it and/or modify
6 * it under the terms of the GNU General Public License version 3 as 6 * it under the terms of the GNU General Public License version 3 as
7 * published by the Free Software Foundation. 7 * published by the Free Software Foundation.
8 * 8 *
9 * Adblock Plus is distributed in the hope that it will be useful, 9 * Adblock Plus is distributed in the hope that it will be useful,
10 * but WITHOUT ANY WARRANTY; without even the implied warranty of 10 * but WITHOUT ANY WARRANTY; without even the implied warranty of
(...skipping 104 matching lines...) Expand 10 before | Expand all | Expand 10 after
115 delete m_tab; 115 delete m_tab;
116 } 116 }
117 117
118 HWND CPluginClass::GetBrowserHWND() const 118 HWND CPluginClass::GetBrowserHWND() const
119 { 119 {
120 if (!m_webBrowser2) 120 if (!m_webBrowser2)
121 { 121 {
122 DEBUG_ERROR_LOG(0, 0, 0, "CPluginClass::GetBrowserHWND - Reached with m_webB rowser2 == nullptr"); 122 DEBUG_ERROR_LOG(0, 0, 0, "CPluginClass::GetBrowserHWND - Reached with m_webB rowser2 == nullptr");
123 return nullptr; 123 return nullptr;
124 } 124 }
125 SHANDLE_PTR hBrowserWndHandle; 125 SHANDLE_PTR hBrowserWndHandle = 0;
126 HRESULT hr = m_webBrowser2->get_HWND(&hBrowserWndHandle); 126 HRESULT hr = m_webBrowser2->get_HWND(&hBrowserWndHandle);
127 if (FAILED(hr)) 127 if (FAILED(hr))
128 { 128 {
129 DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_UI, PLUGIN_ERROR_UI_GET_BROWSER_WINDOW, "Cl ass::GetBrowserHWND - failed") 129 DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_UI, PLUGIN_ERROR_UI_GET_BROWSER_WINDOW, "Cl ass::GetBrowserHWND - failed");
130 return nullptr;
130 } 131 }
131 return (HWND)hBrowserWndHandle; 132 return (HWND)hBrowserWndHandle;
132 } 133 }
133 134
134 bool CPluginClass::IsRootPageBrowser(IWebBrowser2* otherBrowser) 135 bool CPluginClass::IsRootBrowser(IWebBrowser2* otherBrowser)
135 { 136 {
136 /* 137 return m_webBrowser2.IsEqualObject(otherBrowser);
137 * The ownership overhead of 'CComPtr', which we don't need, is what we
138 * pay for the convenience of using its member function 'IsEqualObject'.
139 */
140 CComPtr<IWebBrowser> thisBrowser = m_webBrowser2;
sergei 2015/10/01 16:15:51 NIT: why is `IWebBrowser` and not `IWebBrowser2`?
Eric 2015/11/18 13:57:31 Typo. Fixed.
141 return thisBrowser.IsEqualObject(otherBrowser);
142 } 138 }
143 139
144 CComQIPtr<IWebBrowser2> CPluginClass::GetAsyncBrowser() 140 CComQIPtr<IWebBrowser2> CPluginClass::GetAsyncBrowser()
145 { 141 {
146 CComQIPtr<IWebBrowser2> browser; 142 CComQIPtr<IWebBrowser2> browser;
147 143
148 s_criticalSectionLocal.Lock(); 144 s_criticalSectionLocal.Lock();
149 { 145 {
150 browser = s_asyncWebBrowser2; 146 browser = s_asyncWebBrowser2;
151 } 147 }
152 s_criticalSectionLocal.Unlock(); 148 s_criticalSectionLocal.Unlock();
153 149
154 return browser; 150 return browser;
155 } 151 }
156 152
157 std::wstring CPluginClass::GetBrowserUrl() const 153 std::wstring CPluginClass::GetBrowserUrl() const
158 { 154 {
159 std::wstring url; 155 std::wstring url;
160 if (m_webBrowser2) 156 if (m_webBrowser2)
161 { 157 {
162 CComBSTR bstrURL; 158 CComBSTR bstrURL;
163 if (SUCCEEDED(m_webBrowser2->get_LocationURL(&bstrURL)) && bstrURL) 159 if (SUCCEEDED(m_webBrowser2->get_LocationURL(&bstrURL)) && bstrURL)
164 { 160 {
165 url = std::wstring(bstrURL, SysStringLen(bstrURL)); 161 url = std::wstring(bstrURL, SysStringLen(bstrURL));
166 UnescapeUrl(url);
167 } 162 }
168 } 163 }
169 else 164 else
170 { 165 {
171 DEBUG_ERROR_LOG(0, 0, 0, "CPluginClass::GetBrowserUrl - Reached with m_webBr owser2 == nullptr"); 166 DEBUG_ERROR_LOG(0, 0, 0, "CPluginClass::GetBrowserUrl - Reached with m_webBr owser2 == nullptr");
172 url = m_tab->GetDocumentUrl(); 167 url = m_tab->GetDocumentUrl();
173 } 168 }
174 return url; 169 return url;
175 } 170 }
176 171
(...skipping 28 matching lines...) Expand all
205 200
206 DEBUG_GENERAL(L"========================================================== ======================\nNEW TAB UI\n============================================ ====================================") 201 DEBUG_GENERAL(L"========================================================== ======================\nNEW TAB UI\n============================================ ====================================")
207 202
208 HRESULT hr = ::CoInitialize(NULL); 203 HRESULT hr = ::CoInitialize(NULL);
209 if (FAILED(hr)) 204 if (FAILED(hr))
210 { 205 {
211 DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_COINIT, "Class::SetSite - CoInitialize"); 206 DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_SET_SITE, PLUGIN_ERROR_SET_SITE_COINIT, "Class::SetSite - CoInitialize");
212 } 207 }
213 208
214 /* 209 /*
215 * We have a responsibility to call 'AdRef' on 'unknownSite' here. 210 * We were instantiated as a BHO, so our site is always of type IWebBrowse r2.
216 * We discharge that responsibility by calling our base class function
217 * 'SetSite' at the end of this function.
218 * Since the base class is taking care of that, 'm_webBrowser2' can be
219 * defined as a plain pointer.
220 */ 211 */
sergei 2015/10/01 16:15:51 I'm sorry, but this comment seems just wrong.
Oleksandr 2015/10/05 10:44:47 Whatever the base class SetSite does it doesn't kn
sergei 2015/10/05 11:00:56 The base class calls `AddRef` and corresponding `R
Eric 2015/11/18 13:57:30 I've done this in the new patch set.
Eric 2015/11/18 13:57:31 The base class SetSite() assigns its argument 'unk
221 hr = unknownSite->QueryInterface(IID_IWebBrowser2, (void**)(&m_webBrowser2 )); 212 m_webBrowser2 = ATL::CComQIPtr<IWebBrowser2>(unknownSite);
sergei 2015/10/01 16:15:51 I cannot find the call of `Release` on `m_webBrows
Eric 2015/11/18 13:57:30 Done.
222 if (FAILED(hr)) 213 if (!m_webBrowser2)
223 { 214 {
224 throw std::logic_error("CPluginClass::SetSite - Unable to convert site p ointer to IWebBrowser2*"); 215 throw std::logic_error("CPluginClass::SetSite - Unable to convert site p ointer to IWebBrowser2*");
225 } 216 }
226 217
227 //register the mimefilter 218 //register the mimefilter
228 //and only mimefilter 219 //and only mimefilter
229 //on some few computers the mimefilter does not get properly registered wh en it is done on another thread 220 //on some few computers the mimefilter does not get properly registered wh en it is done on another thread
230
231 s_criticalSectionLocal.Lock(); 221 s_criticalSectionLocal.Lock();
232 { 222 {
233 // Always register on startup, then check if we need to unregister in a separate thread 223 // Always register on startup, then check if we need to unregister in a separate thread
234 s_mimeFilter = CPluginClientFactory::GetMimeFilterClientInstance(); 224 s_mimeFilter = CPluginClientFactory::GetMimeFilterClientInstance();
235 s_asyncWebBrowser2 = unknownSite; 225 s_asyncWebBrowser2 = unknownSite;
236 s_instances.insert(this); 226 s_instances.insert(this);
237 } 227 }
238 s_criticalSectionLocal.Unlock(); 228 s_criticalSectionLocal.Unlock();
239 229
240 try 230 try
(...skipping 59 matching lines...) Expand 10 before | Expand all | Expand 10 after
300 { 290 {
301 s_threadInstances.erase(it); 291 s_threadInstances.erase(it);
302 } 292 }
303 if (s_instances.empty()) 293 if (s_instances.empty())
304 { 294 {
305 // TODO: Explicitly releasing a resource when a container becomes empt y looks like a job better suited for shared_ptr 295 // TODO: Explicitly releasing a resource when a container becomes empt y looks like a job better suited for shared_ptr
306 CPluginClientFactory::ReleaseMimeFilterClientInstance(); 296 CPluginClientFactory::ReleaseMimeFilterClientInstance();
307 } 297 }
308 } 298 }
309 s_criticalSectionLocal.Unlock(); 299 s_criticalSectionLocal.Unlock();
300
301 m_webBrowser2 = nullptr;
310 302
311 DEBUG_GENERAL("=========================================================== =====================\nNEW TAB UI - END\n======================================= =========================================") 303 DEBUG_GENERAL("=========================================================== =====================\nNEW TAB UI - END\n======================================= =========================================")
312 304
313 ::CoUninitialize(); 305 ::CoUninitialize();
314 } 306 }
315 307
316 } 308 }
317 catch (...) 309 catch (...)
318 { 310 {
319 } 311 }
320 IObjectWithSiteImpl<CPluginClass>::SetSite(unknownSite); 312 IObjectWithSiteImpl<CPluginClass>::SetSite(unknownSite);
Eric 2015/08/13 16:58:41 Moving this statement outside the try-block ensure
321 return S_OK; 313 return S_OK;
322 } 314 }
323 315
324 bool CPluginClass::IsStatusBarEnabled() 316 bool CPluginClass::IsStatusBarEnabled()
325 { 317 {
326 DEBUG_GENERAL("IsStatusBarEnabled start"); 318 DEBUG_GENERAL("IsStatusBarEnabled start");
327 HKEY pHkey; 319 HKEY pHkey;
328 HKEY pHkeySub; 320 HKEY pHkeySub;
329 RegOpenCurrentUser(KEY_QUERY_VALUE, &pHkey); 321 RegOpenCurrentUser(KEY_QUERY_VALUE, &pHkey);
330 DWORD truth = 1; 322 DWORD truth = 1;
(...skipping 118 matching lines...) Expand 10 before | Expand all | Expand 10 after
449 ATL::CComQIPtr<IWebBrowser2> webBrowser = frameBrowserDisp; 441 ATL::CComQIPtr<IWebBrowser2> webBrowser = frameBrowserDisp;
450 if (!webBrowser) 442 if (!webBrowser)
451 { 443 {
452 return; 444 return;
453 } 445 }
454 if (!urlVariant || urlVariant->vt != VT_BSTR) 446 if (!urlVariant || urlVariant->vt != VT_BSTR)
455 { 447 {
456 return; 448 return;
457 } 449 }
458 std::wstring url(urlVariant->bstrVal, SysStringLen(urlVariant->bstrVal)); 450 std::wstring url(urlVariant->bstrVal, SysStringLen(urlVariant->bstrVal));
459 UnescapeUrl(url);
460 451
461 // If webbrowser2 is equal to top level browser (as set in SetSite), we are 452 // If webbrowser2 is equal to top level browser (as set in SetSite), we are
462 // navigating new page 453 // navigating new page
463 CPluginClient* client = CPluginClient::GetInstance(); 454 CPluginClient* client = CPluginClient::GetInstance();
464 if (url.find(L"javascript") == 0) 455 if (url.find(L"javascript") == 0)
465 { 456 {
466 } 457 }
467 else if (IsRootPageBrowser(webBrowser)) 458 else if (IsRootBrowser(webBrowser))
468 { 459 {
469 m_tab->OnNavigate(url); 460 m_tab->OnNavigate(url);
470 DEBUG_GENERAL( 461 DEBUG_GENERAL(
471 L"======================================================================== ========\n" 462 L"======================================================================== ========\n"
472 L"Begin main navigation url:" + url + L"\n" 463 L"Begin main navigation url:" + url + L"\n"
473 L"======================================================================== ========") 464 L"======================================================================== ========")
474 465
475 #ifdef ENABLE_DEBUG_RESULT 466 #ifdef ENABLE_DEBUG_RESULT
476 CPluginDebug::DebugResultDomain(url); 467 CPluginDebug::DebugResultDomain(url);
477 #endif 468 #endif
(...skipping 33 matching lines...) Expand 10 before | Expand all | Expand 10 after
511 { 502 {
512 try 503 try
513 { 504 {
514 DEBUG_NAVI(L"Navi::Document Complete"); 505 DEBUG_NAVI(L"Navi::Document Complete");
515 ATL::CComQIPtr<IWebBrowser2> webBrowser2 = frameBrowserDisp; 506 ATL::CComQIPtr<IWebBrowser2> webBrowser2 = frameBrowserDisp;
516 if (!webBrowser2) 507 if (!webBrowser2)
517 { 508 {
518 return; 509 return;
519 } 510 }
520 std::wstring frameSrc = GetLocationUrl(*webBrowser2); 511 std::wstring frameSrc = GetLocationUrl(*webBrowser2);
521 UnescapeUrl(frameSrc); 512 m_tab->OnDocumentComplete(webBrowser2, frameSrc, IsRootBrowser(webBrowser2)) ;
522 m_tab->OnDocumentComplete(webBrowser2, frameSrc, IsRootPageBrowser(webBrowse r2));
523 } 513 }
524 catch (...) 514 catch (...)
525 { 515 {
526 } 516 }
527 } 517 }
528 518
529 // Entry point 519 // Entry point
530 void STDMETHODCALLTYPE CPluginClass::OnWindowStateChanged(unsigned long flags, u nsigned long validFlagsMask) 520 void STDMETHODCALLTYPE CPluginClass::OnWindowStateChanged(unsigned long flags, u nsigned long validFlagsMask)
531 { 521 {
532 try 522 try
(...skipping 559 matching lines...) Expand 10 before | Expand all | Expand 10 after
1092 if (FAILED(hr)) 1082 if (FAILED(hr))
1093 { 1083 {
1094 DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_NAVIGATION, PLUGIN_ERROR_NAVIGATION _SETTINGS, "Navigation::Failed") 1084 DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_NAVIGATION, PLUGIN_ERROR_NAVIGATION _SETTINGS, "Navigation::Failed")
1095 } 1085 }
1096 } 1086 }
1097 } 1087 }
1098 break; 1088 break;
1099 } 1089 }
1100 case ID_MENU_DISABLE_ON_SITE: 1090 case ID_MENU_DISABLE_ON_SITE:
1101 { 1091 {
1102 CPluginSettings* settings = CPluginSettings::GetInstance();
1103 std::wstring urlString = GetTab()->GetDocumentUrl(); 1092 std::wstring urlString = GetTab()->GetDocumentUrl();
1104 std::string filterText = client->GetWhitelistingFilter(urlString); 1093 std::string filterText = client->GetWhitelistingFilter(urlString);
1105 if (!filterText.empty()) 1094 if (!filterText.empty())
1106 { 1095 {
1107 client->RemoveFilter(filterText); 1096 client->RemoveFilter(filterText);
1108 } 1097 }
1109 else 1098 else
1110 { 1099 {
1111 settings->AddWhiteListedDomain(ToCString(client->GetHostFromUrl(urlStrin g))); 1100 CPluginSettings::GetInstance()->AddWhiteListedDomain(client->GetHostFrom Url(urlString));
1112 } 1101 }
1113 } 1102 }
1114 default: 1103 default:
1115 break; 1104 break;
1116 } 1105 }
1117 1106
1118 // Invalidate and redraw the control 1107 // Invalidate and redraw the control
1119 UpdateStatusBar(); 1108 UpdateStatusBar();
1120 } 1109 }
1121 1110
(...skipping 522 matching lines...) Expand 10 before | Expand all | Expand 10 after
1644 s_criticalSectionLocal.Unlock(); 1633 s_criticalSectionLocal.Unlock();
1645 1634
1646 return icon; 1635 return icon;
1647 } 1636 }
1648 1637
1649 ATOM CPluginClass::GetAtomPaneClass() 1638 ATOM CPluginClass::GetAtomPaneClass()
1650 { 1639 {
1651 return s_atomPaneClass; 1640 return s_atomPaneClass;
1652 } 1641 }
1653 1642
LEFTRIGHT

Powered by Google App Engine
This is Rietveld