Issue 3853 - Use new messaging API for the first-run page

ext/common.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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 (function(global)
 {
-  const Ci = Components.interfaces;
-
   if (!global.ext)
     global.ext = {};
 
-  var holder = {
-    get Page()
-    {
-      delete this.Page;
-      this.Page = (typeof require == "function" ?
-          require("ext_background").Page :
-          function() {});
-      return this.Page;
-    }
-  };
+  function wrapFrames(frames)
+  {
+    if (!frames.length)
+      return null;
 
-  var getSender = global.ext._getSender = function(origin)
+    // We have frames as an array, non-Firefox code expects url and parent
+    // properties however.
+    Object.defineProperty(frames, "url", {
+      enumerable: true,
+      get: () => new URL(frames[0].location)
+    });
+
+    Object.defineProperty(frames, "parent", {
+      enumerable: true,
+      get: () => wrapFrames(frames.slice(1))
+    });
+
+    return frames;
+  }
+
+  var EventTarget = global.ext._EventTarget = function(port, windowID)

[Thomas Greiner, 2016/04/18 13:22:50]

Detail: It looks a bit contradictory to export something with a "_" prefix
because that prefix is usually associated with internally used variables.

[Wladimir Palant, 2016/04/18 15:28:56]

It's used internally by the ext layer, not meant for external consumption.

   {
-    if (origin instanceof Ci.nsIDOMXULElement)
-      return origin.messageManager;
-    else if (origin instanceof Ci.nsIMessageSender)
-      return origin;
-    else
-      return null;
-  };
-
-  var MessageProxy = global.ext._MessageProxy = function(messageManager, messageTarget)
-  {
-    this._messageManager = messageManager;
-    this._messageTarget = messageTarget;
-    this._callbacks = new Map();
-    this._responseCallbackCounter = 0;
-
-    this._handleRequest = this._handleRequest.bind(this);
-    this._handleResponse = this._handleResponse.bind(this);
-    this._messageManager.addMessageListener("AdblockPlus:Message", this._handleRequest);
-    this._messageManager.addMessageListener("AdblockPlus:Response", this._handleResponse);
-  };
-  MessageProxy.prototype = {
-    _disconnect: function()
-    {
-      this._messageManager.removeMessageListener("AdblockPlus:Message", this._handleRequest);
-      this._messageManager.removeMessageListener("AdblockPlus:Response", this._handleResponse);
-    },
-
-    _sendResponse: function(sender, callbackId, message)
-    {
-      var response = {
-        callbackId: callbackId
-      };
-      if (typeof response != "undefined")
-        response.payload = message;
-      sender.sendAsyncMessage("AdblockPlus:Response", response);
-    },
-
-    _handleRequest: function(message)
-    {
-      var sender = getSender(message.target);
-      var request = message.data;
-
-      var sent = false;
-      var sendResponse;
-      if (sender && "callbackId" in request)
-      {
-        sendResponse = function(message)
-        {
-          this._sendResponse(sender, request.callbackId, message);
-          sent = true;
-        }.bind(this);
-      }
-      else
-        sendResponse = function() {};
-
-      var results = this._messageTarget._dispatch(request.payload, {
-        page: new holder.Page(sender)
-      }, sendResponse);
-      if (!sent && results.indexOf(true) == -1)
-        sendResponse(undefined);
-    },
-
-    _handleResponse: function(message)
-    {
-      var response = message.data;
-      var callback = this._callbacks.get(response.callbackId);
-      if (callback)
-      {
-        this._callbacks.delete(response.callbackId);
-        if ("payload" in response)
-          callback(response.payload);
-      }
-    },
-
-    sendMessage: function(message, responseCallback)
-    {
-      if (!(this._messageManager instanceof Ci.nsIMessageSender))
-        throw new Error("Not implemented");
-
-      var request = {
-        payload: message
-      };
-      if (responseCallback)
-      {
-        request.callbackId = ++this._responseCallbackCounter;
-        this._callbacks.set(request.callbackId, responseCallback);
-      }
-
-      this._messageManager.sendAsyncMessage("AdblockPlus:Message", request);
-    }
-  };
-
-  var EventTarget = global.ext._EventTarget = function()
-  {
-    this._listeners = [];
+    this._port = port;
+    this._windowID = windowID;
   };
   EventTarget.prototype = {
     addListener: function(listener)
     {
-      if (this._listeners.indexOf(listener) == -1)
-        this._listeners.push(listener);
+      var wrapper = (message, sender) =>

[Thomas Greiner, 2016/04/18 13:22:50]

Detail: I noticed that throughout this review you use `var` in situations where
you'd usually use `let` and that you don't use destructuring. So I wonder what's
up with that?

[Wladimir Palant, 2016/04/18 15:28:56]

The first-run page loads the files via usual <script> tags, without version=1.7
(the latter wouldn't work in Chrome). So let statements are off limits.

[Thomas Greiner, 2016/04/18 17:47:14]

Doesn't the same then also apply to `const`? (see
https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/1.7...)

[Wladimir Palant, 2016/04/19 11:57:28]

It doesn't, Firefox has been supporting const in "regular" JavaScript for a
while, let statements on the other hand only with Firefox 44.

+      {
+        if (this._windowID && this._windowID != message.targetID)
+          return undefined;
+
+        return new Promise((resolve, reject) =>
+        {
+          var sender = {};
+          if (message.senderID)
+          {
+            const Page = require("ext_background").Page;

[Thomas Greiner, 2016/04/18 13:22:50]

Detail: I'm wondering why `Page` is in "ext_background" in the first place and
not in here if it's shared functionality.

[Wladimir Palant, 2016/04/18 15:28:56]

It's not shared functionality - pages only exist for the background "page",
there is no such concept for content.

[Thomas Greiner, 2016/04/18 17:47:14]

Ok, since this part of the code is only executed in the context of the
background page, mind adding a short comment to clarify that?

+            sender.page = new Page(message.senderID);
+          }
+          if (message.frames)
+            sender.frame = wrapFrames(message.frames);
+          if (!listener(message.payload, sender, resolve))
+            resolve(undefined);
+        });
+      };
+      listener._ext_wrapper = wrapper;

[Thomas Greiner, 2016/04/18 13:22:50]

Detail: This property name is not in accordance to JavaScript naming
conventions.

Note that we could use a `Symbol` as a property name instead to avoid potential
conflicts.

[Wladimir Palant, 2016/04/18 15:28:56]

Done.

+      this._port.on("ext_message", wrapper);
     },
+
     removeListener: function(listener)
     {
-      var idx = this._listeners.indexOf(listener);
-      if (idx != -1)
-        this._listeners.splice(idx, 1);
-    },
-    _dispatch: function()
-    {
-      var results = [];
-
-      for (var i = 0; i < this._listeners.length; i++)
-        results.push(this._listeners[i].apply(null, arguments));
-
-      return results;
+      if (listener._ext_wrapper)
+        this._port.off("ext_message", listener._ext_wrapper);
     }
   };
 
   if (typeof exports == "object")
     exports = global.ext;
 })(this);