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

The trouble is that we are manually crafting the filter text from the current URL. As result for the URL http://www.fotor.com/some-path we get '@@||www.fotor.com^$document' and we cannot remove filter '@@||fotor.com^$document' because it's different. The solution to merely always remove 'www.' does not fit either because it's possible to create '@@||www.fotor.com^$document' filter by adding 'www.fotor.com' via settings.

There is also another way (BTW, which is used by our extension for Google Chrome) to find the exception filter and remove it, which I propose to have now. I guess it's closer to the user's intention, he does not care whether the web site is whitelisted by domain or by using another exception filter.
# Actually I would like to have such functionality in the core to avoid any different behaviour on different platforms but it's not easy/fast process to make changes in the core and pull them here.
# See https://issues.adblockplus.org/ticket/2031

BTW, Chrome removes "www." prefix when the user disables ABP on the particular web site, so I've added it as well. As well as I propose to do it even when the domain is being added through the settings page. So, if user added www.some-domain.com and visit some-domain.com it's whitelisted. Even more, here it's possible to enter http://www.some-domain.com/some-path (we had already received some complains from the users that they are doing it and it does not work) and get some-domain.com to be whitelisted.

Additional technical details:
- PROC_ADD_FILTER and PROC_REMOVE_FILTER are renamed to PROC_ADD_WHITELSITED_DOMAIN and PROC_REMOVE_WHITELSITED_DOMAIN, respectively because they don't expect a filter text any more but expect the actual domain.
- PROC_TOGGLE_WHITELISTING is introduced to reduce the number of trips to the engine as well as to move the logic closer to the libadblockplus.
# I hope eventually such functionality will be in the core and it seems quite straightforward to get rid of it in favour of the using API of libadblockplus.
- [Utils] ReplaceString is a template function, so we can use it either with std::wstring or std::string.
- [Utils] move ASCIIStringToLower to Utils, because we need it also in the engine.
# Again, core library normalizes the URL and, as far as I know, supports unicode, so it's yet one reason to share such logic among all platforms rather than implement it in each extension.
- [Utils] add BeginsWith. I've seen we use it in some other parts, however I've not touched them because they are not relevant to the current change.

[Eric, 2015-02-26 16:14:10]

This first round of comments is only on the utility functions. I'm fine with
moving them into the shared library, but they need unit tests when they land
there. I suggest splitting off these utility changes into a separate commit.
There's shouldn't be much to talk about with these changes.

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

http://codereview.adblockplus.org/5081266177179648/diff/5629499534213120/src/...
src/engine/Main.cpp:51: if (BeginsWith<std::string>(host, "https://") ||
BeginsWith<std::string>(host, "http://"))
Explicit type specification for template function.

Beginning with "http" is probably adequate for this test. We know that the page
has loaded, thus the URL is valid, and so if it begins with "http" it will begin
with one of the two strings above.

It's probably easier to extract the initial substring of four characters in the
previous line and then test for equality.

http://codereview.adblockplus.org/5081266177179648/diff/5629499534213120/src/...
src/engine/Main.cpp:56: ReplaceString<std::string>(host, "www.", "");
Explicit type for template function.

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

http://codereview.adblockplus.org/5081266177179648/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:1296: ReplaceString<std::wstring>(ctext, L"?1?",
client->GetHostFromUrl(url));
Explicit type in template function

http://codereview.adblockplus.org/5081266177179648/diff/5629499534213120/src/...
File src/shared/Utils.h (right):

