Issue 1183 - Fix Acceptable Ads hiding

Issue 1183 - Fix Acceptable Ads hiding

[Oleksandr, 2014-08-08 15:52:20]

Looks like this does the trick

[Felix Dahlke, 2014-08-08 16:11:35]

Awesome that it works! Usual nagging below.

http://codereview.adblockplus.org/4784041706389504/diff/5629499534213120/src/...
File src/engine/Main.cpp (right):

http://codereview.adblockplus.org/4784041706389504/diff/5629499534213120/src/...
src/engine/Main.cpp:99: case Communication::PROC_HIDING_ENABLED_ON_DOMAIN:
Since this is conceptually very similar to PROC_IS_WHITELISTED_URL, I propose
this:

1. Rename it to "PROC_IS_ELEMHIDE_WHITELISTED_ON_URL" and move it below that
one.
2. Rename "documentUrl" to "url".

http://codereview.adblockplus.org/4784041706389504/diff/5629499534213120/src/...
src/engine/Main.cpp:101: std::string type;
Unused, isn't it?

http://codereview.adblockplus.org/4784041706389504/diff/5629499534213120/src/...
src/engine/Main.cpp:105: response << (!filter);
Superfluous parentheses.

More importantly, this only checks if there was a filter, not that it was
actually an element hiding filter. Should basically work the same way as
PROC_IS_WHITELISTED_URL (logic negated of course):

response << (match && match->GetType() == AdblockPlus::Filter::TYPE_EXCEPTION);

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

http://codereview.adblockplus.org/4784041706389504/diff/5629499534213120/src/...
src/plugin/PluginTabBase.cpp:188: if
(CPluginClient::GetInstance()->HidingEnabledOnDomain(std::wstring(GetDocumentUrl())))
Oh noes, the arrow pattern! :D Can you merge this with the statement above and
good old &&?

[Felix Dahlke, 2014-08-08 16:41:45]

http://codereview.adblockplus.org/4784041706389504/diff/5657382461898752/src/...
File src/engine/Main.cpp (right):

http://codereview.adblockplus.org/4784041706389504/diff/5657382461898752/src/...
src/engine/Main.cpp:201: std::string documentUrl;
Would make more sense to rename this to "url" IMO, as I suggested.

http://codereview.adblockplus.org/4784041706389504/diff/5657382461898752/src/...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/4784041706389504/diff/5657382461898752/src/...
src/plugin/AdblockPlusClient.cpp:284: bool
CAdblockPlusClient::IsHidingWhitelistedOnDomain(const std::wstring& domain)
s/Hiding/Elemhide?

If you rename documentUrl in Main.cpp, we should rename s/domain/url/ here too.

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

http://codereview.adblockplus.org/4784041706389504/diff/5657382461898752/src/...
src/plugin/PluginTabBase.cpp:187: if
(!client->IsWhitelistedUrl(std::wstring(GetDocumentUrl())) &&
!client->IsHidingWhitelistedOnDomain(std::wstring(GetDocumentUrl())))
std::wstring(GetDocumentUrl()) would like to be a temp, I think.

[Felix Dahlke, 2014-08-08 16:57:38]

Beautiful :D LGTM