Issue 4167 - getSelectorsForDomain criteria + keys

lib/elemHide.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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 18 matching lines @@
  */
 var filterByKey = [];
 
 /**
  * Lookup table, keys of the filters by filter text
  * @type Object
  */
 var keyByFilter = Object.create(null);
 
 /**
- * Indicates whether we are using the getSelectorsForDomain function and
- * therefore maintaining the required filtersByDomain, filterKeysBySelector,
- * unconditionalSelectors and unconditionalFilterKeys lookups.
- * (Will be false for Firefox)
- * @type Boolean
- */
-var usingGetSelectorsForDomain = !("nsIStyleSheetService" in Ci);

[Wladimir Palant, 2016/09/19 08:53:59]

This flag needs to go - we'll be using this functionality in Firefox as well
now.

[kzar, 2016/09/19 16:54:31]

Done.

-
-/**
  * Nested lookup table, filter (or false if inactive) by filter key by domain.
  * (Only contains filters that aren't unconditionally matched for all domains.)
  * @type Object
  */
 var filtersByDomain = Object.create(null);
 
 /**
- * Lookup table, filters keys by selector. (Only contains filters that have a
+ * Lookup table, filter keys by selector. (Only contains filters that have a
  * selector that is unconditionally matched for all domains.)
  */
 var filterKeysBySelector = Object.create(null);
 
 /**
  * This array caches the keys of filterKeysBySelector table (selectors which
  * unconditionally apply on all domains). It will be null if the cache needs to
  * be rebuilt.
  */
 var unconditionalSelectors = null;
 
 /**
  * This array caches the values of filterKeysBySelector table (filterIds for
  * selectors which unconditionally apply on all domains). It will be null if the
- * cache needs to be rebuilt.
+ * cache needs to be rebuilt. Note: Only the first filter key for each selector
+ * is cached.
  */
 var unconditionalFilterKeys = null;
 
 /**
  * Object to be used instead when a filter has a blank domains property.
  */
 var defaultDomains = Object.create(null);
 defaultDomains[""] = true;
 
 /**
@@ skipping 54 matching lines @@
     if (filter instanceof ElemHideException)
     {
       if (filter.text in knownExceptions)
         return;
 
       let selector = filter.selector;
       if (!(selector in exceptions))
         exceptions[selector] = [];
       exceptions[selector].push(filter);
 
-      if (usingGetSelectorsForDomain)
+      // If this is the first exception for a previously unconditionally
+      // applied element hiding selector we need to take care to update the
+      // lookups.
+      let filterKeys = filterKeysBySelector[selector];
+      if (filterKeys)
       {
-        // If this is the first exception for a previously unconditionally
-        // applied element hiding selector we need to take care to update the
-        // lookups.
-        let unconditionalFilterKeys = filterKeysBySelector[selector];
-        if (unconditionalFilterKeys)
+        for (let filterKey of filterKeys)
+          this._addToFiltersByDomain(filterKey, filterByKey[filterKey]);
+        delete filterKeysBySelector[selector];
+        unconditionalSelectors = unconditionalFilterKeys = null;
+      }
+
+      knownExceptions[filter.text] = true;
+    }
+    else
+    {
+      if (filter.text in keyByFilter)
+        return;
+
+      let key = filterByKey.push(filter) - 1;
+      keyByFilter[filter.text] = key;
+
+      if (!(filter.domains || filter.selector in exceptions))
+      {
+        // The new filter's selector is unconditionally applied to all domains
+        let filterKeys = filterKeysBySelector[filter.selector];
+        if (filterKeys)
         {
-          for (let filterKey of unconditionalFilterKeys)
-            this._addToFiltersByDomain(filterKey, filterByKey[filterKey]);
-          delete filterKeysBySelector[selector];
+          filterKeys.push(key);
+        }
+        else
+        {
+          filterKeysBySelector[filter.selector] = [key];
           unconditionalSelectors = unconditionalFilterKeys = null;
         }
       }
-
-      knownExceptions[filter.text] = true;
-    }
-    else
-    {
-      if (filter.text in keyByFilter)
-        return;
-
-      let key = filterByKey.push(filter) - 1;
-      keyByFilter[filter.text] = key;
-
-      if (usingGetSelectorsForDomain)
+      else
       {
-        if (!(filter.domains || filter.selector in exceptions))
+        // The new filter's selector only applies to some domains
+        this._addToFiltersByDomain(key, filter);
+      }
+    }
+
+    FilterNotifier.emit("elemhideupdate");
+  },
+
+  _removeFilterKey: function(key, filter)
+  {
+    let filterKeys = filterKeysBySelector[filter.selector];
+    if (filterKeys)
+    {
+      let index = filterKeys.indexOf(key);
+      if (index >= 0)
+      {
+        if (filterKeys.length > 1)
         {
-          // The new filter's selector is unconditionally applied to all domains
-          let filterKeys = filterKeysBySelector[filter.selector];
-          if (filterKeys)
-          {
-            filterKeys.push(filter);

[Wladimir Palant, 2016/09/19 08:53:59]

Shouldn't you push `key` here?

Given that you have unit tests, I guess that you need to make sure all edge
cases are really tested?

[kzar, 2016/09/19 16:54:30]

As discussed in IRC it turns out this branch was rarely used and mostly
irrelevant even when it was. I have simplified this logic and related data
structure.

-          }
-          else
-          {
-            filterKeysBySelector[filter.selector] = [key];
-            unconditionalSelectors = unconditionalFilterKeys = null;
-          }
+          filterKeys.splice(index, 1);
+          if (index == 0)
+            unconditionalFilterKeys = null;
         }
         else
         {
-          // The new filter's selector only applies to some domains
-          this._addToFiltersByDomain(key, filter);
+          delete filterKeysBySelector[filter.selector];
+          unconditionalSelectors = unconditionalFilterKeys = null;
         }
+        return;
       }
     }
 
-    FilterNotifier.emit("elemhideupdate");
+    // We haven't found this filter in unconditional filters, look in
+    // filtersByDomain.
+    let domains = filter.domains || defaultDomains;
+    for (let domain in domains)
+    {
+      let filters = filtersByDomain[domain];
+      if (filters)
+        delete filters[key];
+    }
   },
 
   /**
    * Removes an element hiding filter
    * @param {ElemHideFilter} filter
    */
   remove: function(filter)
   {
     if (filter instanceof ElemHideException)
     {
       if (!(filter.text in knownExceptions))
         return;
 
       let list = exceptions[filter.selector];
       let index = list.indexOf(filter);
       if (index >= 0)
         list.splice(index, 1);
       delete knownExceptions[filter.text];
     }
     else
     {
       if (!(filter.text in keyByFilter))
         return;
 
       let key = keyByFilter[filter.text];
       delete filterByKey[key];
       delete keyByFilter[filter.text];
-
-      if (usingGetSelectorsForDomain)
-      {
-        let filterKeys = filterKeysBySelector[filter.selector];
-        if (filterKeys)
-        {
-          if (filterKeys.length > 1)
-          {
-            let index = filterKeys.indexOf(key);
-            filterKeys.splice(index, 1);
-          }
-          else
-          {
-            delete filterKeysBySelector[filter.selector];
-            unconditionalSelectors = unconditionalFilterKeys = null;
-          }
-        }
-        else
-        {
-          let domains = filter.domains || defaultDomains;
-          for (let domain in domains)
-          {
-            let filters = filtersByDomain[domain];
-            if (filters)
-              delete filters[key];
-          }
-        }
-      }
+      this._removeFilterKey(key, filter);
     }
 
     FilterNotifier.emit("elemhideupdate");
   },
 
   /**
    * Checks whether an exception rule is registered for a filter on a particular
    * domain.
    */
   getException: function(/**Filter*/ filter, /**String*/ docDomain) /**ElemHideException*/
