Issue 4864 - Start using ESLint for adblockpluschrome

chrome/ext/common.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 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";
 
+(function()

[Sebastian Noack, 2017/03/29 17:14:11]

Apparently, it was decided to put IIFEs everywhere. From a comment above in this
review, block scoping is assumed to be a footgun with function definitions. How
is that? Named functions are block scoped in strict-mode, and since we ensure
strict-mode with ESLint, you cannot argue that somebody might forget to turn
strict-mode on, or do I miss something?

Anyway, you reverted other changes because they seem unrelated to you, how is
adding IIFEs here related to ESLint compliance, or is there an ESLint rule that
enforces them?

[kzar, 2017/03/30 07:20:41]

Apparently it's a syntax error to declare a named function inside of a block
scope like that. Chrome ignored it but Firefox didn't. This meant that some of
the adblockplusui/adblockpluscore (I forget) changes caused problems for Firefox
and we had to switch back to using IIFEs there.

As Wladimir pointed out sometimes we used a block like that, without declaring
named functions. While not a syntax error it was kind of easy to add a function
declaration without remembering to switch to a IIFE, so for that reason and
consistency we changed them for those too.
I would really rather not. Do you insist there?

[Sebastian Noack, 2017/03/30 18:57:56]

Chrome does not "ignore" the error but behaves correctly here. Function
declarations in nested blocks are supported in ES6, with the function name being
block-scoped and hoisted to the top of the lexical block, which is exactly what
Chrome does in strict mode. In ES5, however, the behavior of function
declarations in nested blocks was undefined. Firefox is apparently behind the
standard here, and I confirmed that in strict mode it is throwing an error. So
yeah, it seems we are back to IIFEs. Not only for consistence, but also because
this code has to run on Firefox soon. :(

[kzar, 2017/03/31 03:42:17]

Acknowledged.

 {
   // Workaround since HTMLCollection and NodeList didn't have iterator support
   // before Chrome 51.
   // https://bugs.chromium.org/p/chromium/issues/detail?id=401699
+  let arrayIterator = Array.prototype[Symbol.iterator];
   if (!(Symbol.iterator in HTMLCollection.prototype))
-    HTMLCollection.prototype[Symbol.iterator] = Array.prototype[Symbol.iterator];
+    HTMLCollection.prototype[Symbol.iterator] = arrayIterator;
   if (!(Symbol.iterator in NodeList.prototype))
-    NodeList.prototype[Symbol.iterator] = Array.prototype[Symbol.iterator];
+    NodeList.prototype[Symbol.iterator] = arrayIterator;
 
   /* Message passing */
 
   ext.onMessage = new ext._EventTarget();
 
 
   /* Background page */
 
   ext.backgroundPage = {
     sendMessage: chrome.runtime.sendMessage,
     getWindow()
     {
       return chrome.extension.getBackgroundPage();
     }
   };
 
 
   /* Utils */
 
   ext.getURL = chrome.extension.getURL;
   ext.i18n = chrome.i18n;
-}
+}());