Issue 3869 - Use the new FilterNotifier API

messageResponder.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
@@ skipping 36 matching lines @@
       result[keys[i]] = obj[keys[i]];
     return result;
   }
 
   var convertSubscription = convertObject.bind(null, ["disabled",
     "downloadStatus", "homepage", "lastDownload", "title", "url"]);
   var convertFilter = convertObject.bind(null, ["text"]);
 
   var changeListeners = new global.ext.PageMap();
   var listenedPreferences = [];
+  var listenedFilterChanges = [];
   var messageTypes = {
     "app": "app.listen",
     "filter": "filters.listen",
     "pref": "prefs.listen",
     "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)
         args.push(convertSubscription(arg));
       else if (arg instanceof Filter)
         args.push(convertFilter(arg));
       else
         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)
   {
     var listenerFilters = changeListeners.get(page);
     if (!listenerFilters)
     {
       listenerFilters = Object.create(null);
       changeListeners.set(page, listenerFilters);
     }
@@ skipping 151 matching lines @@
 
             if (!(filter.text in seenFilter))
               FilterStorage.removeFilter(filter);
           }
         }
         break;
       case "filters.listen":
         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.

           delete listenerFilters.filter;
         break;
       case "filters.remove":
         var filter = Filter.fromText(message.text);
         var subscription = null;
         if (message.subscriptionUrl)
           subscription = Subscription.fromURL(message.subscriptionUrl);
 
         if (!subscription)
           FilterStorage.removeFilter(filter);
         else
           FilterStorage.removeFilter(filter, subscription, message.index);
         break;
       case "prefs.get":
         callback(Prefs[message.key]);
         break;
       case "prefs.listen":
         var listenerFilters = getListenerFilters(sender.page);
         if (message.filter)
         {
           message.filter.forEach(function(preference)
           {
             if (listenedPreferences.indexOf(preference) == -1)
             {
               listenedPreferences.push(preference);
               Prefs.on(preference, function()
               {
-                sendMessage("pref", preference, [Prefs[preference]]);
+                sendMessage("pref", preference, Prefs[preference]);
               });
             }
           });
           listenerFilters.pref = message.filter;
         }
         else
           delete listenerFilters.pref;
         break;
       case "prefs.toggle":
         if (message.key == "notifications_ignoredcategories")
           NotificationStorage.toggleIgnoreCategory("*");
         else
           Prefs[message.key] = !Prefs[message.key];
         break;
       case "subscriptions.add":
         var subscription = Subscription.fromURL(message.url);
         if ("title" in message)
           subscription.title = message.title;
         if ("homepage" in message)
           subscription.homepage = message.homepage;
 
         if (message.confirm)
         {
           ext.showOptions(function()
           {
-            sendMessage("app", "addSubscription", [convertSubscription(subscription)]);
+            sendMessage("app", "addSubscription", subscription);
           });
         }
         else
         {
           subscription.disabled = false;
           FilterStorage.addSubscription(subscription);
 
           if (subscription instanceof DownloadableSubscription && !subscription.lastDownload)
             Synchronizer.execute(subscription);
         }
         break;
       case "subscriptions.get":
         var subscriptions = FilterStorage.subscriptions.filter(function(s)
         {
           if (message.ignoreDisabled && s.disabled)
             return false;
           if (s instanceof DownloadableSubscription && message.downloadable)
             return true;
           if (s instanceof SpecialSubscription && message.special)
             return true;
           return false;
         });
         callback(subscriptions.map(convertSubscription));
         break;
       case "subscriptions.listen":
         var listenerFilters = getListenerFilters(sender.page);
         if (message.filter)
         {
-          FilterNotifier.addListener(onFilterChange);
+          addFilterListeners("subscription", message.filter);
           listenerFilters.subscription = message.filter;
         }
         else
           delete listenerFilters.subscription;
         break;
       case "subscriptions.remove":
         var subscription = Subscription.fromURL(message.url);
         if (subscription.url in FilterStorage.knownSubscriptions)
           FilterStorage.removeSubscription(subscription);
         break;
       case "subscriptions.toggle":
         var subscription = Subscription.fromURL(message.url);
         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);
         }
         else
         {
           subscription.disabled = false;
           subscription.title = message.title;
           subscription.homepage = message.homepage;
           FilterStorage.addSubscription(subscription);
           if (!subscription.lastDownload)
             Synchronizer.execute(subscription);
         }
         break;
       case "subscriptions.update":
         var subscriptions = message.url ? [Subscription.fromURL(message.url)] :
                             FilterStorage.subscriptions;
         for (var i = 0; i < subscriptions.length; i++)
         {
           var subscription = subscriptions[i];
           if (subscription instanceof DownloadableSubscription)
             Synchronizer.execute(subscription, true);
         }
         break;
       case "subscriptions.isDownloading":
         callback(Synchronizer.isExecuting(message.url));
         break;
     }
   });
 })(this);