Issue 4878 - Start using ESLint for adblockpluscore

Issue 4878 - Start using ESLint for adblockpluscore

The unit tests still pass and adblockpluschrome still seems to work OK with these changes.

[kzar, 2017-02-20 10:31:50]

Patch Set 1

[Felix Dahlke, 2017-02-20 11:32:14]

> The (non browser) unit tests still pass

What about the browser tests? Those should pass too, they are needed.

[kzar, 2017-02-20 12:15:53]

On 2017/02/20 11:32:14, Felix Dahlke wrote:
> What about the browser tests? Those should pass too, they are needed.

It's not that I broke them, just that I couldn't get them to run. I was about to
submit an issue for that when I noticed the problem no longer happened from a
fresh clone of adblockpluscore. Turns out that deleting the node_modules
directory and running `npm install` again was enough to get the browser tests
running!

So I can now confirm that both the unit and browser tests still pass with these
changes.

For the record here's the error I was seeing:

> node test_runner.js "test/browser/"

internal/child_process.js:289
  var err = this._handle.spawn(options);
                         ^

TypeError: Bad argument
    at TypeError (native)
    at ChildProcess.spawn (internal/child_process.js:289:26)
    at exports.spawn (child_process.js:380:9)
    at Object.exports.execFile (child_process.js:143:15)
    at module.exports
(/home/kzar/Davebox/home/code/adblockpluschrome/adblockpluscore/node_modules/node-qunit-phantomjs/index.js:49:25)
    at Object.<anonymous>
(/home/kzar/Davebox/home/code/adblockpluschrome/adblockpluscore/test_runner.js:68:3)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
npm ERR! Test failed.  See above for more details.

[Sebastian Noack, 2017-02-20 13:14:52]

I didn't make it through all changes yet. But I have to run, and here is what I
found already. Also note that the subject of some comments has more occurrences
across your changes. I only pointed them out the first time. I think you can
easily find the other occurrences using grep.

https://codereview.adblockplus.org/29375915/diff/29375916/lib/downloader.js
File lib/downloader.js (right):

https://codereview.adblockplus.org/29375915/diff/29375916/lib/downloader.js#n...
lib/downloader.js:20: /* globals XMLHttpRequest */
I would rather put this in the .eslintrc.json. Generally, I'd like to avoid
inline boilerplate as much as possible. And if XMLHttpRequest is available here,
it is also available in most (for Chrome and WebExtensions even all) scripts.

https://codereview.adblockplus.org/29375915/diff/29375916/lib/downloader.js#n...
lib/downloader.js:76: * @type {Integer}
There is no type or class called "Integer". This should rather be "number".

https://codereview.adblockplus.org/29375915/diff/29375916/lib/elemHide.js
File lib/elemHide.js (right):

https://codereview.adblockplus.org/29375915/diff/29375916/lib/elemHide.js#new...
lib/elemHide.js:270: let {selector} = filter;
Perhaps just inline filter.selector, in the two occurrences below. This
temporary variable seems kinda unnecessary.

https://codereview.adblockplus.org/29375915/diff/29375916/lib/elemHide.js#new...
lib/elemHide.js:363: /* eslint-disable no-constant-condition */
Ugh. Can we please change esling-config-eyeo, configuring no-constant, setting
the checkLoops option to false. There are certainly legit use cases for
while(true) loops.

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterClasses.j...
lib/filterClasses.js:45: * @type {String}
This should be "string", not "String". The former refers to the type (e.g.
"foo"), the latter refers to the wrapper object (e.g. `new String("foo")`).

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterClasses.j...
lib/filterClasses.js:86: /* eslint-disable max-len */
The "max-len" rule has an "ignoreRegExpLiterals" option. We should rather add
this option to eslint-config-eyeo, than disabling that rule inline here.

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterClasses.j...
lib/filterClasses.js:119: let match = (
Better than wrapping, just use the more concise includes() instead of indexOf():

  let match = (text.includes("#") ? Filter.elemhideRegExp.exec(text) : null)

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterClasses.j...
lib/filterClasses.js:268: * @type {Boolean}
This should be "boolean", not "Boolean". The former refers to the type (e.g.
true), the latter refers to the wrapper object (e.g. `new Boolean(true)`).

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterClasses.j...
lib/filterClasses.js:971: selector = tagName + "." + id + additional + "," +
tagName +
This can be written in one line (with <80 characters) using a template literal:

  selector = `${tagName}.${id}${additional},${tagName}#${id}${additional}`;

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterListener.js
File lib/filterListener.js (right):

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterListener....
lib/filterListener.js:18: /* globals Services, XPCOMUtils */
Aren't these universally available? If so, please put them in .eslintrc.json.

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterNotifier.js
File lib/filterNotifier.js (right):

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterNotifier....
lib/filterNotifier.js:34: * @param {any} newValue
JSdoc interprets "any" as a class of the name "any". The special value that
indicates any type/class is "*". Alternatively, you may just omit the type.

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterStorage.j...
lib/filterStorage.js:705: /* eslint-disable no-warning-comments */
Please just file an issue for this change and remove the TODO comment. This is
what our coding practices dictates.

[kzar, 2017-02-21 05:13:59]

Patch Set 2 : Stop using commonjs, fix other problems

[kzar, 2017-02-21 06:14:00]

Patch Set 3 : Addressed Sebastian's initial feedback

https://codereview.adblockplus.org/29375915/diff/29375916/lib/downloader.js
File lib/downloader.js (right):

https://codereview.adblockplus.org/29375915/diff/29375916/lib/downloader.js#n...
lib/downloader.js:20: /* globals XMLHttpRequest */
On 2017/02/20 13:14:51, Sebastian Noack wrote:
> I would rather put this in the .eslintrc.json. Generally, I'd like to avoid
> inline boilerplate as much as possible. And if XMLHttpRequest is available
here,
> it is also available in most (for Chrome and WebExtensions even all) scripts.

Done.

https://codereview.adblockplus.org/29375915/diff/29375916/lib/downloader.js#n...
lib/downloader.js:76: * @type {Integer}
On 2017/02/20 13:14:51, Sebastian Noack wrote:
> There is no type or class called "Integer". This should rather be "number".

Done.

https://codereview.adblockplus.org/29375915/diff/29375916/lib/elemHide.js
File lib/elemHide.js (right):

https://codereview.adblockplus.org/29375915/diff/29375916/lib/elemHide.js#new...
lib/elemHide.js:270: let {selector} = filter;
On 2017/02/20 13:14:51, Sebastian Noack wrote:
> Perhaps just inline filter.selector, in the two occurrences below. This
> temporary variable seems kinda unnecessary.

Seems like kind of an unrelated change, would rather leave it.

https://codereview.adblockplus.org/29375915/diff/29375916/lib/elemHide.js#new...
lib/elemHide.js:363: /* eslint-disable no-constant-condition */
On 2017/02/20 13:14:51, Sebastian Noack wrote:
> Ugh. Can we please change esling-config-eyeo, configuring no-constant, setting
> the checkLoops option to false. There are certainly legit use cases for
> while(true) loops.

Done. https://codereview.adblockplus.org/29376668/

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterClasses.j...
lib/filterClasses.js:45: * @type {String}
On 2017/02/20 13:14:51, Sebastian Noack wrote:
> This should be "string", not "String". The former refers to the type (e.g.
> "foo"), the latter refers to the wrapper object (e.g. `new String("foo")`).

Done.

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterClasses.j...
lib/filterClasses.js:86: /* eslint-disable max-len */
On 2017/02/20 13:14:51, Sebastian Noack wrote:
> The "max-len" rule has an "ignoreRegExpLiterals" option. We should rather add
> this option to eslint-config-eyeo, than disabling that rule inline here.

Done. https://codereview.adblockplus.org/29376671/

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterClasses.j...
lib/filterClasses.js:119: let match = (
On 2017/02/20 13:14:51, Sebastian Noack wrote:
> Better than wrapping, just use the more concise includes() instead of
indexOf():
> 
>   let match = (text.includes("#") ? Filter.elemhideRegExp.exec(text) : null)

Done.

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterClasses.j...
lib/filterClasses.js:268: * @type {Boolean}
On 2017/02/20 13:14:51, Sebastian Noack wrote:
> This should be "boolean", not "Boolean". The former refers to the type (e.g.
> true), the latter refers to the wrapper object (e.g. `new Boolean(true)`).

Done.

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterClasses.j...
lib/filterClasses.js:971: selector = tagName + "." + id + additional + "," +
tagName +
On 2017/02/20 13:14:51, Sebastian Noack wrote:
> This can be written in one line (with <80 characters) using a template
literal:
> 
>   selector = `${tagName}.${id}${additional},${tagName}#${id}${additional}`;

Done.

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterListener.js
File lib/filterListener.js (right):

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterListener....
lib/filterListener.js:18: /* globals Services, XPCOMUtils */
On 2017/02/20 13:14:51, Sebastian Noack wrote:
> Aren't these universally available? If so, please put them in .eslintrc.json.

Done.

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterNotifier.js
File lib/filterNotifier.js (right):

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterNotifier....
lib/filterNotifier.js:34: * @param {any} newValue
On 2017/02/20 13:14:51, Sebastian Noack wrote:
> JSdoc interprets "any" as a class of the name "any". The special value that
> indicates any type/class is "*". Alternatively, you may just omit the type.

Done.

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29375915/diff/29375916/lib/filterStorage.j...
lib/filterStorage.js:705: /* eslint-disable no-warning-comments */
On 2017/02/20 13:14:51, Sebastian Noack wrote:
> Please just file an issue for this change and remove the TODO comment. This is
> what our coding practices dictates.

Done. https://issues.adblockplus.org/ticket/4919

[Sebastian Noack, 2017-02-21 09:19:32]

https://codereview.adblockplus.org/29375915/diff/29375916/lib/elemHide.js
File lib/elemHide.js (right):

https://codereview.adblockplus.org/29375915/diff/29375916/lib/elemHide.js#new...
lib/elemHide.js:270: let {selector} = filter;
On 2017/02/21 06:13:58, kzar wrote:
> On 2017/02/20 13:14:51, Sebastian Noack wrote:
> > Perhaps just inline filter.selector, in the two occurrences below. This
> > temporary variable seems kinda unnecessary.
> 
> Seems like kind of an unrelated change, would rather leave it.

Well, it's not unrelated, because in order to make ESLint happy we either have
to use destructing or inline the property lookup. I think the latter is
preferable, here.

https://codereview.adblockplus.org/29375915/diff/29376674/lib/elemHide.js
File lib/elemHide.js (right):

https://codereview.adblockplus.org/29375915/diff/29376674/lib/elemHide.js#new...
lib/elemHide.js:249: * @param {Number} key
It's "number" (lowercase), same everywhere else.

https://codereview.adblockplus.org/29375915/diff/29376674/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29375915/diff/29376674/lib/filterClasses.j...
lib/filterClasses.js:483: domain.length - docDomain.length - 1))
Perhaps this rather complex condition is either to understand (and nicer to
read) if you split it up a bit:

  if (this.domains[domain] && domain != docDomain)
  {
    if (domain.length <= docDomain.length)
      return false;

    let pos = domain.indexOf("." + docDomain);
    if (pos != domain.length - docDomain.length - 1)
      return false;
  }

