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

Issue 29405564: Issue 5121 - Use Acceptable Ads API in libadblockplus

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 6 days ago by anton
Modified:
13 hours, 4 minutes ago
Visibility:
Public.

Description

Issue 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: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+125 lines, -67 lines) Patch
M dependencies View 1 2 1 chunk +1 line, -1 line 0 comments Download
M libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java View 1 3 chunks +43 lines, -1 line 2 comments Download
M libadblockplus-android/jni/JniFilterEngine.cpp View 2 chunks +35 lines, -0 lines 0 comments Download
M libadblockplus-android/jni/JniSubscription.cpp View 2 chunks +11 lines, -1 line 0 comments Download
M libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java View 2 chunks +21 lines, -0 lines 0 comments Download
M libadblockplus-android/src/org/adblockplus/libadblockplus/Subscription.java View 2 chunks +7 lines, -0 lines 0 comments Download
M libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java View 2 chunks +7 lines, -64 lines 0 comments Download

Messages

Total messages: 14
anton
2 weeks, 6 days ago (2017-04-07 07:05:13 UTC) #1
anton
https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java (right): https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java#newcode443 libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:443: fail("AA subscription found in listed subscriptions when disabled"); for ...
2 weeks, 6 days ago (2017-04-07 07:08:18 UTC) #2
anton
https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-android/src/org/adblockplus/libadblockplus/Subscription.java File libadblockplus-android/src/org/adblockplus/libadblockplus/Subscription.java (right): https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-android/src/org/adblockplus/libadblockplus/Subscription.java#newcode58 libadblockplus-android/src/org/adblockplus/libadblockplus/Subscription.java:58: public boolean isAcceptableAds() isn't used for now (we do ...
2 weeks, 6 days ago (2017-04-07 07:13:58 UTC) #3
sergei
On a quick overview looks good. https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java (right): https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java#newcode443 libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:443: fail("AA subscription found ...
2 weeks, 6 days ago (2017-04-07 08:01:22 UTC) #4
anton
https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java (right): https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java#newcode443 libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:443: fail("AA subscription found in listed subscriptions when disabled"); On ...
2 weeks, 6 days ago (2017-04-07 08:06:49 UTC) #5
anton
On 2017/04/07 08:06:49, anton wrote: > https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java > File > libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java > (right): > > ...
2 weeks, 6 days ago (2017-04-07 08:10:26 UTC) #6
sergei
https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java (right): https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java#newcode443 libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:443: fail("AA subscription found in listed subscriptions when disabled"); On ...
2 weeks, 6 days ago (2017-04-07 09:51:03 UTC) #7
anton
On 2017/04/07 09:51:03, sergei wrote: > https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java > File > libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java > (right): > > ...
2 weeks, 6 days ago (2017-04-07 09:56:23 UTC) #8
anton
On 2017/04/07 09:51:03, sergei wrote: > https://codereview.adblockplus.org/29405564/diff/29405565/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java > File > libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java > (right): > > ...
2 weeks, 6 days ago (2017-04-07 10:01:48 UTC) #9
anton
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-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java ...
2 weeks, 6 days ago (2017-04-07 10:17:05 UTC) #10
sergei
https://codereview.adblockplus.org/29405564/diff/29405612/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java (right): https://codereview.adblockplus.org/29405564/diff/29405612/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java#newcode413 libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:413: // AA subscription listed even if AA is disabled ...
2 weeks, 6 days ago (2017-04-07 10:22:39 UTC) #11
anton
On 2017/04/07 10:22:39, sergei wrote: > https://codereview.adblockplus.org/29405564/diff/29405612/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java > File > libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java > (right): > > ...
2 weeks, 6 days ago (2017-04-07 10:27:44 UTC) #12
anton
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-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java ...
1 week, 6 days ago (2017-04-14 12:16:07 UTC) #13
diegocarloslima
13 hours, 4 minutes ago (2017-04-27 13:32:02 UTC) #14
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)
Sign in to reply to this message.

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