Issue 10948032: Fix domain-based whitelisting

Issue 10948032: Fix domain-based whitelisting (Closed)

Created:
June 14, 2013, 2:38 p.m. by Felix Dahlke

Modified:
June 25, 2013, 10:11 a.m.

Reviewers:
Oleksandr, Wladimir Palant

Visibility:
Public.

Description

This required two changes: - Actually remove the filter - Remove whitelist caching (it's not possible to clear the cache across all processes)

Patch Set 2 : #

Created: June 17, 2013, 1:50 p.m.

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+50 lines, -95 lines)			Patch
M	src/engine/main.cpp	View		1 chunk	+7 lines, -0 lines	0 comments	Download
M	src/plugin/AdblockPlusClient.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	src/plugin/AdblockPlusClient.cpp	View		2 chunks	+15 lines, -5 lines	0 comments	Download
M	src/plugin/PluginClass.cpp	View		1 chunk	+0 lines, -4 lines	0 comments	Download
M	src/plugin/PluginClientBase.h	View		2 chunks	+0 lines, -7 lines	0 comments	Download
M	src/plugin/PluginClientBase.cpp	View	1	2 chunks	+9 lines, -49 lines	0 comments	Download
M	src/plugin/PluginSettings.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	src/plugin/PluginSettings.cpp	View	1	2 chunks	+14 lines, -26 lines	0 comments	Download
M	src/plugin/PluginTabBase.cpp	View		1 chunk	+0 lines, -1 line	0 comments	Download
M	src/plugin/PluginUserSettings.cpp	View		1 chunk	+2 lines, -3 lines	0 comments	Download
M	src/shared/Communication.h	View		1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 7

Expand All Messages | Collapse All Messages

Wladimir Palant

http://codereview.adblockplus.org/10948032/diff/2001/src/plugin/PluginClientBase.cpp File src/plugin/PluginClientBase.cpp (right): http://codereview.adblockplus.org/10948032/diff/2001/src/plugin/PluginClientBase.cpp#newcode177 src/plugin/PluginClientBase.cpp:177: CString http = url.Find('/',pos) >= 0 ? url.Tokenize(L"/", pos) ...

June 17, 2013, 9:33 a.m. (2013-06-17 09:33:56 UTC) #2

Felix Dahlke

June 17, 2013, 11:19 a.m. (2013-06-17 11:19:01 UTC) #3

http://codereview.adblockplus.org/10948032/diff/2001/src/plugin/PluginClientB...
File src/plugin/PluginClientBase.cpp (right):

http://codereview.adblockplus.org/10948032/diff/2001/src/plugin/PluginClientB...
src/plugin/PluginClientBase.cpp:177: CString http = url.Find('/',pos) >= 0 ?
url.Tokenize(L"/", pos) : L"";
On 2013/06/17 09:33:56, Wladimir Palant wrote:
> I know that this isn't your code but that variable should be called "scheme".

Done.

http://codereview.adblockplus.org/10948032/diff/2001/src/plugin/PluginClientB...
src/plugin/PluginClientBase.cpp:186: return
pluginSettings->IsWhiteListedDomain(domain);
On 2013/06/17 09:33:56, Wladimir Palant wrote:
> Retrieving all exception domains from the engine just to check whether one is
on
> the list is very wasteful. It should be better to have a special engine call
for
> IsWhiteListedDomain() and do the check there.
> 
> Note the implementation here isn't correct either. An exception rule affecting
> the current page won't necessarily have the expected form and might not even
be
> a user-dependent filter.

Yes, I agree. I frankly didn't dare to change too much here, so I only got rid
of one of two layers of unnecessary caching, the one that caused problems.

Would you be fine with postponing this though? I'd rather move on with the
release and fix this later, adding a TODO comment now.

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

http://codereview.adblockplus.org/10948032/diff/2001/src/plugin/PluginSetting...
src/plugin/PluginSettings.cpp:1283: return;
On 2013/06/17 09:33:56, Wladimir Palant wrote:
> I think that this code block is unnecessary and merely wastes time - the
engine
> won't add a filter twice.

Done.

http://codereview.adblockplus.org/10948032/diff/2001/src/plugin/PluginSetting...
src/plugin/PluginSettings.cpp:1285: s_criticalSectionLocal.Lock();
On 2013/06/17 09:33:56, Wladimir Palant wrote:
> Please remove the pointless critical section use here.

Done.

http://codereview.adblockplus.org/10948032/diff/2001/src/plugin/PluginSetting...
src/plugin/PluginSettings.cpp:1296: return;
On 2013/06/17 09:33:56, Wladimir Palant wrote:
> I think that this code block is unnecessary and merely wastes time - the
engine
> won't remove a filter twice.

