Issue 3839 - Adapt for Prefs event changes

messageResponder.js

Index: messageResponder.js
===================================================================
--- a/messageResponder.js
+++ b/messageResponder.js
@@ -52,7 +52,9 @@
     "downloadStatus", "homepage", "lastDownload", "title", "url"]);
   var convertFilter = convertObject.bind(null, ["text"]);
 
-  var changeListeners = null;
+  var changeListeners = new global.ext.PageMap();
+  var filterListenerAdded = false;
+  var listenedPreferences = [];
   var messageTypes = {
     "app": "app.listen",
     "filter": "filters.listen",
@@ -60,13 +62,14 @@
     "subscription": "subscriptions.listen"
   };
 
-  function sendMessage(type, action, args, page)
+  function sendMessage(type, action, args)
   {
-    var pages = page ? [page] : changeListeners.keys();
+    var pages = changeListeners.keys();
     for (var i = 0; i < pages.length; i++)
     {
       var filters = changeListeners.get(pages[i]);
-      if (filters[type] && filters[type].indexOf(action) >= 0)
+      var actions = filters[type];
+      if (actions && actions.indexOf(action) != -1)
       {
         pages[i].sendMessage({
           type:  messageTypes[type],
@@ -110,31 +113,26 @@
     sendMessage(type, action, args);
   }
 
-  function onPrefChange(name)
+  function getListenerFilters(page, addListener)
   {
-    sendMessage("pref", name, [Prefs[name]]);
+    if (addListener && !filterListenerAdded)
+    {
+      FilterNotifier.addListener(onFilterChange);
+      filterListenerAdded = true;
+    }

[Thomas Greiner, 2016/03/21 17:55:55]

I guess we could just unconditionally start listening when the background page
starts up and stop it when the extension shuts down.

That way we'd get rid of some unnecessary overhead around determining when we
actually need it.

[Sebastian Noack, 2016/03/21 19:40:07]

I considered it, but I rather avoid running the listener on each filter
notification when the options page has never been opened, considering that most
users don't often use the options page, that we dispatch plenty filter
notifications, and that this particular listener does quite some stuff before it
might bail out.

+
+    var listenerFilters = changeListeners.get(page);
+    if (!listenerFilters)
+    {
+      listenerFilters = Object.create(null);
+      changeListeners.set(page, listenerFilters);
+    }
+
+    return listenerFilters;
   }
 
   global.ext.onMessage.addListener(function(message, sender, callback)
   {
-    var listenerFilters = null;
-    if (/\.listen$/.test(message.type))
-    {
-      if (!changeListeners)
-      {
-        changeListeners = new global.ext.PageMap();
-        FilterNotifier.addListener(onFilterChange);
-        Prefs.onChanged.addListener(onPrefChange);
-      }
-
-      listenerFilters = changeListeners.get(sender.page);
-      if (!listenerFilters)
-      {
-        listenerFilters = Object.create(null);
-        changeListeners.set(sender.page, listenerFilters);
-      }
-    }
-
     switch (message.type)
     {
       case "app.get":
@@ -187,6 +185,7 @@
           callback(null);
         break;
       case "app.listen":
+        var listenerFilters = getListenerFilters(sender.page, true);
         if (message.filter)
           listenerFilters.app = message.filter;
         else
@@ -287,6 +286,7 @@
         }
         break;
       case "filters.listen":
+        var listenerFilters = getListenerFilters(sender.page, true);
         if (message.filter)
           listenerFilters.filter = message.filter;
         else
@@ -307,8 +307,22 @@
         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)

[Thomas Greiner, 2016/03/21 17:55:55]

So only the first UI that registers a listener for a preference will actually
get it?

[Sebastian Noack, 2016/03/21 19:40:07]

Yes, the first UI that listens for a particular preference, causes a listener
for that preference to be added. But note that the listener calls sendMessage()
which notifies all pages that are listening for that preference.

+            {
+              listenedPreferences.push(preference);
+              Prefs.on(preference, function()
+              {
+                sendMessage("pref", preference, [Prefs[preference]]);
+              });

[Thomas Greiner, 2016/03/21 17:55:55]

This listener is never removed.

As you pointed out `FilterNotifier.removeListener()` and
`Prefs.onChanged.removeListener()` were never called. So far that hasn't been an
issue since the messageResponder couldn't accumulate more than a single listener
instance and since Chrome (unlike Firefox) throws those away on extension
shutdown anyway.
This change, however, would turn it into a memory leak if we end up having one
listener per preference per UI.

Just FYI: Currently, stopping a listener is achieved by sending "prefs.listen"
without "filter" - probably better to have an explicit "prefs.unlisten" method
but that's how it was implemented so far.

[Sebastian Noack, 2016/03/21 19:40:07]

Removing the listeners isn't trivial, since you cannot send messages in on
unload. Also there are scenarios (e.g. when a tab runs out of memory, crashes
for a different reason, etc.) where onload isn't triggered at all. So we would
have to listen for the tab to be removed, replaced or navigating somewhere else.
The PageMap object accounts for all of these scenarios. However, the abstraction
layer doesn't expose some of these events directly. Neither, do I think that the
addional complexity would be justified. After all, leaking one listener per
preference isn't too bad, and IMO not any worse than before, considering that
the listener we leaked before was called for any preference. Now we have
multiple listeners but they are only called for specific preferences. hence add
less overhead.

+            }
+          });
           listenerFilters.pref = message.filter;
+        }
         else
           delete listenerFilters.pref;
         break;
@@ -355,6 +369,7 @@
         callback(subscriptions.map(convertSubscription));
         break;
       case "subscriptions.listen":
+        var listenerFilters = getListenerFilters(sender.page, true);

[Thomas Greiner, 2016/03/21 17:55:55]

Nice. No idea why we didn't implement it like this before already.

         if (message.filter)
           listenerFilters.subscription = message.filter;
         else