Issue 3823 - Split up message responder code

background.js

Index: background.js
===================================================================
--- a/background.js
+++ b/background.js
@@ -24,7 +24,6 @@
 with(require("whitelisting"))
 {
   this.checkWhitelisted = checkWhitelisted;
-  this.processKey = processKey;
   this.getKey = getKey;
 }
 with(require("url"))
@@ -39,10 +38,9 @@
 var ElemHide = require("elemHide").ElemHide;
 var defaultMatcher = require("matcher").defaultMatcher;
 var Prefs = require("prefs").Prefs;
-var parseFilters = require("filterValidation").parseFilters;
-var composeFilters = require("filterComposer").composeFilters;
 var updateIcon = require("icon").updateIcon;
 var showNextNotificationForUrl = require("notificationHelper").showNextNotificationForUrl;
+var port = require("messaging").port;

[Wladimir Palant, 2016/03/21 15:50:12]

Arguably, this should use getPort() as well and no default port exposed - if you
expose a default port people will use it by accident and listeners will stay
around.

[Sebastian Noack, 2016/03/21 17:15:42]

background.js is matter to go away. In the mid-term all code will be
encapsulated in modules. This patch is just the first step. And I'd rather avoid
unnecessary boilerplate to get the port in each module, as well as logic to
cache the port which wouldn't be necessary otherwise.

I consider getPort() a temporarily hack, that is matter to be removed once we
use asynchronous messaging properly in the popup. And except for that edge case,
all messaging code using the port API runs in the background page (hence in the
same thread and for the same window) anyway, where we don't have to disconnect
the port.

 var devtools = require("devtools");
 
 // Special-case domains for which we cannot use style-based hiding rules.
@@ -120,171 +118,97 @@
   return {filters: filters, exceptions: exceptions};
 }
 
