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

lib/child/contentPolicy.js

Index: lib/child/contentPolicy.js
===================================================================
--- a/lib/child/contentPolicy.js
+++ b/lib/child/contentPolicy.js
@@ -32,16 +32,17 @@ try
 catch (e)
 {
   Cu.reportError(e);
 }
 
 let {XPCOMUtils} = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
 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
@@ -68,23 +69,18 @@ let storedNodes = new Map();
 let nodesIDPrefix = Services.appinfo.processID + " ";
 
 /**
  * Counter used to generate unique nodes identifiers in storeNodes().
  * @type number
  */
 let maxNodesID = 0;
 
-addMessageListener("AdblockPlus:DeleteNodes", onDeleteNodes);
-addMessageListener("AdblockPlus:RefilterNodes", onRefilterNodes);
-
-onShutdown.add(() => {
-  removeMessageListener("AdblockPlus:DeleteNodes", onDeleteNodes);
-  removeMessageListener("AdblockPlus:RefilterNodes", onRefilterNodes);
-});
+port.on("deleteNodes", onDeleteNodes);
+port.on("refilterNodes", onRefilterNodes);
 
 /**
  * Processes parent's response to the ShouldAllow message.
  * @param {nsIDOMWindow} window window that the request is associated with
  * @param {nsIDOMElement} node  DOM element that the request is associated with
  * @param {Object|undefined} response  object received as response
  * @return {Boolean} false if the request should be blocked
  */
@@ -131,17 +127,17 @@ function processPolicyResponse(window, n
  * @param {nsIDOMWindow} window
  * @param {nsIDOMElement} node
  * @param {String} contentType
  * @param {String} location location of the request, filter key if contentType is ELEMHIDE
  * @return {Boolean} false if the request should be blocked
  */
 let shouldAllow = exports.shouldAllow = function(window, node, contentType, location)
 {
-  return processPolicyResponse(window, node, sendSyncMessage("AdblockPlus:ShouldAllow", {
+  return processPolicyResponse(window, node, port.emitSync("shouldAllow", {
     contentType,
     location,
     frames: getFrames(window),
     isPrivate: isPrivate(window)
   }));
 };
 
 /**
@@ -150,22 +146,25 @@ let shouldAllow = exports.shouldAllow = 
  * @param {nsIDOMElement} node
  * @param {String} contentType
  * @param {String} location location of the request, filter key if contentType is ELEMHIDE
  * @param {Function} callback  callback to be called with a boolean value, if
  *                             false the request should be blocked
  */
 let shouldAllowAsync = exports.shouldAllowAsync = function(window, node, contentType, location, callback)
 {
-  sendAsyncMessage("AdblockPlus:ShouldAllow", {
+  port.emitWithResponse("shouldAllow", {
     contentType,
     location,
     frames: getFrames(window),
     isPrivate: isPrivate(window)
-  }, response => callback(processPolicyResponse(window, node, response)));
+  }).then(response =>
+  {
+    callback(processPolicyResponse(window, node, response));
+  });
 };
 
 /**
  * Stores nodes and generates a unique ID for them that can be used for
  * Policy.refilterNodes() later. It's important that Policy.deleteNodes() is
  * called later, otherwise the nodes will be leaked.
  * @param {DOMNode[]} nodes  list of nodes to be stored
  * @return {string}  unique ID for the nodes
@@ -175,27 +174,26 @@ let storeNodes = exports.storeNodes = fu
   let id = nodesIDPrefix + (++maxNodesID);
   storedNodes.set(id, nodes);
   return id;
 };
 
 /**
  * Called via message whenever Policy.deleteNodes() is called in the parent.
  */
-function onDeleteNodes(message)
+function onDeleteNodes(id, sender)
 {
-  storedNodes.delete(message.data);
+  storedNodes.delete(id);
 }
 
 /**
  * Called via message whenever Policy.refilterNodes() is called in the parent.
  */
-function onRefilterNodes(message)
+function onRefilterNodes({nodesID, entry}, sender)
 {
-  let {nodesID, entry} = message.data;
   let nodes = storedNodes.get(nodesID);
   if (nodes)
     for (let node of nodes)
       if (node.nodeType == Ci.nsIDOMNode.ELEMENT_NODE)
         Utils.runAsync(refilterNode.bind(this, node, entry));
 }
 
 /**
@@ -454,24 +452,33 @@ function schedulePostProcess(/**Element*
 }
 
 /**
  * Processes nodes scheduled for post-processing (typically hides them).
  */
 function postProcessNodes()
 {
   if (!collapsedClass)
-    collapsedClass = sendSyncMessage("AdblockPlus:GetCollapsedClass");
+  {
+    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...

 
-  if (!collapsedClass)
-    return;
-
   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);