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

Unified Diff: src/plugin/PluginTabBase.cpp

Issue 29334397: Issue #2230, #3391 - Load filters on "download begin" event
Patch Set: Created Jan. 22, 2016, 6:02 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
Index: src/plugin/PluginTabBase.cpp
===================================================================
--- a/src/plugin/PluginTabBase.cpp
+++ b/src/plugin/PluginTabBase.cpp
@@ -29,8 +29,6 @@
: m_isActivated(false)
, m_continueThreadRunning(true)
{
- m_filter.hideFiltersLoadedEvent = CreateEvent(NULL, true, false, NULL);
-
CPluginClient* client = CPluginClient::GetInstance();
if (AdblockPlus::IE::InstalledMajorVersion() < 10)
{
@@ -79,38 +77,29 @@
m_isActivated = true;
}
-namespace
+void CPluginTab::OnNavigate(const std::wstring& url)
{
- // Entry Point
- void FilterLoader(CPluginFilter* filter, const std::wstring& domain)
+ SetDocumentUrl(url);
+}
+
+void CPluginTab::OnDownloadBegin()
sergei 2016/01/29 10:03:20 DownloadComplete and DownloadBegin are fired sever
sergei 2016/01/29 10:03:21 Are all DownloadBegin and DownloadComplete called
Oleksandr 2016/02/01 02:12:27 In my tests they are called from the same thread.
Eric 2016/02/03 18:03:20 See reply to Oleksandr's concern about the same is
Eric 2016/02/03 18:03:20 I believe this is part of the "apartment-like" thr
+{
+ std::wstring domain = GetDocumentDomain();
+ ClearFrameCache(domain);
+ if (!filter)
{
try
{
- filter->LoadHideFilters(CPluginClient::GetInstance()->GetElementHidingSelectors(domain));
- SetEvent(filter->hideFiltersLoadedEvent);
+ filter.reset(new CPluginFilter(domain));
sergei 2016/01/29 10:03:21 That's really non-readable here that initializatio
Eric 2016/02/03 18:03:20 It really doesn't matter here how 'CPluginFilter'
}
- catch (...)
+ catch (const std::exception& e)
{
- // As a thread-main function, we truncate any C++ exception.
+ DEBUG_EXCEPTION(e);
}
}
-}
-
-void CPluginTab::OnNavigate(const std::wstring& url)
-{
- SetDocumentUrl(url);
- ClearFrameCache(GetDocumentDomain());
- std::wstring domainString = GetDocumentDomain();
- ResetEvent(m_filter.hideFiltersLoadedEvent);
- try
+ else
{
- std::thread filterLoaderThread(&FilterLoader, &m_filter, GetDocumentDomain());
- filterLoaderThread.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 start filter loader thread");
+ DEBUG_GENERAL("Suspicious: Reached CPluginTab::OnDownloadBegin with a non-null filter")
}
}
@@ -261,9 +250,15 @@
std::wstring url = GetDocumentUrl();
if (!client->IsWhitelistedUrl(url) && !client->IsElemhideWhitelistedOnDomain(url))
{
- CPluginDomTraverser traversal(&m_filter);
- traversal.TraverseDocumentRoot(browser, GetDocumentDomain(), GetDocumentUrl());
+ CPluginFilter* f = filter.get();
+ if (f)
sergei 2016/01/29 10:03:20 What the trick are you trying to use here? it look
Oleksandr 2016/02/01 02:12:27 +1. I don't understand what is this testing.
Eric 2016/02/03 18:03:20 It's defensive programming. The declaration change
+ {
+ f->EnsureInitialized();
sergei 2016/01/29 10:03:21 It would be better to have some wrapper around CPl
Eric 2016/02/03 18:03:20 It's a trade-off. You could declare two classes, a
+ CPluginDomTraverser traversal(f);
+ traversal.TraverseDocumentRoot(browser, GetDocumentDomain(), GetDocumentUrl());
+ }
}
+ filter.reset();
InjectABP(browser);
}

Powered by Google App Engine
This is Rietveld