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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 11 months ago by Sebastian Noack
Modified:
4 years, 2 months ago
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
4 years, 11 months ago (2015-04-01 11:36:56 UTC) #1
Sebastian Noack
4 years, 10 months ago (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, ...
4 years, 8 months ago (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 ...
4 years, 2 months ago (2015-12-16 11:02:26 UTC) #4
Wladimir Palant
LGTM. However, you need to rebase again - part of this landed in #3421.
4 years, 2 months ago (2015-12-16 11:32:27 UTC) #5
Sebastian Noack
Rebased again.
4 years, 2 months ago (2015-12-16 12:04:46 UTC) #6
Wladimir Palant
4 years, 2 months ago (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...
Sign in to reply to this message.

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