@@ skipping 43 matching lines @@
 
     return domains;
   },
 
   /**
    * Returns a list of selectors that apply on each website unconditionally.
    * @returns {String[]}
    */
   getUnconditionalSelectors: function()
   {
-    if (!usingGetSelectorsForDomain)
-      throw new Error("getUconditionalSelectors can not be used in Firefox!");
-
     if (!unconditionalSelectors)
       unconditionalSelectors = Object.keys(filterKeysBySelector);
     return unconditionalSelectors.slice();
   },
 
   /**
    * Returns a list of all selectors active on a particular domain.
    * Returns a list of filterKeys for selectors that apply on each website
    * unconditionally.
    * @returns {Number[]}
    */
   getUnconditionalFilterKeys: function()
   {
-    if (!usingGetSelectorsForDomain)
-      throw new Error("getUconditionalFilterKeys can not be used in Firefox!");
-
     if (!unconditionalFilterKeys)
     {
       let selectors = this.getUnconditionalSelectors();
       unconditionalFilterKeys = [];
       for (let selector of selectors)
-        for (let key of filterKeysBySelector[selector])
-          unconditionalFilterKeys.push(key);

[Wladimir Palant, 2016/09/19 08:53:59]

On Firefox we actually need a mapping from selectors to filter keys (one key per
selector). So it should be best if we push only the first key for each selector,
then the selector and key at the same position in the array will belong
together.

[kzar, 2016/09/19 16:54:31]

Since we only store one key per unconditional selector now anyway this has been
taken care of :)

+        unconditionalFilterKeys.push(filterKeysBySelector[selector][0]);
     }
     return unconditionalFilterKeys.slice();
   },
 
