Issue 3499 - Use new messaging API in the content policy implementation

lib/child/contentPolicy.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 27 matching lines @@
 let {Services} = Cu.import("resource://gre/modules/Services.jsm", {});
 
 let {port} = require("messaging");
 let {Utils} = require("utils");
 let {getFrames, isPrivate} = require("child/utils");
 let {objectMouseEventHander} = require("child/objectTabs");
 let {RequestNotifier} = require("child/requestNotifier");
 
 /**
  * Randomly generated class name, to be applied to collapsed nodes.
- * @type string
- */
-let collapsedClass = null;
+ * @type Promise.<string>
+ */
+let collapsedClass = port.emitWithResponse("getCollapsedClass");
 
 /**
  * Maps numerical content type IDs to strings.
  * @type Map.<number,string>
  */
 let types = new Map();
 
 /**
  * Contains nodes stored by storeNodes() mapped by their IDs.
  * @type Map.<string,DOMNode[]>
@@ skipping 388 matching lines @@
     scheduledNodes = [node];
     Utils.runAsync(postProcessNodes);
   }
 }
 
 /**
  * Processes nodes scheduled for post-processing (typically hides them).
  */
 function postProcessNodes()
 {
-  if (!collapsedClass)
-  {
-    port.emitWithResponse("getCollapsedClass").then(cls =>
-    {
-      // We might have sent this message multiple times, ignore response if a
-      // previous response was already processed.
-      if (collapsedClass)
-        return;
-
-      collapsedClass = cls;
-      postProcessNodes();
-    });
-    return;
-  }
-
-  let nodes = scheduledNodes;
-  scheduledNodes = null;

[Erik, 2016/03/15 20:57:41]

It appears that `postProcessNodes` is run twice now instead of once before
during the first execution, and won't this `scheduledNodes` value be incorrect
when `postProcessNodes` is run a second time?

Either way the code is a little confusing to me, I wonder if there is a less
confusing way to do this?

Perhaps we can generate the class when the add-on starts up and pass that to the
`lib/child/bootstrap.js` file when it is loaded? or leave this to another time
and just use a sync message here for the time being?

[Wladimir Palant, 2016/03/16 10:13:00]

No, we don't run this code multiple times (return statement above is important)
- we merely delay it until the class is available. While scheduledNodes list
might increase in the meantime, that's not an issue because all nodes will be
processed.

Passing parameters to child/bootstrap.js will break code locality - all the
sudden the involved parties are no longer contentPolicy.js and
child/contentPolicy.js but also main.js and child/bootstrap.js. Using
synchronous messaging is also not really an option, this was only synchronous
here originally because async messaging with responses was very complex.

Let's try promises, this will hopefully make this easier to follow...

-
-  for (let node of nodes)
-  {
-    // adjust frameset's cols/rows for frames
-    let parentNode = node.parentNode;
-    if (parentNode && parentNode instanceof Ci.nsIDOMHTMLFrameSetElement)
-    {
-      let hasCols = (parentNode.cols && parentNode.cols.indexOf(",") > 0);
-      let hasRows = (parentNode.rows && parentNode.rows.indexOf(",") > 0);
-      if ((hasCols || hasRows) && !(hasCols && hasRows))
+  collapsedClass.then(cls =>

[Thomas Greiner, 2016/03/23 18:47:59]

Detail: Anything wrong with calling it "collapsedClass" instead of "cls"?
Because the promise is just a different representation of that same value.

[Wladimir Palant, 2016/03/24 07:27:26]

They are not the same thing however. I would consider this highly confusing if
collapsedClass could refer to both the promise (variable outside the closure)
and actual value (parameter).

+  {
+    let nodes = scheduledNodes;
+    scheduledNodes = null;
+
+    // Resolving class is async initially so the nodes might have already been
+    // processed in the meantime.
+    if (!nodes)
+      return;
+
+    for (let node of nodes)
+    {
+      // adjust frameset's cols/rows for frames
+      let parentNode = node.parentNode;
+      if (parentNode && parentNode instanceof Ci.nsIDOMHTMLFrameSetElement)
       {
-        let index = -1;
-        for (let frame = node; frame; frame = frame.previousSibling)
-          if (frame instanceof Ci.nsIDOMHTMLFrameElement || frame instanceof Ci.nsIDOMHTMLFrameSetElement)
-            index++;
-
-        let property = (hasCols ? "cols" : "rows");
-        let weights = parentNode[property].split(",");
-        weights[index] = "0";
-        parentNode[property] = weights.join(",");
+        let hasCols = (parentNode.cols && parentNode.cols.indexOf(",") > 0);
+        let hasRows = (parentNode.rows && parentNode.rows.indexOf(",") > 0);
+        if ((hasCols || hasRows) && !(hasCols && hasRows))
+        {
+          let index = -1;
+          for (let frame = node; frame; frame = frame.previousSibling)
+            if (frame instanceof Ci.nsIDOMHTMLFrameElement || frame instanceof Ci.nsIDOMHTMLFrameSetElement)
+              index++;
+
+          let property = (hasCols ? "cols" : "rows");
+          let weights = parentNode[property].split(",");
+          weights[index] = "0";
+          parentNode[property] = weights.join(",");
+        }
       }
-    }
-    else
-      node.classList.add(collapsedClass);
-  }
-}
+      else
+        node.classList.add(cls);
+    }
+  });
+}