Issue 4864 - Start using ESLint for adblockpluschrome

Issue 4864 - Start using ESLint for adblockpluschrome

[kzar, 2017-02-07 17:06:17]

Patch Set 1

Changes as requested, but as mentioned in IRC this is a WIP and also I haven't
had a chance to check over them again.

[kzar, 2017-02-08 07:29:05]

Patch Set 2 : Reduce inline eslint-disable comments

[kzar, 2017-02-08 09:04:20]

Patch Set 3 : Use var for ext declarations again

Unfortunately using `let` for the ext declarations caused other scoping issues I
couldn't solve, for example the popup page broke. I've reverted those changes
and therefore this is no longer blocked by the messageResponder changes.

[Sebastian Noack, 2017-02-09 01:04:52]

https://codereview.adblockplus.org/29374674/diff/29374779/.eslintignore
File .eslintignore (right):

https://codereview.adblockplus.org/29374674/diff/29374779/.eslintignore#newcode1
.eslintignore:1: lib/punycode.js
I really don't like how the ESLint configuration is spread out over multiple
configuration files (not to mention the inline configurations). Wouldn't it be
possible to files to be ignored in .eslintrc.json, at least? Also let me know if
you have a better idea for qunit/.eslintrc.json.

https://codereview.adblockplus.org/29374674/diff/29374779/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/chrome/ext/backgrou...
chrome/ext/background.js:385: );
The parenthesis here seem unnecessary.

https://codereview.adblockplus.org/29374674/diff/29374779/chrome/ext/backgrou...
chrome/ext/background.js:395: propagateHandlerBehaviorChange
I wonder whether we can perhaps shorten the variable name, rather than awkwardly
wrapping those lines. The variable name can be up to 19 characters in order to
avoid exceeding 80 characters total.

https://codereview.adblockplus.org/29374674/diff/29374779/chrome/ext/backgrou...
chrome/ext/background.js:513: return results.indexOf(true) != -1;
This would be another way to wrap the code here, without a temporary variable:

  return ext.onMessage._dispatch(message,
                                 sender,
                                 sendResponse).indexOf(true) != -1;

No strong opinion though. What do you think?

https://codereview.adblockplus.org/29374674/diff/29374779/chrome/ext/common.js
File chrome/ext/common.js (left):

https://codereview.adblockplus.org/29374674/diff/29374779/chrome/ext/common.j...
chrome/ext/common.js:20: {
I just noticed separating ext/common.js and chrome/ext/common.js does no longer
make sense since Safari is gone. So while changing the whole code here (due to
indentation) anyway, how about merging those files instead. That shouldn't make
the patch much larger, but would simplify the code structure a bit, while
getting rid of the redundant block.

https://codereview.adblockplus.org/29374674/diff/29374779/chrome/ext/popup.js
File chrome/ext/popup.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/chrome/ext/popup.js...
chrome/ext/popup.js:7: var ext = Object.create(backgroundPage.ext);
On 2017/02/08 09:04:20, kzar wrote:
> Unfortunately using `let` for the ext declarations caused other scoping issues
I
> couldn't solve, for example the popup page broke. I've reverted those changes
> and therefore this is no longer blocked by the messageResponder changes.

The popup code (also) broke because you used Object.create(backgroundPage)
instead of Object.create(backgroundPage.ext). I cannot see any other obvious
problem. If there is, please figure out and explain what is happening there. It
really should be possible to avoid var here.

https://codereview.adblockplus.org/29374674/diff/29374779/composer.js
File composer.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/composer.js#newcode18
composer.js:18: /* globals $ */
I guess we should rather enable the "jquery" environment, than whitelisting "$"
explicitly, if it is just to avoid confusion with Chrome's built-in "$".

Also I'm not too happy with the inline configurations spread across our code. I
wonder whether it would be worth to add the "jQuery" environment to the global
configuration. On the other hand, it's not available in all files. But then
again, this is also true for some of the other environments configured globally,
and this isn't the only file using jQuery either.

https://codereview.adblockplus.org/29374674/diff/29374779/composer.postload.js
File composer.postload.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/composer.postload.j...
composer.postload.js:18: /* globals checkCollapse, elemhide, getURLsFromElement,
typeMap  */
As mentioned in the other review for the base ESLint configuration, I'm opposed
to add this boilerplate. But it points out an issue: global variables aren't
great. So instead making the situation worse by explicitly/redundantly
specifying these globals, we should get rid of them. Note that if we'd use
modules for the related logic reused here, the only global we'd need is
require() which is already known.

https://codereview.adblockplus.org/29374674/diff/29374779/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/include.preload.js#...
include.preload.js:22: // This variable is also used by our other content
scripts.
This comment is kinda redundant with the /* globals ... */ comment in
composer.postload.js. But as I said there, these global variables aren't great
in particular in regard to the additional boilerplate. This should rather be a
module export.

https://codereview.adblockplus.org/29374674/diff/29374779/include.preload.js#...
include.preload.js:365: randomEventName + "-" + event.detail.url, {detail:
block}
If you rename the argument to "detail", you can use the shorthand syntax here.

https://codereview.adblockplus.org/29374674/diff/29374779/include.preload.js#...
include.preload.js:510: document.documentElement).appendChild(this.style);
Is the fallback to document.documentElement even still necessary?