What do you think?

https://codereview.adblockplus.org/29375915/diff/29376674/lib/filterClasses.j...
lib/filterClasses.js:884: replace(/^~[^,]+,?/, "").toLowerCase();
I think it's more readable when indented like this:

  this.selectorDomain = domains.replace(/,~[^,]+/g, "")
                               .replace(/^~[^,]+,?/, "").toLowerCase();

https://codereview.adblockplus.org/29375915/diff/29376674/lib/notification.js
File lib/notification.js (right):

https://codereview.adblockplus.org/29375915/diff/29376674/lib/notification.js...
lib/notification.js:26: let {Prefs} = require("prefs");
Didn't we just agree to use const for module imports?

https://codereview.adblockplus.org/29375915/diff/29376674/lib/rsa.js
File lib/rsa.js (right):

https://codereview.adblockplus.org/29375915/diff/29376674/lib/rsa.js#newcode112
lib/rsa.js:112: let end;
These variables aren't used outside of the loop, so the declaration should go
inside the loop head.

https://codereview.adblockplus.org/29375915/diff/29376674/lib/rsa.js#newcode190
lib/rsa.js:190: // Empty
I start to wonder what's the point of the no-empty rule. So far we only
encountered instances of legit empty blocks. Unfortunately, there isn't an
option to allow while-loops with empty block, neither does the "// Empty"
comment add any useful information. Considering that we didn't see a single
inappropriate use of empty blocks yet, and that those would be unlikely to make
it past our code review, we might want to remove that rule altogether.

https://codereview.adblockplus.org/29375915/diff/29376674/lib/subscriptionCla...
File lib/subscriptionClasses.js (right):

https://codereview.adblockplus.org/29375915/diff/29376674/lib/subscriptionCla...
lib/subscriptionClasses.js:280: filter((type) => type in
SpecialSubscription.defaultsMap).join(" "));
If you break after the dot, it looks weird and is hard to recognize this as
method call. So prefer to wrap code like this, like that:

  buffer.push("defaults=" + this.defaults.filter(
    type => type in SpecialSubscription.defaultsMap).join(" ")
  );

https://codereview.adblockplus.org/29375915/diff/29376674/lib/synchronizer.js
File lib/synchronizer.js (right):

https://codereview.adblockplus.org/29375915/diff/29376674/lib/synchronizer.js...
lib/synchronizer.js:343: if (match && match[1] == "301" &&           // Moved
permanently
Why so many spaces?

https://codereview.adblockplus.org/29375915/diff/29376674/lib/synchronizer.js...
lib/synchronizer.js:349: subscription.filters.map((f) => f.text).join("\n");
The parentheses around the arrow function's argument are redundant.

https://codereview.adblockplus.org/29375915/diff/29376674/test/_common.js
File test/_common.js (right):

https://codereview.adblockplus.org/29375915/diff/29376674/test/_common.js#new...
test/_common.js:407: /* eslint-disable no-console */
Why is this necessary anyway? We shouldn't have any occurrence of console.log()
anymore. And ESLint makes sure that we don't regress.

[Sebastian Noack, 2017-02-21 09:20:13]

https://codereview.adblockplus.org/29375915/diff/29375916/lib/elemHide.js
File lib/elemHide.js (right):

https://codereview.adblockplus.org/29375915/diff/29375916/lib/elemHide.js#new...
lib/elemHide.js:270: let {selector} = filter;
On 2017/02/21 09:19:30, Sebastian Noack wrote:
> On 2017/02/21 06:13:58, kzar wrote:
> > On 2017/02/20 13:14:51, Sebastian Noack wrote:
> > > Perhaps just inline filter.selector, in the two occurrences below. This
> > > temporary variable seems kinda unnecessary.
> > 
> > Seems like kind of an unrelated change, would rather leave it.
> 
> Well, it's not unrelated, because in order to make ESLint happy we either have
> to use destructing or inline the property lookup. I think the latter is
> preferable, here.

s/it's not related/it's related/

[kzar, 2017-02-21 10:37:04]

Patch Set 4 : Addressed further feedback from Sebastian

https://codereview.adblockplus.org/29375915/diff/29375916/lib/elemHide.js
File lib/elemHide.js (right):

https://codereview.adblockplus.org/29375915/diff/29375916/lib/elemHide.js#new...
lib/elemHide.js:270: let {selector} = filter;
On 2017/02/21 09:20:13, Sebastian Noack wrote:
> On 2017/02/21 09:19:30, Sebastian Noack wrote:
> > On 2017/02/21 06:13:58, kzar wrote:
> > > On 2017/02/20 13:14:51, Sebastian Noack wrote:
> > > > Perhaps just inline filter.selector, in the two occurrences below. This
> > > > temporary variable seems kinda unnecessary.
> > > 
> > > Seems like kind of an unrelated change, would rather leave it.
> > 
> > Well, it's not unrelated, because in order to make ESLint happy we either
have
> > to use destructing or inline the property lookup. I think the latter is
> > preferable, here.
> 
> s/it's not related/it's related/

Done.

https://codereview.adblockplus.org/29375915/diff/29376674/lib/elemHide.js
File lib/elemHide.js (right):

https://codereview.adblockplus.org/29375915/diff/29376674/lib/elemHide.js#new...
lib/elemHide.js:249: * @param {Number} key
On 2017/02/21 09:19:30, Sebastian Noack wrote:
> It's "number" (lowercase), same everywhere else.

Done.

https://codereview.adblockplus.org/29375915/diff/29376674/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29375915/diff/29376674/lib/filterClasses.j...
lib/filterClasses.js:483: domain.length - docDomain.length - 1))
On 2017/02/21 09:19:31, Sebastian Noack wrote:
> Perhaps this rather complex condition is either to understand (and nicer to
> read) if you split it up a bit:
> 
>   if (this.domains[domain] && domain != docDomain)
>   {
>     if (domain.length <= docDomain.length)
>       return false;
> 
>     let pos = domain.indexOf("." + docDomain);
>     if (pos != domain.length - docDomain.length - 1)
>       return false;
>   }
> 
> What do you think?

