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

Issue 29886700: Issue 6969 - Implement abort-on-property-read snippet (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 5 months ago by hub
Modified:
1 year, 3 months ago
Reviewers:
Manish Jethani
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Description

Issue 6969 - Implement abort-on-property-read snippet

Patch Set 1 #

Patch Set 2 : eslint fixes #

Patch Set 3 : Allow multiple properties as arguments. #

Total comments: 23

Patch Set 4 : Addressed all review but the snippets parameters. #

Total comments: 2

Patch Set 5 : Revert to having one single property per call. #

Patch Set 6 : Rebase #

Patch Set 7 : Minor style changes #

Total comments: 4

Patch Set 8 : Split out the property wrap #

Total comments: 40

Patch Set 9 : Review changes #

Total comments: 6

Patch Set 10 : More changes from review #

Total comments: 2

Patch Set 11 : Bigger random ID #

Total comments: 5

Patch Set 12 : Update from review #

Total comments: 12

Patch Set 13 : Fixed jsdoc #

Total comments: 1

Patch Set 14 : Fix wrap property #

Total comments: 2

Patch Set 15 : Rework wrapPropertyAccess #

Total comments: 9

Patch Set 16 : Another round of changes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+66 lines, -0 lines) Patch
M lib/content/snippets.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +66 lines, -0 lines 0 comments Download

Messages

