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

include.preload.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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 174 matching lines @@
   try
   {
     return element.contentDocument;
   }
   catch (e)
   {
     return null;
   }
 }
 
-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;
+    };
 
-    // Find all selectors that match any hidden element inside the given nodes.
-    for (var i = 0; i < this.selectors.length; i++)
+    if (document.readyState == "loading")
+      document.addEventListener("DOMContentLoaded", _start);
+    else
+      _start();
+  },
+
+ 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++)
+  checkNodes: function(nodes, filters)
+  {
+    let matchedFilters = [];
+
+    for (let [filter, selectors] of filters)
+    {
+      for (let selector of selectors)
       {
-        var elements = nodes[j].querySelectorAll(selector);
-        var matched = false;
+        for (let node of nodes)
+        {
+          let elements = node.querySelectorAll(selector);
+          let matched = false;
 
-        for (var k = 0; k < elements.length; k++)
-        {
-          // 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")
+          for (let element of elements)
           {
-            matchedSelectors.push(selector);
-            matched = true;
+            // 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;
   },
 
   observe: function(mutations)
   {
     // Forget previously changed nodes that are no longer in the DOM.
     for (var i = 0; i < this.changedNodes.length; i++)
     {
       if (!document.contains(this.changedNodes[i]))
@@ skipping 42 matching lines @@
 
     // Check only nodes whose descendants have changed, and not more often
     // than once a second. Otherwise large pages with a lot of DOM mutations
     // (like YouTube) freeze when the devtools panel is active.
     if (this.timeout == null)
       this.timeout = setTimeout(this.onTimeout.bind(this), 1000);
   },
 
   trace: function()
   {
-    this.checkNodes([document]);
+    this.checkNodes([document], this.filters);
 
     this.observer.observe(
       document,
       {
         childList: true,
         attributes: true,
         subtree: true
       }
     );
   },
@@ skipping 157 matching lines @@
             var shadow = shadowRoot(this);
             return shadow == ourShadowRoot ? null : shadow;
           }
         });
       }, null);
     }
 
     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
       // the shadow DOM if possible. Otherwise fallback to the <head> or
       // <html> element. If we have injected a style element before that
       // has been removed (the sheet property is null), create a new one.
       this.style = document.createElement("style");
       (this.shadow || document.head
                    || document.documentElement).appendChild(this.style);
 
@@ skipping 29 matching lines @@
     {
       var selector = selectors.slice(i, i + this.selectorGroupSize).join(", ");
       this.style.sheet.insertRule(selector + "{display: none !important;}",
                                   this.style.sheet.cssRules.length);
     }
   },
 
   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);
     });
   }
 };
 
 if (document instanceof HTMLDocument)
 {
   checkSitekey();
   wrapWebSocket();
 
   var elemhide = new ElemHide();
   elemhide.apply();
 
   document.addEventListener("error", function(event)
   {
     checkCollapse(event.target);
   }, true);
 
   document.addEventListener("load", function(event)
   {
     var element = event.target;
     if (/^i?frame$/.test(element.localName))
       checkCollapse(element);
   }, true);
 }