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

Issue 5462707926990848: Issue 1434 - Removed remaining non-standard JavaScript code from buildtools (Closed)

Created:
April 1, 2015, 11:32 a.m. by Sebastian Noack
Modified:
Dec. 16, 2015, 1 p.m.
Reviewers:
Wladimir Palant
CC:
tschuster
Visibility:
Public.

Description

Issue 1434 - Removed remaining non-standard JavaScript code from buildtools

Patch Set 1 #

Patch Set 2 : Replaced some more function expressions #

Patch Set 3 : Rebased #

Patch Set 4 : Make Prefs enumerable again #

Total comments: 1

Patch Set 5 : Rebased #

Patch Set 6 : Rebased #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+30 lines, -40 lines) Patch
M bootstrap.js.tmpl View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M lib/hooks.js View 2 chunks +21 lines, -28 lines 1 comment Download
M lib/keySelector.js View 1 2 3 4 5 2 chunks +8 lines, -11 lines 0 comments Download

Messages

Total messages: 7
Sebastian Noack
April 1, 2015, 11:36 a.m. (2015-04-01 11:36:56 UTC) #1
Sebastian Noack
April 13, 2015, 10:37 a.m. (2015-04-13 10:37:06 UTC) #2
tschuster
https://codereview.adblockplus.org/5462707926990848/diff/5738600293466112/lib/hooks.js File lib/hooks.js (right): https://codereview.adblockplus.org/5462707926990848/diff/5738600293466112/lib/hooks.js#newcode61 lib/hooks.js:61: obj[name] = value; I would almost use Object.defineProperty here, ...
June 17, 2015, 11:01 a.m. (2015-06-17 11:01:56 UTC) #3
Sebastian Noack
I just rebased. And as some of these have already been addressed separately, the patch ...
Dec. 16, 2015, 11:02 a.m. (2015-12-16 11:02:26 UTC) #4
Wladimir Palant
LGTM. However, you need to rebase again - part of this landed in #3421.
Dec. 16, 2015, 11:32 a.m. (2015-12-16 11:32:27 UTC) #5
Sebastian Noack
Rebased again.
Dec. 16, 2015, 12:04 p.m. (2015-12-16 12:04:46 UTC) #6
Wladimir Palant
Dec. 16, 2015, 1 p.m. (2015-12-16 13:00:26 UTC) #7
Message was sent while issue was closed.
https://codereview.adblockplus.org/5462707926990848/diff/29332793/lib/hooks.js
File lib/hooks.js (right):

https://codereview.adblockplus.org/5462707926990848/diff/29332793/lib/hooks.j...
lib/hooks.js:70: Object.defineProperty(obj, name, origDesc);
For reference, this introduced a behavior change. Previously we were restoring
orig here which could have been modified by other extensions since we read it
out. Now we are always restoring the unchanged descriptor of the original
property here. This is relevant if initialization and uninitialization order of
the extensions isn't the reverse of each other. Hard to tell which approach will
result in more issues, at least my tests all succeed with the new one...

Powered by Google App Engine
This is Rietveld