Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(254)

Issue 29423569: Issue 4796 - Use a modern JS engine in the browser tests and convert all files to ECMAScript 6 (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 5 months ago by Wladimir Palant
Modified:
2 years, 5 months ago
Reviewers:
kzar, Felix Dahlke
CC:
hub
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Description

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.

Patch Set 1 #

Total comments: 8

Patch Set 2 : Removed redundant configuration change #

Total comments: 10

Patch Set 3 : Addressed some nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+556 lines, -305 lines) Patch
M .eslintignore View 1 chunk +0 lines, -7 lines 0 comments Download
M .eslintrc.json View 1 1 chunk +1 line, -13 lines 0 comments Download
M .gitignore View 1 chunk +1 line, -0 lines 0 comments Download
M .hgignore View 1 chunk +1 line, -0 lines 0 comments Download
M README.md View 1 chunk +1 line, -3 lines 0 comments Download
A chrome/content/.eslintrc.json View 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/content/elemHideEmulation.js View 2 chunks +60 lines, -43 lines 0 comments Download
A chromium_process.js View 1 chunk +272 lines, -0 lines 0 comments Download
A lib/.eslintrc.json View 1 chunk +14 lines, -0 lines 0 comments Download
M lib/common.js View 1 chunk +1 line, -2 lines 0 comments Download
M package.json View 1 chunk +3 lines, -2 lines 0 comments Download
A test/browser/.eslintrc.json View 1 chunk +8 lines, -0 lines 0 comments Download
M test/browser/_bootstrap.js View 1 2 1 chunk +74 lines, -113 lines 0 comments Download
M test/browser/elemHideEmulation.js View 1 chunk +70 lines, -97 lines 0 comments Download
M test_runner.js View 2 chunks +45 lines, -25 lines 0 comments Download

Messages

Total messages: 9
Wladimir Palant
2 years, 5 months ago (2017-04-27 11:21:05 UTC) #1
Wladimir Palant
https://codereview.adblockplus.org/29423569/diff/29423570/.eslintrc.json File .eslintrc.json (right): https://codereview.adblockplus.org/29423569/diff/29423570/.eslintrc.json#newcode5 .eslintrc.json:5: "ecmaVersion": 2017 This is necessary because our default config ...
2 years, 5 months ago (2017-04-27 11:46:41 UTC) #2
Wladimir Palant
Dave, not asking you to review chromium_process.js and _bootstrap.js but maybe you could review the ...
2 years, 5 months ago (2017-04-27 13:03:18 UTC) #3
kzar
On 2017/04/27 13:03:18, Wladimir Palant wrote: > Dave, not asking you to review chromium_process.js > ...
2 years, 5 months ago (2017-04-27 15:19:36 UTC) #4
Wladimir Palant
https://codereview.adblockplus.org/29423569/diff/29423570/.eslintrc.json File .eslintrc.json (right): https://codereview.adblockplus.org/29423569/diff/29423570/.eslintrc.json#newcode5 .eslintrc.json:5: "ecmaVersion": 2017 On 2017/04/27 15:19:36, kzar wrote: > On ...
2 years, 5 months ago (2017-04-27 18:04:50 UTC) #5
kzar
LGTM for all but those two files. https://codereview.adblockplus.org/29423569/diff/29423570/test/browser/elemHideEmulation.js File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29423569/diff/29423570/test/browser/elemHideEmulation.js#newcode22 test/browser/elemHideEmulation.js:22: let myUrl ...
2 years, 5 months ago (2017-04-28 09:03:31 UTC) #6
Felix Dahlke
I mainly just looked at chromium_process.js and _bootstrap.js. Looks good, pretty much. Just a couple ...
2 years, 5 months ago (2017-05-03 09:54:52 UTC) #7
Wladimir Palant
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#newcode236 chromium_process.js:236: arguments: bootstrapArgs.map(url => ({value: url})) On 2017/05/03 09:54:51, Felix ...
2 years, 5 months ago (2017-05-03 12:19:43 UTC) #8
Felix Dahlke
2 years, 5 months ago (2017-05-04 12:27:26 UTC) #9
LGTM
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5