Total messages: 38
hub
1 year, 5 months ago (2018-09-20 22:12:58 UTC) #1
hub
This currently only handle single properties. not properties of properties.
1 year, 5 months ago (2018-09-20 22:13:45 UTC) #2
hub
I need to pass eslint first.
1 year, 5 months ago (2018-09-20 22:20:55 UTC) #3
hub
fixed eslint
1 year, 5 months ago (2018-09-20 22:27:08 UTC) #4
hub
now support multiple argument syntax.
1 year, 5 months ago (2018-09-21 20:11:07 UTC) #5
Manish Jethani
https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippets.js#newcode442 lib/content/snippets.js:442: // 2176782336 is 36^6 which mean 6 chars [a-z0-9] ...
1 year, 5 months ago (2018-09-24 11:08:36 UTC) #6
hub
updated patch https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippets.js#newcode442 lib/content/snippets.js:442: // 2176782336 is 36^6 which mean 6 ...
1 year, 5 months ago (2018-09-25 04:28:52 UTC) #7
Manish Jethani
https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippets.js#newcode442 lib/content/snippets.js:442: // 2176782336 is 36^6 which mean 6 chars [a-z0-9] ...
1 year, 5 months ago (2018-09-25 12:25:22 UTC) #8
hub
Updated patch https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippets.js#newcode442 lib/content/snippets.js:442: // 2176782336 is 36^6 which mean 6 ...
1 year, 5 months ago (2018-09-25 15:40:44 UTC) #9
Manish Jethani
Are these inline scripts, by the way? If they are scripts loaded off the network, ...
1 year, 4 months ago (2018-10-02 08:51:03 UTC) #10
hub
On 2018/10/02 08:51:03, Manish Jethani wrote: > Are these inline scripts, by the way? If ...
1 year, 4 months ago (2018-10-02 17:01:50 UTC) #11
Manish Jethani
On 2018/10/02 17:01:50, hub wrote: > On 2018/10/02 08:51:03, Manish Jethani wrote: > > Are ...
1 year, 4 months ago (2018-10-02 17:03:56 UTC) #12
Manish Jethani
https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippets.js#newcode454 lib/content/snippets.js:454: function abortOnPropertyRead(...props) On 2018/09/25 15:40:43, hub wrote: > On ...
1 year, 4 months ago (2018-10-22 14:21:02 UTC) #13
hub
https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippets.js#newcode454 lib/content/snippets.js:454: function abortOnPropertyRead(...props) On 2018/10/22 14:21:01, Manish Jethani wrote: > ...
1 year, 4 months ago (2018-10-22 14:52:49 UTC) #14
Manish Jethani
https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippets.js#newcode454 lib/content/snippets.js:454: function abortOnPropertyRead(...props) On 2018/10/22 14:52:49, hub wrote: > On ...
1 year, 4 months ago (2018-10-22 20:21:54 UTC) #15
Manish Jethani
https://codereview.adblockplus.org/29886700/diff/29900555/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29900555/lib/content/snippets.js#newcode530 lib/content/snippets.js:530: return onerror(this, message, ...rest); This needs to be onerror.call ...
1 year, 4 months ago (2018-10-23 15:31:25 UTC) #16
hub
https://codereview.adblockplus.org/29886700/diff/29900555/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29900555/lib/content/snippets.js#newcode530 lib/content/snippets.js:530: return onerror(this, message, ...rest); On 2018/10/23 15:31:24, Manish Jethani ...
1 year, 4 months ago (2018-10-23 18:23:17 UTC) #17
Manish Jethani
https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippets.js#newcode500 lib/content/snippets.js:500: // for when Math.random() returns 0.0 The last line ...
1 year, 4 months ago (2018-10-24 20:47:46 UTC) #18
Manish Jethani
https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippets.js#newcode542 lib/content/snippets.js:542: let {onerror} = window; Hmm ... I think we're ...
1 year, 4 months ago (2018-10-24 21:01:48 UTC) #19
Manish Jethani
https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippets.js#newcode542 lib/content/snippets.js:542: let {onerror} = window; On 2018/10/24 21:01:48, Manish Jethani ...
1 year, 4 months ago (2018-10-24 21:12:28 UTC) #20
hub
Updated patch https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippets.js#newcode500 lib/content/snippets.js:500: // for when Math.random() returns 0.0 On ...
1 year, 4 months ago (2018-10-24 22:17:39 UTC) #21
Manish Jethani
https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippets.js#newcode523 lib/content/snippets.js:523: * Will patch a property on the window object ...
1 year, 4 months ago (2018-10-24 23:02:10 UTC) #22
hub
Updated patch https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippets.js#newcode523 lib/content/snippets.js:523: * Will patch a property on the ...
1 year, 4 months ago (2018-10-25 14:32:29 UTC) #23
Manish Jethani
https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippets.js#newcode545 lib/content/snippets.js:545: if (typeof message == "string" && message.includes(rid)) On 2018/10/25 ...
1 year, 4 months ago (2018-10-25 16:12:17 UTC) #24
hub
Updated patch https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippets.js#newcode545 lib/content/snippets.js:545: if (typeof message == "string" && message.includes(rid)) ...
1 year, 4 months ago (2018-10-25 20:26:11 UTC) #25
Manish Jethani
https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippets.js#newcode548 lib/content/snippets.js:548: return onerror.call(this, message, ...rest); On 2018/10/25 20:26:09, hub wrote: ...
1 year, 4 months ago (2018-10-25 21:32:25 UTC) #26
Manish Jethani
https://codereview.adblockplus.org/29886700/diff/29926555/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29926555/lib/content/snippets.js#newcode501 lib/content/snippets.js:501: function randomId() Do you like this version of the ...
1 year, 4 months ago (2018-10-25 22:22:52 UTC) #27
Manish Jethani
https://codereview.adblockplus.org/29886700/diff/29926555/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29926555/lib/content/snippets.js#newcode501 lib/content/snippets.js:501: function randomId() Just to clarify, I'm sorry about leading ...
1 year, 4 months ago (2018-10-25 22:59:58 UTC) #28
hub
updated patch https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippets.js#newcode548 lib/content/snippets.js:548: return onerror.call(this, message, ...rest); On 2018/10/25 21:32:25, ...
1 year, 4 months ago (2018-10-26 15:19:06 UTC) #29
Manish Jethani
https://codereview.adblockplus.org/29886700/diff/29928555/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29928555/lib/content/snippets.js#newcode496 lib/content/snippets.js:496: * Generates a random alphanumeric ID consisting 6 base-36 ...
1 year, 3 months ago (2018-10-28 21:34:06 UTC) #30
hub
updated patch https://codereview.adblockplus.org/29886700/diff/29928555/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29928555/lib/content/snippets.js#newcode496 lib/content/snippets.js:496: * Generates a random alphanumeric ID consisting ...
1 year, 3 months ago (2018-10-29 16:34:03 UTC) #31
hub
updated patch https://codereview.adblockplus.org/29886700/diff/29930573/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29930573/lib/content/snippets.js#newcode520 lib/content/snippets.js:520: Object.defineProperty(object, property, currentDescriptor); Here the problem is ...
1 year, 3 months ago (2018-10-29 20:02:21 UTC) #32
Manish Jethani
https://codereview.adblockplus.org/29886700/diff/29931197/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29931197/lib/content/snippets.js#newcode515 lib/content/snippets.js:515: { OK, so here's the thing: We don't need ...
1 year, 3 months ago (2018-10-30 22:57:28 UTC) #33
hub
Updated patch https://codereview.adblockplus.org/29886700/diff/29931197/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29931197/lib/content/snippets.js#newcode515 lib/content/snippets.js:515: { On 2018/10/30 22:57:28, Manish Jethani wrote: ...
1 year, 3 months ago (2018-10-31 16:35:48 UTC) #34
Manish Jethani
https://codereview.adblockplus.org/29886700/diff/29932555/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29932555/lib/content/snippets.js#newcode544 lib/content/snippets.js:544: let {onerror} = window; Can we move this line ...
1 year, 3 months ago (2018-10-31 19:38:16 UTC) #35
Manish Jethani
Leave nits aside, LGTM https://codereview.adblockplus.org/29886700/diff/29932555/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29932555/lib/content/snippets.js#newcode524 lib/content/snippets.js:524: * No error will be ...
1 year, 3 months ago (2018-10-31 19:54:50 UTC) #36
hub
Updated patch https://codereview.adblockplus.org/29886700/diff/29932555/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29932555/lib/content/snippets.js#newcode524 lib/content/snippets.js:524: * No error will be printed in ...
1 year, 3 months ago (2018-10-31 20:44:17 UTC) #37
Manish Jethani
1 year, 3 months ago (2018-10-31 20:52:23 UTC) #38
LGTM

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

https://codereview.adblockplus.org/29886700/diff/29932555/lib/content/snippet...
lib/content/snippets.js:544: let {onerror} = window;
On 2018/10/31 20:44:16, hub wrote:
> On 2018/10/31 19:38:16, Manish Jethani wrote:
> > Can we move this line outside the if block? In theory somebody could want to
> > wrap "window.onerror" and then this would bomb.
> 
> It can be moved.
> 
> But it would still bomb if it was wrapped, on the line below.

Alright, fair enough.
Sign in to reply to this message.

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