Issue 3823 - Split up message responder code

popup.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/>.
  */
 
 var backgroundPage = ext.backgroundPage.getWindow();
 var require = backgroundPage.require;
 
 var Filter = require("filterClasses").Filter;
 var FilterStorage = require("filterStorage").FilterStorage;
 var Prefs = require("prefs").Prefs;
 var checkWhitelisted = require("whitelisting").checkWhitelisted;
 var getDecodedHostname = require("url").getDecodedHostname;
-var port = require("messaging").port;
 
 var page = null;
 
 function onLoad()
 {
   ext.pages.query({active: true, lastFocusedWindow: true}, function(pages)
   {
     page = pages[0];
 
     // Mark page as 'local' or 'nohtml' to hide non-relevant elements
     if (!page || (page.url.protocol != "http:" &&
                   page.url.protocol != "https:"))
       document.body.classList.add("local");
     else if (!backgroundPage.htmlPages.has(page))
+    {
       document.body.classList.add("nohtml");
+      require("messaging").getPort(window).on(
+        "composer.ready", function(message, sender)
+        {
+          if (sender.page.id == page.id)
+            document.body.classList.remove("nohtml");
+        }
+      );
+    }
 
     // Ask content script whether clickhide is active. If so, show cancel button.
     // If that isn't the case, ask background.html whether it has cached filters. If so,
     // ask the user whether she wants those filters.
     // Otherwise, we are in default state.
     if (page)
     {
       if (checkWhitelisted(page))
         document.body.classList.add("disabled");
 
       page.sendMessage({type: "composer.content.getState"}, function(response)
       {
         if (response && response.active)
           document.body.classList.add("clickhide-active");
       });
     }
   });
 
-  // Attach event listeners
-  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);
   document.getElementById("options").addEventListener("click", function()
   {
     ext.showOptions();
   }, false);
 
   // Set up collapsing of menu items
   var collapsers = document.getElementsByClassName("collapse");
   for (var i = 0; i < collapsers.length; i++)
   {
     var collapser = collapsers[i];
     collapser.addEventListener("click", toggleCollapse, false);
     if (!Prefs[collapser.dataset.option])
       document.getElementById(collapser.dataset.collapsable).classList.add("collapsed");
   }
-}
-
-function onUnload()
-{
-  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 onComposerReady(message, sender)
-{
-  if (sender.page.id == page.id)
-    document.body.classList.remove("nohtml");
 }
 
 function toggleEnabled()
 {
   var disabled = document.body.classList.toggle("disabled");
   if (disabled)
   {
     var host = getDecodedHostname(page.url).replace(/^www\./, "");
     var filter = Filter.fromText("@@||" + host + "^$document");
     if (filter.subscriptions.length && filter.disabled)
@@ skipping 39 matching lines @@
 }
 
 function toggleCollapse(event)
 {
   var collapser = event.currentTarget;
   Prefs[collapser.dataset.option] = !Prefs[collapser.dataset.option];
   collapser.parentNode.classList.toggle("collapsed");
 }
 
 document.addEventListener("DOMContentLoaded", onLoad, false);
-window.addEventListener("unload", onUnload, false);