Issue 6957 - Add generateStyleSheetForDomain function to ElemHide module

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 38 matching lines @@
 
 /**
  * This array caches the keys of filterBySelector table (selectors
  * which unconditionally apply on all domains). It will be null if the
  * cache needs to be rebuilt.
  * @type {?string[]}
  */
 let unconditionalSelectors = null;
 
 /**
+ * The default style sheet that applies on all domains. This is based on the
+ * value of <code>{@link unconditionalSelectors}</code>.
+ * @type {?string}
+ */
+let defaultStyleSheet = null;
+
+/**
  * Map to be used instead when a filter has a blank domains property.
  * @type {Map.<string,boolean>}
  * @const
  */
 let defaultDomains = new Map([["", true]]);
 
 /**
  * Set containing known element hiding filters
  * @type {Set.<ElemHideFilter>}
  */
@@ skipping 24 matching lines @@
  * @returns {string[]}
  */
 function getUnconditionalSelectors()
 {
   if (!unconditionalSelectors)
     unconditionalSelectors = [...filterBySelector.keys()];
 
   return unconditionalSelectors;
 }
 
+/**
+ * Returns the list of selectors that apply on a given domain from the subset
+ * of filters that do not apply unconditionally on all domains.
+ *
+ * @param {string} domain The domain.
+ * @param {boolean} [specificOnly=false] Whether selectors from generic filters
+ *   should be included.
+ *
+ * @returns {Array.<string>} The list of selectors.
+ */
+function getConditionalSelectorsForDomain(domain, specificOnly = false)
+{
+  let selectors = [];
+
+  let excluded = new Set();
+  let currentDomain = domain ? domain.replace(/\.+$/, "").toLowerCase() : "";
+
+  // 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 && currentDomain == "")
+      break;
+
+    let filters = filtersByDomain.get(currentDomain);
+    if (filters)
+    {
+      for (let [filter, isIncluded] of filters)
+      {
+        if (!isIncluded)
+        {
+          excluded.add(filter);
+        }
+        else
+        {
+          let {selector} = filter;
+          if ((excluded.size == 0 || !excluded.has(filter)) &&
+              !ElemHideExceptions.getException(selector, domain))
+          {
+            selectors.push(selector);
+          }
+        }
+      }
+    }
+
+    if (currentDomain == "")
+      break;
+
+    let nextDot = currentDomain.indexOf(".");
+    currentDomain = nextDot == -1 ? "" : currentDomain.substr(nextDot + 1);
+  }
+
+  return selectors;
+}
+
+/**
+ * Returns the default style sheet that applies on all domains.
+ * @returns {string}
+ */
+function getDefaultStyleSheet()
+{
+  if (!defaultStyleSheet)
+    defaultStyleSheet = createStyleSheet(getUnconditionalSelectors());
+
+  return defaultStyleSheet;
+}
+
 ElemHideExceptions.on("added", ({selector}) =>
 {
   // If this is the first exception for a previously unconditionally applied
   // element hiding selector we need to take care to update the lookups.
   let unconditionalFilterForSelector = filterBySelector.get(selector);
   if (unconditionalFilterForSelector)
   {
     addToFiltersByDomain(unconditionalFilterForSelector);
     filterBySelector.delete(selector);
     unconditionalSelectors = null;
+    defaultStyleSheet = null;
   }
 });
 
 /**
  * Container for element hiding filters
  * @class
  */
 exports.ElemHide = {
   /**
    * Removes all known filters
    */
   clear()
   {
     for (let collection of [filtersByDomain, filterBySelector, knownFilters])
       collection.clear();
 
     unconditionalSelectors = null;
+    defaultStyleSheet = null;
+
     filterNotifier.emit("elemhideupdate");
   },
 
   /**
    * Add a new element hiding filter
    * @param {ElemHideFilter} filter
    */
   add(filter)
   {
     if (knownFilters.has(filter))
       return;
 
     let {selector} = filter;
 
     if (!(filter.domains || ElemHideExceptions.hasExceptions(selector)))
     {
       // The new filter's selector is unconditionally applied to all domains
       filterBySelector.set(selector, filter);
       unconditionalSelectors = null;
+      defaultStyleSheet = null;
     }
     else
     {
       // The new filter's selector only applies to some domains
       addToFiltersByDomain(filter);
     }
 
     knownFilters.add(filter);
     filterNotifier.emit("elemhideupdate");
   },
 
   /**
    * Removes an element hiding filter
    * @param {ElemHideFilter} filter
    */
   remove(filter)
   {
     if (!knownFilters.has(filter))
       return;
 
     let {selector} = filter;
 
     // Unconditially applied element hiding filters
     if (filterBySelector.get(selector) == filter)
     {
       filterBySelector.delete(selector);
       unconditionalSelectors = null;
+      defaultStyleSheet = null;
     }
     // Conditionally applied element hiding filters
     else
     {
       let domains = filter.domains || defaultDomains;
       for (let domain of domains.keys())
       {
         let filters = filtersByDomain.get(domain);
         if (filters)
         {
@@ skipping 11 matching lines @@
 
   /**
    * Determines from the current filter list which selectors should be applied
    * on a particular host name.
    * @param {string} domain
    * @param {boolean} [specificOnly] true if generic filters should not apply.
    * @returns {string[]} List of selectors.
    */
   getSelectorsForDomain(domain, specificOnly = false)
   {
-    let selectors = [];

[Manish Jethani, 2018/09/20 12:27:24]

This entire block of code is moved into getConditionalSelectorsForDomain

-
-    let excluded = new Set();
-    let currentDomain = domain ? domain.replace(/\.+$/, "").toLowerCase() : "";
-
-    // 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 && currentDomain == "")
-        break;
-
-      let filters = filtersByDomain.get(currentDomain);
-      if (filters)
-      {
-        for (let [filter, isIncluded] of filters)
-        {
-          if (!isIncluded)
-          {
-            excluded.add(filter);
-          }
-          else
-          {
-            let {selector} = filter;
-            if ((excluded.size == 0 || !excluded.has(filter)) &&
-                !ElemHideExceptions.getException(selector, domain))
-            {
-              selectors.push(selector);
-            }
-          }
-        }
-      }
-
-      if (currentDomain == "")
-        break;
-
-      let nextDot = currentDomain.indexOf(".");
-      currentDomain = nextDot == -1 ? "" : currentDomain.substr(nextDot + 1);
-    }
+    let selectors = getConditionalSelectorsForDomain(domain, specificOnly);
 
     if (!specificOnly)
       selectors = getUnconditionalSelectors().concat(selectors);
 
     return selectors;
+  },
+
+  /**
+   * @typedef {object} ElemHideStyleSheet
+   * @property {string} code CSS code.
+   * @property {Array.<string>} selectors List of selectors.
+   */
+
+  /**
+   * Generates a style sheet for a given domain based on the current set of
+   * filters.
+   *
+   * @param {string} domain The domain.
+   * @param {boolean} [specificOnly=false] Whether selectors from generic
+   *   filters should be included.
+   *
+   * @returns {ElemHideStyleSheet} An object containing the CSS code and the
+   *   list of selectors.
+   */
+  generateStyleSheetForDomain(domain, specificOnly = false)

[Manish Jethani, 2018/09/20 12:27:24]

The idea is that calling this function will be faster than calling
getSelectorsForDomain followed by createStyleSheet, because this one uses an
already cached style sheet.

[Jon Sonesen, 2018/09/23 18:01:59]

Acknowledged.

+  {
+    let selectors = getConditionalSelectorsForDomain(domain, specificOnly);
+    let code = (specificOnly ? "" : getDefaultStyleSheet()) +
+               createStyleSheet(selectors);

[Jon Sonesen, 2018/09/23 18:01:59]

Regarding the use of this ternary operation, I wonder how performance critical
this particular block is?

I ask because I generally prefer strict equals for readability. Notably our code
base tends to do a lot of in-lining which in this case ternary fits the visual
aspect of this. However, I think using strict if/else is quite a bit more
readable I have even heard ternary operations referred to as "ninja code" not
that I think it is, but they have a point.

Additionally, I got curious about the time differences for processing the two
types of evaluations and found an interesting jsperf page[1]  Here their results
tend to show that if/else with specificOnly and !specificOnly could be up to 77%
percent faster (at least based on the truthy/falsy section of the posted link)

What do you think?

[1]: https://jsperf.com/ternary-vs-simple-if-else

[Manish Jethani, 2018/09/24 11:51:14]

This function is supposed to be very performance critical, in fact it has been
optimized and micro-optimized over time.

Let me try to run some tests and get back.
Fair enough, makes sense. Let me update the patch, along with some other
changes.
The way I'm reading this, it seems we only need to look at the last two here
(truthy/falsy vs. ternary truthy/falsy). On my machine here, truthy/falsy is the
fastest, while ternary truthy/falsy is 5% slower. Nevertheless, let me actually
test this implementation itself.

[Jon Sonesen, 2018/09/24 15:09:44]

Ah yeah, that's a good point here. My bad, haha I misread the ternary
truthy/falsy also: it seems that the result is not 100% reliable performance
wise today I ran the jsperf thing a few different times and got different
results, also I am aware that jsperf is not the best benchmark as there are many
factors at play regarding time efficiency that jsperf is not able to recreate
especially since our use case is significantly more complex. 

So I am basically saying that my heart is not set on this being if/else at this
point especially if it is in fact less efficient.

[Manish Jethani, 2018/09/27 15:58:29]

So I tested this by the way, it seems the Patch Set 2 with ternary operator is
the best option.

[Manish Jethani, 2018/09/27 15:58:29]

Acknowledged.

+
+    if (!specificOnly)
+      selectors = getUnconditionalSelectors().concat(selectors);
+
+    return {code, selectors};
   }
 };
 
 /**
  * Splits a list of selectors into groups determined by the value of
  * <code>{@link selectorGroupSize}</code>.
  *
  * @param {Array.<string>} selectors
  * @yields {Array.<string>}
  */
@@ skipping 37 matching lines @@
 {
   let styleSheet = "";
 
   for (let rule of createRules(selectors))
     styleSheet += rule + "\n";
 
   return styleSheet;
 }
 
 exports.createStyleSheet = createStyleSheet;