Issue 4871 - Start using ESLint for adblockplusui

Issue 4871 - Start using ESLint for adblockplusui

adblockpluschrome still seems to work when using these changes, but I need to go through and test more thoroughly, especially things like the new options page.

[kzar, 2017-02-20 10:33:45]

Patch Set 1

[kzar, 2017-02-21 05:16:19]

Patch Set 2 : Stop using commonjs, fix other problems

[Sebastian Noack, 2017-02-21 12:21:21]

https://codereview.adblockplus.org/29375899/diff/29376653/background.js
File background.js (right):

https://codereview.adblockplus.org/29375899/diff/29376653/background.js#newco...
background.js:135: /* eslint-disable object-shorthand */
Note that the object-shorthand rule has an ignoreConstructors option, in order
to ignore capitalized properties.

Alternatively, we could just define the constructur beforehand, as we did in at
least one other place already, in order to avoid triggering this rule:

  function Subscription()
  {
    ...
  }
  modules.subscriptionClasses = {
    Subscription,
    ...
  };

Not sure which I prefer. But either option seems better than the inline
configuration.

https://codereview.adblockplus.org/29375899/diff/29376653/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29375899/diff/29376653/ext/common.js#newco...
ext/common.js:174: /* eslint-disable no-constant-condition */
As discussed in the changes for adblockpluscore, we should rather remove this
rule.

https://codereview.adblockplus.org/29375899/diff/29376653/i18n.js
File i18n.js (right):

https://codereview.adblockplus.org/29375899/diff/29376653/i18n.js#newcode22
i18n.js:22: let {i18n} = ext;
Is this even still neccessary? Check the comment above.

https://codereview.adblockplus.org/29375899/diff/29376653/i18n.js#newcode80
i18n.js:80: let {className} = node;
Perhaps just inline node.className below.

https://codereview.adblockplus.org/29375899/diff/29376653/i18n.js#newcode97
i18n.js:97: /* eslint-disable camelcase */
Just rename i18n_timeDateStrings and i18n_formatDateTime, in order to match the
camelcase naming convention. The latter isn't even used outside of
adblockplusui, and the call to i18n_timeDateStrings in
adblockpluschrome/options.js can just be adapted when updating the dependency.

https://codereview.adblockplus.org/29375899/diff/29376653/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29375899/diff/29376653/messageResponder.js...
messageResponder.js:21: if (typeof ext == "undefined")
For consistency, please refer to: https://codereview.adblockplus.org/29370594/

That way, we also avoid defining a global variable here.

https://codereview.adblockplus.org/29375899/diff/29376653/messageResponder.js...
messageResponder.js:94: for (let i = 0; i < args.length; i++)
If you keep creating a new array here, you could use for..of, resulting into
simpler code. And IIRC, this would also avoid a deoptimization, triggered by
modifying rest arguments.

https://codereview.adblockplus.org/29375899/diff/29376653/messageResponder.js...
messageResponder.js:420: : FilterStorage.subscriptions;
Instead of wrapping a ternary operation, the following might be more readable:

  let subscriptions = FilterStorage.subscriptions;
  if (message.url)
    subscriptions = [Subscription.fromURL(message.url)];

https://codereview.adblockplus.org/29375899/diff/29376653/new-options.js
File new-options.js (right):

