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 257 matching lines @@
             filtersByDomain.delete(domain);
         }
       }
     }
 
     knownFilters.delete(filter);
     filterNotifier.emit("elemhideupdate");
   },
 
   /**
-   * 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 = 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.

+    let code = specificOnly ? createStyleSheet(selectors) :
+                 (getDefaultStyleSheet() + createStyleSheet(selectors));
 
     if (!specificOnly)
       selectors = getUnconditionalSelectors().concat(selectors);
 
     return {code, selectors};
   }
 };
 
 /**
  * Splits a list of selectors into groups determined by the value of
@@ skipping 13 matching lines @@
   // calculate the sizes of the selectors and divide them up accordingly, but
   // this approach is more efficient and has worked well in practice. In theory
   // this could still lead to some selectors not working on Chromium, but it is
   // highly unlikely.
   // See issue #6298 and https://crbug.com/804179
   for (let i = 0; i < selectors.length; i += selectorGroupSize)
     yield selectors.slice(i, i + selectorGroupSize);
 }
 
 /**
- * Creates element hiding CSS rules for a given list of selectors. Each rule
- * contains no more than the maximum number of selectors as determined by the
- * value of <code>{@link selectorGroupSize}</code>.
+ * Creates an element hiding CSS rule for a given list of selectors.
  *
  * @param {Array.<string>} selectors
- * @yields {string}
- */
-function* createRules(selectors)
-{
-  for (let selectorGroup of splitSelectors(selectors))
-    yield selectorGroup.join(", ") + " {display: none !important;}";
+ * @returns {string}
+ */
+function createRule(selectors)
+{
+  let rule = "";
+
+  for (let i = 0; i < selectors.length - 1; i++)
+    rule += selectors[i] + ", ";
+
+  rule += selectors[selectors.length - 1] + " {display: none !important;}\n";
+
+  return rule;
 }
 
 /**
  * Creates an element hiding CSS style sheet from a given list of selectors.
  * @param {Array.<string>} selectors
  * @returns {string}
  */
 function createStyleSheet(selectors)
 {
   let styleSheet = "";
 
-  for (let rule of createRules(selectors))
-    styleSheet += rule + "\n";
+  for (let selectorGroup of splitSelectors(selectors))
+    styleSheet += createRule(selectorGroup);
 
   return styleSheet;
 }
 
 exports.createStyleSheet = createStyleSheet;