[adblockpluscore] Issue 4825 - Let ElemHideEmulation associate selector and filter when calling add…

chrome/content/elemHideEmulation.js

 var propertySelectorRegExp = /\[\-abp\-properties=(["'])([^"']+)\1\]/;
 
 function splitSelector(selector)
 {
   if (selector.indexOf(",") == -1)
     return [selector];
 
   var selectors = [];
   var start = 0;
   var level = 0;
@@ skipping 55 matching lines @@
     {
       return new URL(stylesheet.href).origin == this.window.location.origin;
     }
     catch (e)
     {
       // Invalid URL, assume that it is first-party.
       return true;
     }
   },
 
-  findSelectors: function(stylesheet, selectors, filters)
+  findSelectors: function(stylesheet, filters)
   {
     // Explicitly ignore third-party stylesheets to ensure consistent behavior
     // between Firefox and Chrome.
     if (!this.isSameOrigin(stylesheet))
       return;
 
-    var rules = stylesheet.cssRules;
+    let rules = stylesheet.cssRules;
     if (!rules)
       return;
 
-    for (var i = 0; i < rules.length; i++)
+    for (let rule of rules)
     {
-      var rule = rules[i];
       if (rule.type != rule.STYLE_RULE)
         continue;
 
-      var style = this.stringifyStyle(rule.style);
-      for (var j = 0; j < this.patterns.length; j++)
+      let style = this.stringifyStyle(rule.style);
+

[Sebastian Noack, 2017/02/07 15:37:57]

Nit: The blank line added here is unrelated at least, and arguably doesn't make
the code any more readable.

[wspee, 2017/02/09 08:40:34]

Done.

+      for (let pattern of this.patterns)
       {
-        var pattern = this.patterns[j];
         if (pattern.regexp.test(style))
         {
-          var subSelectors = splitSelector(rule.selectorText);
-          for (var k = 0; k < subSelectors.length; k++)
-            selectors.push(pattern.prefix + subSelectors[k] + pattern.suffix);
-          filters[pattern.text] = true;
+          let selectors = filters.get(pattern.text);
+          if (!selectors)
+          {
+            selectors = [];
+            filters.set(pattern.text, selectors);
+          }
+
+          for (let subSelector of splitSelector(rule.selectorText))
+            selectors.push(pattern.prefix + subSelector + pattern.suffix);
         }
       }
     }
   },
 
   addSelectors: function(stylesheets)
   {
-    var selectors = [];
-    var filters = {};
-    for (var i = 0; i < stylesheets.length; i++)
-      this.findSelectors(stylesheets[i], selectors, filters);
-    this.addSelectorsFunc(selectors, Object.keys(filters));
+    let filters = new Map();
+    for (let stylesheet of stylesheets)
+      this.findSelectors(stylesheet, filters);
+    this.addSelectorsFunc(filters);

[Sebastian Noack, 2017/02/07 15:37:57]

I just realized, the implications for when this function is called for regular
element hiding. In that case we just have a plain array of selectors. While we
could potentially always pass a Map object, we don't need the addional metadata
for regular element hiding, and performance is much more a concern there than
when handling emulation filters. So I think the first argument should still be a
plain array of selectors, and a second optional argument should specify the
addional metadata in case of element hiding emulation.

However, if we pass just the selectors as first argument and then the current
filters map as second argument, those data structures will have redundancy,
which is not great. So it seems the most appropriate data structure for the
second argument would be an array of filters that correspond to the selectors in
the first argument. Internally, you may use a Map object, using the active
selectors as keys, and the corresponding filter as the respective value.

Does that sound about right? I may still have missed something.

[Sebastian Noack, 2017/02/07 15:59:56]

Another advantage of this approach is, that the API remains backwards
compatible. Instead of a unique list of the corresponding filters in arbitrary
order, you merely get a list of filters whose index match their resulting
selector. So Adblock Plus for Firefox will just keep working with its current
limitations, without further changes. And that should be good enough, as it gets
replaced with the code in adblockpluschrome anyway, later this year.

The only potential limitation I can see with this approach is, that if more than
one emulation filter will result into the same selector, only one of the filters
gets logged. But that is consistent with blocking filters, where it has never
been an issue. If a request matches more than one blocking filter, the matcher
bails after the first match, and only the first filter match is recorded.

[Sebastian Noack, 2017/02/08 07:12:29]

Perhaps even better, just use two arrays (instead of a Map object), one for the
selectors, and one for the filters. If a selector is generated, also add the
filter, and just pass both arrays to addSelectorsFunc(). This would be even
simpler, and in the case there are multiple filters that result into the same
selector, both are logged.

[wspee, 2017/02/09 08:40:34]

Done.

   },
 
   onLoad: function(event)
   {
     var stylesheet = event.target.sheet;
     if (stylesheet)
       this.addSelectors([stylesheet]);
   },
 
   load: function(callback)
@@ skipping 32 matching lines @@
   apply: function()
   {
     if (this.patterns.length > 0)
     {
       var document = this.window.document;
       this.addSelectors(document.styleSheets);
       document.addEventListener("load", this.onLoad.bind(this), true);
     }
   }
 };