Looks much clearer to me, Done.

https://codereview.adblockplus.org/29375915/diff/29376674/lib/filterClasses.j...
lib/filterClasses.js:884: replace(/^~[^,]+,?/, "").toLowerCase();
On 2017/02/21 09:19:30, Sebastian Noack wrote:
> I think it's more readable when indented like this:
> 
>   this.selectorDomain = domains.replace(/,~[^,]+/g, "")
>                                .replace(/^~[^,]+,?/, "").toLowerCase();

Done.

https://codereview.adblockplus.org/29375915/diff/29376674/lib/notification.js
File lib/notification.js (right):

https://codereview.adblockplus.org/29375915/diff/29376674/lib/notification.js...
lib/notification.js:26: let {Prefs} = require("prefs");
On 2017/02/21 09:19:31, Sebastian Noack wrote:
> Didn't we just agree to use const for module imports?

Done.

https://codereview.adblockplus.org/29375915/diff/29376674/lib/rsa.js
File lib/rsa.js (right):

https://codereview.adblockplus.org/29375915/diff/29376674/lib/rsa.js#newcode112
lib/rsa.js:112: let end;
On 2017/02/21 09:19:31, Sebastian Noack wrote:
> These variables aren't used outside of the loop, so the declaration should go
> inside the loop head.

They are used outside of the loop.

https://codereview.adblockplus.org/29375915/diff/29376674/lib/rsa.js#newcode190
lib/rsa.js:190: // Empty
On 2017/02/21 09:19:31, Sebastian Noack wrote:
> I start to wonder what's the point of the no-empty rule. So far we only
> encountered instances of legit empty blocks. Unfortunately, there isn't an
> option to allow while-loops with empty block, neither does the "// Empty"
> comment add any useful information. Considering that we didn't see a single
> inappropriate use of empty blocks yet, and that those would be unlikely to
make
> it past our code review, we might want to remove that rule altogether.

Done. https://codereview.adblockplus.org/29376719/

https://codereview.adblockplus.org/29375915/diff/29376674/lib/subscriptionCla...
File lib/subscriptionClasses.js (right):

https://codereview.adblockplus.org/29375915/diff/29376674/lib/subscriptionCla...
lib/subscriptionClasses.js:280: filter((type) => type in
SpecialSubscription.defaultsMap).join(" "));
On 2017/02/21 09:19:31, Sebastian Noack wrote:
> If you break after the dot, it looks weird and is hard to recognize this as
> method call. So prefer to wrap code like this, like that:
> 
>   buffer.push("defaults=" + this.defaults.filter(
>     type => type in SpecialSubscription.defaultsMap).join(" ")
>   );

Done.

https://codereview.adblockplus.org/29375915/diff/29376674/lib/synchronizer.js
File lib/synchronizer.js (right):

https://codereview.adblockplus.org/29375915/diff/29376674/lib/synchronizer.js...
lib/synchronizer.js:343: if (match && match[1] == "301" &&           // Moved
permanently
On 2017/02/21 09:19:32, Sebastian Noack wrote:
> Why so many spaces?

Done.

https://codereview.adblockplus.org/29375915/diff/29376674/lib/synchronizer.js...
lib/synchronizer.js:349: subscription.filters.map((f) => f.text).join("\n");
On 2017/02/21 09:19:32, Sebastian Noack wrote:
> The parentheses around the arrow function's argument are redundant.

Done.

https://codereview.adblockplus.org/29375915/diff/29376674/test/_common.js
File test/_common.js (right):

https://codereview.adblockplus.org/29375915/diff/29376674/test/_common.js#new...
test/_common.js:407: /* eslint-disable no-console */
On 2017/02/21 09:19:32, Sebastian Noack wrote:
> Why is this necessary anyway? We shouldn't have any occurrence of
console.log()
> anymore. And ESLint makes sure that we don't regress.

Honestly I'm not sure why but when running our unit tests console.warn is not
defined, so I map it to console.log here.

[Sebastian Noack, 2017-02-21 11:11:56]

https://codereview.adblockplus.org/29375915/diff/29375916/.eslintignore
File .eslintignore (right):

https://codereview.adblockplus.org/29375915/diff/29375916/.eslintignore#newcode5
.eslintignore:5: # https://issues.adblockplus.org/ticket/4796
I really dislike how PhantomJS (not even being a target platform but merely a
test environment) limits our content scripts to ES5. This is just backwards. I'd
rather require building PhantomJS, with the latest JavaScriptCore, from source,
than maintaining this situation until they finally release version 2.5.

https://codereview.adblockplus.org/29375915/diff/29376674/test/_common.js
File test/_common.js (right):

https://codereview.adblockplus.org/29375915/diff/29376674/test/_common.js#new...
test/_common.js:407: /* eslint-disable no-console */
On 2017/02/21 10:37:03, kzar wrote:
> On 2017/02/21 09:19:32, Sebastian Noack wrote:
> > Why is this necessary anyway? We shouldn't have any occurrence of
> console.log()
> > anymore. And ESLint makes sure that we don't regress.
> 
> Honestly I'm not sure why but when running our unit tests console.warn is not
> defined, so I map it to console.log here.

Please first figure out what's going on here. Perhaps another limitation of
PhantomJS? At least, Node.js does have console.warn(). But then, don't we only
run content scripts with PhantomJS, while we have no consle.warn() in there?

If there is really no way around that hack, I might prefer to just disable the
no-console rule for the tests, as you already have an .eslintrc.json in the
tests directory, where you already disable other rules anyway.

[kzar, 2017-02-21 11:27:42]

Patch Set 5 : Disable no-console for the tests

https://codereview.adblockplus.org/29375915/diff/29375916/.eslintignore
File .eslintignore (right):

https://codereview.adblockplus.org/29375915/diff/29375916/.eslintignore#newcode5
.eslintignore:5: # https://issues.adblockplus.org/ticket/4796
On 2017/02/21 11:11:55, Sebastian Noack wrote:
> I really dislike how PhantomJS (not even being a target platform but merely a
> test environment) limits our content scripts to ES5. This is just backwards.
I'd
> rather require building PhantomJS, with the latest JavaScriptCore, from
source,
> than maintaining this situation until they finally release version 2.5.

Yea, it's not ideal. I already discussed this with Felix and Wladimir though, so
I guess this is how it will be for now.

https://codereview.adblockplus.org/29375915/diff/29376674/test/_common.js
File test/_common.js (right):

https://codereview.adblockplus.org/29375915/diff/29376674/test/_common.js#new...
test/_common.js:407: /* eslint-disable no-console */
On 2017/02/21 11:11:56, Sebastian Noack wrote:
> On 2017/02/21 10:37:03, kzar wrote:
> > On 2017/02/21 09:19:32, Sebastian Noack wrote:
> > > Why is this necessary anyway? We shouldn't have any occurrence of
> > console.log()
> > > anymore. And ESLint makes sure that we don't regress.
> > 
> > Honestly I'm not sure why but when running our unit tests console.warn is
not
> > defined, so I map it to console.log here.
> 
> Please first figure out what's going on here. Perhaps another limitation of
> PhantomJS? At least, Node.js does have console.warn(). But then, don't we only
> run content scripts with PhantomJS, while we have no consle.warn() in there?

Well I've tried but I couldn't figure it out. I start Node.js and confirmed
console.warn worked as I expected. Then I ran the tests and I get this trace (I
stripped the paths):

TypeError: console.warn is not a function
    at verifySignature (.../adblockpluscore/lib/rsa.js:200:13)
    at Object.exports.testVeryLongData
(.../adblockpluscore/test/signatures.js:129:12)
    at .../adblockpluscore/node_modules/nodeunit/lib/core.js:232:20
    at .../adblockpluscore/node_modules/nodeunit/deps/async.js:168:13
    at .../adblockpluscore/node_modules/nodeunit/deps/async.js:131:25
    at .../adblockpluscore/node_modules/nodeunit/deps/async.js:165:17
    at .../adblockpluscore/node_modules/nodeunit/deps/async.js:463:34
    at Object.exports.setUp (.../adblockpluscore/test/signatures.js:31:3)
    at .../adblockpluscore/node_modules/nodeunit/lib/core.js:260:35
    at .../adblockpluscore/node_modules/nodeunit/deps/async.js:458:21

> If there is really no way around that hack, I might prefer to just disable the
> no-console rule for the tests, as you already have an .eslintrc.json in the
> tests directory, where you already disable other rules anyway.

Done.

[Wladimir Palant, 2017-02-21 11:54:03]

I only looked at the configuration at this point, not at the proposed changes.

https://codereview.adblockplus.org/29375915/diff/29376785/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29375915/diff/29376785/.eslintrc.json#newc...
.eslintrc.json:5: "shared-node-browser": true
There is some context missing here, why not commonjs? I take it, commonjs
environment will always object on unused global variables. And this seems to be
perfectly fine when applied to actual CommonJS modules. I assume that this is
about lib/common.js which can also run in browser environment - a comment in
that file declaring its environment properly should do then.

Note that shared-node-browser seems to make little sense here, given that we
occasionally (Firefox and libadblockplus) run these files in an environment
which is neither the browser nor Node.js - all globals are being set explicitly
then.

https://codereview.adblockplus.org/29375915/diff/29376785/.eslintrc.json#newc...
.eslintrc.json:13: "Buffer": true,
This is wrong, Buffer isn't part of our usual environment - this is Node.js.
Unit tests are running in Node and declaring the correct environment via comment
in the file seems to be the best course of action (only one file affected IMHO).

https://codereview.adblockplus.org/29375915/diff/29376785/.eslintrc.json#newc...
.eslintrc.json:20: "XPCOMUtils": true,
Rather than declaring FileUtils, Services and XPCOMUtils here  even though they
aren't really globals, I'd suggest importing them properly:

  let {FileUtils} = Cu.import("resource://gre/modules/FileUtils.jsm", {});

This will need slight changes to lib/compat.js in adblockpluschrome. Long-term
goal is getting rid of these Firefox implementation details of course.

[kzar, 2017-02-23 10:53:56]

Patch Set 6 : Remove shared-node-browser env and Buffer global

https://codereview.adblockplus.org/29375915/diff/29376785/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29375915/diff/29376785/.eslintrc.json#newc...
.eslintrc.json:5: "shared-node-browser": true
On 2017/02/21 11:54:02, Wladimir Palant wrote:
> There is some context missing here, why not commonjs? I take it, commonjs
> environment will always object on unused global variables.

Yep, using commonjs objects to unused global variables which results in the
following ESLint errors:

.../adblockpluscore/lib/elemHide.js
  24:8  error  'Utils' is assigned a value but never used  no-unused-vars

.../adblockpluscore/lib/elemHideEmulation.js
  33:5  error  'ElemHideEmulation' is assigned a value but never used 
no-unused-vars

.../adblockpluscore/lib/matcher.js
   25:16  error  'RegExpFilter' is assigned a value but never used   
no-unused-vars
  465:5   error  'defaultMatcher' is assigned a value but never used 
no-unused-vars

.../adblockpluscore/lib/synchronizer.js
  30:16  error  'CommentFilter' is assigned a value but never used 
no-unused-vars

> Note that shared-node-browser seems to make little sense here, given that we
> occasionally (Firefox and libadblockplus) run these files in an environment
> which is neither the browser nor Node.js - all globals are being set
explicitly
> then.

Acknowledged.

https://codereview.adblockplus.org/29375915/diff/29376785/.eslintrc.json#newc...
.eslintrc.json:13: "Buffer": true,
On 2017/02/21 11:54:02, Wladimir Palant wrote:
> This is wrong, Buffer isn't part of our usual environment - this is Node.js.
> Unit tests are running in Node and declaring the correct environment via
comment
> in the file seems to be the best course of action (only one file affected
IMHO).

Done.

https://codereview.adblockplus.org/29375915/diff/29376785/.eslintrc.json#newc...
.eslintrc.json:20: "XPCOMUtils": true,
On 2017/02/21 11:54:02, Wladimir Palant wrote:
> Rather than declaring FileUtils, Services and XPCOMUtils here  even though
they
> aren't really globals, I'd suggest importing them properly:
> 
>   let {FileUtils} = Cu.import("resource://gre/modules/FileUtils.jsm", {});
> 
> This will need slight changes to lib/compat.js in adblockpluschrome. Long-term
> goal is getting rid of these Firefox implementation details of course.

I'd rather do this afterwards as a separate change, the changes here (and to
adblockpluschrome + adblockplusui) are already huge enough as it is.