https://codereview.adblockplus.org/29375899/diff/29376653/new-options.js#newc...
new-options.js:785: tweaks.forEach(key =>
According to our coding pracitces, for..of loops shoud be used instead of the
forEach() method. (Please check other calls to forEach() as well.)

https://codereview.adblockplus.org/29375899/diff/29376653/new-options.js#newc...
new-options.js:892: for (let i = 0; i < subscriptions.length; i++)
Can't this be a for..of loop instead? (Please check other loops as well.)

[kzar, 2017-02-23 11:10:22]

Patch Set 3 : Started addressing Sebastian's initial feedback

Ran out of time, so didn't get through all your comments or test this latest
patchset at all.

https://codereview.adblockplus.org/29375899/diff/29376653/background.js
File background.js (right):

https://codereview.adblockplus.org/29375899/diff/29376653/background.js#newco...
background.js:135: /* eslint-disable object-shorthand */
On 2017/02/21 12:21:20, Sebastian Noack wrote:
> Note that the object-shorthand rule has an ignoreConstructors option, in order
> to ignore capitalized properties.
> 
> Alternatively, we could just define the constructur beforehand, as we did in
at
> least one other place already, in order to avoid triggering this rule:
> 
>   function Subscription()
>   {
>     ...
>   }
>   modules.subscriptionClasses = {
>     Subscription,
>     ...
>   };
> 
> Not sure which I prefer. But either option seems better than the inline
> configuration.

Done.

https://codereview.adblockplus.org/29375899/diff/29376653/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29375899/diff/29376653/ext/common.js#newco...
ext/common.js:174: /* eslint-disable no-constant-condition */
On 2017/02/21 12:21:20, Sebastian Noack wrote:
> As discussed in the changes for adblockpluscore, we should rather remove this
> rule.

Done.

https://codereview.adblockplus.org/29375899/diff/29376653/i18n.js
File i18n.js (right):

https://codereview.adblockplus.org/29375899/diff/29376653/i18n.js#newcode22
i18n.js:22: let {i18n} = ext;
On 2017/02/21 12:21:20, Sebastian Noack wrote:
> Is this even still neccessary? Check the comment above.

Well I guess we could change options.js if that's the only reason for this. Any
idea Thomas?

https://codereview.adblockplus.org/29375899/diff/29376653/i18n.js#newcode80
i18n.js:80: let {className} = node;
On 2017/02/21 12:21:20, Sebastian Noack wrote:
> Perhaps just inline node.className below.

I don't think that makes sense.

https://codereview.adblockplus.org/29375899/diff/29376653/i18n.js#newcode97
i18n.js:97: /* eslint-disable camelcase */
On 2017/02/21 12:21:20, Sebastian Noack wrote:
> Just rename i18n_timeDateStrings and i18n_formatDateTime, in order to match
the
> camelcase naming convention. The latter isn't even used outside of
> adblockplusui, and the call to i18n_timeDateStrings in
> adblockpluschrome/options.js can just be adapted when updating the dependency.

Done.

[kzar, 2017-03-01 05:36:21]

Patch Set 4 : Addressed the rest of Sebastian's feedback

https://codereview.adblockplus.org/29375899/diff/29376653/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29375899/diff/29376653/messageResponder.js...
messageResponder.js:21: if (typeof ext == "undefined")
On 2017/02/21 12:21:21, Sebastian Noack wrote:
> For consistency, please refer to: https://codereview.adblockplus.org/29370594/
> 
> That way, we also avoid defining a global variable here.

I don't think that will work since we can't redeclare ext now that we've used
`let`.

https://codereview.adblockplus.org/29375899/diff/29376653/messageResponder.js...
messageResponder.js:94: for (let i = 0; i < args.length; i++)
On 2017/02/21 12:21:21, Sebastian Noack wrote:
> If you keep creating a new array here, you could use for..of, resulting into
> simpler code. And IIRC, this would also avoid a deoptimization, triggered by
> modifying rest arguments.

Done.

https://codereview.adblockplus.org/29375899/diff/29376653/messageResponder.js...
messageResponder.js:420: : FilterStorage.subscriptions;
On 2017/02/21 12:21:21, Sebastian Noack wrote:
> Instead of wrapping a ternary operation, the following might be more readable:
> 
>   let subscriptions = FilterStorage.subscriptions;
>   if (message.url)
>     subscriptions = [Subscription.fromURL(message.url)];

Done.

https://codereview.adblockplus.org/29375899/diff/29376653/new-options.js
File new-options.js (right):

https://codereview.adblockplus.org/29375899/diff/29376653/new-options.js#newc...
new-options.js:785: tweaks.forEach(key =>
On 2017/02/21 12:21:21, Sebastian Noack wrote:
> According to our coding pracitces, for..of loops shoud be used instead of the
> forEach() method. (Please check other calls to forEach() as well.)

Done.

https://codereview.adblockplus.org/29375899/diff/29376653/new-options.js#newc...
new-options.js:892: for (let i = 0; i < subscriptions.length; i++)
On 2017/02/21 12:21:21, Sebastian Noack wrote:
> Can't this be a for..of loop instead? (Please check other loops as well.)

Done.

[kzar, 2017-03-01 08:26:25]

Patch Set 5 : Fix regressions with the new options page

[Thomas Greiner, 2017-03-01 17:39:36]

https://codereview.adblockplus.org/29375899/diff/29375900/background.js
File background.js (right):

https://codereview.adblockplus.org/29375899/diff/29375900/background.js#newco...
background.js:105: Object.keys(prefs).forEach(key =>
Detail: Mind keeping the brackets? At least in Flattr Plus we agreed on using
brackets for single argument arrow functions for consistency.

https://codereview.adblockplus.org/29375899/diff/29375900/background.js#newco...
background.js:135: /* eslint-disable object-shorthand */
Detail: What about moving the constructors outside of the object definition
instead? The reasoning behind it being that it might not be wise to disable
rules for larger code blocks.

https://codereview.adblockplus.org/29375899/diff/29377713/README.md
File README.md (right):

https://codereview.adblockplus.org/29375899/diff/29377713/README.md#newcode103
README.md:103: eslint *.js lib ext
This is only useful with a config file so we should probably instead mention:

eslint -c <path to eslint-config-eyeo> *.js lib ext

https://codereview.adblockplus.org/29375899/diff/29377713/background.js
File background.js (right):

https://codereview.adblockplus.org/29375899/diff/29377713/background.js#newco...
background.js:58: for (let param of window.location.search.substr(1).split("&"))
Detail: Putting that long statement within the for-of loop makes it more
difficult to read so I'd prefer keeping the interstitial `params` variable.

https://codereview.adblockplus.org/29375899/diff/29377713/background.js#newco...
background.js:208: let {fromURL} = modules.subscriptionClasses.Subscription;
Detail: You can now refer to `Subscription` directly so
`modules.subscriptionClasses.` is redundant.

https://codereview.adblockplus.org/29375899/diff/29377713/background.js#newco...
background.js:263: Filter.fromText = text =>
Detail: Mind writing this as follows?

Filter.fromText = (text) => new Filter(text);

1) Suggestion: For consistency, always having brackets around arrow function
parameters makes changing code a bit easier.

2) The `return` is redundant in arrow functions.

3) You can refer to `Filter` directly now so `modules.filterClasses.` is
redundant.

