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/>.
  */
 
 "use strict";
 
 {
-  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;
+  var ext = ext || require("ext_background");
+
+  const {port} = require("messaging");
+  const {Prefs} = require("prefs");
+  const {Utils} = require("utils");
+  const {FilterStorage} = require("filterStorage");
+  const {FilterNotifier} = require("filterNotifier");
+  const {defaultMatcher} = require("matcher");
+  const {ElemHideEmulation} = require("elemHideEmulation");
+  const {Notification: NotificationStorage} = require("notification");
+
+  const {Filter, BlockingFilter, RegExpFilter} = require("filterClasses");
+  const {Synchronizer} = require("synchronizer");
+
+  const info = require("info");
+  const {Subscription,
+         DownloadableSubscription,
+         SpecialSubscription} = require("subscriptionClasses");
 
   // 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)
   {
     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)
   {
     let obj = convertObject(["disabled", "downloadStatus", "homepage",
                              "lastDownload", "title", "url"], subscription);
     obj.isDownloading = Synchronizer.isExecuting(subscription.url);
     return obj;
   }
@@ skipping 38 matching lines @@
           type: messageTypes[type],
           action: action,
           args: args
         });
       }
     }
   }
 
   function addFilterListeners(type, actions)
   {
-    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.

+    for (let action of actions)
     {
       let name;
       if (type == "filter" && action == "loaded")
         name = "load";
       else
         name = type + "." + action;
 
       if (!(name in listenedFilterChanges))
       {
         listenedFilterChanges[name] = null;
         FilterNotifier.on(name, function()
         {
           let args = [type, action];
           for (let arg of arguments)
             args.push(arg);
           sendMessage.apply(null, args);
         });
       }
-    });
+    }
   }
 
   function getListenerFilters(page)
   {
     let listenerFilters = changeListeners.get(page);
     if (!listenerFilters)
     {
       listenerFilters = Object.create(null);
       changeListeners.set(page, listenerFilters);
     }
@@ skipping 71 matching lines @@
       message.thirdParty);
 
     return filter instanceof BlockingFilter;
   });
 
   port.on("filters.get", (message, sender) =>
   {
     if (message.what == "elemhideemulation")
     {
       let filters = [];
-      let checkWhitelisted = require("whitelisting").checkWhitelisted;
+      const {checkWhitelisted} = require("whitelisting");
 
       if (Prefs.enabled && !checkWhitelisted(sender.page, sender.frame,
                             RegExpFilter.typeMap.DOCUMENT |
                             RegExpFilter.typeMap.ELEMHIDE))
       {
         let hostname = sender.frame.url.hostname;
         filters = ElemHideEmulation.getRulesForDomain(hostname);
         filters = filters.map(filter =>
         {
           return {
@@ skipping 10 matching lines @@
       return [];
 
     return subscription.filters.map(convertFilter);
   });
 
   port.on("filters.importRaw", (message, sender) =>
   {
     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;
 
     let seenFilter = Object.create(null);
     for (let filter of result.filters)
     {
       FilterStorage.addFilter(filter);
       seenFilter[filter.text] = null;
     }
@@ skipping 40 matching lines @@
   });
 
   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(preference =>
+    for (let preference of message.filter)
     {
       if (!(preference in listenedPreferences))
       {
         listenedPreferences[preference] = null;
         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];
   });
 
@@ skipping 70 matching lines @@
       if (!subscription.lastDownload)
         Synchronizer.execute(subscription);
     }
   });
 
   port.on("subscriptions.update", (message, sender) =>
   {
     let subscriptions = message.url ? [Subscription.fromURL(message.url)] :
                         FilterStorage.subscriptions;
     for (let subscription of subscriptions)
+    {
       if (subscription instanceof DownloadableSubscription)
         Synchronizer.execute(subscription, true);
+    }
   });
 }