Done.

http://codereview.adblockplus.org/10948032/diff/2001/src/plugin/PluginSetting...
src/plugin/PluginSettings.cpp:1298: s_criticalSectionLocal.Lock();
On 2013/06/17 09:33:56, Wladimir Palant wrote:
> Please remove the pointless critical section use here.

Done.

Oleksandr

On 2013/06/17 11:19:01, Felix H. Dahlke wrote: > http://codereview.adblockplus.org/10948032/diff/2001/src/plugin/PluginClientBase.cpp > File src/plugin/PluginClientBase.cpp (right): > > ...

June 17, 2013, 12:35 p.m. (2013-06-17 12:35:59 UTC) #4

On 2013/06/17 11:19:01, Felix H. Dahlke wrote:
>
http://codereview.adblockplus.org/10948032/diff/2001/src/plugin/PluginClientB...
> File src/plugin/PluginClientBase.cpp (right):
> 
>
http://codereview.adblockplus.org/10948032/diff/2001/src/plugin/PluginClientB...
> src/plugin/PluginClientBase.cpp:177: CString http = url.Find('/',pos) >= 0 ?
> url.Tokenize(L"/", pos) : L"";
> On 2013/06/17 09:33:56, Wladimir Palant wrote:
> > I know that this isn't your code but that variable should be called
"scheme".
> 
> Done.
> 
>
http://codereview.adblockplus.org/10948032/diff/2001/src/plugin/PluginClientB...
> src/plugin/PluginClientBase.cpp:186: return
> pluginSettings->IsWhiteListedDomain(domain);
> On 2013/06/17 09:33:56, Wladimir Palant wrote:
> > Retrieving all exception domains from the engine just to check whether one
is
> on
> > the list is very wasteful. It should be better to have a special engine call
> for
> > IsWhiteListedDomain() and do the check there.
> > 
> > Note the implementation here isn't correct either. An exception rule
affecting
> > the current page won't necessarily have the expected form and might not even
> be
> > a user-dependent filter.
> 
> Yes, I agree. I frankly didn't dare to change too much here, so I only got rid
> of one of two layers of unnecessary caching, the one that caused problems.
> 
> Would you be fine with postponing this though? I'd rather move on with the
> release and fix this later, adding a TODO comment now.
> 
>
http://codereview.adblockplus.org/10948032/diff/2001/src/plugin/PluginSetting...
> File src/plugin/PluginSettings.cpp (right):
> 
>
http://codereview.adblockplus.org/10948032/diff/2001/src/plugin/PluginSetting...
> src/plugin/PluginSettings.cpp:1283: return;
> On 2013/06/17 09:33:56, Wladimir Palant wrote:
> > I think that this code block is unnecessary and merely wastes time - the
> engine
> > won't add a filter twice.
> 
> Done.
> 
>
http://codereview.adblockplus.org/10948032/diff/2001/src/plugin/PluginSetting...
> src/plugin/PluginSettings.cpp:1285: s_criticalSectionLocal.Lock();
> On 2013/06/17 09:33:56, Wladimir Palant wrote:
> > Please remove the pointless critical section use here.
> 
> Done.
> 
>
http://codereview.adblockplus.org/10948032/diff/2001/src/plugin/PluginSetting...
> src/plugin/PluginSettings.cpp:1296: return;
> On 2013/06/17 09:33:56, Wladimir Palant wrote:
> > I think that this code block is unnecessary and merely wastes time - the
> engine
> > won't remove a filter twice.
> 
> Done.
> 
>
http://codereview.adblockplus.org/10948032/diff/2001/src/plugin/PluginSetting...
> src/plugin/PluginSettings.cpp:1298: s_criticalSectionLocal.Lock();
> On 2013/06/17 09:33:56, Wladimir Palant wrote:
> > Please remove the pointless critical section use here.
> 
> Done.

LGTM

Felix Dahlke

Uploaded a new patch set addressing the current comments. Since this seems to be generally ...

June 17, 2013, 1:51 p.m. (2013-06-17 13:51:56 UTC) #5

Wladimir Palant

LGTM even though I would prefer that TODO comment to be addressed sooner rather than ...

June 18, 2013, 5:23 a.m. (2013-06-18 05:23:07 UTC) #6

Felix Dahlke

June 25, 2013, 9:47 a.m. (2013-06-25 09:47:49 UTC) #7

Closing this review, there's a new one for my latest changes:
http://codereview.adblockplus.org/10948032/

Expand All Messages | Collapse All Messages