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)
 {
   if (!global.ext)
     global.ext = {};
+
+  var wrapperSymbol = Symbol("ext-wrapper");
 
   function wrapFrames(frames)
   {
     if (!frames.length)
       return null;
 
     // 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.

   {
     this._port = port;
     this._windowID = windowID;
   };
   EventTarget.prototype = {
     addListener: function(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)
           {
+            // We will only get here on the background side so we can access
+            // the Page object.
             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.

+      listener[wrapperSymbol] = wrapper;
       this._port.on("ext_message", wrapper);
     },
 
     removeListener: function(listener)
     {
-      if (listener._ext_wrapper)
-        this._port.off("ext_message", listener._ext_wrapper);
+      if (listener[wrapperSymbol])
+        this._port.off("ext_message", listener[wrapperSymbol]);
     }
   };
 
   if (typeof exports == "object")
     exports = global.ext;
 })(this);