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

Issue 29995559: Issue 7236 - Handle sub properties in abort-on-property snippets (Closed)

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

Description

Issue 7236 - Handle sub properties in abort-on-property snippets

Patch Set 1 #

Total comments: 10

Patch Set 2 : Address comments. #

Patch Set 3 : use typeof instead. Remove test. #

Total comments: 6

Patch Set 4 : Pass magic to wrapPropertyAccess. Fix return value. #

Total comments: 2

Patch Set 5 : Check that the property is configurable. #

Patch Set 6 : added the tests back now that we can run them #

Total comments: 8

Patch Set 7 : Remove the magic from the setter. #

Total comments: 3

Patch Set 8 : Handle properties being overridden. #

Total comments: 6

Patch Set 9 : Fixes the result of wrap property #

Patch Set 10 : don't return a value from wrapProperty and unconditionally create error handler. #

Total comments: 11

Patch Set 11 : Addressed nits #

Total comments: 16

Patch Set 12 : A few test adjustements. #

Total comments: 2

Patch Set 13 : Cleanup test #

Total comments: 21

Patch Set 14 : Test cleanup #

Total comments: 13

Patch Set 15 : Fix message #

Total comments: 4

Patch Set 16 : More test cleanup #

Total comments: 2

Patch Set 17 : Nits #

Total comments: 4

Patch Set 18 : Formatting #

Patch Set 19 : Fix bogus test #

Total comments: 16

Patch Set 20 : more cleanup #

Total comments: 10

Patch Set 21 : tweak property names #

Unified diffs Side-by-side diffs Delta from patch set Stats (+176 lines, -18 lines) Patch
M lib/content/snippets.js View 1 2 3 4 5 6 7 8 9 10 4 chunks +34 lines, -10 lines 0 comments Download
A test/browser/_utils.js View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +28 lines, -0 lines 0 comments Download
M test/browser/elemHideEmulation.js View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +1 line, -8 lines 0 comments Download
A test/browser/snippets.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +113 lines, -0 lines 0 comments Download

Messages