https://codereview.adblockplus.org/29374674/diff/29374779/lib/devtools.js
File lib/devtools.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/lib/devtools.js#new...
lib/devtools.js:141: addRecord(panel, {url, type, docDomain, thirdParty,
sitekey, specificOnly},
Perhaps use a temporary variable again for the long literal. The way this
function call is wrapped isn't any better than the original code here.

https://codereview.adblockplus.org/29374674/diff/29374779/lib/filterComposer.js
File lib/filterComposer.js (left):

https://codereview.adblockplus.org/29374674/diff/29374779/lib/filterComposer....
lib/filterComposer.js:228: id:      message.id,
Could we perhaps break this down to two lines, if we use desctructuring +
shorthand syntax? Otherwise, I think it is fine as it is.

https://codereview.adblockplus.org/29374674/diff/29374779/lib/filterValidatio...
File lib/filterValidation.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/lib/filterValidatio...
lib/filterValidation.js:144: return {error: new
FilterParsingError("invalid-filter",
It seems we could turn four lines into three and avoid ugly wrapping, if you
destructure the filter above.

https://codereview.adblockplus.org/29374674/diff/29374779/lib/icon.js
File lib/icon.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/lib/icon.js#newcode206
lib/icon.js:206: * @param {string} type The notification type (i.e:
"information" or
The redundant space between the type and description made it easier to parse
(for humans) if the description isn't capitalized. I wouldn't insist on keeping
up that practice, but please make sure that all argument descriptions across our
code will be capitalized then, to don't regress the readability.

https://codereview.adblockplus.org/29374674/diff/29374779/lib/io.js
File lib/io.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/lib/io.js#newcode18
lib/io.js:18: /* global FakeFile */
Is FakeFile used anywhere else? Can't we just turn lib/compat.js into a module?
Same for "Services" which I've seen to be used as global somewhere else.

As far as adblockpluscore is concerned, with the current Adblock Plus for
Firefox on the way out, having this compatibility layer seems wrong to have in
the first place. But for the time being I wouldn't mind to keep it, but at least
we should turn it into a module, to avoid globals in adblockpluschrome, and
assign to window where necessary for compatibility with adblockpluscore.

https://codereview.adblockplus.org/29374674/diff/29374779/notification.js
File notification.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/notification.js#new...
notification.js:102: // Falls through.
Interesting, this isn't a regular eslint comment, and without one it should
complain here. Either way, how about turning the switch/case into an if instead?

https://codereview.adblockplus.org/29374674/diff/29374779/options.js
File options.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/options.js#newcode36
options.js:36: function wrapper(baseMessage, ...paramKeys)
How about calling the rest arguments just "args" as you did elsewhere?

[kzar, 2017-02-20 10:27:34]

Patch Set 4 : Addressed some of Sebastian's feedback, made other improvements

https://codereview.adblockplus.org/29374674/diff/29374779/.eslintignore
File .eslintignore (right):

https://codereview.adblockplus.org/29374674/diff/29374779/.eslintignore#newcode1
.eslintignore:1: lib/punycode.js
On 2017/02/09 01:04:48, Sebastian Noack wrote:
> I really don't like how the ESLint configuration is spread out over multiple
> configuration files (not to mention the inline configurations). Wouldn't it be
> possible to files to be ignored in .eslintrc.json, at least?

I don't think there's another way, other than to specify the files to ignore as
command line arguments.

http://eslint.org/docs/user-guide/configuring#ignoring-files-and-directories
https://github.com/eslint/eslint/issues/3529

> Also let me know if you have a better idea for 
> qunit/.eslintrc.json.

The only other way I see is to add the qunit env to adblockpluschrome's root
.eslintrc.json which IMO is worse.

https://codereview.adblockplus.org/29374674/diff/29374779/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/chrome/ext/backgrou...
chrome/ext/background.js:385: );
On 2017/02/09 01:04:49, Sebastian Noack wrote:
> The parenthesis here seem unnecessary.

Done.

https://codereview.adblockplus.org/29374674/diff/29374779/chrome/ext/backgrou...
chrome/ext/background.js:395: propagateHandlerBehaviorChange
On 2017/02/09 01:04:49, Sebastian Noack wrote:
> I wonder whether we can perhaps shorten the variable name, rather than
awkwardly
> wrapping those lines. The variable name can be up to 19 characters in order to
> avoid exceeding 80 characters total.

I don't mind the wrapping too much here and I can't think of a better name. But
I don't mind changing the name if you can think of something shorter either.

https://codereview.adblockplus.org/29374674/diff/29374779/chrome/ext/backgrou...
chrome/ext/background.js:513: return results.indexOf(true) != -1;
On 2017/02/09 01:04:48, Sebastian Noack wrote:
> This would be another way to wrap the code here, without a temporary variable:
> 
>   return ext.onMessage._dispatch(message,
>                                  sender,
>                                  sendResponse).indexOf(true) != -1;
> 
> No strong opinion though. What do you think?

I've wrapped it in a similar way which I preferred, that OK?

https://codereview.adblockplus.org/29374674/diff/29374779/chrome/ext/common.js
File chrome/ext/common.js (left):

https://codereview.adblockplus.org/29374674/diff/29374779/chrome/ext/common.j...
chrome/ext/common.js:20: {
On 2017/02/09 01:04:49, Sebastian Noack wrote:
> I just noticed separating ext/common.js and chrome/ext/common.js does no
longer
> make sense since Safari is gone. So while changing the whole code here (due to
> indentation) anyway, how about merging those files instead. That shouldn't
make
> the patch much larger, but would simplify the code structure a bit, while
> getting rid of the redundant block.

Done.

https://codereview.adblockplus.org/29374674/diff/29374779/chrome/ext/popup.js
File chrome/ext/popup.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/chrome/ext/popup.js...
chrome/ext/popup.js:7: var ext = Object.create(backgroundPage.ext);
On 2017/02/09 01:04:49, Sebastian Noack wrote:
> On 2017/02/08 09:04:20, kzar wrote:
> > Unfortunately using `let` for the ext declarations caused other scoping
issues
> I
> > couldn't solve, for example the popup page broke. I've reverted those
changes
> > and therefore this is no longer blocked by the messageResponder changes.
> 
> The popup code (also) broke because you used Object.create(backgroundPage)
> instead of Object.create(backgroundPage.ext). I cannot see any other obvious
> problem. If there is, please figure out and explain what is happening there.
It
> really should be possible to avoid var here.

Ah well spotted, Done.

https://codereview.adblockplus.org/29374674/diff/29374779/composer.js
File composer.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/composer.js#newcode18
composer.js:18: /* globals $ */
On 2017/02/09 01:04:50, Sebastian Noack wrote:
> I guess we should rather enable the "jquery" environment, than whitelisting
"$"
> explicitly, if it is just to avoid confusion with Chrome's built-in "$".
> 
> Also I'm not too happy with the inline configurations spread across our code.
I
> wonder whether it would be worth to add the "jQuery" environment to the global
> configuration. On the other hand, it's not available in all files. But then
> again, this is also true for some of the other environments configured
globally,
> and this isn't the only file using jQuery either.

IMO explicitly adding global comments at the top of relevant files like this
isn't too big a deal, and as Wladimir pointed out they are actually useful as
they act as documentation of the global variables in the code for humans too.

That said I don't have a strong opinion about if `/* globals $ */` or `/*
eslint-env jquery */` looks nicer here, so up to you I guess.

I think adding the jQuery environment globally to the configuration would be a
footgun since like you say those variables aren't available throughout. So the
fact the configuration implied they were would be misleading and also we would
lose some of the benefit of using the no-undef rule in the process.

https://codereview.adblockplus.org/29374674/diff/29374779/composer.postload.js
File composer.postload.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/composer.postload.j...
composer.postload.js:18: /* globals checkCollapse, elemhide, getURLsFromElement,
typeMap  */
On 2017/02/09 01:04:50, Sebastian Noack wrote:
> As mentioned in the other review for the base ESLint configuration, I'm
opposed
> to add this boilerplate. But it points out an issue: global variables aren't
> great. So instead making the situation worse by explicitly/redundantly
> specifying these globals, we should get rid of them. Note that if we'd use
> modules for the related logic reused here, the only global we'd need is
> require() which is already known.

We're not making the situation worse by documenting the global variables, we're
just making it explicit which IMO is actually an improvement.

Either way I agree that the less global state the better, so I'd also like to
reduce that. As for using modules here, how can we do that since this is a
content script and our "module framework stuff" lives in lib/compat.js which is
only included in the background. Do you mean I should add lib/compat.js to the
content scripts too?

https://codereview.adblockplus.org/29374674/diff/29374779/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/include.preload.js#...
include.preload.js:365: randomEventName + "-" + event.detail.url, {detail:
block}
On 2017/02/09 01:04:50, Sebastian Noack wrote:
> If you rename the argument to "detail", you can use the shorthand syntax here.


I think block is a better name for the variable though, since it's a boolean
indicating if the request should be blocked or not.

https://codereview.adblockplus.org/29374674/diff/29374779/include.preload.js#...
include.preload.js:510: document.documentElement).appendChild(this.style);
On 2017/02/09 01:04:50, Sebastian Noack wrote:
> Is the fallback to document.documentElement even still necessary?

I have no idea.

https://codereview.adblockplus.org/29374674/diff/29374779/lib/devtools.js
File lib/devtools.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/lib/devtools.js#new...
lib/devtools.js:141: addRecord(panel, {url, type, docDomain, thirdParty,
sitekey, specificOnly},
On 2017/02/09 01:04:50, Sebastian Noack wrote:
> Perhaps use a temporary variable again for the long literal. The way this
> function call is wrapped isn't any better than the original code here.

I think it's slightly nicer like this and would prefer to leave it.

https://codereview.adblockplus.org/29374674/diff/29374779/lib/filterComposer.js
File lib/filterComposer.js (left):

https://codereview.adblockplus.org/29374674/diff/29374779/lib/filterComposer....
lib/filterComposer.js:228: id:      message.id,
On 2017/02/09 01:04:51, Sebastian Noack wrote:
> Could we perhaps break this down to two lines, if we use desctructuring +
> shorthand syntax? Otherwise, I think it is fine as it is.

I know what you mean but I wonder if assigning a bunch of variables just to save
a few lines is worth it.

https://codereview.adblockplus.org/29374674/diff/29374779/lib/filterValidatio...
File lib/filterValidation.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/lib/filterValidatio...
lib/filterValidation.js:144: return {error: new
FilterParsingError("invalid-filter",
On 2017/02/09 01:04:51, Sebastian Noack wrote:
> It seems we could turn four lines into three and avoid ugly wrapping, if you
> destructure the filter above.

I just tried it but unfortunately the invalid-css-selector line is still one
character too long for ESLint. We could give the selector variable an
abbreviated name but I'm not sure if that's better than what we have now.

https://codereview.adblockplus.org/29374674/diff/29374779/lib/icon.js
File lib/icon.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/lib/icon.js#newcode206
lib/icon.js:206: * @param {string} type The notification type (i.e:
"information" or
On 2017/02/09 01:04:51, Sebastian Noack wrote:
> The redundant space between the type and description made it easier to parse
> (for humans) if the description isn't capitalized. I wouldn't insist on
keeping
> up that practice, but please make sure that all argument descriptions across
our
> code will be capitalized then, to don't regress the readability.

Done.

https://codereview.adblockplus.org/29374674/diff/29374779/lib/io.js
File lib/io.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/lib/io.js#newcode18
lib/io.js:18: /* global FakeFile */
On 2017/02/09 01:04:51, Sebastian Noack wrote:
> Is FakeFile used anywhere else?

Not that I can see, no.

> Can't we just turn lib/compat.js into a module?
> Same for "Services" which I've seen to be used as global somewhere else.

Well how would that work since lib/compat.js is where we define `require`. Do
you suggest we split the module framework stuff away from the rest of
lib/compat.js and then turn what's left into a module?

> ...and
> assign to window where necessary for compatibility with adblockpluscore.

IMO it's better to use a global variable and explicitly document it's use with a
ESLint comment than it is to shove the same variable inside the global `window`
Object in order to pretend it doesn't exist. At least then the global state is
documented. (FWIW I agree to reduce global state is almost always a good thing.)

https://codereview.adblockplus.org/29374674/diff/29374779/notification.js
File notification.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/notification.js#new...
notification.js:102: // Falls through.
On 2017/02/09 01:04:51, Sebastian Noack wrote:
> Interesting, this isn't a regular eslint comment, and without one 
> it should complain here.

Yea, I thought this was kind of interesting too. I do understand the logic
though, I think case clauses which fall through can be a little confusing but
are fine so long as that they're documented.

> Either way, how about turning the switch/case into an if instead?

I'm not sure why we should change it? IMO using a switch here is OK.

https://codereview.adblockplus.org/29374674/diff/29374779/options.js
File options.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/options.js#newcode36
options.js:36: function wrapper(baseMessage, ...paramKeys)
On 2017/02/09 01:04:51, Sebastian Noack wrote:
> How about calling the rest arguments just "args" as you did elsewhere?

Since paramKeys is a more useful name. (I used `args` elsewhere when I couldn't
think of a better name, but I wasn't too happy about doing so.)

The function that wrapper returns accepts parameters based upon the names given
by paramKeys. The paramKeys and passed in paramValues are then combined into the
message Object which is sent. I think renaming paramKeys to args would make that
less clear.

[kzar, 2017-02-21 05:10:22]

Patch Set 5 : Stop using commonjs

[kzar, 2017-02-28 15:54:59]

Patch Set 6 : Rebased.

Adding Winsley to Cc since this latest Patch Set merges in his recent property
filter tracing changes. Winsley if you have a minute could you check this Patch
Set doesn't trash your work there?

[wspee, 2017-03-02 10:27:14]

This doesn't break my latest changes.

I also looked through the other changes and made sure that no comment got
overlooked.

The only unanswered comment I found is the one below, although I believe it has
already been answered through other comments but you might want to respond to
this as well.

So LGTM

https://codereview.adblockplus.org/29374674/diff/29374779/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/include.preload.js#...
include.preload.js:22: // This variable is also used by our other content
scripts.
On 2017/02/09 01:04:50, Sebastian Noack wrote:
> This comment is kinda redundant with the /* globals ... */ comment in
> composer.postload.js. But as I said there, these global variables aren't great
> in particular in regard to the additional boilerplate. This should rather be a
> module export.

You might want to also comment on this? But as far as I got it from the other
similar comments this is going to stay this way?

[kzar, 2017-03-07 11:43:18]

Patch Set 7 : Rebased.
Patch Set 8 : Add arrow-parens rule to match existing convention

Felix do you mind having a quick look over this too in Sebastian's absence? I'm
hoping to push these changes after the release next Tuesday.

https://codereview.adblockplus.org/29374674/diff/29374779/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/include.preload.js#...
include.preload.js:22: // This variable is also used by our other content
scripts.
On 2017/03/02 10:27:13, wspee wrote:
> On 2017/02/09 01:04:50, Sebastian Noack wrote:
> > This comment is kinda redundant with the /* globals ... */ comment in
> > composer.postload.js. But as I said there, these global variables aren't
great
> > in particular in regard to the additional boilerplate. This should rather be
a
> > module export.
> 
> You might want to also comment on this? But as far as I got it from the other
> similar comments this is going to stay this way?

Yes, I think for now this stuff will stay as it is. We need to get the ESLint
changes to these repositories pushed soon, since it's blocking other people from
collaborating or at least making it much harder.

I do agree with Sebastian that we should reduce the use of globals via modules,
and I'm going to keep #4864 open until we've done that. When Sebastian's back
I'll work with him on that.

[kzar, 2017-03-08 12:27:42]

Patch Set 9 : Update Cu.import use to correspond with adblockpluscore changes,
removed unused utils.js file

[kzar, 2017-03-09 10:22:17]

Patch Set 10 : Remove the arrow-parens rule

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

Any chance you could also have a look through this review Wladimir?

[kzar, 2017-03-14 08:10:22]

Patch Set 11 : Restored IIFEs and chrome/ext/common.js

As discussed in the other reviews using a block instead of IIFE is a footgun.
Even if it doesn't lead to a syntax error straight away it's easy to
accidentally cause one when adding a function declaration afterwards.

Also I realised that moving chrome/ext/common.js into ext/common.js here was a
mistake, that's a totally unrelated change and we should make it separately.

https://codereview.adblockplus.org/29374674/diff/29378983/notification.js
File notification.js (right):

https://codereview.adblockplus.org/29374674/diff/29378983/notification.js#new...
notification.js:20: const {require} = ext.backgroundPage.getWindow();
Due to rebase, sorry forgot to upload separately.

[kzar, 2017-03-14 10:33:41]

Patch Set 12 : Fixed typo with shadowRoot getter

[Wladimir Palant, 2017-03-14 13:03:34]

https://codereview.adblockplus.org/29374674/diff/29383722/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29374674/diff/29383722/.eslintrc.json#newc...
.eslintrc.json:6: "webextensions": true
Isn't "browser" already implied by "webextensions"? Did you test that?

https://codereview.adblockplus.org/29374674/diff/29383722/chrome/ext/popup.js
File chrome/ext/popup.js (right):

https://codereview.adblockplus.org/29374674/diff/29383722/chrome/ext/popup.js...
chrome/ext/popup.js:6: ext = Object.create(backgroundPage.ext);
As far as the browser is concerned, ext is an undeclared variable here. So IMHO
this should throw because of "use strict". Assign to window.ext instead?

https://codereview.adblockplus.org/29374674/diff/29383722/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29374674/diff/29383722/ext/common.js#newco...
ext/common.js:22: /* eslint-enable no-var */
Use `window.ext = {}` inside the IIFE instead of disabling no-var?

https://codereview.adblockplus.org/29374674/diff/29383722/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29374674/diff/29383722/include.preload.js#...
include.preload.js:398: websocket = new RealWebSocket(url, args[0]);
From the look of it, this should really be:

  let websocket = new RealWebSocket(url, ...args);

The if block no longer serves a purpose.

https://codereview.adblockplus.org/29374674/diff/29383722/include.preload.js#...
include.preload.js:502: document.documentElement).appendChild(this.style);
Please don't use one-space indentation, it's too easy to miss. Maybe something
like that:

  (
    this.shadow || document.head || document.documentElement
  ).appendChild(this.style);

https://codereview.adblockplus.org/29374674/diff/29383722/lib/compat.js
File lib/compat.js (right):

https://codereview.adblockplus.org/29374674/diff/29383722/lib/compat.js#newco...
lib/compat.js:84: return {[resourceName]:
Cu.import.resources.get(resourceName)};
As with the implementation in adblockplusui, this should throw for unknown
modules instead of returning undefined.

https://codereview.adblockplus.org/29374674/diff/29383722/lib/devtools.js
File lib/devtools.js (right):

https://codereview.adblockplus.org/29374674/diff/29383722/lib/devtools.js#new...
lib/devtools.js:89: : record.request.type == request.type) &&
Please don't use one-space indentations. Besides, operator is supposed to be
before the line break. So rather something like:

  (
    record.request.type == "DOCUMENT" ?
    nonRequestTypes.indexOf(request.type) != -1 :
    record.request.type == request.type
  ) &&
  ...

https://codereview.adblockplus.org/29374674/diff/29383722/lib/filterValidatio...
File lib/filterValidation.js (right):

https://codereview.adblockplus.org/29374674/diff/29383722/lib/filterValidatio...
lib/filterValidation.js:118: * See {@link
module:filterValidation~FilterParsingError FilterParsingError}
Please indent this line.

https://codereview.adblockplus.org/29374674/diff/29383722/lib/filterValidatio...
lib/filterValidation.js:159: * See {@link
module:filterValidation~FilterParsingError FilterParsingError}
Please indent this line.

https://codereview.adblockplus.org/29374674/diff/29383722/lib/notificationHel...
File lib/notificationHelper.js (right):

https://codereview.adblockplus.org/29374674/diff/29383722/lib/notificationHel...
lib/notificationHelper.js:212: let notif = new Notification(
I understand that the point here is to avoid masking the notification parameter.
However, this change doesn't really help making this code more obvious. I.e.,
what's the difference between notification and notif variables? How about
indicating this one as a UI element by naming it "widget"?

https://codereview.adblockplus.org/29374674/diff/29383722/lib/utils.js
File lib/utils.js (right):

https://codereview.adblockplus.org/29374674/diff/29383722/lib/utils.js#newcod...
lib/utils.js:111: Utils.appLocale);
I'd rather put the entire .replace on the next line and align the dots.

https://codereview.adblockplus.org/29374674/diff/29383722/options.js
File options.js (right):

https://codereview.adblockplus.org/29374674/diff/29383722/options.js#newcode38
options.js:38: return function(...paramValues /* , callback */)
I wonder why this isn't using promises. Not something for this review of course.

https://codereview.adblockplus.org/29374674/diff/29383722/options.js#newcode372
options.js:372: .value.trim();
Nit: align dots maybe?

https://codereview.adblockplus.org/29374674/diff/29383722/qunit/common.js
File qunit/common.js (right):

https://codereview.adblockplus.org/29374674/diff/29383722/qunit/common.js#new...
qunit/common.js:30: importAll("utils", this);
importAll is a serious hack and I am guilty of introducing it. With almost all
unit tests doing the right thing, how about replacing it by proper requires here
instead of asking ESLint for forgiveness via comment? AFAICT, only prefs.js test
will have to be adjusted to call require("prefs") explicitly.

[kzar, 2017-03-15 04:57:56]

Patch Set 13 : Addressed Wladimir's feedback

https://codereview.adblockplus.org/29374674/diff/29383722/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29374674/diff/29383722/.eslintrc.json#newc...
.eslintrc.json:6: "webextensions": true
On 2017/03/14 13:03:22, Wladimir Palant wrote:
> Isn't "browser" already implied by "webextensions"? Did you test that?

Apparently not, when I remove "browser" I get a bunch of no-undef errors for
things like `window`, `document` and `console`.

https://codereview.adblockplus.org/29374674/diff/29383722/chrome/ext/popup.js
File chrome/ext/popup.js (right):

https://codereview.adblockplus.org/29374674/diff/29383722/chrome/ext/popup.js...
chrome/ext/popup.js:6: ext = Object.create(backgroundPage.ext);
On 2017/03/14 13:03:22, Wladimir Palant wrote:
> As far as the browser is concerned, ext is an undeclared variable here. So
IMHO
> this should throw because of "use strict". Assign to window.ext instead?

Done.

https://codereview.adblockplus.org/29374674/diff/29383722/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29374674/diff/29383722/ext/common.js#newco...
ext/common.js:22: /* eslint-enable no-var */
On 2017/03/14 13:03:23, Wladimir Palant wrote:
> Use `window.ext = {}` inside the IIFE instead of disabling no-var?

Done.

https://codereview.adblockplus.org/29374674/diff/29383722/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29374674/diff/29383722/include.preload.js#...
include.preload.js:398: websocket = new RealWebSocket(url, args[0]);
On 2017/03/14 13:03:24, Wladimir Palant wrote:
> From the look of it, this should really be:
> 
>   let websocket = new RealWebSocket(url, ...args);
> 
> The if block no longer serves a purpose.

Done.

https://codereview.adblockplus.org/29374674/diff/29383722/include.preload.js#...
include.preload.js:502: document.documentElement).appendChild(this.style);
On 2017/03/14 13:03:24, Wladimir Palant wrote:
> Please don't use one-space indentation, it's too easy to miss. Maybe something
> like that:
> 
>   (
>     this.shadow || document.head || document.documentElement
>   ).appendChild(this.style);

How about this?

https://codereview.adblockplus.org/29374674/diff/29383722/lib/compat.js
File lib/compat.js (right):

https://codereview.adblockplus.org/29374674/diff/29383722/lib/compat.js#newco...
lib/compat.js:84: return {[resourceName]:
Cu.import.resources.get(resourceName)};
On 2017/03/14 13:03:25, Wladimir Palant wrote:
> As with the implementation in adblockplusui, this should throw for unknown
> modules instead of returning undefined.

Done.

https://codereview.adblockplus.org/29374674/diff/29383722/lib/devtools.js
File lib/devtools.js (right):

https://codereview.adblockplus.org/29374674/diff/29383722/lib/devtools.js#new...
lib/devtools.js:89: : record.request.type == request.type) &&
On 2017/03/14 13:03:27, Wladimir Palant wrote:
> Please don't use one-space indentations. Besides, operator is supposed to be
> before the line break. So rather something like:
> 
>   (
>     record.request.type == "DOCUMENT" ?
>     nonRequestTypes.indexOf(request.type) != -1 :
>     record.request.type == request.type
>   ) &&
>   ...

It turns out we're using the default configuration for the operator-linebreak
rule in our configuration, which makes an exception for the `?` and `:`
operators:

  "after", { "overrides": { "?": "before", ":": "before" } }

That seems like a mistake to me so I've opened a review to fix that
https://codereview.adblockplus.org/29384022/

https://codereview.adblockplus.org/29374674/diff/29383722/lib/filterValidatio...
File lib/filterValidation.js (right):

https://codereview.adblockplus.org/29374674/diff/29383722/lib/filterValidatio...
lib/filterValidation.js:118: * See {@link
module:filterValidation~FilterParsingError FilterParsingError}
On 2017/03/14 13:03:28, Wladimir Palant wrote:
> Please indent this line.

Done.

https://codereview.adblockplus.org/29374674/diff/29383722/lib/filterValidatio...
lib/filterValidation.js:159: * See {@link
module:filterValidation~FilterParsingError FilterParsingError}
On 2017/03/14 13:03:27, Wladimir Palant wrote:
> Please indent this line.

Done.

https://codereview.adblockplus.org/29374674/diff/29383722/lib/notificationHel...
File lib/notificationHelper.js (right):

https://codereview.adblockplus.org/29374674/diff/29383722/lib/notificationHel...
lib/notificationHelper.js:212: let notif = new Notification(
On 2017/03/14 13:03:28, Wladimir Palant wrote:
> I understand that the point here is to avoid masking the notification
parameter.
> However, this change doesn't really help making this code more obvious. I.e.,
> what's the difference between notification and notif variables? How about
> indicating this one as a UI element by naming it "widget"?

Done.

https://codereview.adblockplus.org/29374674/diff/29383722/lib/utils.js
File lib/utils.js (right):

https://codereview.adblockplus.org/29374674/diff/29383722/lib/utils.js#newcod...
lib/utils.js:111: Utils.appLocale);
On 2017/03/14 13:03:29, Wladimir Palant wrote:
> I'd rather put the entire .replace on the next line and align the dots.

Done.

https://codereview.adblockplus.org/29374674/diff/29383722/options.js
File options.js (right):

https://codereview.adblockplus.org/29374674/diff/29383722/options.js#newcode38
options.js:38: return function(...paramValues /* , callback */)
On 2017/03/14 13:03:30, Wladimir Palant wrote:
> I wonder why this isn't using promises. Not something for this review of
course.

Acknowledged.

https://codereview.adblockplus.org/29374674/diff/29383722/options.js#newcode372
options.js:372: .value.trim();
On 2017/03/14 13:03:30, Wladimir Palant wrote:
> Nit: align dots maybe?

Done.

https://codereview.adblockplus.org/29374674/diff/29383722/qunit/common.js
File qunit/common.js (right):

https://codereview.adblockplus.org/29374674/diff/29383722/qunit/common.js#new...
qunit/common.js:30: importAll("utils", this);
On 2017/03/14 13:03:32, Wladimir Palant wrote:
> importAll is a serious hack and I am guilty of introducing it. With almost all
> unit tests doing the right thing, how about replacing it by proper requires
here
> instead of asking ESLint for forgiveness via comment? AFAICT, only prefs.js
test
> will have to be adjusted to call require("prefs") explicitly.

Good point, Done.

[kzar, 2017-03-15 11:45:42]

Patch Set 14 : Update adblockpluscore dependency

[kzar, 2017-03-16 08:35:48]

Patch Set 15 : Update adblockplusui dependency

[kzar, 2017-03-17 12:46:40]

Patch Set 16 : Rebased

[kzar, 2017-03-19 06:52:27]

Patch Set 17 : Update adblockplusui dependency to include the notification fix

[kzar, 2017-03-22 07:22:17]

Patch Set 18 : Rebased, restored adblockplusui dependency revision

[Sebastian Noack, 2017-03-23 06:38:40]

https://codereview.adblockplus.org/29374674/diff/29391670/dependencies
File dependencies (right):

https://codereview.adblockplus.org/29374674/diff/29391670/dependencies#newcode6
dependencies:6: adblockplusui = adblockplusui hg:a78bfd4024b7 git:0feaaf7
It seems you did a mistake while rebasing. The last commit in the adblockplusui
repository is 8ceaabb9c639, which is already referenced here.

[kzar, 2017-03-23 06:51:04]

Patch Set 19 : Rebased

https://codereview.adblockplus.org/29374674/diff/29391670/dependencies
File dependencies (right):

https://codereview.adblockplus.org/29374674/diff/29391670/dependencies#newcode6
dependencies:6: adblockplusui = adblockplusui hg:a78bfd4024b7 git:0feaaf7
On 2017/03/23 06:38:37, Sebastian Noack wrote:
> It seems you did a mistake while rebasing. The last commit in the
adblockplusui
> repository is 8ceaabb9c639, which is already referenced here.

Not a mistake, I rebased before Jon's commit landed to update the adblockplusui
dependency. Rebased again to include that now.

(Also not a mistake that I did not update the dependency to be the latest
revision, I did that on purpose. Since Wladimir pointed out with the
anti-adblock notification problem we should be careful when updating
dependencies for unrelated issues, like I did in #4136.)

[kzar, 2017-03-23 09:36:15]

Patch Set 20 : Remove some unnecessary changes

[kzar, 2017-03-23 10:32:53]

Patch Set 21 : Remove extra space

[Sebastian Noack, 2017-03-29 17:14:21]

https://codereview.adblockplus.org/29374674/diff/29374779/composer.postload.js
File composer.postload.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/composer.postload.j...
composer.postload.js:18: /* globals checkCollapse, elemhide, getURLsFromElement,
typeMap  */
On 2017/02/20 10:27:30, kzar wrote:
> We're not making the situation worse by documenting the global variables,
we're
> just making it explicit which IMO is actually an improvement.

JavaScript is a dynamic language, which comes with some advantages including
that you can just use stuff that exists on the global (or any) object at
runtime. When we now require to formally declare the intend to use any global
name, this restricts the dynamic of the language, and I get the impression you
would prefer much to write your code in Java instead. ;)

As I said before, the job of a linter is to make the code easier to maintain,
not to bloat it with additional boilerplate. So IMO, it actually makes things
worse. I would be much less concerned if metadata like this could be separated
from the code, which however, isn't possible with ESLint. But if (and only if)
we could get rid of our globals, none of this would matter, anymore.

> Either way I agree that the less global state the better, so I'd also like to
> reduce that. As for using modules here, how can we do that since this is a
> content script and our "module framework stuff" lives in lib/compat.js which
is
> only included in the background. Do you mean I should add lib/compat.js to the
> content scripts too?

We could just separate these 11 lines from lib/compat.js and have buildtools add
them when bundling the modules, so that the module bundle works without any
addional support code. Then we can have one bundle for the background page, and
one for the content side.

https://codereview.adblockplus.org/29374674/diff/29392699/README.md
File README.md (right):

https://codereview.adblockplus.org/29374674/diff/29392699/README.md#newcode59
README.md:59: eslint *.js lib/ qunit/ ext/ chrome/
Do you have to specify all files/subdirectories? Can't you just run `eslint .`?

https://codereview.adblockplus.org/29374674/diff/29392699/chrome/ext/common.js
File chrome/ext/common.js (right):

https://codereview.adblockplus.org/29374674/diff/29392699/chrome/ext/common.j...
chrome/ext/common.js:20: (function()
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?

https://codereview.adblockplus.org/29374674/diff/29392699/composer.postload.js
File composer.postload.js (right):

https://codereview.adblockplus.org/29374674/diff/29392699/composer.postload.j...
composer.postload.js:511: {
Apparently we turned the "curly" rule off in the end, so changes like this are
unrelated and unnecessary now.

https://codereview.adblockplus.org/29374674/diff/29392699/dependencies
File dependencies (right):

https://codereview.adblockplus.org/29374674/diff/29392699/dependencies#newcode5
dependencies:5: adblockpluscore = adblockpluscore hg:68a4dc4e79e8 git:2574147
Why exactly do we have to update the adblockpluscore dependency for these
changes again? If it is just so we use a version of adblockpluscore that follows
the same coding style, this would arguably be unrelated. But if this dependency
update is actually necessary, so that everything works with these changes, the
issue description should probably mention it (along the changes pulled in).
While speaking about the issue description, it seems a bit outdated.

https://codereview.adblockplus.org/29374674/diff/29392699/lib/csp.js
File lib/csp.js (right):

https://codereview.adblockplus.org/29374674/diff/29392699/lib/csp.js#newcode50
lib/csp.js:50: "frame-src http:; object-src http:"
I would align the strings here, so that they start on the same column, for
better readability:

  value: "connect-src http:; child-src http:; " +
         "frame-src http:; object-src http:"

https://codereview.adblockplus.org/29374674/diff/29392699/lib/devtools.js
File lib/devtools.js (right):

https://codereview.adblockplus.org/29374674/diff/29392699/lib/devtools.js#new...
lib/devtools.js:146: addRecord(panel, {url, type, docDomain, thirdParty,
sitekey, specificOnly},
I'd rather keep the "request" variable, in order to avoid wrapping the function
call, which was the reason I added it in the first place.

[kzar, 2017-03-30 07:20:54]

Patch Set 22 : Rebased to include Jon's webRequest WebSocket blocking changes
Patch Set 23 : Address Sebastian's feedback, don't update adblockpluscore

Copying you in Jon since I've rebased to include your recent webRequest changes
for WebSocket blocking.

https://codereview.adblockplus.org/29374674/diff/29374779/composer.postload.js
File composer.postload.js (right):

https://codereview.adblockplus.org/29374674/diff/29374779/composer.postload.j...
composer.postload.js:18: /* globals checkCollapse, elemhide, getURLsFromElement,
typeMap  */
On 2017/03/29 17:14:07, Sebastian Noack wrote:
> We could just separate these 11 lines from lib/compat.js and have buildtools
add
> them when bundling the modules, so that the module bundle works without any
> addional support code. Then we can have one bundle for the background page,
and
> one for the content side.

As discussed in IRC sounds good. I've opened
https://issues.adblockplus.org/ticket/5060 for the buildtools changes, and will
make the changes to adblockpluschrome as a separate patch set but still for this
issue #4864.

https://codereview.adblockplus.org/29374674/diff/29392699/chrome/ext/common.js
File chrome/ext/common.js (right):

https://codereview.adblockplus.org/29374674/diff/29392699/chrome/ext/common.j...
chrome/ext/common.js:20: (function()
On 2017/03/29 17:14:11, Sebastian Noack wrote:
> 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?

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.

> 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?

I would really rather not. Do you insist there?

https://codereview.adblockplus.org/29374674/diff/29392699/composer.postload.js
File composer.postload.js (right):

https://codereview.adblockplus.org/29374674/diff/29392699/composer.postload.j...
composer.postload.js:511: {
On 2017/03/29 17:14:12, Sebastian Noack wrote:
> Apparently we turned the "curly" rule off in the end, so changes like this are
> unrelated and unnecessary now.

I've removed most of those unrelated curly changes, but I left the ones that are
consistent with our coding style guide. For example the braces are required here
since the block spans over multiple lines.

While technically not related to ESLint now it seems pretty pointless to change
them back to violate our style guide.

https://codereview.adblockplus.org/29374674/diff/29392699/dependencies
File dependencies (right):

https://codereview.adblockplus.org/29374674/diff/29392699/dependencies#newcode5
dependencies:5: adblockpluscore = adblockpluscore hg:68a4dc4e79e8 git:2574147
On 2017/03/29 17:14:14, Sebastian Noack wrote:
> Why exactly do we have to update the adblockpluscore dependency 
> for these changes again?

Well I guess since I was testing the changes all together to make sure they
really worked. Also some changes to the Cu.import stub were required for the
adblockpluscore dependency update. But I guess you're right that strictly the
dependency update could be done separately. I've opened
https://issues.adblockplus.org/ticket/5061 for that.

https://codereview.adblockplus.org/29374674/diff/29392699/lib/csp.js
File lib/csp.js (right):

https://codereview.adblockplus.org/29374674/diff/29392699/lib/csp.js#newcode50
lib/csp.js:50: "frame-src http:; object-src http:"
On 2017/03/29 17:14:15, Sebastian Noack wrote:
> I would align the strings here, so that they start on the same column, for
> better readability:
> 
>   value: "connect-src http:; child-src http:; " +
>          "frame-src http:; object-src http:"

Done.

https://codereview.adblockplus.org/29374674/diff/29392699/lib/devtools.js
File lib/devtools.js (right):

https://codereview.adblockplus.org/29374674/diff/29392699/lib/devtools.js#new...
lib/devtools.js:146: addRecord(panel, {url, type, docDomain, thirdParty,
sitekey, specificOnly},
On 2017/03/29 17:14:16, Sebastian Noack wrote:
> I'd rather keep the "request" variable, in order to avoid wrapping the
function
> call, which was the reason I added it in the first place.

Done.

[Sebastian Noack, 2017-03-30 18:58:04]

Looks mostly good to me, except for globals situation, which as discussed above,
I expect to be addressed separately right after this lands.

However, I didn't make it through all files, in the previous review round. So I
missed two more things, see below.

https://codereview.adblockplus.org/29374674/diff/29392699/chrome/ext/common.js
File chrome/ext/common.js (right):

https://codereview.adblockplus.org/29374674/diff/29392699/chrome/ext/common.j...
chrome/ext/common.js:20: (function()
On 2017/03/30 07:20:41, kzar wrote:
> On 2017/03/29 17:14:11, Sebastian Noack wrote:
> > 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?
> 
> 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.
> 
> > 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?
> 
> I would really rather not. Do you insist there?

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. :(

https://codereview.adblockplus.org/29374674/diff/29398605/lib/devtools.js
File lib/devtools.js (right):

https://codereview.adblockplus.org/29374674/diff/29398605/lib/devtools.js#new...
lib/devtools.js:88: record.request.type == "DOCUMENT" ?
I found this not quite as readable as in the previous form. But you could argue
that these were way to many chained conditions in the firs place. And now where
we have for..of, Array.some() which caused this mess doesn't provide much of a
benefit anymore. Perhaps the following would be easier to read, while also
compliant with our ESLint configuration:


  for (let record of panel.records)
  {
    if (record.request.url != request.url)
      continue;
    if (record.request.docDomain != request.docDomain)
      continue;
    if (record.request.type == "DOCUMENT")
    {
      if (nonRequestTypes.indexOf(request.type) == -1)
        continue;
    }
    else
    {
      if (record.request.type != request.type)
        continue;
    }

    return true;
  }
  return false;

https://codereview.adblockplus.org/29374674/diff/29398605/notification.js
File notification.js (right):

https://codereview.adblockplus.org/29374674/diff/29398605/notification.js#new...
notification.js:100: // Falls through.
This switch has mere 3 cases. If we discourage fallthrough, it seems to make
more sense to convert it to if/else, which I assumed was the purpose of this
rule in the first place, not to add comments.

[kzar, 2017-03-31 03:42:24]

Patch Set 24 : Tidy up hasRecord and some notification logic

https://codereview.adblockplus.org/29374674/diff/29392699/chrome/ext/common.js
File chrome/ext/common.js (right):

https://codereview.adblockplus.org/29374674/diff/29392699/chrome/ext/common.j...
chrome/ext/common.js:20: (function()
On 2017/03/30 18:57:56, Sebastian Noack wrote:
> On 2017/03/30 07:20:41, kzar wrote:
> > On 2017/03/29 17:14:11, Sebastian Noack wrote:
> > > 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?
> > 
> > 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.
> > 
> > > 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?
> > 
> > I would really rather not. Do you insist there?
> 
> 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. :(

Acknowledged.

https://codereview.adblockplus.org/29374674/diff/29398605/lib/devtools.js
File lib/devtools.js (right):

https://codereview.adblockplus.org/29374674/diff/29398605/lib/devtools.js#new...
lib/devtools.js:88: record.request.type == "DOCUMENT" ?
On 2017/03/30 18:57:58, Sebastian Noack wrote:
> I found this not quite as readable as in the previous form. But you could
argue
> that these were way to many chained conditions in the firs place. And now
where
> we have for..of, Array.some() which caused this mess doesn't provide much of a
> benefit anymore. Perhaps the following would be easier to read, while also
> compliant with our ESLint configuration:
> 
> 
>   for (let record of panel.records)
>   {
>     if (record.request.url != request.url)
>       continue;
>     if (record.request.docDomain != request.docDomain)
>       continue;
>     if (record.request.type == "DOCUMENT")
>     {
>       if (nonRequestTypes.indexOf(request.type) == -1)
>         continue;
>     }
>     else
>     {
>       if (record.request.type != request.type)
>         continue;
>     }
> 
>     return true;
>   }
>   return false;

I've had a go, what do you think?

https://codereview.adblockplus.org/29374674/diff/29398605/notification.js
File notification.js (right):

https://codereview.adblockplus.org/29374674/diff/29398605/notification.js#new...
notification.js:100: // Falls through.
On 2017/03/30 18:57:59, Sebastian Noack wrote:
> This switch has mere 3 cases. If we discourage fallthrough, it seems to make
> more sense to convert it to if/else, which I assumed was the purpose of this
> rule in the first place, not to add comments.

Done.

[kzar, 2017-03-31 07:51:38]

Patch Set 25 : Remove two globals from tests

[Sebastian Noack, 2017-03-31 07:59:45]

https://codereview.adblockplus.org/29374674/diff/29398605/lib/devtools.js
File lib/devtools.js (right):

https://codereview.adblockplus.org/29374674/diff/29398605/lib/devtools.js#new...
lib/devtools.js:88: record.request.type == "DOCUMENT" ?
On 2017/03/31 03:42:18, kzar wrote:
> On 2017/03/30 18:57:58, Sebastian Noack wrote:
> > I found this not quite as readable as in the previous form. But you could
> argue
> > that these were way to many chained conditions in the firs place. And now
> where
> > we have for..of, Array.some() which caused this mess doesn't provide much of
a
> > benefit anymore. Perhaps the following would be easier to read, while also
> > compliant with our ESLint configuration:
> > 
> > 
> >   for (let record of panel.records)
> >   {
> >     if (record.request.url != request.url)
> >       continue;
> >     if (record.request.docDomain != request.docDomain)
> >       continue;
> >     if (record.request.type == "DOCUMENT")
> >     {
> >       if (nonRequestTypes.indexOf(request.type) == -1)
> >         continue;
> >     }
> >     else
> >     {
> >       if (record.request.type != request.type)
> >         continue;
> >     }
> > 
> >     return true;
> >   }
> >   return false;
> 
> I've had a go, what do you think?

Hmm, in the end it doesn't seem to be any more readable, and my plan to at least
get rid of wrapped lines didn't work out either. After all, your initial changes
don't look too bad. Perhaps just add indentation, and keep the change to
Array.includes(), sorry for that:

  (record.request.type == "DOCUMENT" ?
     nonRequestTypes.includes(request.type) :
     record.request.type == request.type) &&

https://codereview.adblockplus.org/29374674/diff/29398952/lib/devtools.js
File lib/devtools.js (right):

https://codereview.adblockplus.org/29374674/diff/29398952/lib/devtools.js#new...
lib/devtools.js:98: // so we also have to match the CSS selector in order to
distinguish them.
While thinking about a nicer way to do these checks, it took me quite a while to
wrap my head around this comment. I know, I wrote it initially. :D But I think
the intention is clearer when we change it like this:
 
  ... so we have to compare the selector in order to avoid duplicates.

https://codereview.adblockplus.org/29374674/diff/29398952/notification.js
File notification.js (right):

https://codereview.adblockplus.org/29374674/diff/29398952/notification.js#new...
notification.js:98: notificationElement.hidden = true;
If you just duplicate these lines, you could have sticked with switch/case, no
fall through needed then.

The point of using if/else here is that you can implement the same logic with
less duplication but no fall through:

  if (event.target.id == "notification-close")
    notificationElement.classList.add("closing");
  else if (event.target.id == "notification-optout" ||
           event.target.id == "notification-hide")
  {
    if (event.target.id == "notification-optout")
      Notification.toggleIgnoreCategory("*", true);

    notificationElement.hidden = true;
    notification.onClicked();
  }

[kzar, 2017-03-31 08:27:44]

Patch Set 26 : Addressed more feedback

https://codereview.adblockplus.org/29374674/diff/29398605/lib/devtools.js
File lib/devtools.js (right):

https://codereview.adblockplus.org/29374674/diff/29398605/lib/devtools.js#new...
lib/devtools.js:88: record.request.type == "DOCUMENT" ?
On 2017/03/31 07:59:35, Sebastian Noack wrote:
> On 2017/03/31 03:42:18, kzar wrote:
> > On 2017/03/30 18:57:58, Sebastian Noack wrote:
> > > I found this not quite as readable as in the previous form. But you could
> > argue
> > > that these were way to many chained conditions in the firs place. And now
> > where
> > > we have for..of, Array.some() which caused this mess doesn't provide much
of
> a
> > > benefit anymore. Perhaps the following would be easier to read, while also
> > > compliant with our ESLint configuration:
> > > 
> > > 
> > >   for (let record of panel.records)
> > >   {
> > >     if (record.request.url != request.url)
> > >       continue;
> > >     if (record.request.docDomain != request.docDomain)
> > >       continue;
> > >     if (record.request.type == "DOCUMENT")
> > >     {
> > >       if (nonRequestTypes.indexOf(request.type) == -1)
> > >         continue;
> > >     }
> > >     else
> > >     {
> > >       if (record.request.type != request.type)
> > >         continue;
> > >     }
> > > 
> > >     return true;
> > >   }
> > >   return false;
> > 
> > I've had a go, what do you think?
> 
> Hmm, in the end it doesn't seem to be any more readable, and my plan to at
least
> get rid of wrapped lines didn't work out either. After all, your initial
changes
> don't look too bad. Perhaps just add indentation, and keep the change to
> Array.includes(), sorry for that:
> 
>   (record.request.type == "DOCUMENT" ?
>      nonRequestTypes.includes(request.type) :
>      record.request.type == request.type) &&

Done.

https://codereview.adblockplus.org/29374674/diff/29398952/lib/devtools.js
File lib/devtools.js (right):

https://codereview.adblockplus.org/29374674/diff/29398952/lib/devtools.js#new...
lib/devtools.js:98: // so we also have to match the CSS selector in order to
distinguish them.
On 2017/03/31 07:59:37, Sebastian Noack wrote:
> While thinking about a nicer way to do these checks, it took me quite a while
to
> wrap my head around this comment. I know, I wrote it initially. :D But I think
> the intention is clearer when we change it like this:
>  
>   ... so we have to compare the selector in order to avoid duplicates.

Done.

[kzar, 2017-03-31 08:39:38]

Patch Set 27 : Use .includes again

[Sebastian Noack, 2017-03-31 08:40:39]

LGTM!