https://codereview.adblockplus.org/29375899/diff/29377713/common.js
File common.js (right):

https://codereview.adblockplus.org/29375899/diff/29377713/common.js#newcode18
common.js:18: /* globals Components, E */
Detail: You're also defining `E` as a local variable via `function E(id)` so do
you even need to specify it here?

https://codereview.adblockplus.org/29375899/diff/29377713/common.js#newcode47
common.js:47: window.openSharePopup = function(url)
Interesting that we're not using arrow functions for such assignments. Is this
expected?

https://codereview.adblockplus.org/29375899/diff/29377713/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29375899/diff/29377713/ext/common.js#newco...
ext/common.js:119: let catalogFile = window.location.pathname.replace(/.*\//,
"").
Did we agree on placing dots at the end of lines?

https://codereview.adblockplus.org/29375899/diff/29377713/i18n.js
File i18n.js (right):

https://codereview.adblockplus.org/29375899/diff/29377713/i18n.js#newcode72
i18n.js:72: for (let node of
containerElement.querySelectorAll("[class^='i18n_']"))
Detail: Again, it'd be nice if we could keep this in a separate line to make it
more readable.

https://codereview.adblockplus.org/29375899/diff/29377713/i18n.js#newcode95
i18n.js:95: function i18nTimeDateStrings(when)
Note that this function is used in the current options page in adblockpluschrome
so renaming it here means that we'll also have to rename it there.

https://codereview.adblockplus.org/29375899/diff/29377713/lib/antiadblockInit.js
File lib/antiadblockInit.js (right):

