Issue 29995559: Issue 7236 - Handle sub properties in abort-on-property snippets (Closed)
|
Created:
Feb. 1, 2019, 8:14 p.m. by hub Modified:
March 20, 2019, 4:54 p.m. Reviewers:
Manish Jethani Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionIssue 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 #
MessagesTotal messages: 57
The test fail on Firefox for some reason I need to figure out. Also the tests trigger a bug in the WebDriver runner (ie when you have more than one test) that I will need to figure out as well.
On 2019/02/01 20:16:54, hub wrote: > The test fail on Firefox for some reason I need to figure out. > > Also the tests trigger a bug in the WebDriver runner (ie when you have more than > one test) that I will need to figure out as well. Maybe we should keep this patch about the sub-properties and move the tests to another patch? Because I see that as a separate issue, not related to this particular feature of wrapping sub-properties.
https://codereview.adblockplus.org/29995559/diff/29995560/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/29995560/lib/content/snippet... lib/content/snippets.js:714: let prop = property.slice(0, dot); We could just call this `name` instead of `prop`. https://codereview.adblockplus.org/29995559/diff/29995560/lib/content/snippet... lib/content/snippets.js:717: if (typeof value != "undefined") What if `value` is `null`? I think we want to check `value != null`. https://codereview.adblockplus.org/29995559/diff/29995560/lib/content/snippet... lib/content/snippets.js:731: if (a instanceof Object) This check is inconsistent with the one above. I think we can just check `a != null`. All other values will get coerced to object. Oh wait, but `Object.defineProperty` will fail. Well then let's just check for `a && typeof a == "object"`? https://codereview.adblockplus.org/29995559/diff/29995560/lib/content/snippet... lib/content/snippets.js:734: setter[magic] = undefined; If we just want to tag the setter, I think it's better to set it to `null` rather than `undefined`.
On 2019/02/03 14:30:23, Manish Jethani wrote: > On 2019/02/01 20:16:54, hub wrote: > > The test fail on Firefox for some reason I need to figure out. > > > > Also the tests trigger a bug in the WebDriver runner (ie when you have more > than > > one test) that I will need to figure out as well. > > Maybe we should keep this patch about the sub-properties and move the tests to > another patch? Because I see that as a separate issue, not related to this > particular feature of wrapping sub-properties. 1. There is one bug in the runner that I should fix for the tests to work normally and for which I'll file an issue. 2. There is an actual test failure on Firefox that should be fixed as it is an indication it doesn't work there. I think the tests should be included, but with 1. we might want to delay checking the patch in. In any case 2. should be addressed here.
patch updated. (test failure on Firefox not yet addressed). https://codereview.adblockplus.org/29995559/diff/29995560/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/29995560/lib/content/snippet... lib/content/snippets.js:714: let prop = property.slice(0, dot); On 2019/02/03 14:30:38, Manish Jethani wrote: > We could just call this `name` instead of `prop`. Done. https://codereview.adblockplus.org/29995559/diff/29995560/lib/content/snippet... lib/content/snippets.js:717: if (typeof value != "undefined") On 2019/02/03 14:30:38, Manish Jethani wrote: > What if `value` is `null`? I think we want to check `value != null`. Actually I think we want `value intanceof Object` here. Fixing the inconsistency with below. https://codereview.adblockplus.org/29995559/diff/29995560/lib/content/snippet... lib/content/snippets.js:731: if (a instanceof Object) On 2019/02/03 14:30:38, Manish Jethani wrote: > This check is inconsistent with the one above. I think we can just check `a != > null`. All other values will get coerced to object. > > Oh wait, but `Object.defineProperty` will fail. Well then let's just check for > `a && typeof a == "object"`? `a instanceof Object` is false if a is null. Unlike `typeof a` that will be `object` even for null. https://codereview.adblockplus.org/29995559/diff/29995560/lib/content/snippet... lib/content/snippets.js:734: setter[magic] = undefined; On 2019/02/03 14:30:38, Manish Jethani wrote: > If we just want to tag the setter, I think it's better to set it to `null` > rather than `undefined`. undefined is good. We check it with hasOwnProperty().
https://codereview.adblockplus.org/29995559/diff/29995560/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/29995560/lib/content/snippet... lib/content/snippets.js:731: if (a instanceof Object) On 2019/02/04 15:48:02, hub wrote: > On 2019/02/03 14:30:38, Manish Jethani wrote: > > This check is inconsistent with the one above. I think we can just check `a != > > null`. All other values will get coerced to object. > > > > Oh wait, but `Object.defineProperty` will fail. Well then let's just check for > > `a && typeof a == "object"`? > > `a instanceof Object` is false if a is null. Unlike `typeof a` that will be > `object` even for null. We've been here before: https://hg.adblockplus.org/adblockpluscore/file/91cd83c4c272/lib/content/snip... instanceof is more expensive which is why we avoid it in Core. It doesn't matter for snippets, but for consistency I think we can continue using `a && typeof a == "object"`.
Updated patch https://codereview.adblockplus.org/29995559/diff/29995560/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/29995560/lib/content/snippet... lib/content/snippets.js:731: if (a instanceof Object) On 2019/02/04 17:56:33, Manish Jethani wrote: > On 2019/02/04 15:48:02, hub wrote: > > On 2019/02/03 14:30:38, Manish Jethani wrote: > > > This check is inconsistent with the one above. I think we can just check `a > != > > > null`. All other values will get coerced to object. > > > > > > Oh wait, but `Object.defineProperty` will fail. Well then let's just check > for > > > `a && typeof a == "object"`? > > > > `a instanceof Object` is false if a is null. Unlike `typeof a` that will be > > `object` even for null. > > We've been here before: > > https://hg.adblockplus.org/adblockpluscore/file/91cd83c4c272/lib/content/snip... > > instanceof is more expensive which is why we avoid it in Core. It doesn't matter > for snippets, but for consistency I think we can continue using `a && typeof a > == "object"`. So be it. And this actually make the test pass on Firefox.
https://codereview.adblockplus.org/29995559/diff/29998567/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/29998567/lib/content/snippet... lib/content/snippets.js:718: return wrapPropertyAccess(value, property, descriptor); Don't we have to pass along magic here? https://codereview.adblockplus.org/29995559/diff/29998567/lib/content/snippet... lib/content/snippets.js:722: magic && currentDescriptor.set.hasOwnProperty(magic)) Here check for magic being truthy seems redundant (unless it's for performance reasons). `currentDescriptor.set.hasOwnProperty(magic)` will be `false` if `magic` is falsy. https://codereview.adblockplus.org/29995559/diff/29998567/lib/content/snippet... lib/content/snippets.js:736: } What about the return value here?
updated patch https://codereview.adblockplus.org/29995559/diff/29998567/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/29998567/lib/content/snippet... lib/content/snippets.js:718: return wrapPropertyAccess(value, property, descriptor); On 2019/02/05 14:47:17, Manish Jethani wrote: > Don't we have to pass along magic here? Yes we do. Done. https://codereview.adblockplus.org/29995559/diff/29998567/lib/content/snippet... lib/content/snippets.js:722: magic && currentDescriptor.set.hasOwnProperty(magic)) On 2019/02/05 14:47:17, Manish Jethani wrote: > Here check for magic being truthy seems redundant (unless it's for performance > reasons). `currentDescriptor.set.hasOwnProperty(magic)` will be `false` if > `magic` is falsy. might as well remove it. It's a corner case anyway. https://codereview.adblockplus.org/29995559/diff/29998567/lib/content/snippet... lib/content/snippets.js:736: } On 2019/02/05 14:47:17, Manish Jethani wrote: > What about the return value here? Done.
https://codereview.adblockplus.org/29995559/diff/29999574/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/29999574/lib/content/snippet... lib/content/snippets.js:735: Object.defineProperty(object, name, {get: () => v, set: setter}); What if object.name is non-configurable? Similar to the start of this function, I think we should check for that and return false.
updated patch https://codereview.adblockplus.org/29995559/diff/29999574/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/29999574/lib/content/snippet... lib/content/snippets.js:735: Object.defineProperty(object, name, {get: () => v, set: setter}); On 2019/02/05 15:39:08, Manish Jethani wrote: > What if object.name is non-configurable? Similar to the start of this function, > I think we should check for that and return false. Exception will be thrown. I'll address this.
https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippet... lib/content/snippets.js:736: setter[magic] = undefined; I do not quite understand why this tag is necessary. Can you give an example of a case where this would help?
https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippet... lib/content/snippets.js:736: setter[magic] = undefined; On 2019/02/08 10:49:48, Manish Jethani wrote: > I do not quite understand why this tag is necessary. Can you give an example of > a case where this would help? See above line 723 where we check if it's here and in that case move on.
https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippet... lib/content/snippets.js:736: setter[magic] = undefined; On 2019/02/08 14:14:03, hub wrote: > On 2019/02/08 10:49:48, Manish Jethani wrote: > > I do not quite understand why this tag is necessary. Can you give an example > of > > a case where this would help? > > See above line 723 where we check if it's here and in that case move on. Yes, but my question is more about why we need the tag at all. i.e. what if we didn't tag it? Would it break something?
https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippet... lib/content/snippets.js:736: setter[magic] = undefined; On 2019/02/12 16:12:38, Manish Jethani wrote: > On 2019/02/08 14:14:03, hub wrote: > > On 2019/02/08 10:49:48, Manish Jethani wrote: > > > I do not quite understand why this tag is necessary. Can you give an example > > of > > > a case where this would help? > > > > See above line 723 where we check if it's here and in that case move on. > > Yes, but my question is more about why we need the tag at all. i.e. what if we > didn't tag it? Would it break something? so we don't try to set it again if it has already been, in case of recursion. better be safe than sorry.
https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippet... lib/content/snippets.js:736: setter[magic] = undefined; On 2019/02/12 18:11:28, hub wrote: > On 2019/02/12 16:12:38, Manish Jethani wrote: > > On 2019/02/08 14:14:03, hub wrote: > > > On 2019/02/08 10:49:48, Manish Jethani wrote: > > > > I do not quite understand why this tag is necessary. Can you give an > example > > > of > > > > a case where this would help? > > > > > > See above line 723 where we check if it's here and in that case move on. > > > > Yes, but my question is more about why we need the tag at all. i.e. what if we > > didn't tag it? Would it break something? > > so we don't try to set it again if it has already been, in case of recursion. > better be safe than sorry. Well in any case it's redundant because configurable will be false for our own descriptor. There's no need to set the magic property, nor to check for it.
https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippet... lib/content/snippets.js:736: setter[magic] = undefined; > Well in any case it's redundant because configurable will be false for our own > descriptor. There's no need to set the magic property, nor to check for it. How do we know we are the one who set it?
https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippet... lib/content/snippets.js:736: setter[magic] = undefined; On 2019/02/19 15:36:48, hub wrote: > > > Well in any case it's redundant because configurable will be false for our own > > descriptor. There's no need to set the magic property, nor to check for it. > > How do we know we are the one who set it? We don't know, but if it's not configurable we can't set it anyway. (The code will never reach the point where we check for the magic property.)
https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30000591/lib/content/snippet... lib/content/snippets.js:736: setter[magic] = undefined; On 2019/02/19 18:13:07, Manish Jethani wrote: > On 2019/02/19 15:36:48, hub wrote: > > > > > Well in any case it's redundant because configurable will be false for our > own > > > descriptor. There's no need to set the magic property, nor to check for it. > > > > How do we know we are the one who set it? > > We don't know, but if it's not configurable we can't set it anyway. (The code > will never reach the point where we check for the magic property.) Done.
https://codereview.adblockplus.org/29995559/diff/30011566/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30011566/lib/content/snippet... lib/content/snippets.js:724: let v; There are two things missing in this implementation: 1. If `typeof value` is not object (let's say it's number), the getter below will return undefined instead of the value 2. When wrapping `foo.bar.lambda`, if `foo.bar` exists and is an object, and if `foo.bar` is reassigned a different object, the wrapper is gone; however this is not the case if the `foo.bar` does not exist initially (asymmetry)
https://codereview.adblockplus.org/29995559/diff/30011566/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30011566/lib/content/snippet... lib/content/snippets.js:724: let v; On 2019/02/20 06:20:46, Manish Jethani wrote: > There are two things missing in this implementation: Let me elaborate a little ... > 1. If `typeof value` is not object (let's say it's number), the getter below > will return undefined instead of the value Suppose we are wrapping `foo.bar.lambda` but only `foo.bar` exists and the value is 4. Our getter for `foo.bar` returns `undefined`, the original value 4 is lost. > 2. When wrapping `foo.bar.lambda`, if `foo.bar` exists and is an object, and > if `foo.bar` is reassigned a different object, the wrapper is gone; however this > is not the case if the `foo.bar` does not exist initially (asymmetry) Once again, suppose we are wrapping `foo.bar.lambda`. There are two scenarios (asymmetry): Scenario 1: `foo.bar` does not exist. In this case, when `foo.bar` is assigned a value that is an object, the setter ensures that `foo.bar.lambda` is wrapped. Scenario 2: `foo.bar` exists and is an object. In this case, when `foo.bar.lambda` is wrapped; however, unlike the previous case, if `foo.bar` is assigned a different object (i.e. `foo.bar = {}`), the new `foo.bar.lambda` is not wrapped. In the previous case, no matter how many different objects are assigned to `foo.bar`, each new assignment is always wrapped. So these asymmetries and inconsistencies is something we could address. I don't know if it means much in practice, at least for now, but in the long run it's always better to have a robust implementation.
updated patch https://codereview.adblockplus.org/29995559/diff/30011566/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30011566/lib/content/snippet... lib/content/snippets.js:724: let v; On 2019/02/20 06:28:53, Manish Jethani wrote: > So these asymmetries and inconsistencies is something we could address. I don't > know if it means much in practice, at least for now, but in the long run it's > always better to have a robust implementation. Fixed them.
Sorry I have not had time to look at this but I'll get to it on Monday.
I'd like to experiment with this code a little bit, but in the meanwhile a couple of comments ... https://codereview.adblockplus.org/29995559/diff/30014568/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30014568/lib/content/snippet... lib/content/snippets.js:723: return result; If I understand this correctly, we return the result of the leaf property. i.e. in `foo.bar.lambda`, if `foo.bar = {}`, we return whether we wrapped `lambda`. However if `foo.bar` is not an object, then we basically always return `true` - even if the descriptor for `foo.bar` is non-configurable. For example: window.foo = {}; Object.defineProperty(window.foo, "bar", {value: "fixed value"}); Now wrapping `foo.bar.lambda` will return `true`. Right? This is another inconsistency. I'm wondering if we really need a return value? We could always just override the `onerror` handler in the calling code, it shouldn't matter if it is wrapped multiple times. https://codereview.adblockplus.org/29995559/diff/30014568/lib/content/snippet... lib/content/snippets.js:725: let setter = a => Nit: Instead of `a` this would seem better as `newValue`.
updated patch https://codereview.adblockplus.org/29995559/diff/30014568/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30014568/lib/content/snippet... lib/content/snippets.js:723: return result; On 2019/03/04 13:28:49, Manish Jethani wrote: > If I understand this correctly, we return the result of the leaf property. i.e. > in `foo.bar.lambda`, if `foo.bar = {}`, we return whether we wrapped `lambda`. > However if `foo.bar` is not an object, then we basically always return `true` - > even if the descriptor for `foo.bar` is non-configurable. > > For example: > > window.foo = {}; > Object.defineProperty(window.foo, "bar", {value: "fixed value"}); > > Now wrapping `foo.bar.lambda` will return `true`. Right? > > This is another inconsistency. I'm wondering if we really need a return value? > We could always just override the `onerror` handler in the calling code, it > shouldn't matter if it is wrapped multiple times. I should have set `result` to `false` (line 714). Fixing this. https://codereview.adblockplus.org/29995559/diff/30014568/lib/content/snippet... lib/content/snippets.js:725: let setter = a => On 2019/03/04 13:28:49, Manish Jethani wrote: > Nit: Instead of `a` this would seem better as `newValue`. Done.
https://codereview.adblockplus.org/29995559/diff/30014568/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30014568/lib/content/snippet... lib/content/snippets.js:723: return result; On 2019/03/05 13:49:13, hub wrote: > On 2019/03/04 13:28:49, Manish Jethani wrote: > > If I understand this correctly, we return the result of the leaf property. > i.e. > > in `foo.bar.lambda`, if `foo.bar = {}`, we return whether we wrapped `lambda`. > > However if `foo.bar` is not an object, then we basically always return `true` > - > > even if the descriptor for `foo.bar` is non-configurable. > > > > For example: > > > > window.foo = {}; > > Object.defineProperty(window.foo, "bar", {value: "fixed value"}); > > > > Now wrapping `foo.bar.lambda` will return `true`. Right? > > > > This is another inconsistency. I'm wondering if we really need a return value? > > We could always just override the `onerror` handler in the calling code, it > > shouldn't matter if it is wrapped multiple times. > > I should have set `result` to `false` (line 714). Fixing this. This has the opposite problem though. If we're trying to wrap `foo.bar.lambda` and `foo` exists but `foo.bar` does not exist, it will return `false`; but later when `foo.bar` is set, and then `foo.bar.lambda` is set, then `foo.bar.lambda` will be wrapped. But because we returned `false` initially there will be no `onerror` handler. It's hard to do this right. How about we just always add an `onerror` handler and return neither `true` nor `false` from this function?
updated patch https://codereview.adblockplus.org/29995559/diff/30014568/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30014568/lib/content/snippet... lib/content/snippets.js:723: return result; On 2019/03/06 19:07:12, Manish Jethani wrote: > On 2019/03/05 13:49:13, hub wrote: > > On 2019/03/04 13:28:49, Manish Jethani wrote: > > > If I understand this correctly, we return the result of the leaf property. > > i.e. > > > in `foo.bar.lambda`, if `foo.bar = {}`, we return whether we wrapped > `lambda`. > > > However if `foo.bar` is not an object, then we basically always return > `true` > > - > > > even if the descriptor for `foo.bar` is non-configurable. > > > > > > For example: > > > > > > window.foo = {}; > > > Object.defineProperty(window.foo, "bar", {value: "fixed value"}); > > > > > > Now wrapping `foo.bar.lambda` will return `true`. Right? > > > > > > This is another inconsistency. I'm wondering if we really need a return > value? > > > We could always just override the `onerror` handler in the calling code, it > > > shouldn't matter if it is wrapped multiple times. > > > > I should have set `result` to `false` (line 714). Fixing this. > > This has the opposite problem though. If we're trying to wrap `foo.bar.lambda` > and `foo` exists but `foo.bar` does not exist, it will return `false`; but later > when `foo.bar` is set, and then `foo.bar.lambda` is set, then `foo.bar.lambda` > will be wrapped. But because we returned `false` initially there will be no > `onerror` handler. > > It's hard to do this right. How about we just always add an `onerror` handler > and return neither `true` nor `false` from this function? Silly me, I should just return true at then end since we did define the property. I'm ok to make the creation of the onerror handler unconditional.
Aside from the nits the change in lib/ looks good to me. I'll take a look at the test/ changes next. https://codereview.adblockplus.org/29995559/diff/30024564/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30024564/lib/content/snippet... lib/content/snippets.js:703: let dot = property.indexOf("."); Nit: When finding the index of a dot in a string (domain names), we have used the name `dotIndex` in other places. Would be nice to be consistent, but up to you. https://codereview.adblockplus.org/29995559/diff/30024564/lib/content/snippet... lib/content/snippets.js:706: // simple property case. Nit: I don't know if this comment adds much value, but it's up to you. https://codereview.adblockplus.org/29995559/diff/30024564/lib/content/snippet... lib/content/snippets.js:714: let name = property.slice(0, dot); Nit: A blank line after the `if` block would make it more readable IMO. https://codereview.adblockplus.org/29995559/diff/30024564/lib/content/snippet... lib/content/snippets.js:717: if (value && typeof value == "object") Suggestion: It might be better if we defined this formally as: let isObject = value => value && typeof value == "object"; But it's your call, I'm not too keen on this either. https://codereview.adblockplus.org/29995559/diff/30024564/lib/content/snippet... lib/content/snippets.js:730: Object.defineProperty(object, name, {get: () => value, set: setter}); Nit: Blank line before this would be nice.
updated patch https://codereview.adblockplus.org/29995559/diff/30024564/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30024564/lib/content/snippet... lib/content/snippets.js:703: let dot = property.indexOf("."); On 2019/03/07 14:12:22, Manish Jethani wrote: > Nit: When finding the index of a dot in a string (domain names), we have used > the name `dotIndex` in other places. Would be nice to be consistent, but up to > you. Done. https://codereview.adblockplus.org/29995559/diff/30024564/lib/content/snippet... lib/content/snippets.js:706: // simple property case. On 2019/03/07 14:12:22, Manish Jethani wrote: > Nit: I don't know if this comment adds much value, but it's up to you. Acknowledged. https://codereview.adblockplus.org/29995559/diff/30024564/lib/content/snippet... lib/content/snippets.js:714: let name = property.slice(0, dot); On 2019/03/07 14:12:22, Manish Jethani wrote: > Nit: A blank line after the `if` block would make it more readable IMO. Done. https://codereview.adblockplus.org/29995559/diff/30024564/lib/content/snippet... lib/content/snippets.js:717: if (value && typeof value == "object") On 2019/03/07 14:12:22, Manish Jethani wrote: > Suggestion: It might be better if we defined this formally as: > > let isObject = value => value && typeof value == "object"; > > But it's your call, I'm not too keen on this either. I'll leave it as is https://codereview.adblockplus.org/29995559/diff/30024564/lib/content/snippet... lib/content/snippets.js:730: Object.defineProperty(object, name, {get: () => value, set: setter}); On 2019/03/07 14:12:23, Manish Jethani wrote: > Nit: Blank line before this would be nice. Done.
Some comments ... (By the way, we could split this into two patches, a separate one for the test, since there are some things to discuss about it.) https://codereview.adblockplus.org/29995559/diff/30024564/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30024564/lib/content/snippet... lib/content/snippets.js:717: if (value && typeof value == "object") On 2019/03/07 21:32:27, hub wrote: > On 2019/03/07 14:12:22, Manish Jethani wrote: > > Suggestion: It might be better if we defined this formally as: > > > > let isObject = value => value && typeof value == "object"; > > > > But it's your call, I'm not too keen on this either. > > I'll leave it as is Acknowledged. https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippe... test/browser/snippets.js:23: window.browser = Nit: The normal style for object literals we use is to place the opening brace on the same line. https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippe... test/browser/snippets.js:37: testDocument = iframe.contentDocument; Where is `testDocument` being used in this file? We should create the same conditions in which the snippet will run in the extension. Ideally this would be a top-level frame that is created and destroyed for each test, but an inner frame would be OK too (for most snippets). https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippe... test/browser/snippets.js:53: return new Promise((resolve, reject) => We don't need `reject` here, and we could also lose the parentheses. https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippe... test/browser/snippets.js:55: window.setTimeout(resolve, delay); `setTimeout()` is a global function also available on contexts that do not have a `window` object (e.g. Node.js). We should just call it directly. PS: What about placing this function in a common place so it can be shared with `browser/tests/elemHideEmulation.js`? https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippe... test/browser/snippets.js:59: async function runSnippet(test, snippetName, ...args) We could rename this to `runScript(test, script)` and use the `parseScript()` function from `lib/snippets.js` to get the commands and arguments; then we simply iterate over the commands and execute each one with its arguments.
Since I am just testing this snippet here, I don't think it is worth splitting the tests out. https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippe... test/browser/snippets.js:23: window.browser = On 2019/03/12 07:00:09, Manish Jethani wrote: > Nit: The normal style for object literals we use is to place the opening brace > on the same line. Done. https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippe... test/browser/snippets.js:37: testDocument = iframe.contentDocument; On 2019/03/12 07:00:09, Manish Jethani wrote: > Where is `testDocument` being used in this file? > > We should create the same conditions in which the snippet will run in the > extension. Ideally this would be a top-level frame that is created and destroyed > for each test, but an inner frame would be OK too (for most snippets). Used Line 44. Declared line 31. This is the exact same thing as in `test/browser/elementHidingEmulation.js` We create a document in an iframe on setUp and remove it on tearDown https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippe... test/browser/snippets.js:53: return new Promise((resolve, reject) => On 2019/03/12 07:00:09, Manish Jethani wrote: > We don't need `reject` here, and we could also lose the parentheses. tbh this is a cut&paste from `test/browser/elementHidingEmulation.js` and it has the same. The original source. https://codereview.adblockplus.org/29494577/diff/29523754/test/browser/elemHi... Will fix here. https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippe... test/browser/snippets.js:55: window.setTimeout(resolve, delay); On 2019/03/12 07:00:09, Manish Jethani wrote: > `setTimeout()` is a global function also available on contexts that do not have > a `window` object (e.g. Node.js). We should just call it directly. This is always run in the browser context. But see above. > PS: What about placing this function in a common place so it can be shared with > `browser/tests/elemHideEmulation.js`? Done https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippe... test/browser/snippets.js:59: async function runSnippet(test, snippetName, ...args) On 2019/03/12 07:00:09, Manish Jethani wrote: > We could rename this to `runScript(test, script)` and use the `parseScript()` > function from `lib/snippets.js` to get the commands and arguments; then we > simply iterate over the commands and execute each one with its arguments. The goal was to individually test the snippet, simply. Not to verify the injection and parsing.
https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippe... test/browser/snippets.js:23: window.browser = On 2019/03/12 14:29:17, hub wrote: > On 2019/03/12 07:00:09, Manish Jethani wrote: > > Nit: The normal style for object literals we use is to place the opening brace > > on the same line. > > Done. The same would apply to the nested `runtime` property. https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippe... test/browser/snippets.js:37: testDocument = iframe.contentDocument; On 2019/03/12 14:29:18, hub wrote: > On 2019/03/12 07:00:09, Manish Jethani wrote: > > Where is `testDocument` being used in this file? > > > > We should create the same conditions in which the snippet will run in the > > extension. Ideally this would be a top-level frame that is created and > destroyed > > for each test, but an inner frame would be OK too (for most snippets). > > Used Line 44. Declared line 31. > > This is the exact same thing as in `test/browser/elementHidingEmulation.js` Yes, but over there we have this line: elemHideEmulation.document = testDocument; The document used by the element hiding emulation code is the test document. Over here, are we using the test document at all? I don't see it. https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippe... test/browser/snippets.js:53: return new Promise((resolve, reject) => On 2019/03/12 14:29:17, hub wrote: > On 2019/03/12 07:00:09, Manish Jethani wrote: > > We don't need `reject` here, and we could also lose the parentheses. > > tbh this is a cut&paste from `test/browser/elementHidingEmulation.js` and it has > the same. > > The original source. > https://codereview.adblockplus.org/29494577/diff/29523754/test/browser/elemHi... > > Will fix here. Acknowledged. https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippe... test/browser/snippets.js:55: window.setTimeout(resolve, delay); On 2019/03/12 14:29:17, hub wrote: > On 2019/03/12 07:00:09, Manish Jethani wrote: > > `setTimeout()` is a global function also available on contexts that do not > have > > a `window` object (e.g. Node.js). We should just call it directly. > > This is always run in the browser context. But see above. > > > PS: What about placing this function in a common place so it can be shared > with > > `browser/tests/elemHideEmulation.js`? > > Done Acknowledged. https://codereview.adblockplus.org/29995559/diff/30026555/test/browser/_utils.js File test/browser/_utils.js (right): https://codereview.adblockplus.org/29995559/diff/30026555/test/browser/_utils... test/browser/_utils.js:28: module.exports.timeout = timeout; Why is this `module.exports` and not just `exports`?
updated patch https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippe... test/browser/snippets.js:23: window.browser = On 2019/03/12 14:48:49, Manish Jethani wrote: > On 2019/03/12 14:29:17, hub wrote: > > On 2019/03/12 07:00:09, Manish Jethani wrote: > > > Nit: The normal style for object literals we use is to place the opening > brace > > > on the same line. > > > > Done. > > The same would apply to the nested `runtime` property. I wonder why the linter is silent. Done https://codereview.adblockplus.org/29995559/diff/30025582/test/browser/snippe... test/browser/snippets.js:37: testDocument = iframe.contentDocument; On 2019/03/12 14:48:49, Manish Jethani wrote: > On 2019/03/12 14:29:18, hub wrote: > > On 2019/03/12 07:00:09, Manish Jethani wrote: > > > Where is `testDocument` being used in this file? > > > > > > We should create the same conditions in which the snippet will run in the > > > extension. Ideally this would be a top-level frame that is created and > > destroyed > > > for each test, but an inner frame would be OK too (for most snippets). > > > > Used Line 44. Declared line 31. > > > > This is the exact same thing as in `test/browser/elementHidingEmulation.js` > > Yes, but over there we have this line: > > elemHideEmulation.document = testDocument; > > The document used by the element hiding emulation code is the test document. > Over here, are we using the test document at all? I don't see it. On second thought, we don't need this iframe either. https://codereview.adblockplus.org/29995559/diff/30026555/test/browser/_utils.js File test/browser/_utils.js (right): https://codereview.adblockplus.org/29995559/diff/30026555/test/browser/_utils... test/browser/_utils.js:28: module.exports.timeout = timeout; On 2019/03/12 14:48:50, Manish Jethani wrote: > Why is this `module.exports` and not just `exports`? Either are equivalent since they reference the same empty object. will go to the shorter form.
https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... test/browser/snippets.js:37: await timeout(100); Do we really need this timeout here? https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... test/browser/snippets.js:40: exports.testAbortPropertyReadSnippet = async function(test) Nit: We should maybe name this `testAbortOnPropertyReadSnippet`. https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... test/browser/snippets.js:44: async function testProperty(property, result = true) This function doesn't have to be async from what I can tell. https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... test/browser/snippets.js:46: let properties = property.split("."); Nit: We could call this `path` instead of `properties` as we have done in `lib/content/snippets.js`. Your call. https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... test/browser/snippets.js:47: test.ok(properties.length >= 1); This seems redundant. It is not possible for this check to fail. https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... test/browser/snippets.js:51: try Nit: Would be cleaner to read if a line was left before and after the try/catch. https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... test/browser/snippets.js:63: test.equal(exceptionCaught, result, `The property "${property}" didn't trigger and exception.`); The message here doesn't make complete sense. What if `result` is `false` and an exception _is_ triggered? https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... test/browser/snippets.js:64: test.equal(value, result ? 1 : undefined, `The value for "${property}" shouldn't have been read.`); Same as above. Both of these cases assume `result` is `true`.
updated patch https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... test/browser/snippets.js:37: await timeout(100); On 2019/03/13 16:36:14, Manish Jethani wrote: > Do we really need this timeout here? Without it, the test fail. With a 0 timeout, it fails just on Chrome. https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... test/browser/snippets.js:40: exports.testAbortPropertyReadSnippet = async function(test) On 2019/03/13 16:36:14, Manish Jethani wrote: > Nit: We should maybe name this `testAbortOnPropertyReadSnippet`. Done. https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... test/browser/snippets.js:44: async function testProperty(property, result = true) On 2019/03/13 16:36:14, Manish Jethani wrote: > This function doesn't have to be async from what I can tell. Done. https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... test/browser/snippets.js:46: let properties = property.split("."); On 2019/03/13 16:36:15, Manish Jethani wrote: > Nit: We could call this `path` instead of `properties` as we have done in > `lib/content/snippets.js`. Your call. Done. https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... test/browser/snippets.js:47: test.ok(properties.length >= 1); On 2019/03/13 16:36:15, Manish Jethani wrote: > This seems redundant. It is not possible for this check to fail. Done. https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... test/browser/snippets.js:51: try On 2019/03/13 16:36:14, Manish Jethani wrote: > Nit: Would be cleaner to read if a line was left before and after the try/catch. Done. https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... test/browser/snippets.js:63: test.equal(exceptionCaught, result, `The property "${property}" didn't trigger and exception.`); On 2019/03/13 16:36:15, Manish Jethani wrote: > The message here doesn't make complete sense. What if `result` is `false` and an > exception _is_ triggered? Done. https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... test/browser/snippets.js:64: test.equal(value, result ? 1 : undefined, `The value for "${property}" shouldn't have been read.`); On 2019/03/13 16:36:15, Manish Jethani wrote: > Same as above. Both of these cases assume `result` is `true`. Done.
https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... test/browser/snippets.js:60: if (properties.length == 0) Will properties.length every be non-zero here? https://codereview.adblockplus.org/29995559/diff/30027555/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30027555/test/browser/snippe... test/browser/snippets.js:53: let obj = window; What about just doing it exactly like in `lib/content/snippets.js`? let object = window; let path = api.split("."); let name = path.pop(); for (let node of path) object = object[node]; value = object[name]; https://codereview.adblockplus.org/29995559/diff/30027555/test/browser/snippe... test/browser/snippets.js:60: if (path.length == 0) Will `path.length` ever be non-zero here? https://codereview.adblockplus.org/29995559/diff/30027555/test/browser/snippe... test/browser/snippets.js:64: test.equal(exceptionCaught, result, `The property "${property}" ${result ? "didn't" : "did"} trigger an exception.`); Nit: We could use "should" here for consistency: "The property ${property} ${result ? "should" : "shouldn't"} have triggered an exception." https://codereview.adblockplus.org/29995559/diff/30027555/test/browser/snippe... test/browser/snippets.js:100: Unnecessary blank line at the end.
https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... test/browser/snippets.js:37: await timeout(100); On 2019/03/14 10:28:05, hub wrote: > On 2019/03/13 16:36:14, Manish Jethani wrote: > > Do we really need this timeout here? > > Without it, the test fail. With a 0 timeout, it fails just on Chrome. Why is this though? Is it because the `<script>` element needs to be injected?
updated patch https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... test/browser/snippets.js:37: await timeout(100); On 2019/03/14 11:32:08, Manish Jethani wrote: > On 2019/03/14 10:28:05, hub wrote: > > On 2019/03/13 16:36:14, Manish Jethani wrote: > > > Do we really need this timeout here? > > > > Without it, the test fail. With a 0 timeout, it fails just on Chrome. > > Why is this though? Is it because the `<script>` element needs to be injected? that's what I think. https://codereview.adblockplus.org/29995559/diff/30027555/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30027555/test/browser/snippe... test/browser/snippets.js:53: let obj = window; On 2019/03/14 10:58:13, Manish Jethani wrote: > What about just doing it exactly like in `lib/content/snippets.js`? > > let object = window; > let path = api.split("."); > let name = path.pop(); > > for (let node of path) > object = object[node]; > > value = object[name]; I don't think it really matters either way. https://codereview.adblockplus.org/29995559/diff/30027555/test/browser/snippe... test/browser/snippets.js:60: if (path.length == 0) On 2019/03/14 10:58:13, Manish Jethani wrote: > Will `path.length` ever be non-zero here? if any other exception, it could. https://codereview.adblockplus.org/29995559/diff/30027555/test/browser/snippe... test/browser/snippets.js:64: test.equal(exceptionCaught, result, `The property "${property}" ${result ? "didn't" : "did"} trigger an exception.`); On 2019/03/14 10:58:13, Manish Jethani wrote: > Nit: We could use "should" here for consistency: "The property ${property} > ${result ? "should" : "shouldn't"} have triggered an exception." Done. https://codereview.adblockplus.org/29995559/diff/30027555/test/browser/snippe... test/browser/snippets.js:100: On 2019/03/14 10:58:13, Manish Jethani wrote: > Unnecessary blank line at the end. Done.
https://codereview.adblockplus.org/29995559/diff/30027555/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30027555/test/browser/snippe... test/browser/snippets.js:53: let obj = window; On 2019/03/14 12:32:55, hub wrote: > On 2019/03/14 10:58:13, Manish Jethani wrote: > > What about just doing it exactly like in `lib/content/snippets.js`? > > > > let object = window; > > let path = api.split("."); > > let name = path.pop(); > > > > for (let node of path) > > object = object[node]; > > > > value = object[name]; > > I don't think it really matters either way. Alright. https://codereview.adblockplus.org/29995559/diff/30027555/test/browser/snippe... test/browser/snippets.js:60: if (path.length == 0) On 2019/03/14 12:32:55, hub wrote: > On 2019/03/14 10:58:13, Manish Jethani wrote: > > Will `path.length` ever be non-zero here? > > if any other exception, it could. And why would there be any other exception? Can you give an example? It seems unnecessary to me. https://codereview.adblockplus.org/29995559/diff/30027555/test/browser/snippe... test/browser/snippets.js:64: test.equal(exceptionCaught, result, `The property "${property}" ${result ? "didn't" : "did"} trigger an exception.`); On 2019/03/14 12:32:55, hub wrote: > On 2019/03/14 10:58:13, Manish Jethani wrote: > > Nit: We could use "should" here for consistency: "The property ${property} > > ${result ? "should" : "shouldn't"} have triggered an exception." > > Done. "should" should be first, then "shouldn't". https://codereview.adblockplus.org/29995559/diff/30027560/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30027560/test/browser/snippe... test/browser/snippets.js:92: window.foo2 = 5; This test just happens to pass because `testProperty()` throws a `TypeError` at just the right point. It's not the error thrown by the snippet, it's just that `bar2` is `undefined` so you can't access `lambda` anyway. It's a wrong test. After running the snippet, we should set `foo2` and `foo2.bar2` to objects.
updated patch https://codereview.adblockplus.org/29995559/diff/30027555/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30027555/test/browser/snippe... test/browser/snippets.js:60: if (path.length == 0) On 2019/03/14 16:56:22, Manish Jethani wrote: > On 2019/03/14 12:32:55, hub wrote: > > On 2019/03/14 10:58:13, Manish Jethani wrote: > > > Will `path.length` ever be non-zero here? > > > > if any other exception, it could. > > And why would there be any other exception? Can you give an example? It seems > unnecessary to me. it's gone then. https://codereview.adblockplus.org/29995559/diff/30027555/test/browser/snippe... test/browser/snippets.js:64: test.equal(exceptionCaught, result, `The property "${property}" ${result ? "didn't" : "did"} trigger an exception.`); On 2019/03/14 16:56:23, Manish Jethani wrote: > On 2019/03/14 12:32:55, hub wrote: > > On 2019/03/14 10:58:13, Manish Jethani wrote: > > > Nit: We could use "should" here for consistency: "The property ${property} > > > ${result ? "should" : "shouldn't"} have triggered an exception." > > > > Done. > > "should" should be first, then "shouldn't". my bad, fixed.
https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... test/browser/snippets.js:37: await timeout(100); On 2019/03/14 12:32:54, hub wrote: > On 2019/03/14 11:32:08, Manish Jethani wrote: > > On 2019/03/14 10:28:05, hub wrote: > > > On 2019/03/13 16:36:14, Manish Jethani wrote: > > > > Do we really need this timeout here? > > > > > > Without it, the test fail. With a 0 timeout, it fails just on Chrome. > > > > Why is this though? Is it because the `<script>` element needs to be injected? > > that's what I think. OK, I checked. It is indeed because the snippet returns before the code has executed. Let's add a comment here then. Something like: // For snippets that run in the context of the document via a <script> // element (i.e. snippets that use makeInjector()), we need to wait for // execution to be complete. https://codereview.adblockplus.org/29995559/diff/30027565/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30027565/test/browser/snippe... test/browser/snippets.js:63: test.equal(exceptionCaught, result, `The property "${property}" ${result ? "should" : "shouldn't"} trigger an exception.`); Nit: Let's try to reduce the number of columns as much as possible in new test files. In this case, the template string will spill over outside 80 columns but at least we can start it on the next line.
Updated patch https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30026560/test/browser/snippe... test/browser/snippets.js:37: await timeout(100); On 2019/03/16 10:50:24, Manish Jethani wrote: > On 2019/03/14 12:32:54, hub wrote: > > On 2019/03/14 11:32:08, Manish Jethani wrote: > > > On 2019/03/14 10:28:05, hub wrote: > > > > On 2019/03/13 16:36:14, Manish Jethani wrote: > > > > > Do we really need this timeout here? > > > > > > > > Without it, the test fail. With a 0 timeout, it fails just on Chrome. > > > > > > Why is this though? Is it because the `<script>` element needs to be > injected? > > > > that's what I think. > > OK, I checked. It is indeed because the snippet returns before the code has > executed. Let's add a comment here then. Something like: > > // For snippets that run in the context of the document via a <script> > // element (i.e. snippets that use makeInjector()), we need to wait for > // execution to be complete. Done. https://codereview.adblockplus.org/29995559/diff/30027565/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30027565/test/browser/snippe... test/browser/snippets.js:63: test.equal(exceptionCaught, result, `The property "${property}" ${result ? "should" : "shouldn't"} trigger an exception.`); On 2019/03/16 10:50:24, Manish Jethani wrote: > Nit: Let's try to reduce the number of columns as much as possible in new test > files. In this case, the template string will spill over outside 80 columns but > at least we can start it on the next line. Done.
https://codereview.adblockplus.org/29995559/diff/30027560/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30027560/test/browser/snippe... test/browser/snippets.js:92: window.foo2 = 5; On 2019/03/14 16:56:23, Manish Jethani wrote: > This test just happens to pass because `testProperty()` throws a `TypeError` at > just the right point. It's not the error thrown by the snippet, it's just that > `bar2` is `undefined` so you can't access `lambda` anyway. It's a wrong test. > > After running the snippet, we should set `foo2` and `foo2.bar2` to objects. What about this? https://codereview.adblockplus.org/29995559/diff/30028555/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30028555/test/browser/snippe... test/browser/snippets.js:37: // For snippets that run in the context of the document via a <script> Nit: Mind leaving a line before the comment? It's too cluttered, easy to miss relevant parts. https://codereview.adblockplus.org/29995559/diff/30028555/test/browser/snippe... test/browser/snippets.js:67: `The property "${property}" ${result ? "should" : "shouldn't"} trigger an exception.`); The opening tick of the template string should be aligned with the `e` of `exceptionCaught`, as per convention. Same on next line. Alternatively: test.equal( exceptionCaught, result, `The property ...` );
updated patch https://codereview.adblockplus.org/29995559/diff/30028555/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30028555/test/browser/snippe... test/browser/snippets.js:37: // For snippets that run in the context of the document via a <script> On 2019/03/19 07:44:00, Manish Jethani wrote: > Nit: Mind leaving a line before the comment? It's too cluttered, easy to miss > relevant parts. Done. https://codereview.adblockplus.org/29995559/diff/30028555/test/browser/snippe... test/browser/snippets.js:67: `The property "${property}" ${result ? "should" : "shouldn't"} trigger an exception.`); On 2019/03/19 07:44:00, Manish Jethani wrote: > The opening tick of the template string should be aligned with the `e` of > `exceptionCaught`, as per convention. > > Same on next line. > > Alternatively: > > test.equal( > exceptionCaught, > result, > `The property ...` > ); Done.
https://codereview.adblockplus.org/29995559/diff/30027560/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30027560/test/browser/snippe... test/browser/snippets.js:92: window.foo2 = 5; On 2019/03/19 07:43:59, Manish Jethani wrote: > On 2019/03/14 16:56:23, Manish Jethani wrote: > > This test just happens to pass because `testProperty()` throws a `TypeError` > at > > just the right point. It's not the error thrown by the snippet, it's just that > > `bar2` is `undefined` so you can't access `lambda` anyway. It's a wrong test. > > > > After running the snippet, we should set `foo2` and `foo2.bar2` to objects. > > What about this? Looks good now except for this. As I said, it just happens to work by accident.
updated patch https://codereview.adblockplus.org/29995559/diff/30027560/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30027560/test/browser/snippe... test/browser/snippets.js:92: window.foo2 = 5; On 2019/03/19 17:38:23, Manish Jethani wrote: > On 2019/03/19 07:43:59, Manish Jethani wrote: > > On 2019/03/14 16:56:23, Manish Jethani wrote: > > > This test just happens to pass because `testProperty()` throws a `TypeError` > > at > > > just the right point. It's not the error thrown by the snippet, it's just > that > > > `bar2` is `undefined` so you can't access `lambda` anyway. It's a wrong > test. > > > > > > After running the snippet, we should set `foo2` and `foo2.bar2` to objects. > > > > What about this? > > Looks good now except for this. As I said, it just happens to work by accident. Sorry I totally missed that one. Fixed.
https://codereview.adblockplus.org/29995559/diff/30029560/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30029560/test/browser/snippe... test/browser/snippets.js:46: window.abpTest = "foo"; This seems odd standing out on the top. Let's move this to right above its test (the first one). https://codereview.adblockplus.org/29995559/diff/30029560/test/browser/snippe... test/browser/snippets.js:64: test.equal(e.name, errorType); Nit: Since we're comparing with the `name` property of the `Error` instance, would be nice to name it `errorName` instead. https://codereview.adblockplus.org/29995559/diff/30029560/test/browser/snippe... test/browser/snippets.js:84: Nit: Blank line here seems unnecessary if we are grouping things together as in the following tests. https://codereview.adblockplus.org/29995559/diff/30029560/test/browser/snippe... test/browser/snippets.js:88: // Test that we try to catch a property that don't exist yet. Nit: s/don't/doesn't/ https://codereview.adblockplus.org/29995559/diff/30029560/test/browser/snippe... test/browser/snippets.js:96: // Test overwriting the object with another object Nit: Period at the end (I don't mind too much but consistency would be great). Also same below. https://codereview.adblockplus.org/29995559/diff/30029560/test/browser/snippe... test/browser/snippets.js:97: window.foo = {bar: {}}; There are some inconsistencies with the naming here. We start with `abpTest`, then `abpTest2`, `abpTest3`, then suddently switch to `foo`. I suggest we follow this convention: * First-level properties: abpTest, abpTest2, abpTest3, ... * Second-level properties: prop1, prop2, prop3, ... * Third-level properties: foo, bar, lambda * Value: Either 42 (number) or an object It's more or less like this anyway, just some inconsistencies. You could choose some other convention if you like, as long as it's consistent and easier for someone else to recognize and follow the same pattern. https://codereview.adblockplus.org/29995559/diff/30029560/test/browser/snippe... test/browser/snippets.js:107: // We expect that accessing the property cause a TypeError. Nit: s/cause/causes/ (FWIW I'm not sure this comment explains anything that isn't obvious from the code.) https://codereview.adblockplus.org/29995559/diff/30029560/test/browser/snippe... test/browser/snippets.js:110: window.foo2 = {bar2: {}}; Before this line, we could throw in another test: window.foo2 = {bar2: 42}; testProperty("foo2.bar2.lambda", false); In this case no error is thrown, but afterwards when replace `bar2` with a `{}` then in the next access we get an error.
updated patch https://codereview.adblockplus.org/29995559/diff/30029560/test/browser/snippe... File test/browser/snippets.js (right): https://codereview.adblockplus.org/29995559/diff/30029560/test/browser/snippe... test/browser/snippets.js:46: window.abpTest = "foo"; On 2019/03/20 01:42:00, Manish Jethani wrote: > This seems odd standing out on the top. Let's move this to right above its test > (the first one). Done. https://codereview.adblockplus.org/29995559/diff/30029560/test/browser/snippe... test/browser/snippets.js:64: test.equal(e.name, errorType); On 2019/03/20 01:42:01, Manish Jethani wrote: > Nit: Since we're comparing with the `name` property of the `Error` instance, > would be nice to name it `errorName` instead. Done. https://codereview.adblockplus.org/29995559/diff/30029560/test/browser/snippe... test/browser/snippets.js:84: On 2019/03/20 01:41:59, Manish Jethani wrote: > Nit: Blank line here seems unnecessary if we are grouping things together as in > the following tests. Done. https://codereview.adblockplus.org/29995559/diff/30029560/test/browser/snippe... test/browser/snippets.js:88: // Test that we try to catch a property that don't exist yet. On 2019/03/20 01:41:59, Manish Jethani wrote: > Nit: s/don't/doesn't/ Done. https://codereview.adblockplus.org/29995559/diff/30029560/test/browser/snippe... test/browser/snippets.js:96: // Test overwriting the object with another object On 2019/03/20 01:42:01, Manish Jethani wrote: > Nit: Period at the end (I don't mind too much but consistency would be great). > Also same below. Done. https://codereview.adblockplus.org/29995559/diff/30029560/test/browser/snippe... test/browser/snippets.js:97: window.foo = {bar: {}}; On 2019/03/20 01:41:59, Manish Jethani wrote: > There are some inconsistencies with the naming here. We start with `abpTest`, > then `abpTest2`, `abpTest3`, then suddently switch to `foo`. > > I suggest we follow this convention: > > * First-level properties: abpTest, abpTest2, abpTest3, ... > * Second-level properties: prop1, prop2, prop3, ... > * Third-level properties: foo, bar, lambda > * Value: Either 42 (number) or an object > > It's more or less like this anyway, just some inconsistencies. You could choose > some other convention if you like, as long as it's consistent and easier for > someone else to recognize and follow the same pattern. Done. https://codereview.adblockplus.org/29995559/diff/30029560/test/browser/snippe... test/browser/snippets.js:107: // We expect that accessing the property cause a TypeError. On 2019/03/20 01:42:00, Manish Jethani wrote: > Nit: s/cause/causes/ > > (FWIW I'm not sure this comment explains anything that isn't obvious from the > code.) Deleted. https://codereview.adblockplus.org/29995559/diff/30029560/test/browser/snippe... test/browser/snippets.js:110: window.foo2 = {bar2: {}}; On 2019/03/20 01:42:00, Manish Jethani wrote: > Before this line, we could throw in another test: > > window.foo2 = {bar2: 42}; > testProperty("foo2.bar2.lambda", false); > > In this case no error is thrown, but afterwards when replace `bar2` with a `{}` > then in the next access we get an error. Done.
Nits aside, LGTM. (I don't mind if you want to land this as it is. We could make further changes in a separate patch.)
On 2019/03/20 15:11:52, Manish Jethani wrote: > Nits aside, LGTM. > > (I don't mind if you want to land this as it is. We could make further changes > in a separate patch.) Hold on, you beat me to it. Let me take a look ...
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: {}}; If we agree about the convention here then it should be `abpTest4.prop1.foo` rather than `abpTest4.bar.lambda`. https://codereview.adblockplus.org/29995559/diff/30030561/test/browser/snippe... test/browser/snippets.js:105: testProperty("abpTest5.bar2.lambda", true, "TypeError"); Similarly, `abpTest5.prop1.foo` (Sorry, the next person who comes along would see the pattern and follow it. Otherwise it's hard to keep track.)
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:82: window.abpTest2 = {prop1: "foo"}; Would be nice if we did't use "foo" for the value because it's a property name in the tests. It's easier to read code when it follows some rules. I suggested 42 as the value, you could use anything but "foo", "bar", or "lambda". https://codereview.adblockplus.org/29995559/diff/30030561/test/browser/snippe... test/browser/snippets.js:102: window.abpTest5 = 5; Nit: Same as above, would be nice to use 42 consistently as the value when it's not an object.
updated patch 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:82: window.abpTest2 = {prop1: "foo"}; On 2019/03/20 15:20:52, Manish Jethani wrote: > Would be nice if we did't use "foo" for the value because it's a property name > in the tests. It's easier to read code when it follows some rules. I suggested > 42 as the value, you could use anything but "foo", "bar", or "lambda". Done. https://codereview.adblockplus.org/29995559/diff/30030561/test/browser/snippe... test/browser/snippets.js:95: window.abpTest4 = {bar: {}}; 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. https://codereview.adblockplus.org/29995559/diff/30030561/test/browser/snippe... test/browser/snippets.js:102: window.abpTest5 = 5; On 2019/03/20 15:20:52, Manish Jethani wrote: > Nit: Same as above, would be nice to use 42 consistently as the value when it's > not an object. Done. https://codereview.adblockplus.org/29995559/diff/30030561/test/browser/snippe... test/browser/snippets.js:105: testProperty("abpTest5.bar2.lambda", true, "TypeError"); On 2019/03/20 15:17:48, Manish Jethani wrote: > Similarly, `abpTest5.prop1.foo` > > (Sorry, the next person who comes along would see the pattern and follow it. > Otherwise it's hard to keep track.) Done.
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: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.)
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.
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! 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
