Issue 4783 - Use modern JavaScript syntax for the messageResponder

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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-(function(global)
+"use strict";
+
 {
-  if (!global.ext)
-    global.ext = require("ext_background");
+  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.

 
-  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;
+  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;
 
-  var filterClasses = require("filterClasses");
-  var Filter = filterClasses.Filter;
-  var BlockingFilter = filterClasses.BlockingFilter;
-  var RegExpFilter = filterClasses.RegExpFilter;
-  var Synchronizer = require("synchronizer").Synchronizer;
+  let filterClasses = require("filterClasses");
+  let Filter = filterClasses.Filter;
+  let BlockingFilter = filterClasses.BlockingFilter;
+  let RegExpFilter = filterClasses.RegExpFilter;
+  let Synchronizer = require("synchronizer").Synchronizer;
 
-  var info = require("info");
-  var subscriptionClasses = require("subscriptionClasses");
-  var Subscription = subscriptionClasses.Subscription;
-  var DownloadableSubscription = subscriptionClasses.DownloadableSubscription;
-  var SpecialSubscription = subscriptionClasses.SpecialSubscription;
+  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.
   // However, try/catch causes the whole function to to be
   // deoptimized on V8. So we wrap it into another function.
   function tryRequire(module)
   {
     try
     {
       return require(module);
     }
     catch (e)
     {
       return null;
     }
   }
 
   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",
     "subscription": "subscriptions.respond"
   };
 
   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)
         args.push(convertFilter(arg));
       else
         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({
           type: messageTypes[type],
           action: action,
           args: args
         });
       }
     }
   }
 
   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
         name = type + "." + action;
 
       if (!(name in listenedFilterChanges))
       {
         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);
         });
       }
     });
   }
 
   function getListenerFilters(page)
   {
-    var listenerFilters = changeListeners.get(page);
+    let listenerFilters = changeListeners.get(page);
     if (!listenerFilters)
     {
       listenerFilters = Object.create(null);
       changeListeners.set(page, listenerFilters);
     }
     return listenerFilters;
   }
 
   port.on("app.get", (message, sender) =>
   {
     if (message.what == "issues")
     {
-      var subscriptionInit = tryRequire("subscriptionInit");
+      let subscriptionInit = tryRequire("subscriptionInit");
       return {
         filterlistsReinitialized: subscriptionInit ? subscriptionInit.reinitialized : false,
         legacySafariVersion: (info.platform == "safari" && (
             Services.vc.compare(info.platformVersion, "6.0") < 0 ||   // beforeload breaks websites in Safari 5
             Services.vc.compare(info.platformVersion, "6.1") == 0 ||  // extensions are broken in 6.1 and 7.0
             Services.vc.compare(info.platformVersion, "7.0") == 0))
       };
     }
 
     if (message.what == "doclink")
       return Utils.getDocLink(message.link);
 
     if (message.what == "localeInfo")
     {
-      var bidiDir;
+      let bidiDir;
       if ("chromeRegistry" in Utils)
         bidiDir = Utils.chromeRegistry.isLocaleRTL("adblockplus") ? "rtl" : "ltr";
       else
         bidiDir = ext.i18n.getMessage("@@bidi_dir");
 
       return {locale: Utils.appLocale, bidiDir: bidiDir};
     }
 
     if (message.what == "features")
     {
@@ skipping 14 matching lines @@
   });
 
   port.on("app.open", (message, sender) =>
   {
     if (message.what == "options")
       ext.showOptions();
   });
 
   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)
       FilterStorage.addFilter(result.filter);
 
     return errors;
   });
 
   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);
 
     return filter instanceof BlockingFilter;
   });
 
   port.on("filters.get", (message, sender) =>
   {
     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,
             text: filter.text
           };
         });
       }
       return filters;
     }
 
-    var subscription = Subscription.fromURL(message.subscriptionUrl);
+    let subscription = Subscription.fromURL(message.subscriptionUrl);
     if (!subscription)
       return [];
 
     return subscription.filters.map(convertFilter);
   });
 
   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;
     }
 
     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;
 
         if (!(filter.text in seenFilter))
           FilterStorage.removeFilter(filter);
       }
     }
 
     return errors;
   });
 
   port.on("filters.listen", (message, sender) =>
   {
     getListenerFilters(sender.page).filter = message.filter;
     addFilterListeners("filter", message.filter);
   });
 
   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);
 
     if (!subscription)
       FilterStorage.removeFilter(filter);
     else
       FilterStorage.removeFilter(filter, subscription, message.index);
   });
 
   port.on("prefs.get", (message, sender) =>
   {
     return Prefs[message.key];
   });
 
   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]);
         });
       }
     });
   });
 
   port.on("prefs.toggle", (message, sender) =>
   {
     if (message.key == "notifications_ignoredcategories")
       NotificationStorage.toggleIgnoreCategory("*");
     else
       Prefs[message.key] = !Prefs[message.key];
   });
 
   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)
       subscription.homepage = message.homepage;
 
     if (message.confirm)
     {
-      ext.showOptions(function()
+      ext.showOptions(() =>
       {
         sendMessage("app", "addSubscription", subscription);
       });
     }
     else
     {
       subscription.disabled = false;
       FilterStorage.addSubscription(subscription);
 
       if (subscription instanceof DownloadableSubscription && !subscription.lastDownload)
         Synchronizer.execute(subscription);
     }
   });
 
   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;
       if (s instanceof DownloadableSubscription && message.downloadable)
         return true;
       if (s instanceof SpecialSubscription && message.special)
         return true;
       return false;
     });
 
     return subscriptions.map(convertSubscription);
   });
 
   port.on("subscriptions.listen", (message, sender) =>
   {
     getListenerFilters(sender.page).subscription = message.filter;
     addFilterListeners("subscription", message.filter);
   });
 
   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)
         subscription.disabled = !subscription.disabled;
       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);
     }
   });
 
   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);
+}