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

Issue 11427013: Appear fast for IE (Closed)

Created:
Aug. 7, 2013, 5:44 a.m. by Oleksandr
Modified:
Aug. 13, 2013, 9:41 a.m.
Visibility:
Public.

Description

Appear fast for IE

Patch Set 1 #

Total comments: 7

Patch Set 2 : Comments addressed #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+29 lines, -18 lines) Patch
M src/plugin/AdblockPlusClient.cpp View 1 1 chunk +1 line, -1 line 0 comments Download
M src/plugin/Config.h View 1 1 chunk +2 lines, -0 lines 1 comment Download
M src/plugin/PluginClass.cpp View 1 3 chunks +9 lines, -9 lines 0 comments Download
M src/plugin/PluginDomTraverserBase.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M src/plugin/PluginFilter.h View 1 chunk +2 lines, -0 lines 0 comments Download
M src/plugin/PluginSettings.h View 1 1 chunk +1 line, -1 line 0 comments Download
M src/plugin/PluginSettings.cpp View 1 2 chunks +3 lines, -5 lines 1 comment Download
M src/plugin/PluginTabBase.cpp View 1 3 chunks +10 lines, -1 line 2 comments Download
M src/shared/Utils.h View 1 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 6
Oleksandr
The idea here is to remove engine operations from the threads that IE measures. I ...
Aug. 7, 2013, 5:50 a.m. (2013-08-07 05:50:52 UTC) #1
Felix Dahlke
Mostly the traditional nit picking. So the gist is that we have to load the ...
Aug. 7, 2013, 9:34 a.m. (2013-08-07 09:34:49 UTC) #2
Felix Dahlke
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.cpp#newcode105 src/plugin/PluginTabBase.cpp:105: CreateThread(NULL, NULL, &FilterLoader, this, NULL, NULL); How I see ...
Aug. 7, 2013, 1:23 p.m. (2013-08-07 13:23:03 UTC) #3
Oleksandr
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.cpp#newcode105 src/plugin/PluginTabBase.cpp:105: CreateThread(NULL, NULL, &FilterLoader, this, NULL, NULL); This is not ...
Aug. 7, 2013, 4:07 p.m. (2013-08-07 16:07:40 UTC) #4
Felix Dahlke
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#newcode152 src/plugin/Config.h:152: #define TIMEOUT_VALUE 10000 How about ENGINE_STARTUP_TIMEOUT?
Aug. 8, 2013, 8:15 a.m. (2013-08-08 08:15:44 UTC) #5
Wladimir Palant
Aug. 13, 2013, 9:41 a.m. (2013-08-13 09:41:02 UTC) #6
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.

Powered by Google App Engine
This is Rietveld