Fix domain whitelisting and remove unused code

My original intention was to just remove the unused code I didn't remove here:

http://codereview.adblockplus.org/10948032/

However, domain-based whitelisting was currently broken and implemented differently to ABP, so I changed that.

Please note my comments.

[Felix Dahlke, 2013-06-25 09:46:58]

http://codereview.adblockplus.org/10955040/diff/1/src/plugin/AdblockPlusClien...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/10955040/diff/1/src/plugin/AdblockPlusClien...
src/plugin/AdblockPlusClient.cpp:204: bool
CAdblockPlusClient::IsUrlWhiteListed(const CString& url)
Do you think I should change this to accept an std::string? Most methods of
AdblockPlusClient want one.

http://codereview.adblockplus.org/10955040/diff/1/src/plugin/PluginClientBase...
File src/plugin/PluginClientBase.cpp (left):

http://codereview.adblockplus.org/10955040/diff/1/src/plugin/PluginClientBase...
src/plugin/PluginClientBase.cpp:175: if (scheme == L"res:" || scheme ==
L"file:")
Do you think excluding some schemes is still necessary? I figured leaving it to
Matches() should do.

[Felix Dahlke, 2013-06-25 10:21:51]

On 2013/06/25 09:46:58, Felix H. Dahlke wrote:
>
http://codereview.adblockplus.org/10955040/diff/1/src/plugin/AdblockPlusClien...
> File src/plugin/AdblockPlusClient.cpp (right):
> 
>
http://codereview.adblockplus.org/10955040/diff/1/src/plugin/AdblockPlusClien...
> src/plugin/AdblockPlusClient.cpp:204: bool
> CAdblockPlusClient::IsUrlWhiteListed(const CString& url)
> Do you think I should change this to accept an std::string? Most methods of
> AdblockPlusClient want one.
> 
>
http://codereview.adblockplus.org/10955040/diff/1/src/plugin/PluginClientBase...
> File src/plugin/PluginClientBase.cpp (left):
> 
>
http://codereview.adblockplus.org/10955040/diff/1/src/plugin/PluginClientBase...
> src/plugin/PluginClientBase.cpp:175: if (scheme == L"res:" || scheme ==
> L"file:")
> Do you think excluding some schemes is still necessary? I figured leaving it
to
> Matches() should do.

Decided to also move ExtractDomain to PluginTabBase - it's not used anywhere
else now.

[Wladimir Palant, 2013-06-25 11:55:13]

LGTM with the nits addressed.

http://codereview.adblockplus.org/10955040/diff/4001/src/engine/main.cpp
File src/engine/main.cpp (right):

http://codereview.adblockplus.org/10955040/diff/4001/src/engine/main.cpp#newc...
src/engine/main.cpp:127: AdblockPlus::FilterPtr match =
filterEngine->Matches(url, "DOCUMENT", "");
Please use url as the document URL parameter as well here rather than an empty
string (important for $third-party to be handled correctly). Note: the
assumption here is that we will only use that check for top-level documents,
documentUrl will typically be a different one for frames.

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

http://codereview.adblockplus.org/10955040/diff/4001/src/plugin/PluginTabBase...
src/plugin/PluginTabBase.cpp:270: CString domain;
This variable is unused.

[Wladimir Palant, 2013-06-25 12:00:40]

http://codereview.adblockplus.org/10955040/diff/1/src/plugin/AdblockPlusClien...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/10955040/diff/1/src/plugin/AdblockPlusClien...
src/plugin/AdblockPlusClient.cpp:204: bool
CAdblockPlusClient::IsUrlWhiteListed(const CString& url)
On 2013/06/25 09:46:58, Felix H. Dahlke wrote:
> Do you think I should change this to accept an std::string? Most methods of
> AdblockPlusClient want one.

Yes, that would be nice - we should be gradually moving away from CString.

http://codereview.adblockplus.org/10955040/diff/1/src/plugin/PluginClientBase...
File src/plugin/PluginClientBase.cpp (left):

