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

Keyboard Shortcuts

	File
u :	up to issue
m :	publish + mail comments
M :	edit review message
j / k :	jump to file after / before current file
J / K :	jump to next file with a comment after / before current file
	Side-by-side diff
i :	toggle intra-line diffs
e :	expand all comments
c :	collapse all comments
s :	toggle showing all comments
n / p :	next / previous diff chunk or comment
N / P :	next / previous comment
<Up> / <Down> :	next / previous line
<Enter> :	respond to / edit current comment
d :	mark current comment as done

	Issue
u :	up to list of issues
m :	publish + mail comments
j / k :	jump to patch after / before current patch
o / <Enter> :	open current patch in side-by-side view
i :	open current patch in unified diff view

	Issue List
j / k :	jump to issue after / before current issue
o / <Enter> :	open current issue
# :	close issue

	Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :	save comment
<Esc> :	cancel edit

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 2 : Replaced some more function expressions #

Patch Set 4 : Make Prefs enumerable again #

Total comments: 1

Patch Set 6 : Rebased #

Total comments: 1

Created: Dec. 16, 2015, noon

Download [raw] [tar.bz2]

	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

Expand All Messages | Collapse All Messages

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

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...

Expand All Messages | Collapse All Messages