Issue 3823 - Split up message responder code

lib/messaging.js

Index: lib/messaging.js
===================================================================
new file mode 100644
--- /dev/null
+++ b/lib/messaging.js
@@ -0,0 +1,112 @@
+/*
+ * 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/>.
+ */
+
+/** @module messaging */
+
+"use strict";
+
+/**
+ * Communication port wrapping ext.onMessage to receive messages.
+ *
+ * @constructor
+ */
+function Port()
+{
+  this._callbacks = Object.create(null);
+  ext.onMessage.addListener(this._onMessage.bind(this));
+};
+
+Port.prototype = {
+  _onMessage: function(message, sender, sendResponse)
+  {
+    let async = false;
+    let callbacks = this._callbacks[message.type] || [];
+
+    for (let callback of callbacks)
+    {
+      let response = callback(message, sender);
+
+      if (response instanceof Promise)

[Wladimir Palant, 2016/03/21 11:59:40]

This is a bad idea. There can be multiple declarations of the Promise class
(e.g. the native one and a shim, also the same class but in the context of a
different frame), and there can be other classes that should be treated like
promises - e.g. Task. It seems that the usual check is `response && typeof
response.then == "function"` (note how
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...
is talking about "thenable" values). Erik suggested encapsulating this check in
an isPromise() function in the other review.

[Sebastian Noack, 2016/03/21 13:26:02]

I don't think that this is a concern here, since this code is only used in the
background page, so Promise is always either the builtin, or always our
polyfill. But on the other hand the response cannot have any complex values
(like functions) anyway. So fine with me.

+      {
+        response.then(
+          sendResponse,
+          reason => {
+            console.error(reason);
+            sendResponse(undefined);

[Wladimir Palant, 2016/03/21 11:59:40]

I guess that you plan adding Port.emit() as well so that undefined is handled
properly? Currently the behavior is inconsistent: undefined will be suppressed
for synchronous responses but passed as a response to the caller for
asynchronous ones.

[Sebastian Noack, 2016/03/21 13:26:02]

Well, if the promise is rejected we have a bug anyway. So IMO it doesn't matter
what we do then. But yes, if we are going to implement Port.emit() and use it on
the other end we could properly handle that case. But even then I'm not sure
whether it will be worth it.

+          }
+        );
+        async = true;
+      }
+      else if (typeof response != "undefined")
+      {
+        sendResponse(response);
+      }
+    }
+
+    return async;
+  },
+
+  /**
+   * Function to be called when a particular message is received.
+   *
+   * @callback Port~messageCallback
+   * @param {object} message
+   * @param {object} sender
+   * @return The callback can return undefined (no response),
+   *         a value (response to be sent to sender immediately)
+   *         or a promise (asynchronous response).
+   */
+
+  /**
+   * Adds a callback for the specified message.
+   *
+   * The return value of the callback (if not undefined) is sent as response.
+   * @param {string}   name
+   * @param {Port~messageCallback} callback
+   */
+  on: function(name, callback)
+  {
+    if (name in this._callbacks)
+      this._callbacks[name].push(callback);

[Wladimir Palant, 2016/03/21 11:59:40]

The Firefox implementation also ensures that the same callback isn't added
twice. Without this check a callback could still be called even after calling
port.off() - because the second instance stays there. And of course it will be
called twice. IMHO this is inconsistent und unexpected behavior.

[Sebastian Noack, 2016/03/21 13:26:02]

Actually, the implementation and behavior here is consistent with node.js. And
FWIW I think it's a rather theoretical scenario with no real use case that
doesn't justify any addional complexity to be handled here.

[Wladimir Palant, 2016/03/21 13:58:01]

You are right. I verified that Add-on SDK eliminates duplicate listeners whereas
Node.js does not. Let's stick with Node.js behavior then, I'll adjust the
Firefox implementation accordingly.

+    else
+      this._callbacks[name] = [callback];
+  },
+
+  /**
+   * Removes a callback for the specified message.
+   *
+   * @param {string}   name
+   * @param {Port~messageCallback} callback
+   */
+  off: function(name, callback)
+  {
+    let callbacks = this._callbacks[name];
+    if (callbacks)
+    {
+      let idx = callbacks.indexOf(callback);
+      if (idx != -1)
+        callbacks.splice(idx, 1);
+    }
+  }
+};
+
+/**
+ * The default port to receive messages.
+ *
+ * @type {Port}
+ */
+exports.port = new Port();