Appear fast for IE

Appear fast for IE

[Oleksandr, 2013-08-07 05:50:52]

The idea here is to remove engine operations from the threads that IE measures.
I have tried it before, but this time it worked.

[Felix Dahlke, 2013-08-07 09:34:49]

Mostly the traditional nit picking. So the gist is that we have to load the
element hiding rules in a separate thread, and IE won't mind?

Please add Wladimir to CC, I'm sure he'll want to have a look at this.

http://codereview.adblockplus.org/11427013/diff/1/src/plugin/PluginClass.cpp
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/11427013/diff/1/src/plugin/PluginClass.cpp#...
src/plugin/PluginClass.cpp:262: // We always register on startup and then check
if we should do that in a seperate thread
I didn't immediately get this, how about:

// Always register on startup, then check if we need to unregister in a separate
thread

http://codereview.adblockplus.org/11427013/diff/1/src/plugin/PluginClass.cpp#...
src/plugin/PluginClass.cpp:734: // Unregister, if disabled
This comment is kinda redundant, I think.

http://codereview.adblockplus.org/11427013/diff/1/src/plugin/PluginDomTravers...
File src/plugin/PluginDomTraverserBase.h (right):

http://codereview.adblockplus.org/11427013/diff/1/src/plugin/PluginDomTravers...
src/plugin/PluginDomTraverserBase.h:118: DWORD res =
WaitForSingleObject(m_tab->m_filter->hideFiltersLoadedEvent, 1000);
We have a higher timeout in AdblockPlusClient, when spawning the engine or
connecting to it. I think we should have the same here, preferably using a
shared constant.

Also, res is an unused variable.

http://codereview.adblockplus.org/11427013/diff/1/src/plugin/PluginSettings.cpp
File src/plugin/PluginSettings.cpp (right):

http://codereview.adblockplus.org/11427013/diff/1/src/plugin/PluginSettings.c...
src/plugin/PluginSettings.cpp:158:
CPluginSettings::GetInstance()->m_subscriptions =
CPluginClient::GetInstance()->FetchAvailableSubscriptions();
Why get the settings instance here? We're in a member function.

http://codereview.adblockplus.org/11427013/diff/1/src/plugin/PluginTabBase.cpp
File src/plugin/PluginTabBase.cpp (right):

http://codereview.adblockplus.org/11427013/diff/1/src/plugin/PluginTabBase.cp...
src/plugin/PluginTabBase.cpp:32: m_filter->hideFiltersLoadedEvent =
CreateEvent(NULL, TRUE, FALSE, NULL);
Since this is C++, we should use true and false instead of the defines.

[Felix Dahlke, 2013-08-07 13:23:03]

http://codereview.adblockplus.org/11427013/diff/1/src/plugin/PluginTabBase.cpp
File src/plugin/PluginTabBase.cpp (right):

http://codereview.adblockplus.org/11427013/diff/1/src/plugin/PluginTabBase.cp...
src/plugin/PluginTabBase.cpp:105: CreateThread(NULL, NULL, &FilterLoader, this,
NULL, NULL);
How I see it, this is the relevant change, right? The one that makes IE think
we're fast although we're not. I think it's worth adding a comment here on why
filters are being loaded in a separate thread.

[Oleksandr, 2013-08-07 16:07:40]

http://codereview.adblockplus.org/11427013/diff/1/src/plugin/PluginTabBase.cpp
File src/plugin/PluginTabBase.cpp (right):

http://codereview.adblockplus.org/11427013/diff/1/src/plugin/PluginTabBase.cp...
src/plugin/PluginTabBase.cpp:105: CreateThread(NULL, NULL, &FilterLoader, this,
NULL, NULL);
This is not the only relevant change. This is just done for IE not to show us as
a bottleneck of the navigation slowdown (the second value in the Menage Addons).
The first value (the load time) was influenced by us initializing the settings
in the main thread (SetSite essentially). So more like the relevant change is
the "always registering the mime filter, and only unregistering it if it was
disabled". That way we can initialize settings singleton elsewhere (since we
don't need to call the GetPref("enabled"..) in the main thread anymore.
On 2013/08/07 13:23:03, Felix H. Dahlke wrote:
> How I see it, this is the relevant change, right? The one that makes IE think
> we're fast although we're not. I think it's worth adding a comment here on why
> filters are being loaded in a separate thread.

[Felix Dahlke, 2013-08-08 08:15:44]

Otherwise LGTM.

http://codereview.adblockplus.org/11427013/diff/8001/src/plugin/Config.h
File src/plugin/Config.h (right):

http://codereview.adblockplus.org/11427013/diff/8001/src/plugin/Config.h#newc...
src/plugin/Config.h:152: #define TIMEOUT_VALUE 10000
How about ENGINE_STARTUP_TIMEOUT?

[Wladimir Palant, 2013-08-13 09:41:02]

http://codereview.adblockplus.org/11427013/diff/8001/src/plugin/PluginSetting...
File src/plugin/PluginSettings.cpp (right):

http://codereview.adblockplus.org/11427013/diff/8001/src/plugin/PluginSetting...
src/plugin/PluginSettings.cpp:158: m_subscriptions =
CPluginClient::GetInstance()->FetchAvailableSubscriptions();
This should be a local variable, no point keeping it a member variable. This
method can stay const then.

http://codereview.adblockplus.org/11427013/diff/8001/src/plugin/PluginTabBase...
File src/plugin/PluginTabBase.cpp (right):

http://codereview.adblockplus.org/11427013/diff/8001/src/plugin/PluginTabBase...
src/plugin/PluginTabBase.cpp:86: DWORD WINAPI FilterLoader(void* thisPtrVoid)
Use LPVOID rather than void* to match ThreadProc signature? Right now you are
mixing Windows and C++ types.

http://codereview.adblockplus.org/11427013/diff/8001/src/plugin/PluginTabBase...
src/plugin/PluginTabBase.cpp:88: CPluginTabBase* thisPtr =
(CPluginTabBase*)thisPtrVoid;
Given that there really is no "this" pointer here - I would prefer having the
variable names show what they refer to. Meaning e.g. tabBase and threadParam.