Issue 29366747: Issue 4657 - Add Acceptable Ads API (Closed)
Patch Set 1 #
Total comments: 23
Patch Set 2 : address comments and rebase #
Total comments: 6
Patch Set 3 : address comment #Patch Set 4 : rebase and address comment #
Total comments: 2
Patch Set 5 : fix typo #
MessagesTotal messages: 11
https://codereview.adblockplus.org/29366747/diff/29366748/lib/api.js File lib/api.js (right): https://codereview.adblockplus.org/29366747/diff/29366748/lib/api.js#newcode31 lib/api.js:31: // returns first element of the array that satifies predicate. Should I move it into utils.js or extend Array.prototype (https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects...
https://codereview.adblockplus.org/29366747/diff/29366748/lib/api.js File lib/api.js (right): https://codereview.adblockplus.org/29366747/diff/29366748/lib/api.js#newcode31 lib/api.js:31: // returns first element of the array that satifies predicate. On 2016/12/02 16:36:06, sergei wrote: > Should I move it into utils.js or extend Array.prototype > (https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects...) I meant extend in compat.js. The link is https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects...
https://codereview.adblockplus.org/29366747/diff/29366748/include/AdblockPlus... File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/29366747/diff/29366748/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:126: Typo: extra column of spaces https://codereview.adblockplus.org/29366747/diff/29366748/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:282: * Otherwise has not effect. * @param enabled * - if the argument is `true` * - ensure that the filter set includes an enabled AA subscription, adding * it if needed and enabling it if disabled. * - if the argument is `false` * - if an AA subscription is present, disable it. * - if absent, do nothing. https://codereview.adblockplus.org/29366747/diff/29366748/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:287: * Checks whether Acceptable Ads subscription is enaled. "... whether the filter set includes an Acceptable Ads subscription that is enabled." https://codereview.adblockplus.org/29366747/diff/29366748/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:294: * makes it available even if subscription is not add yet. * Retrieve the URL for an Acceptable Ads subscription from the configuration. * Use this URL to add an AA subscription if not already present. https://codereview.adblockplus.org/29366747/diff/29366748/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:297: std::string GetAAURL() const; Nit: According to our style conventions, it should be "GetAAUrl()". Even acronyms get the mixed case treatment. https://codereview.adblockplus.org/29366747/diff/29366748/lib/api.js File lib/api.js (right): https://codereview.adblockplus.org/29366747/diff/29366748/lib/api.js#newcode31 lib/api.js:31: // returns first element of the array that satifies predicate. At the point of use, it would be better to use the syntax of whatever the emerging standard is and avoid a custom function that does the same thing. At the point of definition, use the standard function if already present and add a shim if not. https://codereview.adblockplus.org/29366747/diff/29366748/lib/api.js#newcode152 lib/api.js:152: setAASubscriptionEnabled: function (enabled) Nit: extra space after 'function' https://codereview.adblockplus.org/29366747/diff/29366748/lib/api.js#newcode154 lib/api.js:154: var aaSubscription = find(FilterStorage.subscriptions, API.isAASubscription); See comment on line 173 https://codereview.adblockplus.org/29366747/diff/29366748/lib/api.js#newcode173 lib/api.js:173: var aaSubscription = find(FilterStorage.subscriptions, API.isAASubscription); Also see comment on line 154. With the above line this is repeated code. Not sure it matters a lot, but these "find" statements better belong as a method on "FilterStorage". Consider writing this as an augment on that class that could migrate into adblockpluscore. I'm a bit surprised something like this isn't already there. https://codereview.adblockplus.org/29366747/diff/29366748/src/FilterEngine.cpp File src/FilterEngine.cpp (right): https://codereview.adblockplus.org/29366747/diff/29366748/src/FilterEngine.cp... src/FilterEngine.cpp:282: jsEngine->Evaluate("API.setAASubscriptionEnabled")->Call(*jsEngine->NewValue(enabled)); Why is there a context instantiated at line 134 but not here? https://codereview.adblockplus.org/29366747/diff/29366748/test/FilterEngine.cpp File test/FilterEngine.cpp (right): https://codereview.adblockplus.org/29366747/diff/29366748/test/FilterEngine.c... test/FilterEngine.cpp:647: } This is a bunch of unit tests all combined into one. Tests are much more useful if they test individual behaviors. Written this way, when there are failures it's typical that some will fail and some will pass, and it's much quicker to locate errors. Not tested: -- enable when already enabled -- disable when already disabled -- disable after initialization -- enable after removing AA from subscriptions Line 646 looks redundant.
https://codereview.adblockplus.org/29366747/diff/29366748/include/AdblockPlus... File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/29366747/diff/29366748/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:126: On 2016/12/05 14:40:57, Eric wrote: > Typo: extra column of spaces Done. https://codereview.adblockplus.org/29366747/diff/29366748/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:282: * Otherwise has not effect. On 2016/12/05 14:40:57, Eric wrote: > * @param enabled > * - if the argument is `true` > * - ensure that the filter set includes an enabled AA subscription, adding > * it if needed and enabling it if disabled. > * - if the argument is `false` > * - if an AA subscription is present, disable it. > * - if absent, do nothing. The proposed variant is taken mostly without changes. https://codereview.adblockplus.org/29366747/diff/29366748/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:287: * Checks whether Acceptable Ads subscription is enaled. On 2016/12/05 14:40:57, Eric wrote: > "... whether the filter set includes an Acceptable Ads subscription that is > enabled." > It cannot be enabled and not included in a filter set. https://codereview.adblockplus.org/29366747/diff/29366748/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:294: * makes it available even if subscription is not add yet. On 2016/12/05 14:40:57, Eric wrote: > * Retrieve the URL for an Acceptable Ads subscription from the configuration. > * Use this URL to add an AA subscription if not already present. The aim of this method is not to add the AA subscription using URL, we need this URL only for UI, in particular to let a user to know where the subscription is downloaded from and to take a view on the content of the subscription before the latter is enabled. In addition it's not supposed that this URL can be freely changed in the configuration, if you don't like "provisioned in configuration" then I would rather remove it, "Retrieves the URL of Acceptable Ads subscription, what makes it available even if subscription is not add yet." It's just an internal technical detail which may be not communicated to the user and can be changed. https://codereview.adblockplus.org/29366747/diff/29366748/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:297: std::string GetAAURL() const; On 2016/12/05 14:40:57, Eric wrote: > Nit: According to our style conventions, it should be "GetAAUrl()". Even > acronyms get the mixed case treatment. Done. https://codereview.adblockplus.org/29366747/diff/29366748/lib/api.js File lib/api.js (right): https://codereview.adblockplus.org/29366747/diff/29366748/lib/api.js#newcode31 lib/api.js:31: // returns first element of the array that satifies predicate. On 2016/12/05 14:40:58, Eric wrote: > At the point of use, it would be better to use the syntax of whatever the > emerging standard is and avoid a custom function that does the same thing. > > At the point of definition, use the standard function if already present and add > a shim if not. Done. https://codereview.adblockplus.org/29366747/diff/29366748/lib/api.js#newcode152 lib/api.js:152: setAASubscriptionEnabled: function (enabled) On 2016/12/05 14:40:58, Eric wrote: > Nit: extra space after 'function' Done. https://codereview.adblockplus.org/29366747/diff/29366748/lib/api.js#newcode173 lib/api.js:173: var aaSubscription = find(FilterStorage.subscriptions, API.isAASubscription); On 2016/12/05 14:40:58, Eric wrote: > Also see comment on line 154. > > With the above line this is repeated code. Not sure it matters a lot, but these > "find" statements better belong as a method on "FilterStorage". Consider writing > this as an augment on that class that could migrate into adblockpluscore. I'm a > bit surprised something like this isn't already there. I would like to avoid augmenting of FilterStorage because it looks like a hack. I would like to rather have in the core some ????FilterStorage which extends FilterStorage and has functionality related to Acceptable Ads. https://codereview.adblockplus.org/29366747/diff/29366748/src/FilterEngine.cpp File src/FilterEngine.cpp (right): https://codereview.adblockplus.org/29366747/diff/29366748/src/FilterEngine.cp... src/FilterEngine.cpp:282: jsEngine->Evaluate("API.setAASubscriptionEnabled")->Call(*jsEngine->NewValue(enabled)); On 2016/12/05 14:40:58, Eric wrote: > Why is there a context instantiated at line 134 but not here? JsContext context(jsEngine); is removed on line 134 because we don't use it everywhere else. https://codereview.adblockplus.org/29366747/diff/29366748/test/FilterEngine.cpp File test/FilterEngine.cpp (right): https://codereview.adblockplus.org/29366747/diff/29366748/test/FilterEngine.c... test/FilterEngine.cpp:647: } On 2016/12/05 14:40:58, Eric wrote: > This is a bunch of unit tests all combined into one. Tests are much more useful > if they test individual behaviors. Written this way, when there are failures > it's typical that some will fail and some will pass, and it's much quicker to > locate errors. > > Not tested: > -- enable when already enabled > -- disable when already disabled > -- disable after initialization > -- enable after removing AA from subscriptions > > Line 646 looks redundant. Added more tests and they run in each separate test.
https://codereview.adblockplus.org/29366747/diff/29386653/include/AdblockPlus... File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/29366747/diff/29386653/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:126: bool IsAA(); Shouldn't this be a const method? https://codereview.adblockplus.org/29366747/diff/29386653/lib/api.js File lib/api.js (right): https://codereview.adblockplus.org/29366747/diff/29386653/lib/api.js#newcode154 lib/api.js:154: if (!aaSubscription) { 'nit: new line before the {
https://codereview.adblockplus.org/29366747/diff/29386653/include/AdblockPlus... File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/29366747/diff/29386653/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:126: bool IsAA(); On 2017/03/17 20:04:15, hub wrote: > Shouldn't this be a const method? I do agree that it should be const because for the user logically it does not modify the state, although it's a bit tough decision, therefore I would like to consider it separately and make this change in another commit as well as fix it in other places. So far I would like to have it non-const "for consistency". See, https://issues.adblockplus.org/ticket/5013. https://codereview.adblockplus.org/29366747/diff/29386653/lib/api.js File lib/api.js (right): https://codereview.adblockplus.org/29366747/diff/29386653/lib/api.js#newcode154 lib/api.js:154: if (!aaSubscription) { On 2017/03/17 20:04:15, hub wrote: > 'nit: new line before the { Done.
As it is right now, this patch need to be rebased ; with my other comment resolved, it will ok. https://codereview.adblockplus.org/29366747/diff/29386653/include/AdblockPlus... File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/29366747/diff/29386653/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:126: bool IsAA(); On 2017/03/17 22:10:06, sergei wrote: > On 2017/03/17 20:04:15, hub wrote: > > Shouldn't this be a const method? > > I do agree that it should be const because for the user logically it does not > modify the state, although it's a bit tough decision, therefore I would like to > consider it separately and make this change in another commit as well as fix it > in other places. So far I would like to have it non-const "for consistency". As it is right now, IsUpdating() above is const, so the consistency "const".
rebase and address comment https://codereview.adblockplus.org/29366747/diff/29386653/include/AdblockPlus... File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/29366747/diff/29386653/include/AdblockPlus... include/AdblockPlus/FilterEngine.h:126: bool IsAA(); On 2017/04/05 13:35:02, hub wrote: > On 2017/03/17 22:10:06, sergei wrote: > > On 2017/03/17 20:04:15, hub wrote: > > > Shouldn't this be a const method? > > > > I do agree that it should be const because for the user logically it does not > > modify the state, although it's a bit tough decision, therefore I would like > to > > consider it separately and make this change in another commit as well as fix > it > > in other places. So far I would like to have it non-const "for consistency". > > As it is right now, IsUpdating() above is const, so the consistency "const". fixed.
LGTM https://codereview.adblockplus.org/29366747/diff/29403741/test/FilterEngine.cpp File test/FilterEngine.cpp (right): https://codereview.adblockplus.org/29366747/diff/29403741/test/FilterEngine.c... test/FilterEngine.cpp:912: auto subscruption = filterEngine->GetSubscription(kOtherSubscriptionUrl); 'nit: I didn't see that one. There is a typo. It is "subscription" And below, since this compiles.
https://codereview.adblockplus.org/29366747/diff/29403741/test/FilterEngine.cpp File test/FilterEngine.cpp (right): https://codereview.adblockplus.org/29366747/diff/29403741/test/FilterEngine.c... test/FilterEngine.cpp:912: auto subscruption = filterEngine->GetSubscription(kOtherSubscriptionUrl); On 2017/04/05 14:52:47, hub wrote: > 'nit: I didn't see that one. There is a typo. It is "subscription" > > And below, since this compiles. Done. Good catch.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
