Issue 4796 - Use a modern JS engine in the browser tests and convert all files to ECMAScript 6

I did not test this on platforms other than Linux. I did trigger the download of builds meant for other platforms however, and it seems that unzip cannot unpack the Mac build for some reason.

[Wladimir Palant, 2017-04-27 11:46:41]

https://codereview.adblockplus.org/29423569/diff/29423570/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29423569/diff/29423570/.eslintrc.json#newc...
.eslintrc.json:5: "ecmaVersion": 2017
This is necessary because our default config won't allow async functions. Yes,
we need to fix the default.

https://codereview.adblockplus.org/29423569/diff/29423570/chromium_process.js
File chromium_process.js (right):

https://codereview.adblockplus.org/29423569/diff/29423570/chromium_process.js...
chromium_process.js:202: let {Runtime, Log, Console, Page} = client;
For reference, the available interfaces are documented under
https://chromedevtools.github.io/debugger-protocol-viewer/tot/.

https://codereview.adblockplus.org/29423569/diff/29423570/chromium_process.js...
chromium_process.js:216: await Page.navigate({url: initialPage});
For reference, in headless mode Chrome always starts with a single page that has
about:blank loaded. I wouldn't mind leaving about:blank there but it seems that
about:blank cannot load file:/// URLs regardless of any command line switches.

[Wladimir Palant, 2017-04-27 13:03:18]

Dave, not asking you to review chromium_process.js and _bootstrap.js but maybe
you could review the rest of the changes so that Felix doesn't have to do it.

[kzar, 2017-04-27 15:19:36]

On 2017/04/27 13:03:18, Wladimir Palant wrote:
> Dave, not asking you to review chromium_process.js
> and _bootstrap.js but maybe you could review the
> rest of the changes so that Felix doesn't have to
> do it.

Sure, so I've looked at all but those files. LGTM except those nits.

https://codereview.adblockplus.org/29423569/diff/29423570/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29423569/diff/29423570/.eslintrc.json#newc...
.eslintrc.json:5: "ecmaVersion": 2017
On 2017/04/27 11:46:41, Wladimir Palant wrote:
> This is necessary because our default config won't allow async functions. Yes,
> we need to fix the default.