[kzar, 2017-02-28 16:00:27]

Patch Set 7 : Rebased.

[Wladimir Palant, 2017-03-02 14:07:10]

https://codereview.adblockplus.org/29375915/diff/29376785/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29375915/diff/29376785/.eslintrc.json#newc...
.eslintrc.json:5: "shared-node-browser": true
On 2017/02/23 10:53:55, kzar wrote:
> Yep, using commonjs objects to unused global variables which results in the
> following ESLint errors:
> 
> .../adblockpluscore/lib/elemHide.js
>   24:8  error  'Utils' is assigned a value but never used  no-unused-vars

That's correct, this should be removed - I think Utils used to be necessary
before but they aren't any more.

> .../adblockpluscore/lib/elemHideEmulation.js
>   33:5  error  'ElemHideEmulation' is assigned a value but never used 
> no-unused-vars

Also correct, only assigning to exports.ElemHideEmulation is necessary here. But
it's probably a good idea to change the pattern into:

let ElemHideEmulation = ...;
exports.ElemHideEmulation = ElemHideEmulation;

> .../adblockpluscore/lib/matcher.js
>    25:16  error  'RegExpFilter' is assigned a value but never used   
> no-unused-vars

Same as above.

>   465:5   error  'defaultMatcher' is assigned a value but never used 
> no-unused-vars

Yes, this is a case where we should really drop the local variable.

> .../adblockpluscore/lib/synchronizer.js
>   30:16  error  'CommentFilter' is assigned a value but never used 
> no-unused-vars

Correct, no longer being used there.

https://codereview.adblockplus.org/29375915/diff/29376785/.eslintrc.json#newc...
.eslintrc.json:20: "XPCOMUtils": true,
On 2017/02/23 10:53:55, kzar wrote:
> I'd rather do this afterwards as a separate change, the changes here (and to
> adblockpluschrome + adblockplusui) are already huge enough as it is.

File an issue?

https://codereview.adblockplus.org/29375915/diff/29377625/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29375915/diff/29377625/.eslintrc.json#newc...
.eslintrc.json:6: "console": true,
I'd suggest removing this again, we currently don't have a console variable in
Firefox modules. The only module using it is rsa.js - for the very reason that
it isn't being used in Firefox. It should be explicitly declared as a global for
this module only, if we decide to integrate rsa.js in Firefox (quite possible)
we will either have to change the module or expose a console variable.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/downloader.js
File lib/downloader.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/lib/downloader.js#n...
lib/downloader.js:24: const {Utils} = require("utils");
ESLint doesn't seem to require this change. But if you are doing it, you should
also change the declarations below - these are actual constants.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/downloader.js#n...
lib/downloader.js:38: * @param {number} checkInterval  Interval between the
checks
Nit: Indentation is messy here. I suggest avoiding such issues by putting
comments on the next line:

 * @param {Function} dataSource
 *   Function that will yield downloadable objects on each check
 * @param {number} initialDelay
 *   Number of milliseconds to wait before the first check
 * @param {number} checkInterval
 *   Interval between the checks

