Issue 6652 - Do not push to unconditional selectors array

lib/elemHide.js

Index: lib/elemHide.js
===================================================================
--- a/lib/elemHide.js
+++ b/lib/elemHide.js
@@ -58,16 +58,28 @@
 
 /**
  * Lookup table, lists of element hiding exceptions by selector
  * @type {Map.<string,Filter>}
  */
 let exceptions = new Map();
 
 /**
+ * Returns a list of selectors that apply on each website unconditionally.

[kzar, 2018/05/10 10:59:49]

Please could you also remove this unrelated change? I agree that we might as
well put functions that don't need to be used elsewhere outside of the ElemHide
Object, but that also applies to _addFiltersByDomain for example and can be done
in a separate change.

[Manish Jethani, 2018/05/11 04:07:10]

Well if we're going to keep the function available on the ElemHide object then
we'll have to call Array.slice on the result. Removing that call is part of the
optimization, but we can only remove it if it's made private to the module
(otherwise anyone can mess with the internal state of the module).

Or I could leave the function on the ElemHide object but still remove the call
to Array.slice. What do you think?

Making the function private here was not just about cleaning up code, there was
a more specific reason for it that is related to the optimizations we're trying
to do.

[kzar, 2018/05/11 11:23:43]

Gotya, OK that makes sense.

This LGTM then, I'd prefer you moved all the private stuff out of the ElemHide
Object first as a separate Noissue change, then made this change. But I'll leave
it up to you.

+ * @returns {string[]}
+ */
+function getUnconditionalSelectors()
+{
+  if (!unconditionalSelectors)
+    unconditionalSelectors = [...filterBySelector.keys()];
+
+  return unconditionalSelectors;
+}
+
+/**
  * Container for element hiding filters
  * @class
  */
 let ElemHide = exports.ElemHide = {
   /**
    * Removes all known filters
    */
   clear()
@@ -199,27 +211,16 @@
       if (list[i].isActiveOnDomain(docDomain))
         return list[i];
     }
 
     return null;
   },
 
   /**
-   * Returns a list of selectors that apply on each website unconditionally.
-   * @returns {string[]}
-   */
-  getUnconditionalSelectors()
-  {
-    if (!unconditionalSelectors)
-      unconditionalSelectors = [...filterBySelector.keys()];
-    return unconditionalSelectors.slice();
-  },
-
-  /**
    * Constant used by getSelectorsForDomain to return all selectors applying to
    * a particular hostname.
    */
   ALL_MATCHING: 0,
 
   /**
    * Constant used by getSelectorsForDomain to exclude selectors which apply to
    * all websites without exception.
@@ -243,18 +244,16 @@
    *   List of selectors.
    */
   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 excluded = new Set();
     let currentDomain = domain ? domain.toUpperCase() : "";
 
     // This code is a performance hot-spot, which is why we've made certain
     // micro-optimisations. Please be careful before making changes.
     while (true)
@@ -281,11 +280,14 @@
 
       if (currentDomain == "")
         break;
 
       let nextDot = currentDomain.indexOf(".");
       currentDomain = nextDot == -1 ? "" : currentDomain.substr(nextDot + 1);
     }
 
+    if (criteria < ElemHide.NO_UNCONDITIONAL)
+      selectors = getUnconditionalSelectors().concat(selectors);
+
     return selectors;
   }
 };