Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29371763: Issue 4795 - Use modern JavaScript syntax (Closed)

Created:
Jan. 13, 2017, 12:11 p.m. by kzar
Modified:
Jan. 19, 2017, 4:09 a.m.
Reviewers:
Sebastian Noack
Visibility:
Public.

Description

Issue 4795 - Use modern JavaScript syntax

Patch Set 1 #

Total comments: 4

Patch Set 2 : "use strict"; #

Patch Set 3 : Use method shorthand #

Patch Set 4 : Workaround limiation with iterating element collections in older versions of Chrome #

Total comments: 1

Patch Set 5 : Fix scoping regression #

Total comments: 10

Patch Set 6 : Undo accidental whitespace change #

Total comments: 19

Patch Set 7 : Addressed feedback, used destructuring #

Patch Set 8 : Use const as per the new rules #

Patch Set 9 : Define and destructure backgroundPage more consistently, fix minor whitespace errors #

Total comments: 18

Patch Set 10 : Addressed some more feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+718 lines, -734 lines) Patch
M background.js View 1 2 3 4 5 6 7 2 chunks +14 lines, -12 lines 0 comments Download
M chrome/ext/background.js View 1 2 29 chunks +83 lines, -88 lines 0 comments Download
M chrome/ext/common.js View 1 2 3 4 5 3 chunks +12 lines, -3 lines 0 comments Download
M chrome/ext/content.js View 1 2 3 4 5 6 3 chunks +8 lines, -7 lines 0 comments Download
M chrome/ext/devtools.js View 1 chunk +3 lines, -4 lines 0 comments Download
M chrome/ext/popup.js View 1 2 3 4 5 6 7 8 9 2 chunks +7 lines, -6 lines 0 comments Download
M composer.js View 1 3 chunks +5 lines, -3 lines 0 comments Download
M composer.postload.js View 1 2 3 4 5 6 7 3 chunks +5 lines, -5 lines 0 comments Download
M ext/background.js View 1 2 1 chunk +16 lines, -15 lines 0 comments Download
M ext/common.js View 1 2 3 4 5 6 7 8 9 1 chunk +12 lines, -12 lines 0 comments Download
M include.preload.js View 1 2 3 4 5 6 7 27 chunks +81 lines, -83 lines 0 comments Download
M lib/compat.js View 1 2 10 chunks +41 lines, -49 lines 0 comments Download
M lib/csp.js View 1 2 3 4 5 6 7 1 chunk +4 lines, -4 lines 0 comments Download
M lib/devtools.js View 1 2 3 4 5 6 7 1 chunk +7 lines, -7 lines 0 comments Download
M lib/filterComposer.js View 1 2 3 4 5 6 7 5 chunks +11 lines, -11 lines 0 comments Download
M lib/filterValidation.js View 1 2 3 4 5 6 7 4 chunks +7 lines, -5 lines 0 comments Download
M lib/icon.js View 1 2 3 4 5 6 7 3 chunks +3 lines, -3 lines 0 comments Download
M lib/io.js View 1 2 6 chunks +11 lines, -12 lines 0 comments Download
M lib/messaging.js View 1 2 3 4 5 6 7 5 chunks +5 lines, -5 lines 0 comments Download
M lib/notificationHelper.js View 1 2 3 4 5 6 7 8 9 7 chunks +15 lines, -16 lines 0 comments Download
M lib/popupBlocker.js View 1 2 3 4 5 6 7 1 chunk +5 lines, -5 lines 0 comments Download
M lib/prefs.js View 1 2 3 4 5 6 7 7 chunks +10 lines, -11 lines 0 comments Download
M lib/requestBlocker.js View 1 2 3 4 5 6 7 2 chunks +10 lines, -10 lines 0 comments Download
M lib/stats.js View 1 2 3 4 5 6 7 3 chunks +9 lines, -11 lines 0 comments Download
M lib/subscriptionInit.js View 1 2 3 4 5 6 7 2 chunks +9 lines, -9 lines 0 comments Download
M lib/tldjs.js View 1 2 chunks +3 lines, -1 line 0 comments Download
M lib/uninstall.js View 1 2 3 4 5 6 7 1 chunk +7 lines, -6 lines 0 comments Download
M lib/url.js View 1 2 3 4 5 6 7 5 chunks +8 lines, -6 lines 0 comments Download
M lib/utils.js View 1 2 3 4 5 6 7 8 9 3 chunks +21 lines, -22 lines 0 comments Download
M lib/whitelisting.js View 1 2 3 4 5 6 7 3 chunks +11 lines, -10 lines 0 comments Download
M notification.js View 1 2 3 4 5 6 7 8 9 4 chunks +25 lines, -26 lines 0 comments Download
M options.js View 1 2 3 4 5 6 7 8 29 chunks +111 lines, -117 lines 0 comments Download
M popup.js View 1 2 3 4 5 6 7 8 9 7 chunks +19 lines, -20 lines 0 comments Download
M qunit/common.js View 1 3 chunks +4 lines, -3 lines 0 comments Download
M qunit/tests/cssEscaping.js View 1 2 3 4 5 6 7 4 chunks +15 lines, -19 lines 0 comments Download
M qunit/tests/filterValidation.js View 1 2 3 4 5 6 7 4 chunks +14 lines, -17 lines 0 comments Download
M qunit/tests/prefs.js View 1 2 8 chunks +12 lines, -11 lines 0 comments Download
M qunit/tests/url.js View 1 2 3 4 5 6 5 chunks +13 lines, -14 lines 0 comments Download
M qunit/tests/versionComparator.js View 1 3 chunks +21 lines, -21 lines 0 comments Download
M stats.js View 1 2 3 4 5 6 7 8 9 5 chunks +24 lines, -23 lines 0 comments Download
M subscriptionLink.postload.js View 3 chunks +6 lines, -7 lines 0 comments Download
M utils.js View 1 2 3 4 5 6 7 8 9 1 chunk +11 lines, -15 lines 0 comments Download