https://codereview.adblockplus.org/29375915/diff/29377625/lib/downloader.js#n...
lib/downloader.js:134: // offline, e.g.  during a weekend. Increase soft
expiration
Nit: "offline" should go on the previous line.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/downloader.js#n...
lib/downloader.js:154: continue;
Nit: For multi-line conditions I suggest always adding brackets around the body,
makes it more obvious where the if body actually starts. Same below.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/downloader.js#n...
lib/downloader.js:254: redirectCallback = (url) =>
Nit: Parentheses around parameter are unnecessary here.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:104: * does the basic parsing and calls the right
constructor then.
Nit: "does the" belongs on the previous line.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:115: let match = (text.includes("#") ?
Filter.elemhideRegExp.exec(text) : null);
Please add integration notes to the issue - this is implemented as of Firefox
40, so we will need to increase minVersion when we update the dependency.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:174: // allowed to contain spaces
Nit: "allowed to" belongs on the previous line.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:212: * @param {string[]} buffer  buffer to push the
serialization results into
Do we really want to duplicate this description everywhere? I'd suggest leaving
it out or referring to Filter.serialize(). Same for the other implementations
below.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:243: *                  separated by domainSeparator e.g.
"foo.com|bar.com|~baz.com"
As before, I'd suggest avoiding the messy indentation here by putting
descriptions on the next line.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:300: * beginning of the epoch)
Why change the spacing in this description?

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:486: if (pos != domain.length - docDomain.length - 1)
Just change this into:

  if (!domain.endsWith("." + docDomain))

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:537: *                            apply to
As before, please avoid the messy indentation here.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:680: return true;
Since ESLint won't let you keep the braces here, I suggest removing this if
block altogether:

  return this.contentType & typeMask &&
         (this.thirdParty == null || this.thirdParty == thirdParty) &&
         this.isActiveOnDomain(docDomain, sitekey) &&
         this.regexp.test(location);

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:693: * @return {RegExpFilter}
This should be RegExpFilter|InvalidFilter. Or maybe (if you want to be
consistent with ElemHideBase.fromText() below)
BlockingFilter|WhitelistFilter|InvalidFilter. Or maybe just: @return {Filter}

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:822: *                            content, can be null
Messy indentation as before.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:929: * @param {string} selector     raw CSS selector (can
be empty)
Messy indentation as before.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterListener.js
File lib/filterListener.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterListener....
lib/filterListener.js:211: while (len % step == 0);
Ok, this is bad. I never even knew that you could write do..while loops without
brackets, it seems wrong.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterNotifier.js
File lib/filterNotifier.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterNotifier....
lib/filterNotifier.js:31: * @callback FilterNotifierOnListener
This is a rather weird callback name. How about FilterNotifierCatchAllListener?

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterNotifier....
lib/filterNotifier.js:35: * @param {*} oldValue
newValue and oldValue isn't really correct, it rather depends on the action. So
I'd suggest:

  @param{...*} additionalInfo

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterNotifier....
lib/filterNotifier.js:79: * @param {*} param3
Convert this to rest parameters?

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.j...
lib/filterStorage.js:86: ).getCharPref("data_directory"));
Now you have the closing parenthesis from getDefaultBranch call matched up with
the opening parenthesis from resolveFilePath.

  let dir = Services.prefs.getDefaultBranch("extensions.adblockplus.")
                          .getCharPref("data_directory");
  file = IO.resolveFilePath(dir);

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.j...
lib/filterStorage.js:249: *                          be used when filter list is
reloaded)
Messy indentation here.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.j...
lib/filterStorage.js:256: !s.disabled))
Wrong indentation, this should be aligned with "s instanceof"

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.j...
lib/filterStorage.js:406: let readFile = function(currentSourceFile,
backupIndex)
Why hasn't this been converted to an arrow function?

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.j...
lib/filterStorage.js:409: IO.readFromFile(currentSourceFile, parser,
readFromFileException =>
Ok, having to name every exception is ridiculous...

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.j...
lib/filterStorage.js:449: let doneReading = function(parser)
Why hasn't this been converted to an arrow function?

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.j...
lib/filterStorage.js:604: let writeFilters = function()
Why hasn't this been converted to an arrow function?

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.j...
lib/filterStorage.js:624: let checkBackupRequired =
function(callbackNotRequired, callbackRequired)
Why hasn't this been converted to an arrow function?

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.j...
lib/filterStorage.js:657: let removeLastBackup = function(part1, part2)
Why hasn't this been converted to an arrow function?

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.j...
lib/filterStorage.js:666: let renameBackup = function(part1, part2, index)
Why hasn't this been converted to an arrow function?

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.j...
lib/filterStorage.js:724: *                                    connected to its
filters
Messy indentation here.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/matcher.js
File lib/matcher.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/lib/matcher.js#newc...
lib/matcher.js:110: * @return {string} keyword
"might be an empty string" is actually important here. I'd suggest:

  keyword or an empty string if no keyword could be found

https://codereview.adblockplus.org/29375915/diff/29377625/lib/matcher.js#newc...
lib/matcher.js:183: * @return {Filter|null}
This is properly expressed as {?Filter}.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/matcher.js#newc...
lib/matcher.js:213: * @return {RegExpFilter|null} matching filter or null
This is properly expressed as {?RegExpFilter}.

Also, messy indentation here.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/matcher.js#newc...
lib/matcher.js:393: * @return {RegExpFilter|null} matching filter or null
Please don't duplicate the entire documentation here.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/matcher.js#newc...
lib/matcher.js:436: * @return {RegExpFilter} matching filter or null
Please don't duplicate the entire documentation here.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/notification.js
File lib/notification.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/lib/notification.js...
lib/notification.js:36: let TYPE = {
The values declared above are actually constants.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/notification.js...
lib/notification.js:155: Prefs.notificationdata.hardExpiration] =
The indentation here looks wrong. How about:

[
  Prefs.notificationdata.softExpiration,
  Prefs.notificationdata.hardExpiration
] = ...

https://codereview.adblockplus.org/29375915/diff/29377625/lib/notification.js...
lib/notification.js:396: * @callback approvedListener
QuestionListener? Seems to be the obvious choice given the terminology used
here.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/subscriptionCla...
File lib/subscriptionClasses.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/lib/subscriptionCla...
lib/subscriptionClasses.js:165: *                        created
Messy indentation here.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/subscriptionCla...
lib/subscriptionClasses.js:182: *                        created
Messy indentation here.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/subscriptionCla...
lib/subscriptionClasses.js:272: * @param {string[]} buffer  buffer to push the
serialization results into
Please don't duplicate this documentation all over the place.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/subscriptionCla...
lib/subscriptionClasses.js:279: buffer.push("defaults=" + this.defaults.filter(
Please move everything after "defaults=" to the next line an indent, currently
you are matching up the wrong parentheses.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/subscriptionCla...
lib/subscriptionClasses.js:304: while (url in Subscription.knownSubscriptions);
Another case where I just don't know how to fix it properly :-(

https://codereview.adblockplus.org/29375915/diff/29377625/lib/synchronizer.js
File lib/synchronizer.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/lib/synchronizer.js...
lib/synchronizer.js:39: let DEFAULT_EXPIRATION_INTERVAL = 5 * MILLIS_IN_DAY;
The values declared above are actual constants.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/synchronizer.js...
lib/synchronizer.js:87: *                          (should not trigger fallback
requests)
Messy indentation here.

https://codereview.adblockplus.org/29375915/diff/29377625/test/elemHideEmulat...
File test/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/test/elemHideEmulat...
test/elemHideEmulation.js:73: "example.com",
The indentation is wrong here. Should be:

[
  "example.com##[-abp-properties='foo']",
  "example.com##[-abp-properties='bar']",
  "example.com#@#[-abp-properties='foo']"
],
"example.com",

https://codereview.adblockplus.org/29375915/diff/29377625/test/elemHideEmulat...
test/elemHideEmulation.js:79: "www.example.com",
The added indentation is wrong here, "www.example.com" isn't part of the array
above.

https://codereview.adblockplus.org/29375915/diff/29377625/test/elemHideEmulat...
test/elemHideEmulation.js:93: "other.example.com",
Indentation is wrong here. Should be:

[
  "www.example.com##[-abp-properties='foo']",
  "other.example.com##[-abp-properties='foo']"
],
"other.example.com",

https://codereview.adblockplus.org/29375915/diff/29377625/test/filterStorage_...
File test/filterStorage_readwrite.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/test/filterStorage_...
test/filterStorage_readwrite.js:18: /* globals __dirname */
Is this necessary? Isn't it covered by env: node?

https://codereview.adblockplus.org/29375915/diff/29377625/test/regexpFilters_...
File test/regexpFilters_matching.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/test/regexpFilters_...
test/regexpFilters_matching.js:42: }
Why move an internal function outside the function that is using it?

https://codereview.adblockplus.org/29375915/diff/29377625/test/subscriptionCl...
File test/subscriptionClasses.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/test/subscriptionCl...
test/subscriptionClasses.js:87: "requiredVersion=0.6"],
Please don't do one space indentations, these are too hard to recognize.
Instead:

   [
     ...
   ]

[kzar, 2017-03-08 12:33:59]

Patch Set 8 : Addressed Wladimir's comments

https://codereview.adblockplus.org/29375915/diff/29376785/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29375915/diff/29376785/.eslintrc.json#newc...
.eslintrc.json:5: "shared-node-browser": true
On 2017/03/02 14:06:41, Wladimir Palant wrote:
> On 2017/02/23 10:53:55, kzar wrote:
> > Yep, using commonjs objects to unused global variables which results in the
> > following ESLint errors:
> > 
> > .../adblockpluscore/lib/elemHide.js
> >   24:8  error  'Utils' is assigned a value but never used  no-unused-vars
> 
> That's correct, this should be removed - I think Utils used to be necessary
> before but they aren't any more.
> 
> > .../adblockpluscore/lib/elemHideEmulation.js
> >   33:5  error  'ElemHideEmulation' is assigned a value but never used 
> > no-unused-vars
> 
> Also correct, only assigning to exports.ElemHideEmulation is necessary here.
But
> it's probably a good idea to change the pattern into:
> 
> let ElemHideEmulation = ...;
> exports.ElemHideEmulation = ElemHideEmulation;
> 
> > .../adblockpluscore/lib/matcher.js
> >    25:16  error  'RegExpFilter' is assigned a value but never used   
> > no-unused-vars
> 
> Same as above.
> 
> >   465:5   error  'defaultMatcher' is assigned a value but never used 
> > no-unused-vars
> 
> Yes, this is a case where we should really drop the local variable.
> 
> > .../adblockpluscore/lib/synchronizer.js
> >   30:16  error  'CommentFilter' is assigned a value but never used 
> > no-unused-vars
> 
> Correct, no longer being used there.

Done.

https://codereview.adblockplus.org/29375915/diff/29376785/.eslintrc.json#newc...
.eslintrc.json:20: "XPCOMUtils": true,
On 2017/03/02 14:06:41, Wladimir Palant wrote:
> On 2017/02/23 10:53:55, kzar wrote:
> > I'd rather do this afterwards as a separate change, the changes here (and to
> > adblockpluschrome + adblockplusui) are already huge enough as it is.
> 
> File an issue?

