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

src/engine/Main.cpp

Index: src/engine/Main.cpp
diff --git a/src/engine/Main.cpp b/src/engine/Main.cpp
index 01ddcb6ca49a5ca2dc8dbc692d364abaae0ef70e..759931d9cca638102b0d3a2dc7f95631ac3434bd 100644
--- a/src/engine/Main.cpp
+++ b/src/engine/Main.cpp
@@ -40,6 +40,26 @@ namespace
   CriticalSection activeConnectionsLock;
   HWND callbackWindow;
 
+  std::string CreateDomainWhitelistingFilterText(const std::string& domain)
+  {
+    return "@@||" + domain + "^$document";
+  }
+
+  void AddWhitelistedUrl(const std::string& url)
+  {
+    std::string host = ASCIIStringToLower(url);
+    if (BeginsWith<std::string>(host, "https://") || BeginsWith<std::string>(host, "http://"))

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

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.

[sergei, 2015/03/02 13:01:04]

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.

+    {
+      host = filterEngine->GetHostFromURL(host);
+    }
+
+    ReplaceString<std::string>(host, "www.", "");

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

Explicit type for template function.

[Eric, 2015/02/26 18:16:01]

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.

[sergei, 2015/03/02 13:01:04]

Right, it's incorrect, we need to replace only the prefix "www." and don't touch
the rest part of the URL.
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.
I would say we don't need it, see https://issues.adblockplus.org/ticket/2070.
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.

+    if (!host.empty())
+    {
+      filterEngine->GetFilter(CreateDomainWhitelistingFilterText(host))->AddToList();
+    }
+  }
+
   void WriteSubscriptions(Communication::OutputBuffer& response,
       const std::vector<AdblockPlus::SubscriptionPtr>& subscriptions)
   {
@@ -217,19 +237,40 @@ namespace
         response << (match && match->GetType() == AdblockPlus::Filter::TYPE_EXCEPTION);
         break;
       }
-      case Communication::PROC_ADD_FILTER:
+      case Communication::PROC_ADD_WHITELSITED_DOMAIN:
       {
-        std::string text;
-        request >> text;
-
-        filterEngine->GetFilter(text)->AddToList();
+        std::string url;
+        request >> url;
+        AddWhitelistedUrl(url);
         break;
       }
-      case Communication::PROC_REMOVE_FILTER:
+      case Communication::PROC_REMOVE_WHITELSITED_DOMAIN:
       {

[Eric, 2015/02/26 18:16:01]

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.

[sergei, 2015/03/02 13:01:04]

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.

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

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.
It would be better to have a matched pair, even if the duration of all of this
is short-lived.

-        std::string text;
-        request >> text;
-        filterEngine->GetFilter(text)->RemoveFromList();
+        std::string domain;
+        request >> domain;
+        AdblockPlus::FilterPtr filter
+          = filterEngine->GetFilter(CreateDomainWhitelistingFilterText(domain));
+        if (filter && filter->GetType() == AdblockPlus::Filter::TYPE_EXCEPTION)
+        {
+          filter->RemoveFromList();
+        }
+        break;
+      }
+      case Communication::PROC_TOGGLE_WHITELISTING:
+      {
+        std::string url;
+        request >> url;
+        auto filter = filterEngine->Matches(url,
+            AdblockPlus::FilterEngine::ContentType::CONTENT_TYPE_DOCUMENT, "");
+        bool isWhitelisted = filter && filter->GetType() == AdblockPlus::Filter::TYPE_EXCEPTION;
+        if (isWhitelisted)
+        {
+          filter->RemoveFromList();
+        }
+        else
+        {
+          AddWhitelistedUrl(url);
+        }
         break;
       }
       case Communication::PROC_SET_PREF: