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

Issue 29979555: Issue 7207 - Implement abort-on-property-write snippet (Closed)

Created:
Jan. 11, 2019, 2:39 p.m. by hub
Modified:
Jan. 15, 2019, 5:34 p.m.
Reviewers:
Manish Jethani
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Description

Issue 7207 - Implement abort-on-property-write snippet

Patch Set 1 #

Total comments: 7

Patch Set 2 : Addressed comment typo #

Unified diffs Side-by-side diffs Delta from patch set Stats (+33 lines, -1 line) Patch
M lib/content/snippets.js View 1 2 chunks +33 lines, -1 line 0 comments Download

Messages

Total messages: 5
hub
Jan. 11, 2019, 2:40 p.m. (2019-01-11 14:40:02 UTC) #1
Manish Jethani
https://codereview.adblockplus.org/29979555/diff/29979556/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29979555/diff/29979556/lib/content/snippets.js#newcode763 lib/content/snippets.js:763: * property is written (set). Nit: I don't think ...
Jan. 15, 2019, 2:13 p.m. (2019-01-15 14:13:30 UTC) #2
Manish Jethani
LGTM, modulo the one correction.
Jan. 15, 2019, 2:53 p.m. (2019-01-15 14:53:03 UTC) #3
hub
updated patch https://codereview.adblockplus.org/29979555/diff/29979556/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29979555/diff/29979556/lib/content/snippets.js#newcode763 lib/content/snippets.js:763: * property is written (set). On 2019/01/15 ...
Jan. 15, 2019, 4:40 p.m. (2019-01-15 16:40:30 UTC) #4
Manish Jethani
Jan. 15, 2019, 5:24 p.m. (2019-01-15 17:24:16 UTC) #5
LGTM

https://codereview.adblockplus.org/29979555/diff/29979556/lib/content/snippet...
File lib/content/snippets.js (right):

https://codereview.adblockplus.org/29979555/diff/29979556/lib/content/snippet...
lib/content/snippets.js:784: if (wrapPropertyAccess(window, property, {set:
abort}))
On 2019/01/15 16:40:30, hub wrote:
> On 2019/01/15 14:13:29, Manish Jethani wrote:
> > Do we need a getter here?
> 
> We don't.

Acknowledged.

Powered by Google App Engine
This is Rietveld