Why not, done it after all. (See corresponding changes to the adblockpluschrome
review https://codereview.adblockplus.org/29374674/ )

https://codereview.adblockplus.org/29375915/diff/29377625/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29375915/diff/29377625/.eslintrc.json#newc...
.eslintrc.json:6: "console": true,
On 2017/03/02 14:06:42, Wladimir Palant wrote:
> I'd suggest removing this again, we currently don't have a console variable in
> Firefox modules. The only module using it is rsa.js - for the very reason that
> it isn't being used in Firefox. It should be explicitly declared as a global
for
> this module only, if we decide to integrate rsa.js in Firefox (quite possible)
> we will either have to change the module or expose a console variable.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/downloader.js
File lib/downloader.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/lib/downloader.js#n...
lib/downloader.js:24: const {Utils} = require("utils");
On 2017/03/02 14:06:43, Wladimir Palant wrote:
> ESLint doesn't seem to require this change. But if you are doing it, you
should
> also change the declarations below - these are actual constants.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/downloader.js#n...
lib/downloader.js:38: * @param {number} checkInterval  Interval between the
checks
On 2017/03/02 14:06:42, Wladimir Palant wrote:
> Nit: Indentation is messy here. I suggest avoiding such issues by putting
> comments on the next line:
> 
>  * @param {Function} dataSource
>  *   Function that will yield downloadable objects on each check
>  * @param {number} initialDelay
>  *   Number of milliseconds to wait before the first check
>  * @param {number} checkInterval
>  *   Interval between the checks

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/downloader.js#n...
lib/downloader.js:134: // offline, e.g.  during a weekend. Increase soft
expiration
On 2017/03/02 14:06:44, Wladimir Palant wrote:
> Nit: "offline" should go on the previous line.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/downloader.js#n...
lib/downloader.js:154: continue;
On 2017/03/02 14:06:43, Wladimir Palant wrote:
> Nit: For multi-line conditions I suggest always adding brackets around the
body,
> makes it more obvious where the if body actually starts. Same below.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/downloader.js#n...
lib/downloader.js:254: redirectCallback = (url) =>
On 2017/03/02 14:06:44, Wladimir Palant wrote:
> Nit: Parentheses around parameter are unnecessary here.

Done. (Also added the "as-needed" arrow-parens rule, since that's the convention
here.)

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:104: * does the basic parsing and calls the right
constructor then.
On 2017/03/02 14:06:45, Wladimir Palant wrote:
> Nit: "does the" belongs on the previous line.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:115: let match = (text.includes("#") ?
Filter.elemhideRegExp.exec(text) : null);
On 2017/03/02 14:06:47, Wladimir Palant wrote:
> Please add integration notes to the issue - this is implemented as of Firefox
> 40, so we will need to increase minVersion when we update the dependency.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:174: // allowed to contain spaces
On 2017/03/02 14:06:45, Wladimir Palant wrote:
> Nit: "allowed to" belongs on the previous line.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:212: * @param {string[]} buffer  buffer to push the
serialization results into
On 2017/03/02 14:06:46, Wladimir Palant wrote:
> Do we really want to duplicate this description everywhere? I'd suggest
leaving
> it out or referring to Filter.serialize(). Same for the other implementations
> below.

Well I've removed those duplicate descriptions but now we're getting some ESLint
failures "Missing JSDoc for parameter 'buffer'  valid-jsdoc".

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:243: *                  separated by domainSeparator e.g.
"foo.com|bar.com|~baz.com"
On 2017/03/02 14:06:49, Wladimir Palant wrote:
> As before, I'd suggest avoiding the messy indentation here by putting
> descriptions on the next line.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:300: * beginning of the epoch)
On 2017/03/02 14:06:48, Wladimir Palant wrote:
> Why change the spacing in this description?

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:486: if (pos != domain.length - docDomain.length - 1)
On 2017/03/02 14:06:48, Wladimir Palant wrote:
> Just change this into:
> 
>   if (!domain.endsWith("." + docDomain))

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:537: *                            apply to
On 2017/03/02 14:06:48, Wladimir Palant wrote:
> As before, please avoid the messy indentation here.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:680: return true;
On 2017/03/02 14:06:45, Wladimir Palant wrote:
> Since ESLint won't let you keep the braces here, I suggest removing this if
> block altogether:
> 
>   return this.contentType & typeMask &&
>          (this.thirdParty == null || this.thirdParty == thirdParty) &&
>          this.isActiveOnDomain(docDomain, sitekey) &&
>          this.regexp.test(location);

We can keep the braces now.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:693: * @return {RegExpFilter}
On 2017/03/02 14:06:49, Wladimir Palant wrote:
> This should be RegExpFilter|InvalidFilter. Or maybe (if you want to be
> consistent with ElemHideBase.fromText() below)
> BlockingFilter|WhitelistFilter|InvalidFilter. Or maybe just: @return {Filter}

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:822: *                            content, can be null
On 2017/03/02 14:06:47, Wladimir Palant wrote:
> Messy indentation as before.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterClasses.j...
lib/filterClasses.js:929: * @param {string} selector     raw CSS selector (can
be empty)
On 2017/03/02 14:06:46, Wladimir Palant wrote:
> Messy indentation as before.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterListener.js
File lib/filterListener.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterListener....
lib/filterListener.js:211: while (len % step == 0);
On 2017/03/02 14:06:52, Wladimir Palant wrote:
> Ok, this is bad. I never even knew that you could write do..while loops
without
> brackets, it seems wrong.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterNotifier.js
File lib/filterNotifier.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterNotifier....
lib/filterNotifier.js:31: * @callback FilterNotifierOnListener
On 2017/03/02 14:06:52, Wladimir Palant wrote:
> This is a rather weird callback name. How about
FilterNotifierCatchAllListener?

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterNotifier....
lib/filterNotifier.js:35: * @param {*} oldValue
On 2017/03/02 14:06:53, Wladimir Palant wrote:
> newValue and oldValue isn't really correct, it rather depends on the action.
So
> I'd suggest:
> 
>   @param{...*} additionalInfo

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterNotifier....
lib/filterNotifier.js:79: * @param {*} param3
On 2017/03/02 14:06:53, Wladimir Palant wrote:
> Convert this to rest parameters?

Mind if I don't for now?

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.j...
lib/filterStorage.js:86: ).getCharPref("data_directory"));
On 2017/03/02 14:06:57, Wladimir Palant wrote:
> Now you have the closing parenthesis from getDefaultBranch call matched up
with
> the opening parenthesis from resolveFilePath.
> 
>   let dir = Services.prefs.getDefaultBranch("extensions.adblockplus.")
>                           .getCharPref("data_directory");
>   file = IO.resolveFilePath(dir);