https://codereview.adblockplus.org/29375899/diff/29377713/lib/antiadblockInit...
lib/antiadblockInit.js:76: {
I assume this is necessary due to the "no-shadow" rule but it's pretty
non-intuitive nevertheless because it looks like we do this because
`subscription` is being redeclared here.

Therefore it'd be great if we could handle this conflict in a clearer way.

https://codereview.adblockplus.org/29375899/diff/29377713/new-options.js
File new-options.js (right):

https://codereview.adblockplus.org/29375899/diff/29377713/new-options.js#newc...
new-options.js:29: let filterErrors = new Map([
Why do you use `Map` here but not in messageResponder.js for `messageTypes`?

https://codereview.adblockplus.org/29375899/diff/29377713/new-options.js#newc...
new-options.js:752: addonVersion =>
This being on the same level as the function calls above and below makes it look
quite confusing. Is this really the way we intended indentation to behave?

[kzar, 2017-03-02 04:36:02]

(Just replying to a few comments for now.)

https://codereview.adblockplus.org/29375899/diff/29375900/background.js
File background.js (right):

https://codereview.adblockplus.org/29375899/diff/29375900/background.js#newco...
background.js:105: Object.keys(prefs).forEach(key =>
On 2017/03/01 17:39:32, Thomas Greiner wrote:
> Detail: Mind keeping the brackets? At least in Flattr Plus we agreed on using
> brackets for single argument arrow functions for consistency.

I've opened a codereview to add the arrow-parens rule to our ESLint
configuration. Let's clear this up there once and for all
https://codereview.adblockplus.org/29377779/

https://codereview.adblockplus.org/29375899/diff/29377713/README.md
File README.md (right):

https://codereview.adblockplus.org/29375899/diff/29377713/README.md#newcode103
README.md:103: eslint *.js lib ext
On 2017/03/01 17:39:32, Thomas Greiner wrote:
> This is only useful with a config file so we should probably instead mention:
> 
> eslint -c <path to eslint-config-eyeo> *.js lib ext

Not required since the ESLint configuration in this repository extends from our
eslint-config-eyeo configuration.

https://codereview.adblockplus.org/29375899/diff/29377713/i18n.js
File i18n.js (right):

https://codereview.adblockplus.org/29375899/diff/29377713/i18n.js#newcode95
i18n.js:95: function i18nTimeDateStrings(when)
On 2017/03/01 17:39:35, Thomas Greiner wrote:
> Note that this function is used in the current options page in
adblockpluschrome
> so renaming it here means that we'll also have to rename it there.

Yep, don't worry I've done that. Actually come to think of it perhaps I didn't
update that codereview yet... will check now.

https://codereview.adblockplus.org/29375899/diff/29377713/new-options.js
File new-options.js (right):

https://codereview.adblockplus.org/29375899/diff/29377713/new-options.js#newc...
new-options.js:752: addonVersion =>
On 2017/03/01 17:39:35, Thomas Greiner wrote:
> This being on the same level as the function calls above and below makes it
look
> quite confusing. Is this really the way we intended indentation to behave?

Yes, I think this is how we're expecting the indentation to look. Perhaps if I
add a blank line in between it will help.

[kzar, 2017-03-07 12:48:33]

Patch Set 6 : Addressed Thomas' feedback

https://codereview.adblockplus.org/29375899/diff/29375900/background.js
File background.js (right):

https://codereview.adblockplus.org/29375899/diff/29375900/background.js#newco...
background.js:105: Object.keys(prefs).forEach(key =>
On 2017/03/02 04:36:01, kzar wrote:
> On 2017/03/01 17:39:32, Thomas Greiner wrote:
> > Detail: Mind keeping the brackets? At least in Flattr Plus we agreed on
using
> > brackets for single argument arrow functions for consistency.
> 
> I've opened a codereview to add the arrow-parens rule to our ESLint
> configuration. Let's clear this up there once and for all
> https://codereview.adblockplus.org/29377779/

As discussed we've decided against adding the arrow-parens rule globally. I'll
add it to the configuration here instead.

https://codereview.adblockplus.org/29375899/diff/29375900/background.js#newco...
background.js:135: /* eslint-disable object-shorthand */
On 2017/03/01 17:39:32, Thomas Greiner wrote:
> Detail: What about moving the constructors outside of the object definition
> instead? The reasoning behind it being that it might not be wise to disable
> rules for larger code blocks.

I think I've already done this when addressing Sebastian's feedback.

https://codereview.adblockplus.org/29375899/diff/29377713/background.js
File background.js (right):

https://codereview.adblockplus.org/29375899/diff/29377713/background.js#newco...
background.js:58: for (let param of window.location.search.substr(1).split("&"))
On 2017/03/01 17:39:32, Thomas Greiner wrote:
> Detail: Putting that long statement within the for-of loop makes it more
> difficult to read so I'd prefer keeping the interstitial `params` variable.

Done.

https://codereview.adblockplus.org/29375899/diff/29377713/background.js#newco...
background.js:208: let {fromURL} = modules.subscriptionClasses.Subscription;
On 2017/03/01 17:39:34, Thomas Greiner wrote:
> Detail: You can now refer to `Subscription` directly so
> `modules.subscriptionClasses.` is redundant.

Done.

https://codereview.adblockplus.org/29375899/diff/29377713/background.js#newco...
background.js:263: Filter.fromText = text =>
On 2017/03/01 17:39:34, Thomas Greiner wrote:
> Detail: Mind writing this as follows?
> 
> Filter.fromText = (text) => new Filter(text);
> 
> 1) Suggestion: For consistency, always having brackets around arrow function
> parameters makes changing code a bit easier.
> 
> 2) The `return` is redundant in arrow functions.
> 
> 3) You can refer to `Filter` directly now so `modules.filterClasses.` is
> redundant.

Done.

https://codereview.adblockplus.org/29375899/diff/29377713/common.js
File common.js (right):

https://codereview.adblockplus.org/29375899/diff/29377713/common.js#newcode18
common.js:18: /* globals Components, E */
On 2017/03/01 17:39:34, Thomas Greiner wrote:
> Detail: You're also defining `E` as a local variable via `function E(id)` so
do
> you even need to specify it here?

Well as it is with `window.E = function E(id)` then yes it's somehow required.
But I wonder why we don't simply do `function E(id)` instead, which removes the
need to specify it in the globals comment too. Can you think of any reason to
also assign it to window.E? (Same for the other functions below too.)

https://codereview.adblockplus.org/29375899/diff/29377713/common.js#newcode47
common.js:47: window.openSharePopup = function(url)
On 2017/03/01 17:39:34, Thomas Greiner wrote:
> Interesting that we're not using arrow functions for such assignments. Is this
> expected?

Well I've changed this to a regular named function now, see above.

https://codereview.adblockplus.org/29375899/diff/29377713/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29375899/diff/29377713/ext/common.js#newco...
ext/common.js:119: let catalogFile = window.location.pathname.replace(/.*\//,
"").
On 2017/03/01 17:39:35, Thomas Greiner wrote:
> Did we agree on placing dots at the end of lines?

No, I don't think we agreed anything specific there. Do you have a preference
how this one should look?

https://codereview.adblockplus.org/29375899/diff/29377713/i18n.js
File i18n.js (right):

https://codereview.adblockplus.org/29375899/diff/29377713/i18n.js#newcode72
i18n.js:72: for (let node of
containerElement.querySelectorAll("[class^='i18n_']"))
On 2017/03/01 17:39:35, Thomas Greiner wrote:
> Detail: Again, it'd be nice if we could keep this in a separate line to make
it
> more readable.

Done.

https://codereview.adblockplus.org/29375899/diff/29377713/lib/antiadblockInit.js
File lib/antiadblockInit.js (right):

https://codereview.adblockplus.org/29375899/diff/29377713/lib/antiadblockInit...
lib/antiadblockInit.js:76: {
On 2017/03/01 17:39:35, Thomas Greiner wrote:
> I assume this is necessary due to the "no-shadow" rule but it's pretty
> non-intuitive nevertheless because it looks like we do this because
> `subscription` is being redeclared here.
> 
> Therefore it'd be great if we could handle this conflict in a clearer way.

Done.

https://codereview.adblockplus.org/29375899/diff/29377713/new-options.js
File new-options.js (right):

https://codereview.adblockplus.org/29375899/diff/29377713/new-options.js#newc...
new-options.js:29: let filterErrors = new Map([
On 2017/03/01 17:39:35, Thomas Greiner wrote:
> Why do you use `Map` here but not in messageResponder.js for `messageTypes`?

Well I changed this one since it helped fix some long lines. But you're right we
should really change the other one too. Done.

[kzar, 2017-03-07 12:56:51]

Patch Set 7 : Rebased.

[Thomas Greiner, 2017-03-07 13:33:03]

Only a small question regarding the "max-len" rule and an answer to your
question in regards to whether or not to put the dot at the end of the line.

Maybe adding the "arrow-parens" rule to the ESLint config could be considered to
be too strict but I guess we can just change it whenever we decide that we don't
want to enforce stricter rules in modules.

https://codereview.adblockplus.org/29375899/diff/29375900/background.js
File background.js (right):

https://codereview.adblockplus.org/29375899/diff/29375900/background.js#newco...
background.js:105: Object.keys(prefs).forEach(key =>
On 2017/03/07 12:48:29, kzar wrote:
> On 2017/03/02 04:36:01, kzar wrote:
> > On 2017/03/01 17:39:32, Thomas Greiner wrote:
> > > Detail: Mind keeping the brackets? At least in Flattr Plus we agreed on
> using
> > > brackets for single argument arrow functions for consistency.
> > 
> > I've opened a codereview to add the arrow-parens rule to our ESLint
> > configuration. Let's clear this up there once and for all
> > https://codereview.adblockplus.org/29377779/
> 
> As discussed we've decided against adding the arrow-parens rule globally. I'll
> add it to the configuration here instead.

I appreciate that because according to Felix it's up to the author to decide
whether or not to add parentheses or not.

https://codereview.adblockplus.org/29375899/diff/29377713/README.md
File README.md (right):

https://codereview.adblockplus.org/29375899/diff/29377713/README.md#newcode103
README.md:103: eslint *.js lib ext
On 2017/03/02 04:36:01, kzar wrote:
> On 2017/03/01 17:39:32, Thomas Greiner wrote:
> > This is only useful with a config file so we should probably instead
mention:
> > 
> > eslint -c <path to eslint-config-eyeo> *.js lib ext
> 
> Not required since the ESLint configuration in this repository extends from
our
> eslint-config-eyeo configuration.

Right, nevermind then.

https://codereview.adblockplus.org/29375899/diff/29377713/common.js
File common.js (right):

https://codereview.adblockplus.org/29375899/diff/29377713/common.js#newcode18
common.js:18: /* globals Components, E */
On 2017/03/07 12:48:31, kzar wrote:
> On 2017/03/01 17:39:34, Thomas Greiner wrote:
> > Detail: You're also defining `E` as a local variable via `function E(id)` so
> do
> > you even need to specify it here?
> 
> Well as it is with `window.E = function E(id)` then yes it's somehow required.
> But I wonder why we don't simply do `function E(id)` instead, which removes
the
> need to specify it in the globals comment too. Can you think of any reason to
> also assign it to window.E? (Same for the other functions below too.)

Since we no longer have it inside a closure we no longer need to explicitly
assign it to `window`. Therefore I agree with that change.

https://codereview.adblockplus.org/29375899/diff/29377713/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29375899/diff/29377713/ext/common.js#newco...
ext/common.js:119: let catalogFile = window.location.pathname.replace(/.*\//,
"").
On 2017/03/07 12:48:32, kzar wrote:
> On 2017/03/01 17:39:35, Thomas Greiner wrote:
> > Did we agree on placing dots at the end of lines?
> 
> No, I don't think we agreed anything specific there. Do you have a preference
> how this one should look?

Only a personal preference of having it at the beginning of the line. I guess
it's up to the author though.

Pro: Less chance of confusion with other statements due to being prefixed with
dot.
Con: Less chance of accidental semi-colon insertion.

https://codereview.adblockplus.org/29375899/diff/29377713/i18n.js
File i18n.js (right):

https://codereview.adblockplus.org/29375899/diff/29377713/i18n.js#newcode95
i18n.js:95: function i18nTimeDateStrings(when)
On 2017/03/02 04:36:01, kzar wrote:
> On 2017/03/01 17:39:35, Thomas Greiner wrote:
> > Note that this function is used in the current options page in
> adblockpluschrome
> > so renaming it here means that we'll also have to rename it there.
> 
> Yep, don't worry I've done that. Actually come to think of it perhaps I didn't
> update that codereview yet... will check now.

Great, thanks.

https://codereview.adblockplus.org/29375899/diff/29377713/new-options.js
File new-options.js (right):

https://codereview.adblockplus.org/29375899/diff/29377713/new-options.js#newc...
new-options.js:752: addonVersion =>
On 2017/03/02 04:36:01, kzar wrote:
> On 2017/03/01 17:39:35, Thomas Greiner wrote:
> > This being on the same level as the function calls above and below makes it
> look
> > quite confusing. Is this really the way we intended indentation to behave?
> 
> Yes, I think this is how we're expecting the indentation to look. Perhaps if I
> add a blank line in between it will help.

Nah, because the issue is that putting it in the same line makes it look like
its own statement and separating them even further would only strengthen that
confusion, I'd argue.

Anyway, if this is how it's supposed to be I won't stand in the way.

https://codereview.adblockplus.org/29375899/diff/29378596/new-options.js
File new-options.js (right):

https://codereview.adblockplus.org/29375899/diff/29378596/new-options.js#newc...
new-options.js:18: /* globals checkShareResource, getDocLink,
i18nFormatDateTime, openSharePopup,
Detail: Isn't this violating the "max-len" rule?

[kzar, 2017-03-08 10:29:33]

Patch Set 8 : Move dots to the start of the line

Also I should mention that I've not tested patch sets 6, 7 or 8 yet. I'm going
to test everything together once I've finished updating the adblockpluscore
review.

https://codereview.adblockplus.org/29375899/diff/29377713/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29375899/diff/29377713/ext/common.js#newco...
ext/common.js:119: let catalogFile = window.location.pathname.replace(/.*\//,
"").
On 2017/03/07 13:33:01, Thomas Greiner wrote:
> On 2017/03/07 12:48:32, kzar wrote:
> > On 2017/03/01 17:39:35, Thomas Greiner wrote:
> > > Did we agree on placing dots at the end of lines?
> > 
> > No, I don't think we agreed anything specific there. Do you have a
preference
> > how this one should look?
> 
> Only a personal preference of having it at the beginning of the line. I guess
> it's up to the author though.
> 
> Pro: Less chance of confusion with other statements due to being prefixed with
> dot.
> Con: Less chance of accidental semi-colon insertion.

Done.

https://codereview.adblockplus.org/29375899/diff/29377713/new-options.js
File new-options.js (right):

https://codereview.adblockplus.org/29375899/diff/29377713/new-options.js#newc...
new-options.js:752: addonVersion =>
On 2017/03/07 13:33:01, Thomas Greiner wrote:
> On 2017/03/02 04:36:01, kzar wrote:
> > On 2017/03/01 17:39:35, Thomas Greiner wrote:
> > > This being on the same level as the function calls above and below makes
it
> > look
> > > quite confusing. Is this really the way we intended indentation to behave?
> > 
> > Yes, I think this is how we're expecting the indentation to look. Perhaps if
I
> > add a blank line in between it will help.
> 
> Nah, because the issue is that putting it in the same line makes it look like
> its own statement and separating them even further would only strengthen that
> confusion, I'd argue.
> 
> Anyway, if this is how it's supposed to be I won't stand in the way.

Acknowledged.

https://codereview.adblockplus.org/29375899/diff/29378596/new-options.js
File new-options.js (right):

https://codereview.adblockplus.org/29375899/diff/29378596/new-options.js#newc...
new-options.js:18: /* globals checkShareResource, getDocLink,
i18nFormatDateTime, openSharePopup,
On 2017/03/07 13:33:02, Thomas Greiner wrote:
> Detail: Isn't this violating the "max-len" rule?

No, just under.

[Thomas Greiner, 2017-03-08 11:32:42]

LGTM unless you want to take
https://issues.adblockplus.org/ticket/3692#comment:51 into consideration but I
don't mind either way.

[kzar, 2017-03-08 14:15:54]

https://codereview.adblockplus.org/29375899/diff/29377713/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29375899/diff/29377713/messageResponder.js...
messageResponder.js:21: if (typeof ext == "undefined")
I'm a bit confused by the discussion in
https://codereview.adblockplus.org/29370594/ . Is this (also the similar changes
to ext/common.js, ext/content.js and ext/devtools.js) correct?

[kzar, 2017-03-09 10:36:59]

Patch Set 9 : Remove the arrow-parens rule

[Wladimir Palant, 2017-03-09 15:05:09]

https://codereview.adblockplus.org/29375899/diff/29379555/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29375899/diff/29379555/.eslintrc.json#newc...
.eslintrc.json:11: "require": true
The way I see it, exports and require are mostly listed here for
adblockplusInit.js which is actually a proper CommonJS module. I'd suggest
adding .eslintrc.json to the lib/ directory and declare env: { commonjs: true}
there.

As to require, this is also used by messageResponder.js which is a special case
that could run in both environments. I'd declare require as a global explicitly
in this file only to indicate that.

No other files can/should used exports and require so these globals shouldn't be
defined for the entire repository.

https://codereview.adblockplus.org/29375899/diff/29379555/background.js
File background.js (right):

https://codereview.adblockplus.org/29375899/diff/29379555/background.js#newco...
background.js:21: function EventEmitter()
Please revert to IIFE here - functions should not be declared in the block
scope. This code will throw in strict mode at least in Firefox, and EventEmitter
will still be declared globally despite the block.

https://codereview.adblockplus.org/29375899/diff/29379555/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29375899/diff/29379555/ext/background.js#n...
ext/background.js:31: function PageMap()
Same here, please switch back to IIFE.

https://codereview.adblockplus.org/29375899/diff/29379555/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29375899/diff/29379555/ext/common.js#newco...
ext/common.js:24: function Page(source)
As with other files, please revert to IIFE.

https://codereview.adblockplus.org/29375899/diff/29379555/ext/content.js
File ext/content.js (right):

https://codereview.adblockplus.org/29375899/diff/29379555/ext/content.js#newc...
ext/content.js:20: {
Here we have no functions declared so strictly speaking no need for IIFE. I
would still add it back however, for consistency and so that somebody adding
code later won't run into this trap.

https://codereview.adblockplus.org/29375899/diff/29379555/firstRun.js
File firstRun.js (right):

https://codereview.adblockplus.org/29375899/diff/29379555/firstRun.js#newcode23
firstRun.js:23: function onDOMLoaded()
As with other files, this won't work without IIFE.

https://codereview.adblockplus.org/29375899/diff/29379555/lib/antiadblockInit.js
File lib/antiadblockInit.js (right):

https://codereview.adblockplus.org/29375899/diff/29379555/lib/antiadblockInit...
lib/antiadblockInit.js:18: /* globals Cu */
This global is actually common to all our CommonJS modules and can be declared
for the entire directory.

https://codereview.adblockplus.org/29375899/diff/29379555/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29375899/diff/29379555/messageResponder.js...
messageResponder.js:22: window.ext = require("ext_background");
Note that this is fixed in https://codereview.adblockplus.org/29379570/

[kzar, 2017-03-10 07:29:01]

Patch Set 10 : Addressed Wladimir's feedback, restored IIFEs

https://codereview.adblockplus.org/29375899/diff/29379555/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29375899/diff/29379555/.eslintrc.json#newc...
.eslintrc.json:11: "require": true
On 2017/03/09 15:05:06, Wladimir Palant wrote:
> The way I see it, exports and require are mostly listed here for
> adblockplusInit.js which is actually a proper CommonJS module. I'd suggest
> adding .eslintrc.json to the lib/ directory and declare env: { commonjs: true}
> there.
> 
> As to require, this is also used by messageResponder.js which is a special
case
> that could run in both environments. I'd declare require as a global
explicitly
> in this file only to indicate that.
> 
> No other files can/should used exports and require so these globals shouldn't
be
> defined for the entire repository.

Done.

https://codereview.adblockplus.org/29375899/diff/29379555/background.js
File background.js (right):

https://codereview.adblockplus.org/29375899/diff/29379555/background.js#newco...
background.js:21: function EventEmitter()
On 2017/03/09 15:05:06, Wladimir Palant wrote:
> Please revert to IIFE here - functions should not be declared in the block
> scope. This code will throw in strict mode at least in Firefox, and
EventEmitter
> will still be declared globally despite the block.

Done.

But unless I missed something passing window as a parameter to the IIFE was
pointless, so I didn't revert that part.

https://codereview.adblockplus.org/29375899/diff/29379555/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29375899/diff/29379555/ext/background.js#n...
ext/background.js:31: function PageMap()
On 2017/03/09 15:05:06, Wladimir Palant wrote:
> Same here, please switch back to IIFE.

Done.

https://codereview.adblockplus.org/29375899/diff/29379555/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29375899/diff/29379555/ext/common.js#newco...
ext/common.js:24: function Page(source)
On 2017/03/09 15:05:07, Wladimir Palant wrote:
> As with other files, please revert to IIFE.

Done.

https://codereview.adblockplus.org/29375899/diff/29379555/ext/content.js
File ext/content.js (right):

https://codereview.adblockplus.org/29375899/diff/29379555/ext/content.js#newc...
ext/content.js:20: {
On 2017/03/09 15:05:07, Wladimir Palant wrote:
> Here we have no functions declared so strictly speaking no need for IIFE. I
> would still add it back however, for consistency and so that somebody adding
> code later won't run into this trap.

Done.

https://codereview.adblockplus.org/29375899/diff/29379555/firstRun.js
File firstRun.js (right):

https://codereview.adblockplus.org/29375899/diff/29379555/firstRun.js#newcode23
firstRun.js:23: function onDOMLoaded()
On 2017/03/09 15:05:08, Wladimir Palant wrote:
> As with other files, this won't work without IIFE.

Done.

https://codereview.adblockplus.org/29375899/diff/29379555/lib/antiadblockInit.js
File lib/antiadblockInit.js (right):

https://codereview.adblockplus.org/29375899/diff/29379555/lib/antiadblockInit...
lib/antiadblockInit.js:18: /* globals Cu */
On 2017/03/09 15:05:08, Wladimir Palant wrote:
> This global is actually common to all our CommonJS modules and can be declared
> for the entire directory.

Done.

https://codereview.adblockplus.org/29375899/diff/29379555/lib/antiadblockInit...
lib/antiadblockInit.js:24: let {Utils} = require("utils");
ESLint was complaining this was unused so I've removed it. Not sure how I missed
it until now, perhaps a rebase brought it back.

https://codereview.adblockplus.org/29375899/diff/29379555/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29375899/diff/29379555/messageResponder.js...
messageResponder.js:22: window.ext = require("ext_background");
On 2017/03/09 15:05:08, Wladimir Palant wrote:
> Note that this is fixed in https://codereview.adblockplus.org/29379570/

Acknowledged.

[kzar, 2017-03-14 07:36:59]

Patch Set 11 : Rebased.

[kzar, 2017-03-14 10:13:36]

Patch Set 12 : Fix imports in antiadblockInit.js

[kzar, 2017-03-14 10:57:37]

https://codereview.adblockplus.org/29375899/diff/29383618/lib/antiadblockInit.js
File lib/antiadblockInit.js (right):

https://codereview.adblockplus.org/29375899/diff/29383618/lib/antiadblockInit...
lib/antiadblockInit.js:20: const {Services} =
Cu.import("resource://gre/modules/Services.jsm", {});
I just noticed this gives me a no-unused-vars error. I wonder why we import
Services here if we're not using it?

[Wladimir Palant, 2017-03-14 11:14:58]

LGTM regardless of whether you decide to remove the unnecessary Services import
with this commit or a separate one.

https://codereview.adblockplus.org/29375899/diff/29379555/background.js
File background.js (right):

https://codereview.adblockplus.org/29375899/diff/29379555/background.js#newco...
background.js:21: function EventEmitter()
On 2017/03/10 07:28:58, kzar wrote:
> But unless I missed something passing window as a parameter to the IIFE was
> pointless, so I didn't revert that part.

Not strictly necessary here, it was merely there to avoid assumptions about the
execution context. These files always run in window context, messageResponder
doesn't.

https://codereview.adblockplus.org/29375899/diff/29383618/lib/antiadblockInit.js
File lib/antiadblockInit.js (right):

https://codereview.adblockplus.org/29375899/diff/29383618/lib/antiadblockInit...
lib/antiadblockInit.js:20: const {Services} =
Cu.import("resource://gre/modules/Services.jsm", {});
On 2017/03/14 10:57:36, kzar wrote:
> I just noticed this gives me a no-unused-vars error. I wonder why we import
> Services here if we're not using it?

This seems to be a left-over, even though I don't see why a previous incarnation
of this code would need Services. Feel free to remove it.

[kzar, 2017-03-15 03:16:59]

Patch Set 13 : Removed unused import

https://codereview.adblockplus.org/29375899/diff/29379555/background.js
File background.js (right):

https://codereview.adblockplus.org/29375899/diff/29379555/background.js#newco...
background.js:21: function EventEmitter()
On 2017/03/14 11:14:57, Wladimir Palant wrote:
> On 2017/03/10 07:28:58, kzar wrote:
> > But unless I missed something passing window as a parameter to the IIFE was
> > pointless, so I didn't revert that part.
> 
> Not strictly necessary here, it was merely there to avoid assumptions about
the
> execution context. These files always run in window context, messageResponder
> doesn't.

Acknowledged.

https://codereview.adblockplus.org/29375899/diff/29383618/lib/antiadblockInit.js
File lib/antiadblockInit.js (right):

https://codereview.adblockplus.org/29375899/diff/29383618/lib/antiadblockInit...
lib/antiadblockInit.js:20: const {Services} =
Cu.import("resource://gre/modules/Services.jsm", {});
On 2017/03/14 11:14:57, Wladimir Palant wrote:
> On 2017/03/14 10:57:36, kzar wrote:
> > I just noticed this gives me a no-unused-vars error. I wonder why we import
> > Services here if we're not using it?
> 
> This seems to be a left-over, even though I don't see why a previous
incarnation
> of this code would need Services. Feel free to remove it.

Done.

[kzar, 2017-03-15 04:44:48]

Patch Set 14 : Avoid violating operator-linebreak rule

[Thomas Greiner, 2017-03-15 17:22:48]

LGTM

[Wladimir Palant, 2017-03-16 10:50:26]

LGTM