Issue 30033574: Issue 7419 - Allow wrapping function for abort-on-property-* (Closed)
|
Created:
March 28, 2019, 8:39 p.m. by hub Modified:
March 29, 2019, 4:02 p.m. Reviewers:
Manish Jethani CC:
a.giammarchi Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionIssue 7419 - Allow wrapping function for abort-on-property-*
Patch Set 1 #
Total comments: 9
Patch Set 2 : Adde more tests #Patch Set 3 : Deal with function base property delayed creation #
MessagesTotal messages: 9
https://codereview.adblockplus.org/30033574/diff/30033575/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/30033574/diff/30033575/lib/content/snippet... lib/content/snippets.js:729: if (newValue && typeof newValue == "object") We should change this too. https://codereview.adblockplus.org/30033574/diff/30033575/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/30033574/diff/30033575/test/browser/snippe... test/browser/snippets.js:114: await runSnippet(test, "abort-on-property-read", "Object.keys"); In the interest of completeness we could add the following tests: // Function properties. window.abpTest6 = function() {}; window.abpTest6.prop1 = function() {}; await runSnippet(test, "abort-on-property-read", "abpTest6.prop1"); testProperty("abpTest6.prop1"); // Function properties, with sub-property set afterwards. window.abpTest7 = function() {}; await runSnippet(test, "abort-on-property-read", "abpTest7.prop1"); window.abpTest7.prop1 = function() {}; testProperty("abpTest7.prop1"); // Arrow function properties. window.abpTest8 = () => {}; await runSnippet(test, "abort-on-property-read", "abpTest8"); testProperty("abpTest8"); // Class function properties. window.abpTest9 = class {}; await runSnippet(test, "abort-on-property-read", "abpTest9"); testProperty("abpTest9"); // Class function properties with prototype function properties. window.abpTest10 = class {}; window.abpTest10.prototype.prop1 = function() {}; await runSnippet(test, "abort-on-property-read", "abpTest10.prototype.prop1"); testProperty("abpTest10.prototype.prop1"); // Class function properties with prototype function properties, with // prototype property set afterwards. window.abpTest11 = class {}; await runSnippet(test, "abort-on-property-read", "abpTest11.prototype.prop1"); window.abpTest11.prototype.prop1 = function() {}; testProperty("abpTest11.prototype.prop1");
There is already a commit with nearly the same summary. We could use "Issue 7419 - Wrap function properties in abort-on-* snippets" or just "Issue 7419 - Wrap function properties in wrapPropertyAccess()".
updated patch https://codereview.adblockplus.org/30033574/diff/30033575/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/30033574/diff/30033575/lib/content/snippet... lib/content/snippets.js:729: if (newValue && typeof newValue == "object") On 2019/03/29 07:08:07, Manish Jethani wrote: > We should change this too. I considered changing here unnecessary as this is the wrapping for when intermediate properties are created. https://codereview.adblockplus.org/30033574/diff/30033575/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/30033574/diff/30033575/test/browser/snippe... test/browser/snippets.js:114: await runSnippet(test, "abort-on-property-read", "Object.keys"); On 2019/03/29 07:08:07, Manish Jethani wrote: > In the interest of completeness we could add the following tests: > > // Function properties. > window.abpTest6 = function() {}; > window.abpTest6.prop1 = function() {}; > await runSnippet(test, "abort-on-property-read", "abpTest6.prop1"); > testProperty("abpTest6.prop1"); > > // Function properties, with sub-property set afterwards. > window.abpTest7 = function() {}; > await runSnippet(test, "abort-on-property-read", "abpTest7.prop1"); > window.abpTest7.prop1 = function() {}; > testProperty("abpTest7.prop1"); > > // Arrow function properties. > window.abpTest8 = () => {}; > await runSnippet(test, "abort-on-property-read", "abpTest8"); > testProperty("abpTest8"); > > // Class function properties. > window.abpTest9 = class {}; > await runSnippet(test, "abort-on-property-read", "abpTest9"); > testProperty("abpTest9"); > > // Class function properties with prototype function properties. > window.abpTest10 = class {}; > window.abpTest10.prototype.prop1 = function() {}; > await runSnippet(test, "abort-on-property-read", "abpTest10.prototype.prop1"); > testProperty("abpTest10.prototype.prop1"); > > // Class function properties with prototype function properties, with > // prototype property set afterwards. > window.abpTest11 = class {}; > await runSnippet(test, "abort-on-property-read", "abpTest11.prototype.prop1"); > window.abpTest11.prototype.prop1 = function() {}; > testProperty("abpTest11.prototype.prop1"); for the record, test 8 and 9 already pass. But that's fine. More coverage.
https://codereview.adblockplus.org/30033574/diff/30033575/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/30033574/diff/30033575/lib/content/snippet... lib/content/snippets.js:729: if (newValue && typeof newValue == "object") On 2019/03/29 14:36:27, hub wrote: > On 2019/03/29 07:08:07, Manish Jethani wrote: > > We should change this too. > > I considered changing here unnecessary as this is the wrapping for when > intermediate properties are created. Well so if an intermediate property is a function it should still work, right?
updated patch https://codereview.adblockplus.org/30033574/diff/30033575/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/30033574/diff/30033575/lib/content/snippet... lib/content/snippets.js:729: if (newValue && typeof newValue == "object") On 2019/03/29 14:57:10, Manish Jethani wrote: > On 2019/03/29 14:36:27, hub wrote: > > On 2019/03/29 07:08:07, Manish Jethani wrote: > > > We should change this too. > > > > I considered changing here unnecessary as this is the wrapping for when > > intermediate properties are created. > > Well so if an intermediate property is a function it should still work, right? I was wondering "why would someone do that" but then JavaScript is the answer. Done. And added test case as well.
LGTM https://codereview.adblockplus.org/30033574/diff/30033575/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/30033574/diff/30033575/test/browser/snippe... test/browser/snippets.js:114: await runSnippet(test, "abort-on-property-read", "Object.keys"); On 2019/03/29 14:36:27, hub wrote: > On 2019/03/29 07:08:07, Manish Jethani wrote: > > In the interest of completeness we could add the following tests: > > > > [...] > > for the record, test 8 and 9 already pass. But that's fine. More coverage. The reason I thought we should add these tests is that it makes it clear to anyone else who comes by that this works with arrow functions and classes. It may not be immediately obvious to someone who is not a JS expert that a class is in fact just a function, for example (esp. if they are from a C++ background). This makes the code friendlier to everyone.
https://codereview.adblockplus.org/30033574/diff/30033575/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/30033574/diff/30033575/test/browser/snippe... test/browser/snippets.js:114: await runSnippet(test, "abort-on-property-read", "Object.keys"); On 2019/03/29 15:46:27, Manish Jethani wrote: > On 2019/03/29 14:36:27, hub wrote: > > On 2019/03/29 07:08:07, Manish Jethani wrote: > > > In the interest of completeness we could add the following tests: > > > > > > [...] > > > > for the record, test 8 and 9 already pass. But that's fine. More coverage. > > The reason I thought we should add these tests is that it makes it clear to > anyone else who comes by that this works with arrow functions and classes. It > may not be immediately obvious to someone who is not a JS expert that a class is > in fact just a function, for example (esp. if they are from a C++ background). > This makes the code friendlier to everyone. I was not arguing against. I was just indicating that they already worked. The more. The merrier.
https://codereview.adblockplus.org/30033574/diff/30033575/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/30033574/diff/30033575/test/browser/snippe... test/browser/snippets.js:114: await runSnippet(test, "abort-on-property-read", "Object.keys"); On 2019/03/29 15:58:02, hub wrote: > On 2019/03/29 15:46:27, Manish Jethani wrote: > > On 2019/03/29 14:36:27, hub wrote: > > > On 2019/03/29 07:08:07, Manish Jethani wrote: > > > > In the interest of completeness we could add the following tests: > > > > > > > > [...] > > > > > > for the record, test 8 and 9 already pass. But that's fine. More coverage. > > > > The reason I thought we should add these tests is that it makes it clear to > > anyone else who comes by that this works with arrow functions and classes. It > > may not be immediately obvious to someone who is not a JS expert that a class > is > > in fact just a function, for example (esp. if they are from a C++ background). > > This makes the code friendlier to everyone. > > I was not arguing against. I was just indicating that they already worked. The > more. The merrier. Acknowledged. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
