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

Issue 29405564: Issue 5121 - Use Acceptable Ads API in libadblockplus (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 months, 1 week ago by anton
Modified:
7 months, 1 week 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: 3

Patch Set 4 : removed comment #

Total comments: 4

Patch Set 5 : minor cleanup suggestions by Serge #

Unified diffs Side-by-side diffs Delta from patch set Stats (+123 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 2 3 3 chunks +41 lines, -1 line 0 comments Download
M libadblockplus-android/jni/JniFilterEngine.cpp View 1 2 3 4 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: 19
anton
8 months, 1 week 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 ...
8 months, 1 week 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 ...
8 months, 1 week 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 ...
8 months, 1 week 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 ...
8 months, 1 week 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): > > ...
8 months, 1 week 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 ...
8 months, 1 week 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): > > ...
8 months, 1 week 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): > > ...
8 months, 1 week 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 ...
8 months, 1 week 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 ...
8 months, 1 week 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): > > ...
8 months, 1 week 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 ...
8 months ago (2017-04-14 12:16:07 UTC) #13
diegocarloslima
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 ...
7 months, 3 weeks ago (2017-04-27 13:32:02 UTC) #14
anton
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 ...
7 months, 2 weeks ago (2017-04-28 06:21:37 UTC) #15
diegocarloslima
On 2017/04/28 06:21:37, anton 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): > > ...
7 months, 2 weeks ago (2017-04-28 11:28:44 UTC) #16
sergei
LGTM, I have not checked dependencies though.
7 months, 2 weeks ago (2017-05-03 13:56:41 UTC) #17
sergei
forgot to send comments, you may ignore them. https://codereview.adblockplus.org/29405564/diff/29424592/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/29424592/libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java#newcode416 libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineTest.java:416: assertTrue(filterEngine.isAcceptableAdsEnabled()); ...
7 months, 2 weeks ago (2017-05-03 13:58:23 UTC) #18
anton
7 months, 2 weeks ago (2017-05-04 06:20:06 UTC) #19
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
Sign in to reply to this message.

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