Issue 4783 - Use modern JavaScript syntax for the messageResponder

messageResponder.js

Index: messageResponder.js
diff --git a/messageResponder.js b/messageResponder.js
index 3bb5c9b4016e2d540d6377e0d05fa15d2e42e66f..aa71533d0655332e8d775f38bd45d07645d397bd 100644
--- a/messageResponder.js
+++ b/messageResponder.js
@@ -15,31 +15,32 @@
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-(function(global)
+"use strict";
+
 {
-  if (!global.ext)
-    global.ext = require("ext_background");
-
-  var port = require("messaging").port;
-  var Prefs = require("prefs").Prefs;
-  var Utils = require("utils").Utils;
-  var FilterStorage = require("filterStorage").FilterStorage;
-  var FilterNotifier = require("filterNotifier").FilterNotifier;
-  var defaultMatcher = require("matcher").defaultMatcher;
-  var ElemHideEmulation = require("elemHideEmulation").ElemHideEmulation;
-  var NotificationStorage = require("notification").Notification;
-
-  var filterClasses = require("filterClasses");
-  var Filter = filterClasses.Filter;
-  var BlockingFilter = filterClasses.BlockingFilter;
-  var RegExpFilter = filterClasses.RegExpFilter;
-  var Synchronizer = require("synchronizer").Synchronizer;
-
-  var info = require("info");
-  var subscriptionClasses = require("subscriptionClasses");
-  var Subscription = subscriptionClasses.Subscription;
-  var DownloadableSubscription = subscriptionClasses.DownloadableSubscription;
-  var SpecialSubscription = subscriptionClasses.SpecialSubscription;
+  if (!ext)
+    var ext = require("ext_background");

[Thomas Greiner, 2017/01/17 11:03:51]

Detail: It'd be great if we could avoid this if-block since it's a bit odd to
see a variable defined inside a block solely for use outside of it.

The same effect could be achieved using:

var ext = ext || require("ext_background");

[kzar, 2017/01/17 11:25:29]

Done.

+
+  let port = require("messaging").port;

[Thomas Greiner, 2017/01/17 11:03:51]

Is there a reason not to use `const` for imported variables? The idea being that
we shouldn't modify imports anyway.

[kzar, 2017/01/17 11:25:29]

Well I considered that but we generally use `let` elsewhere.

If it's all the same I'd rather avoid falling down the `const` vs `let` rabbit
hole right now, but FWIW I agree that perhaps we should generally use `const`
much of the time we currently use `let`.

[Sebastian Noack, 2017/01/17 15:37:00]

If it wouldn't be for the consistency with our existing code, I'd agree that
using const for imports sounds reasonable. On the other hand, we are currently
modernizing the code in adblockpluschrome as well. So now would be a good time
to come up with a guideline for using const.

But I'm feeling indecisive what the general rule for using const should be. So
far we use const for global variables of immutable type, that could be inlined
as well, but have been put into variables in order to improve the
maintainability. While imported modules are not immutable, they are similar in
the way that it makes the code more maintainable (and also somewhat faster) when
we put them into variables rather than inlining/duplicating
require(module).symbol everywhere, and there isn't much of a point to reassign a
variable holding a module either.

While I'm all for a rule that suggests using const in those scenarios, I might
be opposed to a rule that suggests using const wherever possible. For example,
consider following code:

  let items = [];
  for (let x of y)
    items.push(y.getValue());

Since "items" isn't reassigned, we could use const instead of let here. But IMO
this would already be weird enough, since the value of "items" is changing
though. Now imagine that the logic is changed later so that under some
circumstances the array has to be cleared:


  let items = [];
  for (let x of y)
    if (x.isFirst)
       items = [];
    items.push(y.getValue());

Now we suddenly have a reassignment, and need to adapt the variable declaration
to use let again. The other way around, if code like this gets changed so that
we no longer reassign we would have to adapt the declaration to use const. A
rule that would require that seems quite impracticable to me.

This is somewhat similar to the question when to use tuples, and when to use
lists, in Python. Initially, we preferred to use tuples whenever possible (i.e
for every sequence that isn't modified by the current code). However, this
caused the same mess I'm afraid of here. So we now have a rule for our Python
code, to use tuples for data that have structure, and lists for data that have
order. Or if you put it more general, use immutability for that kind of data
which have no point in changing them regardless of the current context.

[Thomas Greiner, 2017/01/17 15:48:40]

I agree that we should be careful and only use `const` for some few well-defined
cases. It shouldn't be too difficult to define such unambiguous scenarios (e.g.
module imports or `const MILLIS_IN_SECOND = 1000;`) for which it makes sense.

[Sebastian Noack, 2017/01/17 17:15:48]

Rather than listing specific use cases in our coding styles, I'd like to have a
generic rule, but one that accounts for my concerns above. Perhaps something
along the lines of:

  Use `const` for constant expressions that could as well have been inlined.

If it is helpful we could add the know use cases as examples:

  Use `const` for constant expressions that could as well have been inlined
  (e.g. static parameters, imported modules, etc.).

[Thomas Greiner, 2017/01/17 17:20:08]

Sounds good.

[Sebastian Noack, 2017/01/17 17:30:16]

(Sorry, I first replied in the globally. Copying this comment where it belongs.)

Dave, if you agree, as you are already working on some additions to our coding
practices mind adding this?

[kzar, 2017/01/18 05:50:32]

Sure that all sound sensible enough to me. I've updated this review to use const
and will update the coding practices review now.

+  let Prefs = require("prefs").Prefs;
+  let Utils = require("utils").Utils;
+  let FilterStorage = require("filterStorage").FilterStorage;
+  let FilterNotifier = require("filterNotifier").FilterNotifier;
+  let defaultMatcher = require("matcher").defaultMatcher;
+  let ElemHideEmulation = require("elemHideEmulation").ElemHideEmulation;
+  let NotificationStorage = require("notification").Notification;
+
+  let filterClasses = require("filterClasses");
+  let Filter = filterClasses.Filter;
+  let BlockingFilter = filterClasses.BlockingFilter;
+  let RegExpFilter = filterClasses.RegExpFilter;
+  let Synchronizer = require("synchronizer").Synchronizer;
+
+  let info = require("info");
+  let subscriptionClasses = require("subscriptionClasses");
+  let Subscription = subscriptionClasses.Subscription;
+  let DownloadableSubscription = subscriptionClasses.DownloadableSubscription;
+  let SpecialSubscription = subscriptionClasses.SpecialSubscription;
 
   // Some modules doesn't exist on Firefox. Moreover,
   // require() throws an exception on Firefox in that case.
@@ -59,30 +60,27 @@
 
   function convertObject(keys, obj)
   {
-    var result = {};
-    for (var i = 0; i < keys.length; i++)
-    {
-      var key = keys[i];
+    let result = {};
+    for (let key of keys)

[Thomas Greiner, 2017/01/17 11:03:50]

Detail: Please put braces around blocks that have multiple lines to avoid
potential misinterpretation or mistakes when making changes to the code.

Also applies to other loops that have been modified.

[kzar, 2017/01/17 11:25:29]

Done.

       if (key in obj)
         result[key] = obj[key];
-    }
     return result;
   }
 
   function convertSubscription(subscription)
   {
-    var obj = convertObject(["disabled", "downloadStatus", "homepage",
+    let obj = convertObject(["disabled", "downloadStatus", "homepage",
                              "lastDownload", "title", "url"], subscription);
     obj.isDownloading = Synchronizer.isExecuting(subscription.url);
     return obj;
   }
 
-  var convertFilter = convertObject.bind(null, ["text"]);
+  let convertFilter = convertObject.bind(null, ["text"]);
 
-  var changeListeners = new global.ext.PageMap();
-  var listenedPreferences = Object.create(null);
-  var listenedFilterChanges = Object.create(null);
-  var messageTypes = {
+  let changeListeners = new ext.PageMap();
+  let listenedPreferences = Object.create(null);
+  let listenedFilterChanges = Object.create(null);
+  let messageTypes = {
     "app": "app.respond",
     "filter": "filters.respond",
     "pref": "prefs.respond",
@@ -91,14 +89,14 @@
 
   function sendMessage(type, action)
   {
-    var pages = changeListeners.keys();
+    let pages = changeListeners.keys();
     if (pages.length == 0)
       return;
 
-    var args = [];
-    for (var i = 2; i < arguments.length; i++)
+    let args = [];
+    for (let i = 2; i < arguments.length; i++)
     {
-      var arg = arguments[i];
+      let arg = arguments[i];
       if (arg instanceof Subscription)
         args.push(convertSubscription(arg));
       else if (arg instanceof Filter)
@@ -107,11 +105,10 @@
         args.push(arg);
     }
 
-    for (var j = 0; j < pages.length; j++)
+    for (let page of pages)
     {
-      var page = pages[j];
-      var filters = changeListeners.get(page);
-      var actions = filters[type];
+      let filters = changeListeners.get(page);
+      let actions = filters[type];
       if (actions && actions.indexOf(action) != -1)
       {
         page.sendMessage({
@@ -125,9 +122,9 @@
 
   function addFilterListeners(type, actions)
   {
-    actions.forEach(function(action)
+    actions.forEach(action =>

[Thomas Greiner, 2017/01/17 11:03:51]

Since we're using `let` and for-of now, there should no longer be a need for
using `Array.forEach()` anymore.

Also applies to other loops that have been modified.

[kzar, 2017/01/17 11:25:29]

Done.

     {
-      var name;
+      let name;
       if (type == "filter" && action == "loaded")
         name = "load";
       else
@@ -138,9 +135,9 @@
         listenedFilterChanges[name] = null;
         FilterNotifier.on(name, function()
         {
-          var args = [type, action];
-          for (var i = 0; i < arguments.length; i++)
-            args.push(arguments[i]);
+          let args = [type, action];
+          for (let arg of arguments)
+            args.push(arg);
           sendMessage.apply(null, args);
         });
       }
@@ -149,7 +146,7 @@
 
   function getListenerFilters(page)
   {
-    var listenerFilters = changeListeners.get(page);
+    let listenerFilters = changeListeners.get(page);
     if (!listenerFilters)
     {
       listenerFilters = Object.create(null);
@@ -162,7 +159,7 @@
   {
     if (message.what == "issues")
     {
-      var subscriptionInit = tryRequire("subscriptionInit");
+      let subscriptionInit = tryRequire("subscriptionInit");
       return {
         filterlistsReinitialized: subscriptionInit ? subscriptionInit.reinitialized : false,
         legacySafariVersion: (info.platform == "safari" && (
@@ -177,7 +174,7 @@
 
     if (message.what == "localeInfo")
     {
-      var bidiDir;
+      let bidiDir;
       if ("chromeRegistry" in Utils)
         bidiDir = Utils.chromeRegistry.isLocaleRTL("adblockplus") ? "rtl" : "ltr";
       else
@@ -212,8 +209,8 @@
 
   port.on("filters.add", (message, sender) =>
   {
-    var result = require("filterValidation").parseFilter(message.text);
-    var errors = [];
+    let result = require("filterValidation").parseFilter(message.text);
+    let errors = [];
     if (result.error)
       errors.push(result.error.toString());
     else if (result.filter)
@@ -224,7 +221,7 @@
 
   port.on("filters.blocked", (message, sender) =>
   {
-    var filter = defaultMatcher.matchesAny(message.url,
+    let filter = defaultMatcher.matchesAny(message.url,
       RegExpFilter.typeMap[message.requestType], message.docDomain,
       message.thirdParty);
 
@@ -235,16 +232,16 @@
   {
     if (message.what == "elemhideemulation")
     {
-      var filters = [];
-      var checkWhitelisted = require("whitelisting").checkWhitelisted;
+      let filters = [];
+      let checkWhitelisted = require("whitelisting").checkWhitelisted;
 
       if (Prefs.enabled && !checkWhitelisted(sender.page, sender.frame,
                             RegExpFilter.typeMap.DOCUMENT |
                             RegExpFilter.typeMap.ELEMHIDE))
       {
-        var hostname = sender.frame.url.hostname;
+        let hostname = sender.frame.url.hostname;
         filters = ElemHideEmulation.getRulesForDomain(hostname);
-        filters = filters.map(function(filter)
+        filters = filters.map(filter =>
         {
           return {
             selector: filter.selector,
@@ -255,7 +252,7 @@
       return filters;
     }
 
-    var subscription = Subscription.fromURL(message.subscriptionUrl);
+    let subscription = Subscription.fromURL(message.subscriptionUrl);
     if (!subscription)
       return [];
 
@@ -264,22 +261,18 @@
 
   port.on("filters.importRaw", (message, sender) =>
   {
-    var result = require("filterValidation").parseFilters(message.text);
-    var errors = [];
-    for (var i = 0; i < result.errors.length; i++)
-    {
-      var error = result.errors[i];
+    let result = require("filterValidation").parseFilters(message.text);
+    let errors = [];
+    for (let error of result.errors)
       if (error.type != "unexpected-filter-list-header")
         errors.push(error.toString());
-    }
 
     if (errors.length > 0)
       return errors;
 
-    var seenFilter = Object.create(null);
-    for (var i = 0; i < result.filters.length; i++)
+    let seenFilter = Object.create(null);
+    for (let filter of result.filters)
     {
-      var filter = result.filters[i];
       FilterStorage.addFilter(filter);
       seenFilter[filter.text] = null;
     }
@@ -287,15 +280,14 @@
     if (!message.removeExisting)
       return errors;
 
-    for (var i = 0; i < FilterStorage.subscriptions.length; i++)
+    for (let subscription of FilterStorage.subscriptions)
     {
-      var subscription = FilterStorage.subscriptions[i];
       if (!(subscription instanceof SpecialSubscription))
         continue;
 
-      for (var j = subscription.filters.length - 1; j >= 0; j--)
+      for (let j = subscription.filters.length - 1; j >= 0; j--)
       {
-        var filter = subscription.filters[j];
+        let filter = subscription.filters[j];
         if (/^@@\|\|([^\/:]+)\^\$document$/.test(filter.text))
           continue;
 
@@ -315,8 +307,8 @@
 
   port.on("filters.remove", (message, sender) =>
   {
-    var filter = Filter.fromText(message.text);
-    var subscription = null;
+    let filter = Filter.fromText(message.text);
+    let subscription = null;
     if (message.subscriptionUrl)
       subscription = Subscription.fromURL(message.subscriptionUrl);
 
@@ -334,12 +326,12 @@
   port.on("prefs.listen", (message, sender) =>
   {
     getListenerFilters(sender.page).pref = message.filter;
-    message.filter.forEach(function(preference)
+    message.filter.forEach(preference =>
     {
       if (!(preference in listenedPreferences))
       {
         listenedPreferences[preference] = null;
-        Prefs.on(preference, function()
+        Prefs.on(preference, () =>
         {
           sendMessage("pref", preference, Prefs[preference]);
         });
@@ -357,7 +349,7 @@
 
   port.on("subscriptions.add", (message, sender) =>
   {
-    var subscription = Subscription.fromURL(message.url);
+    let subscription = Subscription.fromURL(message.url);
     if ("title" in message)
       subscription.title = message.title;
     if ("homepage" in message)
@@ -365,7 +357,7 @@
 
     if (message.confirm)
     {
-      ext.showOptions(function()
+      ext.showOptions(() =>
       {
         sendMessage("app", "addSubscription", subscription);
       });
@@ -382,7 +374,7 @@
 
   port.on("subscriptions.get", (message, sender) =>
   {
-    var subscriptions = FilterStorage.subscriptions.filter(function(s)
+    let subscriptions = FilterStorage.subscriptions.filter(s =>
     {
       if (message.ignoreDisabled && s.disabled)
         return false;
@@ -404,14 +396,14 @@
 
   port.on("subscriptions.remove", (message, sender) =>
   {
-    var subscription = Subscription.fromURL(message.url);
+    let subscription = Subscription.fromURL(message.url);
     if (subscription.url in FilterStorage.knownSubscriptions)
       FilterStorage.removeSubscription(subscription);
   });
 
   port.on("subscriptions.toggle", (message, sender) =>
   {
-    var subscription = Subscription.fromURL(message.url);
+    let subscription = Subscription.fromURL(message.url);
     if (subscription.url in FilterStorage.knownSubscriptions)
     {
       if (subscription.disabled || message.keepInstalled)
@@ -432,13 +424,10 @@
 
   port.on("subscriptions.update", (message, sender) =>
   {
-    var subscriptions = message.url ? [Subscription.fromURL(message.url)] :
+    let subscriptions = message.url ? [Subscription.fromURL(message.url)] :
                         FilterStorage.subscriptions;
-    for (var i = 0; i < subscriptions.length; i++)
-    {
-      var subscription = subscriptions[i];
+    for (let subscription of subscriptions)
       if (subscription instanceof DownloadableSubscription)
         Synchronizer.execute(subscription, true);
-    }
   });
-})(this);
+}