Issue 3869 - Use the new FilterNotifier API

messageResponder.js

Index: messageResponder.js
===================================================================
--- a/messageResponder.js
+++ b/messageResponder.js
@@ -54,6 +54,7 @@
 
   var changeListeners = new global.ext.PageMap();
   var listenedPreferences = [];
+  var listenedFilterChanges = [];
   var messageTypes = {
     "app": "app.listen",
     "filter": "filters.listen",
@@ -61,44 +62,14 @@
     "subscription": "subscriptions.listen"
   };
 
-  function sendMessage(type, action, args)
+  function sendMessage(type, action)

[Sebastian Noack, 2016/03/23 23:15:51]

Sorry for refactoring sendMessage() again, but with onFilterChange gone, it made
sense to move the filter/subscription conversion in here. Also "args" are now
accepted as variable arguments instead of an array.

   {
     var pages = changeListeners.keys();
-    for (var i = 0; i < pages.length; i++)
-    {
-      var filters = changeListeners.get(pages[i]);
-      var actions = filters[type];
-      if (actions && actions.indexOf(action) != -1)
-      {
-        pages[i].sendMessage({
-          type:  messageTypes[type],
-          action: action,
-          args: args
-        });
-      }
-    }
-  }
-
-  function onFilterChange(action)
-  {
-    var type;
-    if (action == "load")
-    {
-      type = "filter";
-      action = "loaded";
-    }
-    else
-    {
-      var parts = action.split(".");
-      type = parts[0];
-      action = parts[1];
-    }
-
-    if (!(type in messageTypes))
+    if (pages.length == 0)
       return;
 
     var args = [];
-    for (var i = 1; i < arguments.length; i++)
+    for (var i = 2; i < arguments.length; i++)
     {
       var arg = arguments[i];
       if (arg instanceof Subscription)
@@ -109,7 +80,44 @@
         args.push(arg);
     }
 
-    sendMessage(type, action, args);
+    for (var j = 0; j < pages.length; j++)
+    {
+      var page = pages[j];
+      var filters = changeListeners.get(page);
+      var actions = filters[type];
+      if (actions && actions.indexOf(action) != -1)
+      {
+        page.sendMessage({
+          type: messageTypes[type],
+          action: action,
+          args: args
+        });
+      }
+    }
+  }
+
+  function addFilterListeners(type, actions)

[Sebastian Noack, 2016/03/23 23:15:51]

Same pattern as for Prefs.on(), the first UI that listens for a particular
preference causes a listener for that preference to be added, which then
propagates the notifications to all pages that listen for it.

+  {
+    actions.forEach(function(action)
+    {
+      var name;
+      if (type == "filter" && action == "loaded")
+        name = "load";
+      else
+        name = type + "." + action;
+
+      if (listenedFilterChanges.indexOf(name) == -1)
+      {
+        listenedFilterChanges.push(name);
+        FilterNotifier.on(name, function()
+        {
+          var args = [type, action];
+          for (var i = 0; i < arguments.length; i++)

[Sebastian Noack, 2016/03/23 23:15:51]

For the record, simply passing the arguments object to sendMessage() or whatever
function would cause a deoptimization in V8. So we need a loop to turn it into
an array anyway.

+            args.push(arguments[i]);
+          sendMessage.apply(null, args);
+        });
+      }
+    });
   }
 
   function getListenerFilters(page)
@@ -281,7 +289,7 @@
         var listenerFilters = getListenerFilters(sender.page);
         if (message.filter)
         {
-          FilterNotifier.addListener(onFilterChange);
+          addFilterListeners("filter", message.filter);
           listenerFilters.filter = message.filter;
         }
         else

[Sebastian Noack, 2016/03/23 23:15:51]

I wonder whether we should prehaps unsupport calling the *.listen messages with
null? Since this logic is duplicated 4 times, it would save us quite some
boilerplate. And the UI can still achieve the same by sending and empty array.

[Thomas Greiner, 2016/03/24 14:22:35]

Sorry, missed this comment.

Yeah, in one of the previous reviews I noted that overloading "*.listen" is not
a great solution in general and that an explicit "*.unlisten" message might be
more appropriate. So by splitting that off, "filter" would no longer be optional
and therefore wouldn't be allowed to be `null` anymore.

[Sebastian Noack, 2016/03/24 14:40:35]

So I read that as you are fine with me simplifying the code by unsupporting
null. There you go, new patch set is up.

As for adding *.unlisten, I don't think that this is necessary. As discussed on
the previous review, we cannot send messages on unload to unregister listeners
anyway, neither do we have to with the current approach. And if in fact, a UI is
just temporarily interested in particular events, it can still update the events
to be listened for by calling *.listen again with an empty array.

@@ -312,7 +320,7 @@
               listenedPreferences.push(preference);
               Prefs.on(preference, function()
               {
-                sendMessage("pref", preference, [Prefs[preference]]);
+                sendMessage("pref", preference, Prefs[preference]);
               });
             }
           });
@@ -338,7 +346,7 @@
         {
           ext.showOptions(function()
           {
-            sendMessage("app", "addSubscription", [convertSubscription(subscription)]);
+            sendMessage("app", "addSubscription", subscription);
           });
         }
         else
@@ -367,7 +375,7 @@
         var listenerFilters = getListenerFilters(sender.page);
         if (message.filter)
         {
-          FilterNotifier.addListener(onFilterChange);
+          addFilterListeners("subscription", message.filter);
           listenerFilters.subscription = message.filter;
         }
         else
@@ -383,11 +391,7 @@
         if (subscription.url in FilterStorage.knownSubscriptions)
         {
           if (subscription.disabled || message.keepInstalled)
-          {
             subscription.disabled = !subscription.disabled;
-            FilterNotifier.triggerListeners("subscription.disabled",

[Sebastian Noack, 2016/03/23 23:15:51]

This belongs into the mock implementation. Otherwise, subscription.disabled
notifications are dispatched redundantly in the extension. While I had to adapt
that line anyway, I removed it and fixed the mock implementation instead.

[Thomas Greiner, 2016/03/24 14:12:16]

I agree. Well spotted.

-                                            subscription);
-          }
           else
             FilterStorage.removeSubscription(subscription);
         }