Issue 1104 - Cannot uncheck Disable on website option in tool bar

[Eric, 2015-03-04 13:55:38]

Also, the description here needs a proper commit message.

http://codereview.adblockplus.org/5115380229996544/diff/5685265389584384/src/...
File src/plugin/AdblockPlusClient.h (right):

http://codereview.adblockplus.org/5115380229996544/diff/5685265389584384/src/...
src/plugin/AdblockPlusClient.h:74: struct ExceptionFilter
I am generally in favor of adding simple classes like this one to promote good
type usage, both for type safety and implicit documentation.

When I first saw this class, though, I wasn't sure we needed it. In the present
change set, it would be simpler just to use a string. What I saw is that we need
an implicit conversion to 'bool' to satisfy the many places in the code where
this function is called. (That there are so many locations seems itself like the
symptom of a design problem, but that's for another discussion elsewhere.) Since
this is not obvious, I'd say that 'operator bool' deserves a short comment to
that effect.

The class name is also slightly off. This class isn't for any exception filter,
but just those for white listing. I think the name 'WhitelistFilter' would work
better.

[Oleksandr, 2015-03-05 07:35:27]

http://codereview.adblockplus.org/5115380229996544/diff/5685265389584384/src/...
File src/plugin/AdblockPlusClient.h (right):

http://codereview.adblockplus.org/5115380229996544/diff/5685265389584384/src/...
src/plugin/AdblockPlusClient.h:82: ExceptionFilter IsWhitelistedUrl(const
std::wstring& url);
How about:

bool IsWhitelistedUrl(const std::wstring& url, std::wstring& filter)

but also keeping the old overload:

bool IsWhitelistedUrl(const std::wstring& url)

No need for a highly specialized ExceptionFilter class then.

[sergei, 2015-03-05 11:32:08]

On 2015/03/05 07:35:27, Oleksandr wrote:
>
http://codereview.adblockplus.org/5115380229996544/diff/5685265389584384/src/...
> File src/plugin/AdblockPlusClient.h (right):
> 
>
http://codereview.adblockplus.org/5115380229996544/diff/5685265389584384/src/...
> src/plugin/AdblockPlusClient.h:82: ExceptionFilter IsWhitelistedUrl(const
> std::wstring& url);
> How about:
> 
> bool IsWhitelistedUrl(const std::wstring& url, std::wstring& filter)
> 
> but also keeping the old overload:
> 
> bool IsWhitelistedUrl(const std::wstring& url)
> 
> No need for a highly specialized ExceptionFilter class then.

changed

[Oleksandr, 2015-03-11 13:10:38]

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

http://codereview.adblockplus.org/5115380229996544/diff/5766466041282560/src/...
src/plugin/PluginClass.cpp:1276: if (client->IsWhitelistedUrl(urlString,
filterText))
Felix has pointed out that we are actually leaking abstraction here. How about
leaving IsWhitelistedUrl as is, and instead adding
CPluginSettings:RemoveWhitelistedUrl that would retrieve the whitelisting filter
inside and remove it

[sergei, 2015-03-11 13:16:06]

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

http://codereview.adblockplus.org/5115380229996544/diff/5766466041282560/src/...
src/plugin/PluginClass.cpp:1276: if (client->IsWhitelistedUrl(urlString,
filterText))
On 2015/03/11 13:10:38, Oleksandr wrote:
> Felix has pointed out that we are actually leaking abstraction here. How about
> leaving IsWhitelistedUrl as is, and instead adding
> CPluginSettings:RemoveWhitelistedUrl that would retrieve the whitelisting
filter
> inside and remove it

Has Felix seen the previous approach, which is much far ahead?

[Oleksandr, 2015-03-12 04:45:15]

On 2015/03/11 13:16:06, sergei wrote:
>
http://codereview.adblockplus.org/5115380229996544/diff/5766466041282560/src/...
> File src/plugin/PluginClass.cpp (right):
> 
>
http://codereview.adblockplus.org/5115380229996544/diff/5766466041282560/src/...
> src/plugin/PluginClass.cpp:1276: if (client->IsWhitelistedUrl(urlString,
> filterText))
> On 2015/03/11 13:10:38, Oleksandr wrote:
> > Felix has pointed out that we are actually leaking abstraction here. How
about
> > leaving IsWhitelistedUrl as is, and instead adding
> > CPluginSettings:RemoveWhitelistedUrl that would retrieve the whitelisting
> filter
> > inside and remove it
> 
> Has Felix seen the previous approach, which is much far ahead?

Yes. I am not sure what you're asking though. We still should not wait for ABP
core to change, just not modify the IsWhitelistedUrl to leak abstraction. The
unwhitelisting (blacklisting?) functionality would be better off in a dedicated
function, so it is just better to move it there.

[sergei, 2015-03-12 09:13:20]

On 2015/03/12 04:45:15, Oleksandr wrote:
> On 2015/03/11 13:16:06, sergei wrote:
> > Has Felix seen the previous approach, which is much far ahead?
> 
> Yes.
I've added Felix in CC.

> I am not sure what you're asking though. We still should not wait for ABP
> core to change
I'm also not sure what the wait is you are talking about. The change in that
"codereview fixes the
case described in #1104, does not touch any UI and does not depend on #2070".

> , just not modify the IsWhitelistedUrl to leak abstraction. The
> unwhitelisting (blacklisting?) functionality would be better off in a
dedicated
> function, so it is just better to move it there.
It still looks like a temporary hack:
 - we don't need to make addition round trips between the client and server to
toggle whitelisting on the web site.
 - the logic should be in the core and it would be better to move it into the
engine, closer to the core.
 - it still requires to add additional "PROC_" to retrieve the whitelisting
filter, which likely won't be needed later as well as the amount of the changes
is similar
 - that codereview also addresses the prefix "www." what we do on the other
platforms
 - that codereview also addresses the cases when the user entered the full URL
on the settings page.
I don't mind to split that codereview into several codereviews but I don't
understand why we need to make a temporary hacks instead of approaching to the
goal.

[Oleksandr, 2015-03-12 16:52:19]

1. Both this codereview and http://codereview.adblockplus.org/5081266177179648/
are temporary hacks until we get functionality in core. I would prefer this,
since it's is far less obtrusive.
2. There is a problem with this codereview, which, admittedly, I suggested. It
is introducing IsWhitelistedUrl which is returning a filter for some reason. A
better way (still a hack) would be to have a function like 
std::string GetWhitelistingFilter(const std::wstring url)
instead of the current overload of IsWhitelistedUrl(const std::wstring url,
std::string& filter). Nothing more, really.

[sergei, 2015-03-13 17:04:06]

On 2015/03/12 16:52:19, Oleksandr wrote:
> 1. Both this codereview and
http://codereview.adblockplus.org/5081266177179648/
> are temporary hacks until we get functionality in core. I would prefer this,
> since it's is far less obtrusive.
> 2. There is a problem with this codereview, which, admittedly, I suggested. It
> is introducing IsWhitelistedUrl which is returning a filter for some reason. A
> better way (still a hack) would be to have a function like 
> std::string GetWhitelistingFilter(const std::wstring url)
> instead of the current overload of IsWhitelistedUrl(const std::wstring url,
> std::string& filter). Nothing more, really.

I've renamed it.

[Oleksandr, 2015-03-23 01:14:36]

LGTM

[Eric, 2015-03-23 13:43:52]

LGTM