Total messages: 57
hub
1 year ago (2019-02-01 20:14:59 UTC) #1
hub
The test fail on Firefox for some reason I need to figure out. Also the ...
1 year ago (2019-02-01 20:16:54 UTC) #2
Manish Jethani
On 2019/02/01 20:16:54, hub wrote: > The test fail on Firefox for some reason I ...
1 year ago (2019-02-03 14:30:23 UTC) #3
Manish Jethani
https://codereview.adblockplus.org/29995559/diff/29995560/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/29995560/lib/content/snippets.js#newcode714 lib/content/snippets.js:714: let prop = property.slice(0, dot); We could just call ...
1 year ago (2019-02-03 14:30:38 UTC) #4
hub
On 2019/02/03 14:30:23, Manish Jethani wrote: > On 2019/02/01 20:16:54, hub wrote: > > The ...
1 year ago (2019-02-03 19:51:08 UTC) #5
hub
patch updated. (test failure on Firefox not yet addressed). https://codereview.adblockplus.org/29995559/diff/29995560/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/29995560/lib/content/snippets.js#newcode714 lib/content/snippets.js:714: ...
1 year ago (2019-02-04 15:48:03 UTC) #6
Manish Jethani
https://codereview.adblockplus.org/29995559/diff/29995560/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/29995560/lib/content/snippets.js#newcode731 lib/content/snippets.js:731: if (a instanceof Object) On 2019/02/04 15:48:02, hub wrote: ...
1 year ago (2019-02-04 17:56:33 UTC) #7
hub
Updated patch https://codereview.adblockplus.org/29995559/diff/29995560/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/29995560/lib/content/snippets.js#newcode731 lib/content/snippets.js:731: if (a instanceof Object) On 2019/02/04 17:56:33, ...
1 year ago (2019-02-04 19:54:37 UTC) #8
Manish Jethani
https://codereview.adblockplus.org/29995559/diff/29998567/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/29998567/lib/content/snippets.js#newcode718 lib/content/snippets.js:718: return wrapPropertyAccess(value, property, descriptor); Don't we have to pass ...
1 year ago (2019-02-05 14:47:18 UTC) #9
hub
updated patch https://codereview.adblockplus.org/29995559/diff/29998567/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/29998567/lib/content/snippets.js#newcode718 lib/content/snippets.js:718: return wrapPropertyAccess(value, property, descriptor); On 2019/02/05 14:47:17, ...
1 year ago (2019-02-05 15:20:21 UTC) #10
Manish Jethani
https://codereview.adblockplus.org/29995559/diff/29999574/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/29999574/lib/content/snippets.js#newcode735 lib/content/snippets.js:735: Object.defineProperty(object, name, {get: () => v, set: setter}); What ...
1 year ago (2019-02-05 15:39:08 UTC) #11
hub
updated patch https://codereview.adblockplus.org/29995559/diff/29999574/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/29999574/lib/content/snippets.js#newcode735 lib/content/snippets.js:735: Object.defineProperty(object, name, {get: () => v, set: ...
1 year ago (2019-02-05 19:22:26 UTC) #12
Manish Jethani
https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippets.js#newcode736 lib/content/snippets.js:736: setter[magic] = undefined; I do not quite understand why ...
1 year ago (2019-02-08 10:49:48 UTC) #13
hub
https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippets.js#newcode736 lib/content/snippets.js:736: setter[magic] = undefined; On 2019/02/08 10:49:48, Manish Jethani wrote: ...
1 year ago (2019-02-08 14:14:04 UTC) #14
Manish Jethani
https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippets.js#newcode736 lib/content/snippets.js:736: setter[magic] = undefined; On 2019/02/08 14:14:03, hub wrote: > ...
1 year ago (2019-02-12 16:12:38 UTC) #15
hub
https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippets.js#newcode736 lib/content/snippets.js:736: setter[magic] = undefined; On 2019/02/12 16:12:38, Manish Jethani wrote: ...
1 year ago (2019-02-12 18:11:28 UTC) #16
Manish Jethani
https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippets.js#newcode736 lib/content/snippets.js:736: setter[magic] = undefined; On 2019/02/12 18:11:28, hub wrote: > ...
1 year ago (2019-02-15 14:45:24 UTC) #17
hub
https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippets.js#newcode736 lib/content/snippets.js:736: setter[magic] = undefined; > Well in any case it's ...
1 year ago (2019-02-19 15:36:49 UTC) #18
Manish Jethani
https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippets.js#newcode736 lib/content/snippets.js:736: setter[magic] = undefined; On 2019/02/19 15:36:48, hub wrote: > ...
1 year ago (2019-02-19 18:13:08 UTC) #19
hub
https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippets.js#newcode736 lib/content/snippets.js:736: setter[magic] = undefined; On 2019/02/19 18:13:07, Manish Jethani wrote: ...
1 year ago (2019-02-19 20:54:52 UTC) #20
Manish Jethani
https://codereview.adblockplus.org/29995559/diff/30011566/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30011566/lib/content/snippets.js#newcode724 lib/content/snippets.js:724: let v; There are two things missing in this ...
1 year ago (2019-02-20 06:20:46 UTC) #21
Manish Jethani
https://codereview.adblockplus.org/29995559/diff/30011566/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30011566/lib/content/snippets.js#newcode724 lib/content/snippets.js:724: let v; On 2019/02/20 06:20:46, Manish Jethani wrote: > ...
1 year ago (2019-02-20 06:28:54 UTC) #22
hub
updated patch https://codereview.adblockplus.org/29995559/diff/30011566/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30011566/lib/content/snippets.js#newcode724 lib/content/snippets.js:724: let v; On 2019/02/20 06:28:53, Manish Jethani ...
1 year ago (2019-02-22 17:36:19 UTC) #23
Manish Jethani
Sorry I have not had time to look at this but I'll get to it ...
12 months ago (2019-03-01 12:59:06 UTC) #24
Manish Jethani
I'd like to experiment with this code a little bit, but in the meanwhile a ...
12 months ago (2019-03-04 13:28:49 UTC) #25
hub
updated patch https://codereview.adblockplus.org/29995559/diff/30014568/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30014568/lib/content/snippets.js#newcode723 lib/content/snippets.js:723: return result; On 2019/03/04 13:28:49, Manish Jethani ...
11 months, 4 weeks ago (2019-03-05 13:49:13 UTC) #26
Manish Jethani
https://codereview.adblockplus.org/29995559/diff/30014568/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30014568/lib/content/snippets.js#newcode723 lib/content/snippets.js:723: return result; On 2019/03/05 13:49:13, hub wrote: > On ...
11 months, 4 weeks ago (2019-03-06 19:07:12 UTC) #27
hub
updated patch https://codereview.adblockplus.org/29995559/diff/30014568/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30014568/lib/content/snippets.js#newcode723 lib/content/snippets.js:723: return result; On 2019/03/06 19:07:12, Manish Jethani ...
11 months, 4 weeks ago (2019-03-06 21:22:04 UTC) #28
Manish Jethani
Aside from the nits the change in lib/ looks good to me. I'll take a ...
11 months, 3 weeks ago (2019-03-07 14:12:23 UTC) #29
hub
updated patch https://codereview.adblockplus.org/29995559/diff/30024564/lib/content/snippets.js File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30024564/lib/content/snippets.js#newcode703 lib/content/snippets.js:703: let dot = property.indexOf("."); On 2019/03/07 14:12:22, ...
11 months, 3 weeks ago (2019-03-07 21:32:28 UTC) #30
Manish Jethani
Some comments ... (By the way, we could split this into two patches, a separate ...
11 months, 3 weeks ago (2019-03-12 07:00:10 UTC) #31
hub
Since I am just testing this snippet here, I don't think it is worth splitting ...
11 months, 3 weeks ago (2019-03-12 14:29:18 UTC) #32
Manish Jethani
https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippets.js File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippets.js#newcode23 test/browser/snippets.js:23: window.browser = On 2019/03/12 14:29:17, hub wrote: > On ...
11 months, 3 weeks ago (2019-03-12 14:48:50 UTC) #33
hub
updated patch https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippets.js File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippets.js#newcode23 test/browser/snippets.js:23: window.browser = On 2019/03/12 14:48:49, Manish Jethani ...
11 months, 3 weeks ago (2019-03-12 15:15:12 UTC) #34
Manish Jethani
https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippets.js File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippets.js#newcode37 test/browser/snippets.js:37: await timeout(100); Do we really need this timeout here? ...
11 months, 3 weeks ago (2019-03-13 16:36:16 UTC) #35
hub
updated patch https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippets.js File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippets.js#newcode37 test/browser/snippets.js:37: await timeout(100); On 2019/03/13 16:36:14, Manish Jethani ...
11 months, 2 weeks ago (2019-03-14 10:28:06 UTC) #36
Manish Jethani
https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippets.js File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippets.js#newcode60 test/browser/snippets.js:60: if (properties.length == 0) Will properties.length every be non-zero ...
11 months, 2 weeks ago (2019-03-14 10:58:14 UTC) #37
Manish Jethani
https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippets.js File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippets.js#newcode37 test/browser/snippets.js:37: await timeout(100); On 2019/03/14 10:28:05, hub wrote: > On ...
11 months, 2 weeks ago (2019-03-14 11:32:09 UTC) #38
hub
updated patch https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippets.js File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippets.js#newcode37 test/browser/snippets.js:37: await timeout(100); On 2019/03/14 11:32:08, Manish Jethani ...
11 months, 2 weeks ago (2019-03-14 12:32:56 UTC) #39
Manish Jethani
https://codereview.adblockplus.org/29995559/diff/30027555/test/browser/snippets.js File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30027555/test/browser/snippets.js#newcode53 test/browser/snippets.js:53: let obj = window; On 2019/03/14 12:32:55, hub wrote: ...
11 months, 2 weeks ago (2019-03-14 16:56:24 UTC) #40
hub
updated patch https://codereview.adblockplus.org/29995559/diff/30027555/test/browser/snippets.js File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30027555/test/browser/snippets.js#newcode60 test/browser/snippets.js:60: if (path.length == 0) On 2019/03/14 16:56:22, ...
11 months, 2 weeks ago (2019-03-14 22:22:12 UTC) #41
Manish Jethani
https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippets.js File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippets.js#newcode37 test/browser/snippets.js:37: await timeout(100); On 2019/03/14 12:32:54, hub wrote: > On ...
11 months, 2 weeks ago (2019-03-16 10:50:25 UTC) #42
hub
Updated patch https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippets.js File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippets.js#newcode37 test/browser/snippets.js:37: await timeout(100); On 2019/03/16 10:50:24, Manish Jethani ...
11 months, 2 weeks ago (2019-03-18 18:27:27 UTC) #43
Manish Jethani
https://codereview.adblockplus.org/29995559/diff/30027560/test/browser/snippets.js File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30027560/test/browser/snippets.js#newcode92 test/browser/snippets.js:92: window.foo2 = 5; On 2019/03/14 16:56:23, Manish Jethani wrote: ...
11 months, 2 weeks ago (2019-03-19 07:44:01 UTC) #44
hub
updated patch https://codereview.adblockplus.org/29995559/diff/30028555/test/browser/snippets.js File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30028555/test/browser/snippets.js#newcode37 test/browser/snippets.js:37: // For snippets that run in the ...
11 months, 2 weeks ago (2019-03-19 17:25:12 UTC) #45
Manish Jethani
https://codereview.adblockplus.org/29995559/diff/30027560/test/browser/snippets.js File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30027560/test/browser/snippets.js#newcode92 test/browser/snippets.js:92: window.foo2 = 5; On 2019/03/19 07:43:59, Manish Jethani wrote: ...
11 months, 2 weeks ago (2019-03-19 17:38:23 UTC) #46
hub
updated patch https://codereview.adblockplus.org/29995559/diff/30027560/test/browser/snippets.js File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30027560/test/browser/snippets.js#newcode92 test/browser/snippets.js:92: window.foo2 = 5; On 2019/03/19 17:38:23, Manish ...
11 months, 2 weeks ago (2019-03-19 18:24:46 UTC) #47
Manish Jethani
https://codereview.adblockplus.org/29995559/diff/30029560/test/browser/snippets.js File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30029560/test/browser/snippets.js#newcode46 test/browser/snippets.js:46: window.abpTest = "foo"; This seems odd standing out on ...
11 months, 2 weeks ago (2019-03-20 01:42:01 UTC) #48
hub
updated patch https://codereview.adblockplus.org/29995559/diff/30029560/test/browser/snippets.js File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30029560/test/browser/snippets.js#newcode46 test/browser/snippets.js:46: window.abpTest = "foo"; On 2019/03/20 01:42:00, Manish ...
11 months, 2 weeks ago (2019-03-20 15:10:08 UTC) #49
Manish Jethani
Nits aside, LGTM. (I don't mind if you want to land this as it is. ...
11 months, 2 weeks ago (2019-03-20 15:11:52 UTC) #50
Manish Jethani
On 2019/03/20 15:11:52, Manish Jethani wrote: > Nits aside, LGTM. > > (I don't mind ...
11 months, 2 weeks ago (2019-03-20 15:12:37 UTC) #51
Manish Jethani
https://codereview.adblockplus.org/29995559/diff/30030561/test/browser/snippets.js File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30030561/test/browser/snippets.js#newcode95 test/browser/snippets.js:95: window.abpTest4 = {bar: {}}; If we agree about the ...
11 months, 2 weeks ago (2019-03-20 15:17:49 UTC) #52
Manish Jethani
https://codereview.adblockplus.org/29995559/diff/30030561/test/browser/snippets.js File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30030561/test/browser/snippets.js#newcode82 test/browser/snippets.js:82: window.abpTest2 = {prop1: "foo"}; Would be nice if we ...
11 months, 2 weeks ago (2019-03-20 15:20:53 UTC) #53
hub
updated patch https://codereview.adblockplus.org/29995559/diff/30030561/test/browser/snippets.js File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30030561/test/browser/snippets.js#newcode82 test/browser/snippets.js:82: window.abpTest2 = {prop1: "foo"}; On 2019/03/20 15:20:52, ...
11 months, 2 weeks ago (2019-03-20 16:19:06 UTC) #54
Manish Jethani
https://codereview.adblockplus.org/29995559/diff/30030561/test/browser/snippets.js File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30030561/test/browser/snippets.js#newcode95 test/browser/snippets.js:95: window.abpTest4 = {bar: {}}; On 2019/03/20 16:19:04, hub wrote: ...
11 months, 2 weeks ago (2019-03-20 16:43:33 UTC) #55
hub
https://codereview.adblockplus.org/29995559/diff/30030561/test/browser/snippets.js File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30030561/test/browser/snippets.js#newcode95 test/browser/snippets.js:95: window.abpTest4 = {bar: {}}; On 2019/03/20 16:43:32, Manish Jethani ...
11 months, 2 weeks ago (2019-03-20 16:52:17 UTC) #56
Manish Jethani
11 months, 2 weeks ago (2019-03-20 16:54:41 UTC) #57
Message was sent while issue was closed.
On 2019/03/20 16:52:17, hub wrote:
>
https://codereview.adblockplus.org/29995559/diff/30030561/test/browser/snippe...
> File test/browser/snippets.js (right):
> 
>
https://codereview.adblockplus.org/29995559/diff/30030561/test/browser/snippe...
> test/browser/snippets.js:95: window.abpTest4 = {bar: {}};
> On 2019/03/20 16:43:32, Manish Jethani wrote:
> > On 2019/03/20 16:19:04, hub wrote:
> > > On 2019/03/20 15:17:48, Manish Jethani wrote:
> > > > If we agree about the convention here then it should be
> `abpTest4.prop1.foo`
> > > > rather than `abpTest4.bar.lambda`.
> > > 
> > > Done.
> > 
> > LGTM
> > 
> > (What I really meant was that within a new level like `abpTest4`, the
> > sub-properties should again start from `prop1`, and within `prop1` they
should
> > again start from `foo`. Anyway this is fine.)
> 
> the intent was to really get these unique across the board.

Alright.

Thanks!
Sign in to reply to this message.

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