Issue 6562 - Remove filter keys from the element hiding code

lib/elemHide.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 142 matching lines @@
 
     // Whitelisting filters
     if (filter instanceof ElemHideException)
     {
       let list = exceptions.get(filter.selector);
       let index = list.indexOf(filter);
       if (index >= 0)
         list.splice(index, 1);
     }
     // Unconditially applied element hiding filters
-    else if (filterBySelector.get(filter.selector) === filter)
+    else if (filterBySelector.get(filter.selector) == filter)
     {
       filterBySelector.delete(filter.selector);
       unconditionalSelectors = null;
     }
     // Conditionally applied element hiding filters
     else
     {
       let domains = filter.domains || defaultDomains;
       for (let domain of domains.keys())
       {
@@ skipping 71 matching lines @@
   getSelectorsForDomain(domain, criteria)
   {
     let selectors = [];
 
     if (typeof criteria == "undefined")
       criteria = ElemHide.ALL_MATCHING;
     if (criteria < ElemHide.NO_UNCONDITIONAL)
       selectors = this.getUnconditionalSelectors();
 
     let specificOnly = (criteria >= ElemHide.SPECIFIC_ONLY);
-    let excludedFilters = new Set();
-
+    let excluded = new Set();
     let currentDomain = domain ? domain.toUpperCase() : "";
-    let currentDomainIsGeneric = currentDomain == "";

[Manish Jethani, 2018/05/02 18:11:40]

Comparison with an empty string really should not need to be cached, it's
probably superoptimized by the JS engine anyway. I would drop this. Also see
below.

[kzar, 2018/05/03 15:36:00]

OK, Done.

 
     // This code is a performance hot-spot, which is why we've made certain
     // micro-optimisations. Please be careful before making changes.
     while (true)
     {
-      if (specificOnly && currentDomainIsGeneric)
+      if (specificOnly && currentDomain == "")
         break;
 
       let filters = filtersByDomain.get(currentDomain);
       if (filters)
       {
         for (let [filter, isIncluded] of filters)
         {
-          if (excludedFilters.size > 0 && excludedFilters.has(filter))

[Manish Jethani, 2018/05/02 18:11:40]

We only need to do this check for included filters, so it should be inside the
if block.

[kzar, 2018/05/03 15:36:01]

Good point, Done.

-            continue;
-
-          if (isIncluded)
+          if (!isIncluded)
           {
-            if (!this.getException(filter, domain))
-              selectors.push(filter.selector);
+            excluded.add(filter);
           }
-          else if (!currentDomainIsGeneric)

[Manish Jethani, 2018/05/02 18:11:40]

currentDomain will always be non-empty if the filter is excluded, we made sure
of that while populating filtersByDomain (which is why I did not mention this
when I thought we would just not add the included filters to the set).

So I think it comes down to this:

    if (!isIncluded)
    {
      excludedFilters.add(filter);
    }
    else if ((excludedFilters.size == 0 || !excludedFilters.has(filter)) &&
             !this.getException(filter, domain))
    {
      selectors.push(filter.selector);
    }

Note there's no check for currentDomain now, I think we can drop all the related
changes there.

[kzar, 2018/05/03 15:36:00]

OK, Done.
Well there still are a couple of checks for currentDomain elsewhere.

-            excludedFilters.add(filter);
+          else if ((excluded.size == 0 || !excluded.has(filter)) &&
+                   !this.getException(filter, domain))
+          {
+            selectors.push(filter.selector);
+          }
         }
       }
 
-      if (currentDomainIsGeneric)
+      if (currentDomain == "")
         break;
 
       let nextDot = currentDomain.indexOf(".");
-      if (nextDot == -1)
-      {
-        currentDomain = "";
-        currentDomainIsGeneric = true;
-      }
-      else
-        currentDomain = currentDomain.substr(nextDot + 1);
+      currentDomain = nextDot == -1 ? "" : currentDomain.substr(nextDot + 1);
     }
 
     return selectors;
   }
 };