[adblockpluschrome] Issue 3596 - Added support for CSS property filters to devtools panel

include.preload.js

Index: include.preload.js
diff --git a/include.preload.js b/include.preload.js
index 86e52c32afceb894f6591a84b0eca3b1842383ce..27b1229c07904935599647e23179bf5b931cc52c 100644
--- a/include.preload.js
+++ b/include.preload.js
@@ -194,7 +194,7 @@ function getContentDocument(element)
 
 function ElementHidingTracer(selectors)
 {
-  this.selectors = selectors;
+  this.selectors = selectors || [];

[Sebastian Noack, 2017/01/11 16:20:40]

It seems you removed the only code that initialized the ElementHidingTracer
without arguments. So unless I missed something, why not just removing support
for the "selectors" argument?

[wspee, 2017/01/12 13:39:58]

No longer relevant.

[wspee, 2017/01/19 16:20:14]

Done.

 
   this.changedNodes = [];
   this.timeout = null;
@@ -219,7 +219,7 @@ ElementHidingTracer.prototype = {
 
       for (var j = 0; j < nodes.length; j++)
       {
-        var elements = nodes[j].querySelectorAll(selector);
+        var elements = nodes[j].querySelectorAll(selector.selector);
         var matched = false;
 
         for (var k = 0; k < elements.length; k++)
@@ -229,7 +229,10 @@ ElementHidingTracer.prototype = {
           // priority, or haven't been circumvented in a different way.
           if (getComputedStyle(elements[k]).display == "none")
           {
-            matchedSelectors.push(selector);
+            if (selector.filter)
+              matchedSelectors.push(selector.filter.replace(/^.*?##/, ""));
+            else
+              matchedSelectors.push(selector.selector);
             matched = true;
             break;
           }
@@ -490,11 +493,31 @@ ElemHide.prototype = {
     return shadow;
   },
 
-  addSelectors: function(selectors)
+  addSelectors: function(selectors, filters)
   {
     if (selectors.length == 0)
       return;
 
+    if (this.tracer != null)
+    {
+      // The selector is beeing used to identify the filter that is responsible
+      // for hiding a particular element in the devtools panel. The
+      // ElementHidingTracer uses the selector to figure out if a filter is
+      // used on the current document. In case of an ElemHideEmulationFilter
+      // the selector doesn't match the selector of the filter
+      // ([-abp-properties=...] vs actual css selector depending on the sites
+      // css). The To still allow the devtools panel to find the correct filter
+      // we pass both the selector and the filter string to the
+      // ElementHidingTracer.
+      for (var i = 0; i < selectors.length; i++)
+      {
+        this.tracer.selectors.push({

[Sebastian Noack, 2017/01/11 16:20:40]

Is it guaranteed that the order of selectors match the order of filters? I have
some doubts from looking at the code in elemHideEmulation.js:

  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));
  },

Note that objects in JavaScript don't guarantee properties to be retrieved in
insertion order (like dicts in Python). However, we could simply use a Set in
elemHideEmulation.js which preserves the insertion order and seems more
appropriate anyway, but wasn't supported by all supported browsers we run that
code on back then.

[Sebastian Noack, 2017/01/11 16:20:40]

Creating a new object for each selector might potentially add a significant
memory footprint. A possibly better solution would be to use two arrays, one for
the selectors and one for the filters. Perhaps you could benchmark the memory
usage with the current approach and compare it against the one I'm suggesting.

[wspee, 2017/01/12 13:39:58]

Indeed ... if you look at findSelectors there can even be more selectors than
filters.

findSelectors: function(stylesheet, selectors, filters)
{
  ...
        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;
  ...
},

So I have switched the whole approach to something else entirely ....

ElemHideEmulationFilter are only created if there is a style that matches the
filter and document changes are also handled by ElemHideEmulation so it wasn't
really necessary to use the ElementHidingTracer for ElemHideEmulationFilter.

Instead I let addSelectors fire devtools.traceElemHide directly similar to how
it works in firefox
(adblockplus/lib/child/elemHideEmulation.js:initElemHideEmulation).

Only drawback is that devtools.traceElemHide gets fired regardless of whether
the devtools are opened or not. I could add a flag to ElemHide but that whould
be somewhat redundant with ElemHide.tracer but I cannot test ElemHide.tracer as
it gets assigned after addSelectors is called by ElemHideEmulation. But
logHiddenElements does (should have?!) check if the panel is opened anyways so
it's only a slight penalty. Let me now if you think otherwise.

[wspee, 2017/01/12 13:39:58]

No longer relevant.

[wspee, 2017/01/19 16:20:14]

According to the chrome profiler an array with 10000 objects uses less memory
than two arrays with 10000 entries, so I kept that approach.

[wspee, 2017/01/19 16:20:14]

Done.

+          selector: selectors[i],
+          filter: filters ? filters[i] : null
+        });
+      }
+    }
+
     if (!this.style)
     {
       // Create <style> element lazily, only if we add styles. Add it to
@@ -544,38 +567,32 @@ ElemHide.prototype = {
   apply: function()
   {
     var selectors = null;
-    var elemHideEmulationLoaded = false;
 
     var checkLoaded = function()
     {
-      if (!selectors || !elemHideEmulationLoaded)
-        return;
-
       if (this.tracer)
         this.tracer.disconnect();
       this.tracer = null;
 
+      if (selectors.trace)
+        this.tracer = new ElementHidingTracer();
+
       if (this.style && this.style.parentElement)
         this.style.parentElement.removeChild(this.style);
       this.style = null;
 
-      this.addSelectors(selectors.selectors);
+      if (selectors.selectors)
+        this.addSelectors(selectors.selectors);
       this.elemHideEmulation.apply();
 
-      if (selectors.trace)
-        this.tracer = new ElementHidingTracer(selectors.selectors);
+      if (this.tracer && this.tracer.selectors.length)

[Sebastian Noack, 2017/01/11 16:20:40]

Any particular reason (except for a hypothetical micro-optimization) to check
for this.tracer.selectors.length?

[wspee, 2017/01/12 13:39:58]

No longer relevant.

[wspee, 2017/01/19 16:20:15]

Done.

+        this.tracer.checkNodes([document]);
     }.bind(this);
 
-    ext.backgroundPage.sendMessage({type: "get-selectors"}, function(response)
+    ext.backgroundPage.sendMessage({type: "get-selectors"}, (response) =>

[Sebastian Noack, 2017/01/11 16:20:40]

The parentheses around the "response" argument are redundant.

[wspee, 2017/01/12 13:39:58]

No longer relevant.

[wspee, 2017/01/19 16:20:14]

Done.

     {
       selectors = response;
-      checkLoaded();
-    });
-
-    this.elemHideEmulation.load(function()
-    {
-      elemHideEmulationLoaded = true;
-      checkLoaded();
+      this.elemHideEmulation.load(() => { checkLoaded(); });

[Sebastian Noack, 2017/01/11 16:20:40]

The arrow function seems redundant. Why not just passing checkLoaded without
wrapping to the load() method?

[wspee, 2017/01/12 13:39:58]

No longer relevant.

[wspee, 2017/01/19 16:20:14]

Done.

     });
   }
 };