http://codereview.adblockplus.org/10955040/diff/1/src/plugin/PluginClientBase...
src/plugin/PluginClientBase.cpp:175: if (scheme == L"res:" || scheme ==
L"file:")
On 2013/06/25 09:46:58, Felix H. Dahlke wrote:
> Do you think excluding some schemes is still necessary? I figured leaving it
to
> Matches() should do.

Yes, leaving it to Matches() is more consistent to what we do in Firefox. There
is a list of schemes that we never block, file: being one of them. Still, a
document loaded from file: isn't immediately whitelisted, requests to http
resources made from it can still be blocked. This used to be different but
people were complaining because they saved web pages locally and suddenly
started seeing ads.

[Oleksandr, 2013-06-25 12:13:58]

LGTM

http://codereview.adblockplus.org/10955040/diff/1/src/plugin/PluginClientBase...
File src/plugin/PluginClientBase.cpp (left):

http://codereview.adblockplus.org/10955040/diff/1/src/plugin/PluginClientBase...
src/plugin/PluginClientBase.cpp:175: if (scheme == L"res:" || scheme ==
L"file:")
We should do as little as possible with these. Especially with res:// links -
those show IE's UI, and IE usually behaves very freely with those, causing the
most weird errors. Since this is IE specific, and Matches isn't IE specific, and
also since Matches uses IPC, which is a heavy operation, I would suggest against
excluding this.  
On 2013/06/25 09:46:58, Felix H. Dahlke wrote:
> Do you think excluding some schemes is still necessary? I figured leaving it
to
> Matches() should do.

[Felix Dahlke, 2013-06-25 14:20:05]

Uploaded a new patch set addressing all the issues.

[Wladimir Palant, 2013-06-25 18:03:58]

http://codereview.adblockplus.org/10955040/diff/12001/src/plugin/AdblockPlusC...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/10955040/diff/12001/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:204: bool
CAdblockPlusClient::IsWhitelistedUrl(const std::string& url)
I would actually expect to see std::wstring here - the conversion to UTF-8 is an
implementation detail of libadblockplus.

http://codereview.adblockplus.org/10955040/diff/12001/src/plugin/PluginTabBas...
File src/plugin/PluginTabBase.cpp (right):

http://codereview.adblockplus.org/10955040/diff/12001/src/plugin/PluginTabBas...
src/plugin/PluginTabBase.cpp:271: CString domain;
Still unused, feel free to remove while you are at it.

[Felix Dahlke, 2013-06-26 09:32:55]

http://codereview.adblockplus.org/10955040/diff/12001/src/plugin/AdblockPlusC...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/10955040/diff/12001/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:204: bool
CAdblockPlusClient::IsWhitelistedUrl(const std::string& url)
On 2013/06/25 18:03:58, Wladimir Palant wrote:
> I would actually expect to see std::wstring here - the conversion to UTF-8 is
an
> implementation detail of libadblockplus.

Yes, that seems to make more sense, since the plugin works with UTF-16
internally. However, all the other methods have std::string in their signatures,
so I opted for that. I suppose it makes sense to change all of that to
std::wstring, would rather to that in a separate commit though.

http://codereview.adblockplus.org/10955040/diff/12001/src/plugin/PluginTabBas...
File src/plugin/PluginTabBase.cpp (right):

http://codereview.adblockplus.org/10955040/diff/12001/src/plugin/PluginTabBas...
src/plugin/PluginTabBase.cpp:271: CString domain;
On 2013/06/25 18:03:58, Wladimir Palant wrote:
> Still unused, feel free to remove while you are at it.

Done.

[Wladimir Palant, 2013-06-27 05:55:02]

LGTM

http://codereview.adblockplus.org/10955040/diff/12001/src/plugin/AdblockPlusC...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/10955040/diff/12001/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.cpp:204: bool
CAdblockPlusClient::IsWhitelistedUrl(const std::string& url)
Yes, the other methods seem wrong as well. I'm fine with changing it all later
then.