Messages

Total messages: 15
kzar
Patch Set 1 (I've quickly tested basic functionality but need to test these changes more ...
Jan. 13, 2017, 12:12 p.m. (2017-01-13 12:12:52 UTC) #1
Sebastian Noack
https://codereview.adblockplus.org/29371763/diff/29371764/chrome/ext/common.js File chrome/ext/common.js (left): https://codereview.adblockplus.org/29371763/diff/29371764/chrome/ext/common.js#oldcode18 chrome/ext/common.js:18: (function() As mentioned on the other review, unless you ...
Jan. 13, 2017, 12:30 p.m. (2017-01-13 12:30:50 UTC) #2
kzar
Patch Set 2 : "use strict"; Patch Set 3 : Use method shorthand https://codereview.adblockplus.org/29371763/diff/29371764/chrome/ext/common.js File ...
Jan. 16, 2017, 4:09 a.m. (2017-01-16 04:09:12 UTC) #3
kzar
Patch Set 4 : Workaround limitation with iterating element collections in older versions of Chrome ...
Jan. 16, 2017, 8:48 a.m. (2017-01-16 08:48:09 UTC) #4
Sebastian Noack
https://codereview.adblockplus.org/29371763/diff/29371998/chrome/ext/common.js File chrome/ext/common.js (right): https://codereview.adblockplus.org/29371763/diff/29371998/chrome/ext/common.js#newcode38 chrome/ext/common.js:38: getWindow() { return chrome.extension.getBackgroundPage(); } Turning this into a ...
Jan. 16, 2017, 2:47 p.m. (2017-01-16 14:47:41 UTC) #5
kzar
Patch Set 6 : Undo accidental whitespace change https://codereview.adblockplus.org/29371763/diff/29371998/chrome/ext/common.js File chrome/ext/common.js (right): https://codereview.adblockplus.org/29371763/diff/29371998/chrome/ext/common.js#newcode38 chrome/ext/common.js:38: getWindow() ...
Jan. 16, 2017, 2:59 p.m. (2017-01-16 14:59:11 UTC) #6
kzar
https://codereview.adblockplus.org/29371763/diff/29371998/lib/notificationHelper.js File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29371763/diff/29371998/lib/notificationHelper.js#newcode38 lib/notificationHelper.js:38: let canUseChromeNotifications = typeof chrome != "undefined" && "notifications" ...
Jan. 16, 2017, 3 p.m. (2017-01-16 15:00:48 UTC) #7
Sebastian Noack
https://codereview.adblockplus.org/29371763/diff/29371998/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29371763/diff/29371998/ext/background.js#newcode21 ext/background.js:21: let nonEmptyPageMaps = Object.create(null); On 2017/01/16 14:59:07, kzar wrote: ...
Jan. 16, 2017, 3:35 p.m. (2017-01-16 15:35:58 UTC) #8
Sebastian Noack
https://codereview.adblockplus.org/29371763/diff/29372051/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29371763/diff/29372051/include.preload.js#newcode84 include.preload.js:84: for (let child of element.children) On 2017/01/16 15:35:55, Sebastian ...
Jan. 16, 2017, 3:37 p.m. (2017-01-16 15:37:24 UTC) #9
kzar
Patch Set 7 : Addressed feedback, used destructuring https://codereview.adblockplus.org/29371763/diff/29371998/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29371763/diff/29371998/include.preload.js#newcode276 include.preload.js:276: for ...
Jan. 17, 2017, 7:42 a.m. (2017-01-17 07:42:51 UTC) #10
Sebastian Noack
Dependent on the outcome of the discussion regarding the usage of const (for module imports) ...
Jan. 17, 2017, 5:18 p.m. (2017-01-17 17:18:35 UTC) #11
kzar
Patch Set 8 : Use const as per the new rules Patch Set 9 : ...
Jan. 18, 2017, 7:36 a.m. (2017-01-18 07:36:41 UTC) #12
Sebastian Noack
https://codereview.adblockplus.org/29371763/diff/29372355/chrome/ext/popup.js File chrome/ext/popup.js (right): https://codereview.adblockplus.org/29371763/diff/29372355/chrome/ext/popup.js#newcode5 chrome/ext/popup.js:5: window.ext = Object.create(backgroundPage.ext); Assigning to the global object (i.e. ...
Jan. 18, 2017, 11:24 a.m. (2017-01-18 11:24:32 UTC) #13
kzar
Patch Set 10 : Addressed some more feedback https://codereview.adblockplus.org/29371763/diff/29372355/chrome/ext/popup.js File chrome/ext/popup.js (right): https://codereview.adblockplus.org/29371763/diff/29372355/chrome/ext/popup.js#newcode5 chrome/ext/popup.js:5: window.ext ...
Jan. 18, 2017, 11:46 a.m. (2017-01-18 11:46:43 UTC) #14
Sebastian Noack
Jan. 18, 2017, 12:12 p.m. (2017-01-18 12:12:27 UTC) #15
LGTM

Powered by Google App Engine
This is Rietveld