Since you're already adding it to the default configuration you can remove this
now. (I'll release a new version of the configuration once that lands.)

https://codereview.adblockplus.org/29423569/diff/29423570/test/browser/elemHi...
File test/browser/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29423569/diff/29423570/test/browser/elemHi...
test/browser/elemHideEmulation.js:22: let myUrl = document.currentScript.src;
Nit: Perhaps append the "/../../.." part here and rename to baseUrl? We seem to
do that every time myUrl is used anyway. That would have the bonus of making
line 87 less than 80 characters.

[Wladimir Palant, 2017-04-27 18:04:50]

https://codereview.adblockplus.org/29423569/diff/29423570/.eslintrc.json
File .eslintrc.json (right):

https://codereview.adblockplus.org/29423569/diff/29423570/.eslintrc.json#newc...
.eslintrc.json:5: "ecmaVersion": 2017
On 2017/04/27 15:19:36, kzar wrote:
> On 2017/04/27 11:46:41, Wladimir Palant wrote:
> > This is necessary because our default config won't allow async functions.
Yes,
> > we need to fix the default.
> 
> Since you're already adding it to the default configuration you can remove
this
> now. (I'll release a new version of the configuration once that lands.)

Done, removing it was the idea of course.

https://codereview.adblockplus.org/29423569/diff/29423570/test/browser/elemHi...
File test/browser/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29423569/diff/29423570/test/browser/elemHi...
test/browser/elemHideEmulation.js:22: let myUrl = document.currentScript.src;
On 2017/04/27 15:19:36, kzar wrote:
> Nit: Perhaps append the "/../../.." part here and rename to baseUrl? We seem
to
> do that every time myUrl is used anyway. That would have the bonus of making
> line 87 less than 80 characters.

I have a better idea - loadScript() should expect relative URLs. That's
something for another commit however, here I wanted to keep changes to the logic
minimal.

[kzar, 2017-04-28 09:03:31]

LGTM for all but those two files.

https://codereview.adblockplus.org/29423569/diff/29423570/test/browser/elemHi...
File test/browser/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29423569/diff/29423570/test/browser/elemHi...
test/browser/elemHideEmulation.js:22: let myUrl = document.currentScript.src;
On 2017/04/27 18:04:50, Wladimir Palant wrote:
> On 2017/04/27 15:19:36, kzar wrote:
> > Nit: Perhaps append the "/../../.." part here and rename to baseUrl? We seem
> to
> > do that every time myUrl is used anyway. That would have the bonus of making
> > line 87 less than 80 characters.
> 
> I have a better idea - loadScript() should expect relative URLs. That's
> something for another commit however, here I wanted to keep changes to the
logic
> minimal.

Acknowledged.

[Felix Dahlke, 2017-05-03 09:54:52]

I mainly just looked at chromium_process.js and _bootstrap.js. Looks good,
pretty much. Just a couple of minor comments.

https://codereview.adblockplus.org/29423569/diff/29423596/chromium_process.js
File chromium_process.js (right):

https://codereview.adblockplus.org/29423569/diff/29423596/chromium_process.js...
chromium_process.js:236: arguments: bootstrapArgs.map(url => ({value: url}))
Nit: Superfluous parentheses around `{value: url}`?

https://codereview.adblockplus.org/29423569/diff/29423596/test/browser/_boots...
File test/browser/_bootstrap.js (right):

https://codereview.adblockplus.org/29423569/diff/29423596/test/browser/_boots...
test/browser/_bootstrap.js:22: (function(nodeunitUrl, ...moduleUrls)
As discussed on IRC, I find this approach of loading the code (i.e. as an
expression whose result is saved) a bit unintuitive. Maybe we could set
`module.exports` instead, CommonJS style?

https://codereview.adblockplus.org/29423569/diff/29423596/test/browser/_boots...
test/browser/_bootstrap.js:39: let loadNext = () =>
I might be missing the reason for using let here, but if it's not actually
necessary, I would suggest to simplify the code like this:

  (function loadNext()
  {
    ...
  })();

https://codereview.adblockplus.org/29423569/diff/29423596/test/browser/_boots...
test/browser/_bootstrap.js:99: });
Nit: How I see it, this could be shortened quite a bit without violating the 80
column rule:

  let errors = assertions.filter(assertion => assertion.failed())
    .map(assertion => assertion.error);

https://codereview.adblockplus.org/29423569/diff/29423596/test/browser/_boots...
test/browser/_bootstrap.js:118: let failures = assertions.filter(assertion =>
Nit: As above, I think this can be shortened:

  let failures = assertions.filter(assertion => assertion.failed());

[Wladimir Palant, 2017-05-03 12:19:43]

https://codereview.adblockplus.org/29423569/diff/29423596/chromium_process.js
File chromium_process.js (right):

https://codereview.adblockplus.org/29423569/diff/29423596/chromium_process.js...
chromium_process.js:236: arguments: bootstrapArgs.map(url => ({value: url}))
On 2017/05/03 09:54:51, Felix Dahlke wrote:
> Nit: Superfluous parentheses around `{value: url}`?

Nope, otherwise the brackets will be interpreted as a block.

https://codereview.adblockplus.org/29423569/diff/29423596/test/browser/_boots...
File test/browser/_bootstrap.js (right):

https://codereview.adblockplus.org/29423569/diff/29423596/test/browser/_boots...
test/browser/_bootstrap.js:22: (function(nodeunitUrl, ...moduleUrls)
On 2017/05/03 09:54:51, Felix Dahlke wrote:
> Maybe we could set `module.exports` instead, CommonJS style?

This isn't a module, we are loading code in global scope. So I'd rather not have
it touch any global symbols.

https://codereview.adblockplus.org/29423569/diff/29423596/test/browser/_boots...
test/browser/_bootstrap.js:39: let loadNext = () =>
On 2017/05/03 09:54:51, Felix Dahlke wrote:
> I might be missing the reason for using let here, but if it's not actually
> necessary, I would suggest to simplify the code like this:
> 
>   (function loadNext()
>   {
>     ...
>   })();

Not sure whether it makes the code better but ok.

https://codereview.adblockplus.org/29423569/diff/29423596/test/browser/_boots...
test/browser/_bootstrap.js:99: });
On 2017/05/03 09:54:51, Felix Dahlke wrote:
> Nit: How I see it, this could be shortened quite a bit without violating the
80
> column rule:
> 
>   let errors = assertions.filter(assertion => assertion.failed())
>     .map(assertion => assertion.error);

Done.

https://codereview.adblockplus.org/29423569/diff/29423596/test/browser/_boots...
test/browser/_bootstrap.js:118: let failures = assertions.filter(assertion =>
On 2017/05/03 09:54:51, Felix Dahlke wrote:
> Nit: As above, I think this can be shortened:
> 
>   let failures = assertions.filter(assertion => assertion.failed());

Done.

[Felix Dahlke, 2017-05-04 12:27:26]

LGTM