-ext.onMessage.addListener(function (msg, sender, sendResponse)
+port.on("get-selectors", function(msg, sender)
 {
-  switch (msg.type)
+  var selectors = [];
+  var trace = devtools && devtools.hasPanel(sender.page);
+
+  if (!checkWhitelisted(sender.page, sender.frame,
+                        RegExpFilter.typeMap.DOCUMENT |
+                        RegExpFilter.typeMap.ELEMHIDE))
   {
-    case "composer.openDialog":
-      ext.windows.create({
-        url: ext.getURL("block.html"),
-        left: 50,
-        top: 50,
-        width: 420,
-        height: 200,
-        focused: true,
-        type: "popup"
-      },
-      function (popupPage) {
-        var popupPageId = popupPage.id;
-        function onRemoved(removedPageId)
-        {
-          if (popupPageId == removedPageId)
-          {
-            sender.page.sendMessage({
-              type: "composer.content.dialogClosed",
-              popupId: popupPageId
-            });
-            ext.pages.onRemoved.removeListener(onRemoved);
-          }
-        }
-        ext.pages.onRemoved.addListener(onRemoved);
+    var noStyleRules = false;
+    var specificOnly = checkWhitelisted(sender.page, sender.frame,
+                                        RegExpFilter.typeMap.GENERICHIDE);
+    var host = extractHostFromFrame(sender.frame);
 
-        sendResponse(popupPageId);
+    for (var i = 0; i < noStyleRulesHosts.length; i++)
+    {
+      var noStyleHost = noStyleRulesHosts[i];
+      if (host == noStyleHost || (host.length > noStyleHost.length &&
+                                  host.substr(host.length - noStyleHost.length - 1) == "." + noStyleHost))
+      {
+        noStyleRules = true;
+      }
+    }
+    selectors = ElemHide.getSelectorsForDomain(host, specificOnly);
+    if (noStyleRules)
+    {
+      selectors = selectors.filter(function(s)
+      {
+        return !/\[style[\^\$]?=/.test(s);
       });
-      return true;
-      break;
-    case "get-selectors":
-      var selectors = [];
-      var trace = devtools && devtools.hasPanel(sender.page);
+    }
+  }
 
-      if (!checkWhitelisted(sender.page, sender.frame,
-                            RegExpFilter.typeMap.DOCUMENT |
-                            RegExpFilter.typeMap.ELEMHIDE))
-      {
-        var noStyleRules = false;
-        var specificOnly = checkWhitelisted(sender.page, sender.frame,
-                                            RegExpFilter.typeMap.GENERICHIDE);
-        var host = extractHostFromFrame(sender.frame);
+  return {selectors: selectors, trace: trace};
+});
 
-        for (var i = 0; i < noStyleRulesHosts.length; i++)
-        {
-          var noStyleHost = noStyleRulesHosts[i];
-          if (host == noStyleHost || (host.length > noStyleHost.length &&
-                                      host.substr(host.length - noStyleHost.length - 1) == "." + noStyleHost))
-          {
-            noStyleRules = true;
-          }
-        }
-        selectors = ElemHide.getSelectorsForDomain(host, specificOnly);
-        if (noStyleRules)
-        {
-          selectors = selectors.filter(function(s)
-          {
-            return !/\[style[\^\$]?=/.test(s);
-          });
-        }
-      }
+port.on("should-collapse", function(msg, sender)
+{
+  if (checkWhitelisted(sender.page, sender.frame))
+    return false;
 
-      sendResponse({selectors: selectors, trace: trace});
-      break;
-    case "should-collapse":
-      if (checkWhitelisted(sender.page, sender.frame))
-      {
-        sendResponse(false);
-        break;
-      }
+  var typeMask = RegExpFilter.typeMap[msg.mediatype];
+  var documentHost = extractHostFromFrame(sender.frame);
+  var sitekey = getKey(sender.page, sender.frame);
+  var blocked = false;
 
-      var typeMask = RegExpFilter.typeMap[msg.mediatype];
-      var documentHost = extractHostFromFrame(sender.frame);
-      var sitekey = getKey(sender.page, sender.frame);
-      var blocked = false;
+  var specificOnly = checkWhitelisted(
+    sender.page, sender.frame,
+    RegExpFilter.typeMap.GENERICBLOCK
+  );
 
-      var specificOnly = checkWhitelisted(
-        sender.page, sender.frame,
-        RegExpFilter.typeMap.GENERICBLOCK
-      );
+  for (var i = 0; i < msg.urls.length; i++)
+  {
+    var url = new URL(msg.urls[i], msg.baseURL);
+    var filter = defaultMatcher.matchesAny(
+      stringifyURL(url), typeMask,
+      documentHost, isThirdParty(url, documentHost),
+      sitekey, specificOnly
+    );
 
-      for (var i = 0; i < msg.urls.length; i++)
-      {
-        var url = new URL(msg.urls[i], msg.baseURL);
-        var filter = defaultMatcher.matchesAny(
-          stringifyURL(url), typeMask,
-          documentHost, isThirdParty(url, documentHost),
-          sitekey, specificOnly
-        );
+    if (filter instanceof BlockingFilter)
+    {
+      if (filter.collapse != null)
+        return filter.collapse;
 
-        if (filter instanceof BlockingFilter)
-        {
-          if (filter.collapse != null)
-          {
-            sendResponse(filter.collapse);
-            return;
-          }
+      blocked = true;
+    }
+  }
 
-          blocked = true;
-        }
-      }
+  return blocked && Prefs.hidePlaceholders;
+});
 
-      sendResponse(blocked && Prefs.hidePlaceholders);
-      break;
-    case "get-domain-enabled-state":
-      // Returns whether this domain is in the exclusion list.
-      // The browser action popup asks us this.
-      if(sender.page)
-      {
-        sendResponse({enabled: !checkWhitelisted(sender.page)});
-        return;
-      }
-      break;
-    case "add-sitekey":
-      processKey(msg.token, sender.page, sender.frame);
-      break;
-    case "composer.ready":
-      htmlPages.set(sender.page, null);
-      refreshIconAndContextMenu(sender.page);
-      break;
-    case "composer.getFilters":
-      sendResponse(composeFilters({
-        tagName: msg.tagName,
-        id: msg.id,
-        src: msg.src,
-        style: msg.style,
-        classes: msg.classes,
-        urls: msg.urls,
-        type: msg.mediatype,
-        baseURL: msg.baseURL,
-        page: sender.page,
-        frame: sender.frame
-      }));
-      break;
-    case "trace-elemhide":
-      devtools.logHiddenElements(
-        sender.page, msg.selectors,
-        extractHostFromFrame(sender.frame)
-      );
-      break;
-    case "has-devtools-panel":
-      sendResponse(Prefs.show_devtools_panel);
-      break;
-    case "forward":
-      var targetPage;
-      if (msg.targetPageId)
-        targetPage = ext.getPage(msg.targetPageId);
-      else
-        targetPage = sender.page;
+port.on("get-domain-enabled-state", function(msg, sender)
+{
+  return {enabled: !checkWhitelisted(sender.page)};
+});
 
-      if (targetPage)
-      {
-        msg.payload.sender = sender.page.id;
-        if (msg.expectsResponse)
-        {
-          targetPage.sendMessage(msg.payload, sendResponse);
-          return true;
-        }
+port.on("forward", function(msg, sender)
+{
+  var targetPage;
+  if (msg.targetPageId)
+    targetPage = ext.getPage(msg.targetPageId);
+  else
+    targetPage = sender.page;
 
-        targetPage.sendMessage(msg.payload);
-      }
-      break;
+  if (targetPage)
+  {
+    msg.payload.sender = sender.page.id;
+    if (msg.expectsResponse)
+      return new Promise(targetPage.sendMessage.bind(targetPage, msg.payload));
+    targetPage.sendMessage(msg.payload);
   }
 });