-  /**
-   * Returns a list of all selectors active on a particular domain. Optionally
-   * a list of corresponding filter keys for the selectors can be returned too.
-   * (Must not be used in Firefox - when usingGetSelectorsForDomain is false).

[Wladimir Palant, 2016/09/19 08:53:59]

Actually, it will be used in Firefox now, that's the only platform using filter
keys ;)

[kzar, 2016/09/19 16:54:31]

Done.

-   */
-  getSelectorsForDomain: function(/**String*/ domain, /**Boolean*/ specificOnly,
-                                  /**Boolean*/ noUnconditional,
-                                  /**Boolean*/ provideFilterKeys)
-  {
-    if (!usingGetSelectorsForDomain)
-      throw new Error("getSelectorsForDomain can not be used in Firefox!");
-
+
+  /**
+   * 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.
+   */
+  NO_UNCONDITIONAL: 1,
+
+  /**
+   * Constant used by getSelectorsForDomain to return only selectors for filters
+   * which specifically match the given host name.
+   */
+  SPECIFIC_ONLY: 2,
+
+  /**
+   * Determines from the current filter list which selectors should be applied
+   * on a particular host name. Optionally returns the corresponding filter
+   * keys.
+   * @param {String} domain
+   * @param {Number} [criteria]
+   *   One of the following: ElemHide.ALL_MATCHING, ElemHide.NO_UNCONDITIONAL or
+   *                         ElemHide.SPECIFIC_ONLY.
+   * @param {Boolean} [provideFilterKeys]
+   *   If true, the function will return a list of corresponding filter keys in
+   *   addition to selectors.
+   * @returns {string[]|Array.<string[]>}
+   *   List of selectors or an array with two elements (list of selectors and
+   *   list of corresponding keys) if provideFilterKeys is true.
+   */
+  getSelectorsForDomain: function(domain, criteria, provideFilterKeys)
+  {
     let filterKeys = [];
     let selectors = [];
-    if (!(specificOnly || noUnconditional))

[Wladimir Palant, 2016/09/19 08:53:59]

Personally, I find this easier to understand if you get rid of parentheses:

  if (!specificOnly && !noUnconditional)

[kzar, 2016/09/19 16:54:30]

Done.

+
+    if (typeof criteria == "undefined")
+      criteria = ElemHide.ALL_MATCHING;
+    if (criteria < ElemHide.NO_UNCONDITIONAL)
     {
       selectors = this.getUnconditionalSelectors();
       if (provideFilterKeys)
         filterKeys = this.getUnconditionalFilterKeys();
     }
 
+    let specificOnly = (criteria >= ElemHide.SPECIFIC_ONLY);
     let seenFilters = Object.create(null);
     let currentDomain = domain ? domain.toUpperCase() : "";
     while (true)
     {
       if (specificOnly && currentDomain == "")
         break;
 
       let filters = filtersByDomain[currentDomain];
       if (filters)
       {
@@ skipping 19 matching lines @@
       let nextDot = currentDomain.indexOf(".");
       currentDomain = nextDot == -1 ? "" : currentDomain.substr(nextDot + 1);
     }
 
     if (provideFilterKeys)
       return [selectors, filterKeys];
     else
       return selectors;
   }
 };