http://codereview.adblockplus.org/5081266177179648/diff/5629499534213120/src/...
src/shared/Utils.h:1: /*
We need unit tests for the new utility functions.

http://codereview.adblockplus.org/5081266177179648/diff/5629499534213120/src/...
src/shared/Utils.h:61: void ReplaceString(T& input, const T& placeholder, const
T& replacement)
We want to be able to use type inference for this function. Simply add template
arguments for 'placeholder' and 'replacement'.

http://codereview.adblockplus.org/5081266177179648/diff/5629499534213120/src/...
src/shared/Utils.h:71: T ASCIIStringToLower(const T& text)
We could take the opportunity to rename this "AsciiToLower". Having the word
"string" within the name is redundant.

http://codereview.adblockplus.org/5081266177179648/diff/5629499534213120/src/...
src/shared/Utils.h:87: }
Here are the unit tests I had previously, adapted for the current function name.
I had only needed to test the constant prefix version previous. These tests
would need to be doubled with a string constructor in the second argument.

TEST( BeginsWith, emptyVsEmpty )
{
  ASSERT_TRUE( BeginsWith( L"", L"" ) );
}

TEST( BeginsWith, emptyVsNonempty )
{
  ASSERT_FALSE( BeginsWith( L"", L"foo" ) );
}

TEST( BeginsWith, NonemptyVsEmpty )
{
  ASSERT_TRUE( BeginsWith( L"foo", L"" ) );
}

TEST( BeginsWith, DoesBegin )
{
  ASSERT_TRUE( BeginsWith( L"foo", L"f" ) );
}

TEST( BeginsWith, DoesNotBegin )
{
  ASSERT_FALSE( BeginsWith( L"foo", L"g" ) );
}

http://codereview.adblockplus.org/5081266177179648/diff/5629499534213120/src/...
src/shared/Utils.h:87: }
I wrote a version of this last summer in the giant-review for strings that was
rejected. We need this function to replace a CString member function.

This function was (essentially) one of the two. The other was for string
constants. In generic form, it looks like this:

template<typename T, size_t N>
bool BeginsWith( const T& s, const T::value_type (&prefix)[N] )
{
  return s.compare(0, N-1, prefix) == 0;
}

Having this other version has a few benefits. It automatically does type
inference correctly against string constants. It's more efficient, too; it saves
a string constructor and a call to length(), neither of which is necessary for
string constants.

[Eric, 2015-02-26 18:16:00]

-- This change set is fixing #1104 by deriving the domain name in the engine,
but it's not the same as that displayed in the popup menu. That's a defect.
Given the need for consistency between what the user sees and what the engine
does, it would be better to derive the domain on the plugin side. It would be
possible to do it the other way, but that means a channel from the engine to the
plugin, which is problematic. Alternately, it would be possible to derive the
domain name in the same way in both the plugin and the engine separately.

-- Accomplishing the task of deriving the domain on the plugin side is easier
said than done, but only because of poor code quality for the UI. Part of the
problem is that the popup menu is not implemented as a separate entity, so
tracking down all the data flows is harder than it should be. If I were doing
this fix myself, I'd take the opportunity to factor out the popup menu code into
its own class (and into a new file in src/plugin/ui/). As I discuss below, this
makes the fix for #1104 pretty clean.

-- At the very least CPluginClass::SetMenuBar should take a domain and not a URL
as its second argument, so that we're not mixing block/whitelist logic in with
display code. This function is only called from CPluginClass::CreatePluginMenu,
whose argument should also be a domain and not a full URL. CreatePluginMenu is
called from two locations. One is from the override to IOleCommandTarget::Exec
and the other from CPluginClass::PaneWindowProc. In both cases these are the
moral equivalent of global functions; in both cases they're using the return
value from GetDocumentUrl() to determine context (since they don't have a proper
'this' pointer to do so). The upshot of this is that we need to start from the
document URL in both cases (unless we want to incur an even more major rewrite,
which is impractical).

-- On the other hand, we're sending the url/domain to the engine in
DisplayPluginMenu under command identifier ID_MENU_DISABLE_ON_SITE. Now both of
the places that call CreatePluginMenu also call DisplayPluginMenu, and these are
the only two places that call DisplayPluginMenu. The only thing they do
differently is (1) to calculate a point at which to display the popup and (2)
specify how to display the popup with respect to that point. 

-- With only a small amount of rewriting, CreatePluginMenu and DisplayPluginMenu
could be combined. In that case, we could derive the domain name once, hold it
in a local variable, and use it both to display the menu and to send a command
to the engine. Alternately (with fewer code changes), we could just derive the
domain name twice with a call to the same function.

-- Exactly analogous to the domain, we want to ensure consistency between (1)
whether the user sees a checked or unchecked popup menu and (2) whether the
domain is enabled or disabled. I dislike the "toggle" interface because it's
computing this boolean in both the engine and plugin, which is not necessary.
Indeed, I'm not even convinced that they're deriving the same value; it's
certainly not obvious that they are. I'm also not convinced that what's already
there is even right. There's a difference between "whitelisted through any
means", "whitelisted by subscription", and "whitelisted explicitly by user".
However it works, though, it would be best to use the same value that we use for
UI presentation as to select which command (add or remove) to send to the
engine.

-- Manipulating filter text for a given domain is properly done on the engine
side, as it is here. That part doesn't need to change, although the functions
should be call 'AddWhitelistedDomain' and 'RemoveWhitelistedDomain' (not with
'...Url' as now).

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

http://codereview.adblockplus.org/5081266177179648/diff/5629499534213120/src/...
src/engine/Main.cpp:56: ReplaceString<std::string>(host, "www.", "");
More importantly, this algorithm is not correct. It will fail on the string
"foo.www.bar.com", for example. That's admittedly unlikely, but still. It will
also fail on "web.bar.com" or "profile.bar.com". The right thing to do is
probably to take the final two domain components.

Even more importantly, this operation needs to be done in the plugin. We need to
display the domain that's about to be blocked/unblocked in the popup menu. This
needs to be the same as what happens on the inside. 

It would be better to derive the relevant name in the plugin and change the
interface to the engine to pass a domain name rather than a URL.

http://codereview.adblockplus.org/5081266177179648/diff/5629499534213120/src/...
src/engine/Main.cpp:248: {
As a point of code clarity, if we have a function AddWhitelistedUrl, we should
also have RemoveWhitelistedUrl. It makes it easier to check that the operations
are actually opposites.

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

http://codereview.adblockplus.org/5081266177179648/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:1296: ReplaceString<std::wstring>(ctext, L"?1?",
client->GetHostFromUrl(url));
We need to display the same name in the popup menu as we're actually
manipulating in the white list. Using GetHostFromUrl() here is not the name
that's being derived in the engine.

http://codereview.adblockplus.org/5081266177179648/diff/5629499534213120/src/...
File src/shared/Communication.h (right):

http://codereview.adblockplus.org/5081266177179648/diff/5629499534213120/src/...
src/shared/Communication.h:46: PROC_ADD_WHITELSITED_DOMAIN,
Typo: "LIST", not "LSIT", here and on the next line.

[Oleksandr, 2015-02-27 10:02:12]

I don't think this is a place to refactor the context menu. I do agree we need
to show the actual domain we are working with. 

What I would do is just add one more return value from PROC_IS_WHITELISTED - the
filter text. That way we will have exactly the domain we are working with to
display in the context menu. This will also allow us to use
RemoveFilter/AddFilter as before. We can just add "www." removal for consistency
with other ABP versions.

I don't think there's any need in PROC_TOGGLE_WHITELISTING at the moment.

[Eric, 2015-02-28 17:28:59]

On 2015/02/27 10:02:12, Oleksandr wrote:
> I don't think this is a place to refactor the context menu.

It could be here, but it doesn't need to be here.

It does need to be done eventually, though.
See the answer here:
https://social.msdn.microsoft.com/forums/ie/en-us/9d52be01-b53b-4c82-829b-e07...
and the blog post (by the same person):
http://jeffcode.blogspot.com/2008/06/your-bho-and-your-activex-control-cant.html
The issue impinges on the problems with SetSite I've been concerned about.

What we can do here, however, short of a full refactoring, is to combine
'CreatePluginMenu' and 'DisplayPluginMenu'. The argument 'nToolbarCmdID' is not
used in DisplayPluginMenu. The HMENU return from 'Create...' that's passed as
the first argument to 'DisplayPluginMenu' is not used in either case. Combining
these two functions is thus a modest amount of code rearrangement. After that,
both of the variables we want to be consistently displayed to the user are local
to the same block.

This change is also a good precursor to a full refactoring, so there's no
regression to make this change.

[sergei, 2015-03-02 13:01:03]

I agree that it's better to have Utils refactoring in another commit, so I will
create it and I will address and comment the points in that code review.

1.
> This change set is fixing #1104 by deriving the domain name in the engine,
> but it's not the same as that displayed in the popup menu. That's a defect.
> Given the need for consistency between what the user sees and what the engine
> does, it would be better to derive the domain on the plugin side. It would be
> possible to do it the other way, but that means a channel from the engine to
the
> plugin, which is problematic. Alternately, it would be possible to derive the
> domain name in the same way in both the plugin and the engine separately.

2.
> At the very least CPluginClass::SetMenuBar should take a domain and not a URL
> as its second argument, so that we're not mixing block/whitelist logic in with
> display code. This function is only called from
CPluginClass::CreatePluginMenu,
> whose argument should also be a domain and not a full URL. CreatePluginMenu is
> called from two locations. One is from the override to IOleCommandTarget::Exec
> and the other from CPluginClass::PaneWindowProc. In both cases these are the
> moral equivalent of global functions; in both cases they're using the return
> value from GetDocumentUrl() to determine context (since they don't have a
proper
> 'this' pointer to do so). The upshot of this is that we need to start from the
> document URL in both cases (unless we want to incur an even more major
rewrite,
> which is impractical).

> 1, 2
That's correct, but let's firstly wait for the decision regarding
https://issues.adblockplus.org/ticket/2070.

3.
> On the other hand, we're sending the url/domain to the engine in
> DisplayPluginMenu under command identifier ID_MENU_DISABLE_ON_SITE. Now both
of
> the places that call CreatePluginMenu also call DisplayPluginMenu, and these
are
> the only two places that call DisplayPluginMenu. The only thing they do
> differently is (1) to calculate a point at which to display the popup and (2)
> specify how to display the popup with respect to that point.

4.
> With only a small amount of rewriting, CreatePluginMenu and DisplayPluginMenu
> could be combined. In that case, we could derive the domain name once, hold it
> in a local variable, and use it both to display the menu and to send a command
> to the engine. Alternately (with fewer code changes), we could just derive the
> domain name twice with a call to the same function.

> 3
Yes, if we need it (see #2070), then it's better to retrieve it only once when
the menu is created and send this value to the engine to disable/enable ABP on
the web site.

5.
> Accomplishing the task of deriving the domain on the plugin side is easier
> said than done, but only because of poor code quality for the UI. Part of the
> problem is that the popup menu is not implemented as a separate entity, so
> tracking down all the data flows is harder than it should be. If I were doing
> this fix myself, I'd take the opportunity to factor out the popup menu code
into
> its own class (and into a new file in src/plugin/ui/). As I discuss below,
this
> makes the fix for #1104 pretty clean.

> 3, 4, 5
It's too much for this fix, this refactoring should be as another issue.

6.
> Exactly analogous to the domain, we want to ensure consistency between (1)
> whether the user sees a checked or unchecked popup menu and (2) whether the
> domain is enabled or disabled. I dislike the "toggle" interface because it's
> computing this boolean in both the engine and plugin, which is not necessary.
> Indeed, I'm not even convinced that they're deriving the same value; it's
> certainly not obvious that they are. I'm also not convinced that what's
already
> there is even right. There's a difference between "whitelisted through any
> means", "whitelisted by subscription", and "whitelisted explicitly by user".
> However it works, though, it would be best to use the same value that we use
for
> UI presentation as to select which command (add or remove) to send to the
> engine.

> 6.
Because of it I've created #2031 and #2070, firstly, I would like to be sure
what we need and then we can do it. I hope that we won't need a domain in the
menu item so we need to merely get from the engine whether it's enabled or not.
Having this information it looks reasonable to have disableABPOn(currentUrl) and
enableABPOn(currentUrl), but we need it on all platforms and, I guess, we can
safely change it to toggling which should simplify the extension. In real life
it seems working (e.g., Num lock button).


> What I would do is just add one more return value from PROC_IS_WHITELISTED -
the
> filter text. That way we will have exactly the domain we are working with to
> display in the context menu. This will also allow us to use
> RemoveFilter/AddFilter as before. We can just add "www." removal for
consistency
> with other ABP versions.

I would say we should not fiddle around with the filter text in each extension.
It can be modified or the logic can be significantly complicated.
Firstly, pay attention, that the fix in this change is not exactly what is
happening in the Chrome. They have possibly infinite loop which removes all
exception filters, but in this change we remove only one. Even more, I would say
that the removing of the filter is not correct, because, I guess, the filter can
be applied to several web sites and we should modify it by removing only our web
site (the logic here is already very error prone), it's better and safer to have
all these stuff in the core.

> I don't think there's any need in PROC_TOGGLE_WHITELISTING at the moment.

What would you suggest?

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

http://codereview.adblockplus.org/5081266177179648/diff/5629499534213120/src/...
src/engine/Main.cpp:51: if (BeginsWith<std::string>(host, "https://") ||
BeginsWith<std::string>(host, "http://"))
On 2015/02/26 16:14:10, Eric wrote:
> Explicit type specification for template function.
> 
> Beginning with "http" is probably adequate for this test. We know that the
page
> has loaded, thus the URL is valid, and so if it begins with "http" it will
begin
> with one of the two strings above.
> 
> It's probably easier to extract the initial substring of four characters in
the
> previous line and then test for equality.

Actually, I guess, we don't need this test at all because the idea here is to
get the domain, I guess, `filterEngine->GetHostFromURL(domain)` anyway returns
'domain' without any modification.

http://codereview.adblockplus.org/5081266177179648/diff/5629499534213120/src/...
src/engine/Main.cpp:56: ReplaceString<std::string>(host, "www.", "");
On 2015/02/26 18:16:01, Eric wrote:
> More importantly, this algorithm is not correct. It will fail on the string
> "foo.www.bar.com", for example.
Right, it's incorrect, we need to replace only the prefix "www." and don't touch
the rest part of the URL.

> That's admittedly unlikely, but still. It will
> also fail on "web.bar.com" or "profile.bar.com". The right thing to do is
> probably to take the final two domain components.
No, we don't care about the amount of subdomains here, we care only about the
beginning of the URL. https://adblockplus.org/en/filters#anchors
The transform function should be
http://www.xxx -> xxx
http://xxx -> xxx
https://www.xxx -> xxx
https://xxx -> xxx
where "xxx" can have any number of subdomains.

> Even more importantly, this operation needs to be done in the plugin. We need
to
> display the domain that's about to be blocked/unblocked in the popup menu.
This
> needs to be the same as what happens on the inside.
I would say we don't need it, see https://issues.adblockplus.org/ticket/2070.
> 
> It would be better to derive the relevant name in the plugin and change the
> interface to the engine to pass a domain name rather than a URL.
If we decide that we work with the domain then I agree, it's better to pass the
domain, but so far it's better to work with the url. Also pay attention that the
actual deriving of the domain is performed on the engine side.

http://codereview.adblockplus.org/5081266177179648/diff/5629499534213120/src/...
src/engine/Main.cpp:248: {
On 2015/02/26 18:16:01, Eric wrote:
> As a point of code clarity, if we have a function AddWhitelistedUrl, we should
> also have RemoveWhitelistedUrl. It makes it easier to check that the
operations
> are actually opposites.
I cannot say that their implementations are opposite, but after having their
functionality in the core they will be opposite here and I expect that we will
not need to have `AddWhitelistedUrl`. It's merely a temporary function to avoid
long scopes in the switch-case and avoid code duplication. So, I would say we
don't need to have `RemoveWhitelistedUrl` because we should get rid of them in
future.

[Oleksandr, 2015-03-02 14:54:01]

I agree with most of the stuff, however I'm afraid #2070 and #2038 might drag
for a while, and I think it would be good to have at least a temporary fix for
this before those. For example returning a filter from PROC_IS_WHITELISTED is
good enough for now, IMO.

>> I don't think there's any need in PROC_TOGGLE_WHITELISTING at the moment.

> What would you suggest?

Returning a filter from PROC_IS_WHITELISTED

[Eric, 2015-03-02 19:24:24]

On 2015/03/02 13:01:03, sergei wrote:
> That's correct, but let's firstly wait for the decision regarding
> https://issues.adblockplus.org/ticket/2070.

I agree wholeheartedly with Oleksandr. Let's not wait. It's taken months for the
UI issue that was a Q4 priority to near completion, it's not even done. Don't
expect this one to happen quickly. If you want to wait, you might as well close
this review and start over when there's a design decision; keeping the code from
rotting in the interim likely isn't worth the effort.

> Yes, if we need it (see #2070), then it's better to retrieve it only once when
> the menu is created and send this value to the engine to disable/enable ABP on
> the web site.

The current UI for IE needs it. If we are going to fix this now, and fix it
without changing the design, we'll need the domain name.

Let me state explicitly that I think we should preserve the current UI for now,
since discussing that really is wasted effort, since the larger question is
already being discussed.

[re: combining popup menu functions]
> It's too much for this fix, this refactoring should be as another issue.

Well, assuming we need to derive a domain name, the best way is to do it after
this refactoring. We agree we need consistent state of "is-whitelisted" (in
whatever variant we're using), and that's also best done in the same place.

I would be fine adding a new issue for this refactoring. It should be marked as
blocking #1104.

> I hope that we won't need a domain in the
> menu item so we need to merely get from the engine whether it's enabled or
not.

I think we do, and I've said so in #2070. Regardless, we do need one for our
current UI.

[Eric, 2015-03-02 19:24:31]

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

http://codereview.adblockplus.org/5081266177179648/diff/5629499534213120/src/...
src/engine/Main.cpp:248: {
On 2015/03/02 13:01:04, sergei wrote:
> I cannot say that their implementations are opposite

I doesn't matter if their implementations are exact opposites, but it does
matter for understanding the code that their behavior should be inverses of each
other. For that reason alone it would be better to have a matched pair.

> So, I would say we
> don't need to have `RemoveWhitelistedUrl` because we should get rid of them in
> future.

It would be better to have a matched pair, even if the duration of all of this
is short-lived.

[sergei, 2015-03-04 11:43:09]

I've implemented it in the suggested way, however, I find it as a temporary
hack. It adds unnecessary things to the addon part and postpones the required
change, while we have a good chance to refactor it.
Please find it here, http://codereview.adblockplus.org/5115380229996544/.


What is the wait are you talking about? The change in this codereview fixes the
case described in #1104, does not touch any UI and does not depend on #2070.

[Eric, 2015-03-04 13:23:20]

On 2015/03/04 11:43:09, sergei wrote:
> What is the wait are you talking about?

When I said this:
> It's taken months for the UI issue that was a Q4 priority to near
> completion, it's not even done. 
I was referring to #1524. See the keyword '2014q4' on the issue.