Issue 3823 - Split up message responder code

popup.js

Index: popup.js
===================================================================
--- a/popup.js
+++ b/popup.js
@@ -23,6 +23,7 @@
 var Prefs = require("prefs").Prefs;
 var checkWhitelisted = require("whitelisting").checkWhitelisted;
 var getDecodedHostname = require("url").getDecodedHostname;
+var port = require("messaging").port;
 
 var page = null;
 
@@ -57,7 +58,7 @@
   });
 
   // Attach event listeners
-  ext.onMessage.addListener(onMessage);
+  port.on("composer.ready", onComposerReady);
   document.getElementById("enabled").addEventListener("click", toggleEnabled, false);
   document.getElementById("clickhide").addEventListener("click", activateClickHide, false);
   document.getElementById("clickhide-cancel").addEventListener("click", cancelClickHide, false);
@@ -79,12 +80,12 @@
 
 function onUnload()
 {
-  ext.onMessage.removeListener(onMessage);
+  port.off("composer.ready", onComposerReady);

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

IMHO, this shouldn't be necessary - each context (here: window) should have its
own port that should detach automatically when that context becomes invalid. In
Chrome this probably means no default port but rather:

  var port = require("messaging").getPort(window);

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

If we would use window.chrome.runtime.onMessage under the hood that shouldn't be
necessary. However, since we currently have no ways to load modules in the
content process, require() and the modules it returns are always the objects
from the background page. Hence extension APIs called under the hood are invoked
in the context of the background page.

Moreover, we currently import the ext namespace from the background page, rather
than including those scripts in the browser action popup. I only vaguely
remember the reason for that, but this makes using the chrome API in the context
of the popup (rather than the background page) even more difficult.

Yes, as a workaround we could implement something like getPort(window) which
essentially would have to register an unload event to detach message listeners.
But that would just move this hack from one place to another introducing even
more complexity. Also note that the code here, including this hack, might likely
get refactored in order to use asynchronous messaging, at some point, anyway.

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

I disagree. Current API requires developers to remember unregistering event
handlers, if they don't bad things will happen. This just isn't a good way to
design an API.

Sure, you'll need a hack in the API implementation - but it will only be one
hack (a fairly simply one), and you could easily make sure it is implemented
properly. Much better than having hacks everywhere the API is used.

If the code is refactored later and things will work "naturally" - all the
better.

[Sebastian Noack, 2016/03/21 15:26:59]

Done.

 }
 
-function onMessage(message, sender, callback)
+function onComposerReady(message, sender)
 {
-  if (message.type == "composer.ready" && sender.page.id == page.id)
+  if (sender.page.id == page.id)
     document.body.classList.remove("nohtml");
 }