Whoops, done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.j...
lib/filterStorage.js:249: *                          be used when filter list is
reloaded)
On 2017/03/02 14:06:54, Wladimir Palant wrote:
> Messy indentation here.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.j...
lib/filterStorage.js:256: !s.disabled))
On 2017/03/02 14:06:57, Wladimir Palant wrote:
> Wrong indentation, this should be aligned with "s instanceof"

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.j...
lib/filterStorage.js:406: let readFile = function(currentSourceFile,
backupIndex)
On 2017/03/02 14:06:56, Wladimir Palant wrote:
> Why hasn't this been converted to an arrow function?

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.j...
lib/filterStorage.js:409: IO.readFromFile(currentSourceFile, parser,
readFromFileException =>
On 2017/03/02 14:06:58, Wladimir Palant wrote:
> Ok, having to name every exception is ridiculous...

Acknowledged.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.j...
lib/filterStorage.js:449: let doneReading = function(parser)
On 2017/03/02 14:06:56, Wladimir Palant wrote:
> Why hasn't this been converted to an arrow function?

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.j...
lib/filterStorage.js:604: let writeFilters = function()
On 2017/03/02 14:06:54, Wladimir Palant wrote:
> Why hasn't this been converted to an arrow function?

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.j...
lib/filterStorage.js:624: let checkBackupRequired =
function(callbackNotRequired, callbackRequired)
On 2017/03/02 14:06:57, Wladimir Palant wrote:
> Why hasn't this been converted to an arrow function?

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.j...
lib/filterStorage.js:657: let removeLastBackup = function(part1, part2)
On 2017/03/02 14:06:55, Wladimir Palant wrote:
> Why hasn't this been converted to an arrow function?

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.j...
lib/filterStorage.js:666: let renameBackup = function(part1, part2, index)
On 2017/03/02 14:06:55, Wladimir Palant wrote:
> Why hasn't this been converted to an arrow function?

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/filterStorage.j...
lib/filterStorage.js:724: *                                    connected to its
filters
On 2017/03/02 14:06:54, Wladimir Palant wrote:
> Messy indentation here.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/matcher.js
File lib/matcher.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/lib/matcher.js#newc...
lib/matcher.js:110: * @return {string} keyword
On 2017/03/02 14:06:59, Wladimir Palant wrote:
> "might be an empty string" is actually important here. I'd suggest:
> 
>   keyword or an empty string if no keyword could be found

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/matcher.js#newc...
lib/matcher.js:183: * @return {Filter|null}
On 2017/03/02 14:07:00, Wladimir Palant wrote:
> This is properly expressed as {?Filter}.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/matcher.js#newc...
lib/matcher.js:213: * @return {RegExpFilter|null} matching filter or null
On 2017/03/02 14:07:00, Wladimir Palant wrote:
> This is properly expressed as {?RegExpFilter}.
> 
> Also, messy indentation here.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/matcher.js#newc...
lib/matcher.js:393: * @return {RegExpFilter|null} matching filter or null
On 2017/03/02 14:06:59, Wladimir Palant wrote:
> Please don't duplicate the entire documentation here.

OK but now we're getting ESLint failures:

  386:3  error  Missing JSDoc @returns for function         valid-jsdoc
  386:3  error  Missing JSDoc for parameter 'typeMask'      valid-jsdoc
  386:3  error  Missing JSDoc for parameter 'docDomain'     valid-jsdoc
  386:3  error  Missing JSDoc for parameter 'thirdParty'    valid-jsdoc
  386:3  error  Missing JSDoc for parameter 'sitekey'       valid-jsdoc
  386:3  error  Missing JSDoc for parameter 'specificOnly'  valid-jsdoc
  386:3  error  Missing JSDoc for parameter 'location'      valid-jsdoc

https://codereview.adblockplus.org/29375915/diff/29377625/lib/matcher.js#newc...
lib/matcher.js:436: * @return {RegExpFilter} matching filter or null
On 2017/03/02 14:06:58, Wladimir Palant wrote:
> Please don't duplicate the entire documentation here.

Same as above, we're getting `valid-jsdoc` failures here too now.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/notification.js
File lib/notification.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/lib/notification.js...
lib/notification.js:36: let TYPE = {
On 2017/03/02 14:07:01, Wladimir Palant wrote:
> The values declared above are actually constants.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/notification.js...
lib/notification.js:155: Prefs.notificationdata.hardExpiration] =
On 2017/03/02 14:07:00, Wladimir Palant wrote:
> The indentation here looks wrong. How about:
> 
> [
>   Prefs.notificationdata.softExpiration,
>   Prefs.notificationdata.hardExpiration
> ] = ...

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/notification.js...
lib/notification.js:396: * @callback approvedListener
On 2017/03/02 14:07:03, Wladimir Palant wrote:
> QuestionListener? Seems to be the obvious choice given the terminology used
> here.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/subscriptionCla...
File lib/subscriptionClasses.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/lib/subscriptionCla...
lib/subscriptionClasses.js:165: *                        created
On 2017/03/02 14:07:05, Wladimir Palant wrote:
> Messy indentation here.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/subscriptionCla...
lib/subscriptionClasses.js:182: *                        created
On 2017/03/02 14:07:05, Wladimir Palant wrote:
> Messy indentation here.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/subscriptionCla...
lib/subscriptionClasses.js:272: * @param {string[]} buffer  buffer to push the
serialization results into
On 2017/03/02 14:07:04, Wladimir Palant wrote:
> Please don't duplicate this documentation all over the place.

OK but now we're getting ESLint errors "Missing JSDoc for parameter 'buffer' 
valid-jsdoc".

https://codereview.adblockplus.org/29375915/diff/29377625/lib/subscriptionCla...
lib/subscriptionClasses.js:279: buffer.push("defaults=" + this.defaults.filter(
On 2017/03/02 14:07:04, Wladimir Palant wrote:
> Please move everything after "defaults=" to the next line an indent, currently
> you are matching up the wrong parentheses.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/subscriptionCla...
lib/subscriptionClasses.js:304: while (url in Subscription.knownSubscriptions);
On 2017/03/02 14:07:04, Wladimir Palant wrote:
> Another case where I just don't know how to fix it properly :-(

Fixed now since we removed the curl rule.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/synchronizer.js
File lib/synchronizer.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/lib/synchronizer.js...
lib/synchronizer.js:39: let DEFAULT_EXPIRATION_INTERVAL = 5 * MILLIS_IN_DAY;
On 2017/03/02 14:07:06, Wladimir Palant wrote:
> The values declared above are actual constants.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/synchronizer.js...
lib/synchronizer.js:87: *                          (should not trigger fallback
requests)
On 2017/03/02 14:07:06, Wladimir Palant wrote:
> Messy indentation here.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/test/elemHideEmulat...
File test/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/test/elemHideEmulat...
test/elemHideEmulation.js:73: "example.com",
On 2017/03/02 14:07:07, Wladimir Palant wrote:
> The indentation is wrong here. Should be:
> 
> [
>   "example.com##[-abp-properties='foo']",
>   "example.com##[-abp-properties='bar']",
>   "example.com#@#[-abp-properties='foo']"
> ],
> "example.com",

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/test/elemHideEmulat...
test/elemHideEmulation.js:79: "www.example.com",
On 2017/03/02 14:07:07, Wladimir Palant wrote:
> The added indentation is wrong here, "www.example.com" isn't part of the array
> above.

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/test/elemHideEmulat...
test/elemHideEmulation.js:93: "other.example.com",
On 2017/03/02 14:07:07, Wladimir Palant wrote:
> Indentation is wrong here. Should be:
> 
> [
>   "www.example.com##[-abp-properties='foo']",
>   "other.example.com##[-abp-properties='foo']"
> ],
> "other.example.com",

Done.

https://codereview.adblockplus.org/29375915/diff/29377625/test/filterStorage_...
File test/filterStorage_readwrite.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/test/filterStorage_...
test/filterStorage_readwrite.js:18: /* globals __dirname */
On 2017/03/02 14:07:08, Wladimir Palant wrote:
> Is this necessary? Isn't it covered by env: node?

Whoops, no it wasn't necessary.

https://codereview.adblockplus.org/29375915/diff/29377625/test/regexpFilters_...
File test/regexpFilters_matching.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/test/regexpFilters_...
test/regexpFilters_matching.js:42: }
On 2017/03/02 14:07:08, Wladimir Palant wrote:
> Why move an internal function outside the function that is using it?

I did this since we were violating no-shadow for the parameters. But thinking
about it we can simply not pass (most of) them instead.

https://codereview.adblockplus.org/29375915/diff/29377625/test/subscriptionCl...
File test/subscriptionClasses.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/test/subscriptionCl...
test/subscriptionClasses.js:87: "requiredVersion=0.6"],
On 2017/03/02 14:07:09, Wladimir Palant wrote:
> Please don't do one space indentations, these are too hard to recognize.
> Instead:
> 
>    [
>      ...
>    ]

Done.

[kzar, 2017-03-09 10:28:37]

Patch Set 9 : Remove the arrow-parens rule

[Wladimir Palant, 2017-03-09 14:37:16]

https://codereview.adblockplus.org/29375915/diff/29377625/lib/matcher.js
File lib/matcher.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/lib/matcher.js#newc...
lib/matcher.js:393: * @return {RegExpFilter|null} matching filter or null
On 2017/03/08 12:33:45, kzar wrote:
> On 2017/03/02 14:06:59, Wladimir Palant wrote:
> > Please don't duplicate the entire documentation here.
> 
> OK but now we're getting ESLint failures:
> 
>   386:3  error  Missing JSDoc @returns for function         valid-jsdoc
>   386:3  error  Missing JSDoc for parameter 'typeMask'      valid-jsdoc
>   386:3  error  Missing JSDoc for parameter 'docDomain'     valid-jsdoc
>   386:3  error  Missing JSDoc for parameter 'thirdParty'    valid-jsdoc
>   386:3  error  Missing JSDoc for parameter 'sitekey'       valid-jsdoc
>   386:3  error  Missing JSDoc for parameter 'specificOnly'  valid-jsdoc
>   386:3  error  Missing JSDoc for parameter 'location'      valid-jsdoc

What if we use @augments Matcher on CombinedMatcher? Or does ESlint expect
@inheritDoc?

https://codereview.adblockplus.org/29375915/diff/29377625/lib/subscriptionCla...
File lib/subscriptionClasses.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/lib/subscriptionCla...
lib/subscriptionClasses.js:272: * @param {string[]} buffer  buffer to push the
serialization results into
On 2017/03/08 12:33:53, kzar wrote:
> On 2017/03/02 14:07:04, Wladimir Palant wrote:
> > Please don't duplicate this documentation all over the place.
> 
> OK but now we're getting ESLint errors "Missing JSDoc for parameter 'buffer' 
> valid-jsdoc".

What if you use @inheritDoc here?

https://codereview.adblockplus.org/29375915/diff/29379029/lib/filterNotifier.js
File lib/filterNotifier.js (right):

https://codereview.adblockplus.org/29375915/diff/29379029/lib/filterNotifier....
lib/filterNotifier.js:34: * @param{...*} additionalInfo
Nit: space before { for consistency?

https://codereview.adblockplus.org/29375915/diff/29379029/lib/notification.js
File lib/notification.js (right):

https://codereview.adblockplus.org/29375915/diff/29379029/lib/notification.js...
lib/notification.js:405: * @callback questionListener
It's type name, should be uppercased IMHO.

https://codereview.adblockplus.org/29375915/diff/29379029/test/_common.js
File test/_common.js (right):

https://codereview.adblockplus.org/29375915/diff/29379029/test/_common.js#new...
test/_common.js:86: return {[resourceName]: resources[resourceName]};
This is a good case for the consistent-return rule :)

Please throw an exception for unknown imports, this will make sure that issues
don't manifest only much further down.

  throw new Error("Attempt to import unknown JavaScript module " + resource);

[kzar, 2017-03-10 06:56:16]

Patch Set 10 : Fixed valid-jsdoc failures and addressed nits

https://codereview.adblockplus.org/29375915/diff/29377625/lib/matcher.js
File lib/matcher.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/lib/matcher.js#newc...
lib/matcher.js:393: * @return {RegExpFilter|null} matching filter or null
On 2017/03/09 14:37:11, Wladimir Palant wrote:
> On 2017/03/08 12:33:45, kzar wrote:
> > On 2017/03/02 14:06:59, Wladimir Palant wrote:
> > > Please don't duplicate the entire documentation here.
> > 
> > OK but now we're getting ESLint failures:
> > 
> >   386:3  error  Missing JSDoc @returns for function         valid-jsdoc
> >   386:3  error  Missing JSDoc for parameter 'typeMask'      valid-jsdoc
> >   386:3  error  Missing JSDoc for parameter 'docDomain'     valid-jsdoc
> >   386:3  error  Missing JSDoc for parameter 'thirdParty'    valid-jsdoc
> >   386:3  error  Missing JSDoc for parameter 'sitekey'       valid-jsdoc
> >   386:3  error  Missing JSDoc for parameter 'specificOnly'  valid-jsdoc
> >   386:3  error  Missing JSDoc for parameter 'location'      valid-jsdoc
> 
> What if we use @augments Matcher on CombinedMatcher? Or does ESlint expect
> @inheritDoc?

Aha, that worked. Done.

https://codereview.adblockplus.org/29375915/diff/29377625/lib/subscriptionCla...
File lib/subscriptionClasses.js (right):

https://codereview.adblockplus.org/29375915/diff/29377625/lib/subscriptionCla...
lib/subscriptionClasses.js:272: * @param {string[]} buffer  buffer to push the
serialization results into
On 2017/03/09 14:37:11, Wladimir Palant wrote:
> On 2017/03/08 12:33:53, kzar wrote:
> > On 2017/03/02 14:07:04, Wladimir Palant wrote:
> > > Please don't duplicate this documentation all over the place.
> > 
> > OK but now we're getting ESLint errors "Missing JSDoc for parameter 'buffer'

> > valid-jsdoc".
> 
> What if you use @inheritDoc here?

Done.

https://codereview.adblockplus.org/29375915/diff/29379029/lib/filterNotifier.js
File lib/filterNotifier.js (right):

https://codereview.adblockplus.org/29375915/diff/29379029/lib/filterNotifier....
lib/filterNotifier.js:34: * @param{...*} additionalInfo
On 2017/03/09 14:37:13, Wladimir Palant wrote:
> Nit: space before { for consistency?

Done.

https://codereview.adblockplus.org/29375915/diff/29379029/lib/notification.js
File lib/notification.js (right):

https://codereview.adblockplus.org/29375915/diff/29379029/lib/notification.js...
lib/notification.js:405: * @callback questionListener
On 2017/03/09 14:37:13, Wladimir Palant wrote:
> It's type name, should be uppercased IMHO.

Done.

https://codereview.adblockplus.org/29375915/diff/29379029/test/_common.js
File test/_common.js (right):

https://codereview.adblockplus.org/29375915/diff/29379029/test/_common.js#new...
test/_common.js:86: return {[resourceName]: resources[resourceName]};
On 2017/03/09 14:37:14, Wladimir Palant wrote:
> This is a good case for the consistent-return rule :)
> 
> Please throw an exception for unknown imports, this will make sure that issues
> don't manifest only much further down.
> 
>   throw new Error("Attempt to import unknown JavaScript module " + resource);

Done.

[kzar, 2017-03-10 07:05:49]

Patch Set 11 : Rebased

[kzar, 2017-03-14 10:53:14]

Patch Set 12 : Fix Cu.imports in synchronizer.js that I missed previously

[kzar, 2017-03-14 10:56:38]

https://codereview.adblockplus.org/29375915/diff/29383634/lib/synchronizer.js
File lib/synchronizer.js (right):

https://codereview.adblockplus.org/29375915/diff/29383634/lib/synchronizer.js...
lib/synchronizer.js:24: const {XPCOMUtils} =
Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
I just noticed no-unused-vars errors for these. Why are these imports here if
we're not using them?

[Wladimir Palant, 2017-03-14 11:08:16]

LGTM regardless of whether you address the comment below with this commit or a
separate one.

https://codereview.adblockplus.org/29375915/diff/29383634/lib/synchronizer.js
File lib/synchronizer.js (right):

https://codereview.adblockplus.org/29375915/diff/29383634/lib/synchronizer.js...
lib/synchronizer.js:24: const {XPCOMUtils} =
Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
On 2017/03/14 10:56:37, kzar wrote:
> I just noticed no-unused-vars errors for these. Why are these imports here if
> we're not using them?

I guess that they were used before Downloader was split out as a separate class
- and there was no linter to remind us that these imports need to be removed as
well. Feel free to do that now.

[kzar, 2017-03-15 03:13:12]

Patch Set 13 : Removed unused imports

https://codereview.adblockplus.org/29375915/diff/29383634/lib/synchronizer.js
File lib/synchronizer.js (right):

https://codereview.adblockplus.org/29375915/diff/29383634/lib/synchronizer.js...
lib/synchronizer.js:24: const {XPCOMUtils} =
Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
On 2017/03/14 11:08:13, Wladimir Palant wrote:
> On 2017/03/14 10:56:37, kzar wrote:
> > I just noticed no-unused-vars errors for these. Why are these imports here
if
> > we're not using them?
> 
> I guess that they were used before Downloader was split out as a separate
class
> - and there was no linter to remind us that these imports need to be removed
as
> well. Feel free to do that now.

Done.

[Wladimir Palant, 2017-03-16 10:51:02]

Still LGTM