Issue 29405564: Issue 5121 - Use Acceptable Ads API in libadblockplus (Closed)
DescriptionIssue 5121 - Use Acceptable Ads API in libadblockplus
depends on #https://codereview.adblockplus.org/29405555/
(revisions update required in `dependencies` file after binaries pushed in the repo).
Patch Set 1 #
Total comments: 8
Patch Set 2 : changed tests after AA in getListedSubscriptions discussion #Patch Set 3 : updated dependency to -binaries #
Total comments: 3
Patch Set 4 : removed comment #
Total comments: 4
Patch Set 5 : minor cleanup suggestions by Serge #
MessagesTotal messages: 19
https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-andr... File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java (right): https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-andr... libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:443: fail("AA subscription found in listed subscriptions when disabled"); for some reason the test fails for now
https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/Subscription.java (right): https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/Subscription.java:58: public boolean isAcceptableAds() isn't used for now (we do filter out aa subscription ourselves (https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and...) instead of iterating-and-filtering-out). we could probably filter out it in `adblockEngine.getListedSubscriptions()` but we should introduce method argument and it break back-compatibility. so just leaving it as is. https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java (right): https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:523: public String getAcceptableAdsSubscriptionURL() for compatibility reason (though it's just forwarding invocation to the engine) https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:528: public boolean isAcceptableAdsEnabled() for compatibility reason (though it's just forwarding invocation to the engine) https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java:533: public void setAcceptableAdsEnabled(final boolean enabled) for compatibility reason (though it's just forwarding invocation to the engine)
On a quick overview looks good. https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-andr... File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java (right): https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-andr... libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:443: fail("AA subscription found in listed subscriptions when disabled"); On 2017/04/07 07:08:17, anton wrote: > for some reason the test fails for now You need to also check whether it's disabled. The subscription is present but it's disabled. https://github.com/adblockplus/libadblockplus/blob/a1572a509ce45074988b823842...
https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-andr... File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java (right): https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-andr... libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:443: fail("AA subscription found in listed subscriptions when disabled"); On 2017/04/07 08:01:22, sergei wrote: > On 2017/04/07 07:08:17, anton wrote: > > for some reason the test fails for now > > You need to also check whether it's disabled. The subscription is present but > it's disabled. > https://github.com/adblockplus/libadblockplus/blob/a1572a509ce45074988b823842... there is (and there were) no 'disabled' property in android API: https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and... what's the purpose of having disabled subscription in the result list of 'getListedSubscriptions()' ?
On 2017/04/07 08:06:49, anton wrote: > https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-andr... > File > libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java > (right): > > https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-andr... > libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:443: > fail("AA subscription found in listed subscriptions when disabled"); > On 2017/04/07 08:01:22, sergei wrote: > > On 2017/04/07 07:08:17, anton wrote: > > > for some reason the test fails for now > > > > You need to also check whether it's disabled. The subscription is present but > > it's disabled. > > > https://github.com/adblockplus/libadblockplus/blob/a1572a509ce45074988b823842... > > there is (and there were) no 'disabled' property in android API: > https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and... > > what's the purpose of having disabled subscription in the result list of > 'getListedSubscriptions()' ? i figured out that Subscription entity does not have 'disabled' property in libadblockplus too. But having AA subscription in the getListedSubscriptions() even if AA is disabled is misleading. What's the reason for it?
https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-andr... File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java (right): https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-andr... libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:443: fail("AA subscription found in listed subscriptions when disabled"); On 2017/04/07 08:06:49, anton wrote: > On 2017/04/07 08:01:22, sergei wrote: > > On 2017/04/07 07:08:17, anton wrote: > > > for some reason the test fails for now > > > > You need to also check whether it's disabled. The subscription is present but > > it's disabled. > > > https://github.com/adblockplus/libadblockplus/blob/a1572a509ce45074988b823842... > > there is (and there were) no 'disabled' property in android API: Actually it's a good point, I think we need either Subscription::IsDisabled or Subscription::IsEnabled because subscription object does have it, https://issues.adblockplus.org/ticket/5123. > https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and... > > what's the purpose of having disabled subscription in the result list of > 'getListedSubscriptions()' ? I think there are several reasons for it, a couple of them are that we have such functionality in browser extensions, a user can add subscriptions and afterwards disable some of them or enable again (for instance, I personally use that functionality), so we need to provide others with API enabling them to implement such functionality in their products based on libadblockplus. Another reason is that `GetListedSubscriptions` "Retrieves all subscriptions.". I would not like to change the latter for present because I'm afraid that filtering AA out the result can break the way AA is used in other products based on libadblockplus. Actually, since libadblockplus is supposed to be merely a bindings to adblockpluscore, I would propose to rather file an issue with moving of `setAASubscriptionEnabled`, `getListedSubscriptions` and other more or less complicated methods into core and discuss the API there.
On 2017/04/07 09:51:03, sergei wrote: > https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-andr... > File > libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java > (right): > > https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-andr... > libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:443: > fail("AA subscription found in listed subscriptions when disabled"); > On 2017/04/07 08:06:49, anton wrote: > > On 2017/04/07 08:01:22, sergei wrote: > > > On 2017/04/07 07:08:17, anton wrote: > > > > for some reason the test fails for now > > > > > > You need to also check whether it's disabled. The subscription is present > but > > > it's disabled. > > > > > > https://github.com/adblockplus/libadblockplus/blob/a1572a509ce45074988b823842... > > > > there is (and there were) no 'disabled' property in android API: > Actually it's a good point, I think we need either Subscription::IsDisabled or > Subscription::IsEnabled because subscription object does have it, > https://issues.adblockplus.org/ticket/5123. > > > https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and... > > > > what's the purpose of having disabled subscription in the result list of > > 'getListedSubscriptions()' ? > > I think there are several reasons for it, a couple of them are that we have such > functionality in browser extensions, a user can add subscriptions and afterwards > disable some of them or enable again (for instance, I personally use that > functionality), so we need to provide others with API enabling them to implement > such functionality in their products based on libadblockplus. Another reason is > that `GetListedSubscriptions` "Retrieves all subscriptions.". I would not like > to change the latter for present because I'm afraid that filtering AA out the > result can break the way AA is used in other products based on libadblockplus. > Actually, since libadblockplus is supposed to be merely a bindings to > adblockpluscore, I would propose to rather file an issue with moving of > `setAASubscriptionEnabled`, `getListedSubscriptions` and other more or less > complicated methods into core and discuss the API there. okay, this still seems to be very misleading since we have neither enabled()/disable() methods in android Subscription entity nor 'isEnabled()' getter too. the fundamental reason is that i can't see any difference between `disabled` and `removed` subscription. Up to Felix to take the decision to save this design or change it.
On 2017/04/07 09:51:03, sergei wrote: > https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-andr... > File > libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java > (right): > > https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-andr... > libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:443: > fail("AA subscription found in listed subscriptions when disabled"); > On 2017/04/07 08:06:49, anton wrote: > > On 2017/04/07 08:01:22, sergei wrote: > > > On 2017/04/07 07:08:17, anton wrote: > > > > for some reason the test fails for now > > > > > > You need to also check whether it's disabled. The subscription is present > but > > > it's disabled. > > > > > > https://github.com/adblockplus/libadblockplus/blob/a1572a509ce45074988b823842... > > > > there is (and there were) no 'disabled' property in android API: > Actually it's a good point, I think we need either Subscription::IsDisabled or > Subscription::IsEnabled because subscription object does have it, > https://issues.adblockplus.org/ticket/5123. > > > https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and... > > > > what's the purpose of having disabled subscription in the result list of > > 'getListedSubscriptions()' ? > > I think there are several reasons for it, a couple of them are that we have such > functionality in browser extensions, a user can add subscriptions and afterwards > disable some of them or enable again (for instance, I personally use that > functionality), so we need to provide others with API enabling them to implement > such functionality in their products based on libadblockplus. Another reason is > that `GetListedSubscriptions` "Retrieves all subscriptions.". I would not like > to change the latter for present because I'm afraid that filtering AA out the > result can break the way AA is used in other products based on libadblockplus. > Actually, since libadblockplus is supposed to be merely a bindings to > adblockpluscore, I would propose to rather file an issue with moving of > `setAASubscriptionEnabled`, `getListedSubscriptions` and other more or less > complicated methods into core and discuss the API there. uploaded patch set #2
On 2017/04/07 10:01:48, anton wrote: > On 2017/04/07 09:51:03, sergei wrote: > > > https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-andr... > > File > > > libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java > > (right): > > > > > https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-andr... > > > libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:443: > > fail("AA subscription found in listed subscriptions when disabled"); > > On 2017/04/07 08:06:49, anton wrote: > > > On 2017/04/07 08:01:22, sergei wrote: > > > > On 2017/04/07 07:08:17, anton wrote: > > > > > for some reason the test fails for now > > > > > > > > You need to also check whether it's disabled. The subscription is present > > but > > > > it's disabled. > > > > > > > > > > https://github.com/adblockplus/libadblockplus/blob/a1572a509ce45074988b823842... > > > > > > there is (and there were) no 'disabled' property in android API: > > Actually it's a good point, I think we need either Subscription::IsDisabled or > > Subscription::IsEnabled because subscription object does have it, > > https://issues.adblockplus.org/ticket/5123. > > > > > > https://hg.adblockplus.org/libadblockplus-android/file/tip/libadblockplus-and... > > > > > > what's the purpose of having disabled subscription in the result list of > > > 'getListedSubscriptions()' ? > > > > I think there are several reasons for it, a couple of them are that we have > such > > functionality in browser extensions, a user can add subscriptions and > afterwards > > disable some of them or enable again (for instance, I personally use that > > functionality), so we need to provide others with API enabling them to > implement > > such functionality in their products based on libadblockplus. Another reason > is > > that `GetListedSubscriptions` "Retrieves all subscriptions.". I would not like > > to change the latter for present because I'm afraid that filtering AA out the > > result can break the way AA is used in other products based on libadblockplus. > > Actually, since libadblockplus is supposed to be merely a bindings to > > adblockpluscore, I would propose to rather file an issue with moving of > > `setAASubscriptionEnabled`, `getListedSubscriptions` and other more or less > > complicated methods into core and discuss the API there. > > uploaded patch set #2 uploaded patch set #3
https://codereview.adblockplus.org/29405564/diff/29405612/libadblockplus-andr... File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java (right): https://codereview.adblockplus.org/29405564/diff/29405612/libadblockplus-andr... libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:413: // AA subscription listed even if AA is disabled with `filterEngine.setAcceptableAdsEnabled(false);` If there is no AA then `filterEngine.setAcceptableAdsEnabled(false);` won't add it, so setAcceptableAdsEnabled(true) is required. The matrix of behavior is here https://github.com/adblockplus/libadblockplus/blob/master/test/FilterEngine.c....
On 2017/04/07 10:22:39, sergei wrote: > https://codereview.adblockplus.org/29405564/diff/29405612/libadblockplus-andr... > File > libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java > (right): > > https://codereview.adblockplus.org/29405564/diff/29405612/libadblockplus-andr... > libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:413: > // AA subscription listed even if AA is disabled with > `filterEngine.setAcceptableAdsEnabled(false);` > If there is no AA then `filterEngine.setAcceptableAdsEnabled(false);` won't add > it, so setAcceptableAdsEnabled(true) is required. > > The matrix of behavior is here > https://github.com/adblockplus/libadblockplus/blob/master/test/FilterEngine.c.... Well, this is not the set for the logics so i can 1) remove the comment if it's wrong 2) replace it with the link 3) leave everything as-is
On 2017/04/07 10:27:44, anton wrote: > On 2017/04/07 10:22:39, sergei wrote: > > > https://codereview.adblockplus.org/29405564/diff/29405612/libadblockplus-andr... > > File > > > libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java > > (right): > > > > > https://codereview.adblockplus.org/29405564/diff/29405612/libadblockplus-andr... > > > libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:413: > > // AA subscription listed even if AA is disabled with > > `filterEngine.setAcceptableAdsEnabled(false);` > > If there is no AA then `filterEngine.setAcceptableAdsEnabled(false);` won't > add > > it, so setAcceptableAdsEnabled(true) is required. > > > > The matrix of behavior is here > > > https://github.com/adblockplus/libadblockplus/blob/master/test/FilterEngine.c.... > > Well, this is not the set for the logics so i can > 1) remove the comment if it's wrong > 2) replace it with the link > 3) leave everything as-is see patch set #3
https://codereview.adblockplus.org/29405564/diff/29405612/libadblockplus-andr... File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java (right): https://codereview.adblockplus.org/29405564/diff/29405612/libadblockplus-andr... libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:413: // AA subscription listed even if AA is disabled with `filterEngine.setAcceptableAdsEnabled(false);` On 2017/04/07 10:22:39, sergei wrote: > If there is no AA then `filterEngine.setAcceptableAdsEnabled(false);` won't add > it, so setAcceptableAdsEnabled(true) is required. > > The matrix of behavior is here > https://github.com/adblockplus/libadblockplus/blob/master/test/FilterEngine.c.... I think this comment should be removed, since as sergei said, if AA subscription is not currently present, it won't be added without calling setAcceptableAdsEnabled(true)
https://codereview.adblockplus.org/29405564/diff/29405612/libadblockplus-andr... File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java (right): https://codereview.adblockplus.org/29405564/diff/29405612/libadblockplus-andr... libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:413: // AA subscription listed even if AA is disabled with `filterEngine.setAcceptableAdsEnabled(false);` On 2017/04/27 13:32:01, diegocarloslima wrote: > On 2017/04/07 10:22:39, sergei wrote: > > If there is no AA then `filterEngine.setAcceptableAdsEnabled(false);` won't > add > > it, so setAcceptableAdsEnabled(true) is required. > > > > The matrix of behavior is here > > > https://github.com/adblockplus/libadblockplus/blob/master/test/FilterEngine.c.... > > I think this comment should be removed, since as sergei said, if AA subscription > is not currently present, it won't be added without calling > setAcceptableAdsEnabled(true) Done. See updated patch set
On 2017/04/28 06:21:37, anton wrote: > https://codereview.adblockplus.org/29405564/diff/29405612/libadblockplus-andr... > File > libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java > (right): > > https://codereview.adblockplus.org/29405564/diff/29405612/libadblockplus-andr... > libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:413: > // AA subscription listed even if AA is disabled with > `filterEngine.setAcceptableAdsEnabled(false);` > On 2017/04/27 13:32:01, diegocarloslima wrote: > > On 2017/04/07 10:22:39, sergei wrote: > > > If there is no AA then `filterEngine.setAcceptableAdsEnabled(false);` won't > > add > > > it, so setAcceptableAdsEnabled(true) is required. > > > > > > The matrix of behavior is here > > > > > > https://github.com/adblockplus/libadblockplus/blob/master/test/FilterEngine.c.... > > > > I think this comment should be removed, since as sergei said, if AA > subscription > > is not currently present, it won't be added without calling > > setAcceptableAdsEnabled(true) > > Done. See updated patch set LGTM
LGTM, I have not checked dependencies though.
forgot to send comments, you may ignore them. https://codereview.adblockplus.org/29405564/diff/29424592/libadblockplus-andr... File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java (right): https://codereview.adblockplus.org/29405564/diff/29424592/libadblockplus-andr... libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:416: assertTrue(filterEngine.isAcceptableAdsEnabled()); Would it be useful to hard code in tests that by default AA is enabled? :) https://codereview.adblockplus.org/29405564/diff/29424592/libadblockplus-andr... File libadblockplus-android/jni/JniFilterEngine.cpp (right): https://codereview.adblockplus.org/29405564/diff/29424592/libadblockplus-andr... libadblockplus-android/jni/JniFilterEngine.cpp:507: return (engine->IsAAEnabled() ? JNI_TRUE : JNI_FALSE); I think that without braces it would be better, but it's up to the project rules. return (engine->IsAAEnabled() ? JNI_TRUE : JNI_FALSE); vs return engine->IsAAEnabled() ? JNI_TRUE : JNI_FALSE;
https://codereview.adblockplus.org/29405564/diff/29424592/libadblockplus-andr... File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java (right): https://codereview.adblockplus.org/29405564/diff/29424592/libadblockplus-andr... libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:416: assertTrue(filterEngine.isAcceptableAdsEnabled()); On 2017/05/03 13:58:22, sergei wrote: > Would it be useful to hard code in tests that by default AA is enabled? :) Not sure. Since it's not actually stated anywhere i'd prefer to enable it explicitly in the test. https://codereview.adblockplus.org/29405564/diff/29424592/libadblockplus-andr... File libadblockplus-android/jni/JniFilterEngine.cpp (right): https://codereview.adblockplus.org/29405564/diff/29424592/libadblockplus-andr... libadblockplus-android/jni/JniFilterEngine.cpp:507: return (engine->IsAAEnabled() ? JNI_TRUE : JNI_FALSE); On 2017/05/03 13:58:22, sergei wrote: > I think that without braces it would be better, but it's up to the project > rules. > return (engine->IsAAEnabled() ? JNI_TRUE : JNI_FALSE); > vs > return engine->IsAAEnabled() ? JNI_TRUE : JNI_FALSE; Done. See updated patch set 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
