[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..5814884c969f8d495ca3c0bc58a1d0466a832b58 100644
--- a/include.preload.js
+++ b/include.preload.js
@@ -192,64 +192,88 @@ function getContentDocument(element)
   }
 }
 
-function ElementHidingTracer(selectors)
+function ElementHidingTracer()
 {
-  this.selectors = selectors;
+  this.filters = new Map();

[Sebastian Noack, 2017/02/09 11:07:39]

Why to use a a Map object? It seems the logic would be much simpler, if you just
have one array for selectors and one for the filters?

[wspee, 2017/02/10 10:46:46]

If you have information that are are dependent from one another it makes sense
to also use a data structure to reflects that. The selectors belong to a filter
so it makes sense to use a key/value data structure for that. The performance
arguments we used against a map previously don't apply here because (as it
stands now) the ElementHidingTracer will hold all ElementHiding filters in
memory anyways (could change that as well but might be out of scope) which is OK
because it will only do that if the panel is opened.

But a an array of arrays is actually simpler here than an map because you don't
have to use nested loops to iterate over all filters and its maps straighter to
an two arrays so the conversion is simpler. The dependency between filter and
selector will still be clear.

So I changed the approach to use a [[filter_a, selector_a_1], [filter_a,
selector_a_2]] instead of a Map([filter_a, [selector_a_1, selector_a_2]]).

What do you think?

[Sebastian Noack, 2017/02/10 14:52:55]

I'd generally agree, that data that have a mutual dependency should be stored in
the same data structure. Though, an n-dimensional array isn't the most efficient
data structure in JavaScript, as each array is a first-class object with notable
memory boilerplate. But more importantly, elemHideEmulation.js works with two
separate arrays, and translating these into a two-dimensional array (or Map
previously) adds quite some complexity, with not much of a benefit since we
access those data only once. Not to mention the leaky abstraction where you
store selectors (redundantly) as filter when the filter is unknown.

 
   this.changedNodes = [];
   this.timeout = null;
+  this.started = false;
 
   this.observer = new MutationObserver(this.observe.bind(this));
   this.trace = this.trace.bind(this);
-
-  if (document.readyState == "loading")
-    document.addEventListener("DOMContentLoaded", this.trace);
-  else
-    this.trace();
 }
 ElementHidingTracer.prototype = {
-  checkNodes: function(nodes)
+  start: function()
   {
-    var matchedSelectors = [];
+    let _start = () => {
+      this.trace();
+      this.started = true;
+    };
+
+    if (document.readyState == "loading")
+      document.addEventListener("DOMContentLoaded", _start);
+    else
+      _start();
+  },
 
-    // Find all selectors that match any hidden element inside the given nodes.
-    for (var i = 0; i < this.selectors.length; i++)
+ addFilters: function(filters)
+  {
+    if (this.started)
+      window.setTimeout(() => {
+        this.checkNodes([document], filters);
+      }, 0);
+
+    for (let [key, value] of filters)
     {
-      var selector = this.selectors[i];
+      if (!this.filters.has(key))
+        this.filters.set(key, []);
+      Array.prototype.push.apply(this.filters.get(key), value);
+    }
+  },
 
-      for (var j = 0; j < nodes.length; j++)
-      {
-        var elements = nodes[j].querySelectorAll(selector);
-        var matched = false;
+  checkNodes: function(nodes, filters)
+  {
+    let matchedFilters = [];
 
-        for (var k = 0; k < elements.length; k++)
+    for (let [filter, selectors] of filters)
+    {
+      for (let selector of selectors)
+      {
+        for (let node of nodes)
         {
-          // Only consider selectors that actually have an effect on the
-          // computed styles, and aren't overridden by rules with higher
-          // priority, or haven't been circumvented in a different way.
-          if (getComputedStyle(elements[k]).display == "none")
+          let elements = node.querySelectorAll(selector);
+          let matched = false;
+
+          for (let element of elements)
           {
-            matchedSelectors.push(selector);
-            matched = true;
-            break;
+            // Only consider selectors that actually have an effect on the
+            // computed styles, and aren't overridden by rules with higher
+            // priority, or haven't been circumvented in a different way.
+            if (getComputedStyle(element).display == "none")
+            {
+              matchedFilters.push(filter.replace(/^.*?##/, ""));
+              matched = true;

[Sebastian Noack, 2017/02/09 11:07:39]

It seems, the "match" variable would be unnecessary, if you break out of the
outer loop here, directly:

  nodes: for (let node of nodes)
  {
    for (let element of elements)
    {
        ...
        break nodes;
      }
    }
  }

[wspee, 2017/02/10 10:46:46]

Done.

+              break;
+            }
           }
-        }
 
-        if (matched)
-          break;
+          if (matched)
+            break;
+        }
       }
     }
 
-    if (matchedSelectors.length > 0)
+    if (matchedFilters.length > 0)
       ext.backgroundPage.sendMessage({
         type: "devtools.traceElemHide",
-        selectors: matchedSelectors
+        selectors: matchedFilters
       });
   },
 
   onTimeout: function()
   {
-    this.checkNodes(this.changedNodes);
+    this.checkNodes(this.changedNodes, this.filters);
     this.changedNodes = [];
     this.timeout = null;
   },
@@ -312,7 +336,7 @@ ElementHidingTracer.prototype = {
 
   trace: function()
   {
-    this.checkNodes([document]);
+    this.checkNodes([document], this.filters);
 
     this.observer.observe(
       document,
@@ -490,11 +514,28 @@ ElemHide.prototype = {
     return shadow;
   },
 
-  addSelectors: function(selectors)
+  addSelectors: function(selectors, filters)
   {
-    if (selectors.length == 0)
+    if (!selectors)

[Sebastian Noack, 2017/02/09 11:07:39]

Why did you change this line? Note that an empty array evaluates to true.

[wspee, 2017/02/10 10:46:46]

Done.

       return;
 
+    if (this.tracer)
+    {
+      let tracedFilters = new Map();
+      for (let i = 0; i < selectors.length; i++)
+      {
+        let filterKey = filters ? filters[i] : selectors[i];
+        let tracedFilter = tracedFilters.get(filterKey);
+        if (!tracedFilter)
+        {
+          tracedFilter = [];
+          tracedFilters.set(filterKey, tracedFilter);
+        }
+        tracedFilter.push(selectors[i]);
+      }
+      this.tracer.addFilters(tracedFilters);
+    }
+
     if (!this.style)
     {
       // Create <style> element lazily, only if we add styles. Add it to
@@ -544,38 +585,33 @@ 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.start();
     }.bind(this);
 
-    ext.backgroundPage.sendMessage({type: "get-selectors"}, function(response)
+    ext.backgroundPage.sendMessage({type: "get-selectors"}, response =>
     {
       selectors = response;
-      checkLoaded();
-    });
-
-    this.elemHideEmulation.load(function()
-    {
-      elemHideEmulationLoaded = true;
-      checkLoaded();
+      this.elemHideEmulation.load(checkLoaded);
     });
   }
 };