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

Issue 29908555: Issue 7035 - Update libadblockplus dependency (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 4 months ago by krystian
Modified:
1 year, 4 months ago
Reviewers:
anton, sergei
CC:
zura
Base URL:
git@github.com:adblockplus/libadblockplus-android.git@d150f08d5d72de8938c7ebbdccd9b0c4e06b4070
Visibility:
Public.

Description

Issue 7035 - Update libadblockplus dependency

Patch Set 1 #

Total comments: 6

Patch Set 2 : Issue 7035 - Update libadblockplus dependency #

Patch Set 3 : Issue 7035 - Update libadblockplus dependency #

Total comments: 1

Patch Set 4 : Issue 7035 - Update libadblockplus dependency #

Patch Set 5 : Issue 7035 - Update libadblockplus dependency #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -750 lines) Patch
D adblock-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineUpdaterTest.java View 1 2 1 chunk +0 lines, -53 lines 0 comments Download
M adblock-android-tests/src/org/adblockplus/libadblockplus/tests/UpdateCheckTest.java View 1 2 3 4 1 chunk +0 lines, -265 lines 0 comments Download
M adblock-android-tests/src/org/adblockplus/libadblockplus/tests/UpdaterTest.java View 1 2 3 4 1 chunk +0 lines, -42 lines 0 comments Download
M adblock-android/jni/Android.mk View 1 2 3 4 1 chunk +0 lines, -1 line 0 comments Download
M adblock-android/jni/JniFilterEngine.cpp View 3 chunks +0 lines, -54 lines 0 comments Download
D adblock-android/jni/JniUpdateAvailableCallback.cpp View 1 2 3 4 1 chunk +0 lines, -70 lines 0 comments Download
D adblock-android/jni/JniUpdateCheckDoneCallback.cpp View 1 2 3 4 1 chunk +0 lines, -70 lines 0 comments Download
M adblock-android/src/org/adblockplus/libadblockplus/FilterEngine.java View 1 2 3 chunks +0 lines, -26 lines 0 comments Download
D adblock-android/src/org/adblockplus/libadblockplus/UpdateAvailableCallback.java View 1 2 3 4 1 chunk +0 lines, -66 lines 0 comments Download
D adblock-android/src/org/adblockplus/libadblockplus/UpdateCheckDoneCallback.java View 1 2 3 4 1 chunk +0 lines, -66 lines 0 comments Download
M adblock-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java View 1 2 3 4 7 chunks +0 lines, -36 lines 0 comments Download
M dependencies View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 21
krystian
I guess I should create also JniUpdater.cpp for changed API in libadblockplus. Unless it is ...
1 year, 4 months ago (2018-10-12 09:17:34 UTC) #1
anton
https://codereview.adblockplus.org/29908555/diff/29908556/adblock-android/jni/JniFilterEngine.cpp File adblock-android/jni/JniFilterEngine.cpp (right): https://codereview.adblockplus.org/29908555/diff/29908556/adblock-android/jni/JniFilterEngine.cpp#newcode176 adblock-android/jni/JniFilterEngine.cpp:176: does it mean we just dropped this functionality? https://codereview.adblockplus.org/29908555/diff/29908556/dependencies ...
1 year, 4 months ago (2018-10-12 09:27:11 UTC) #2
krystian
https://codereview.adblockplus.org/29908555/diff/29908556/adblock-android/jni/JniFilterEngine.cpp File adblock-android/jni/JniFilterEngine.cpp (right): https://codereview.adblockplus.org/29908555/diff/29908556/adblock-android/jni/JniFilterEngine.cpp#newcode176 adblock-android/jni/JniFilterEngine.cpp:176: On 2018/10/12 09:27:11, anton wrote: > does it mean ...
1 year, 4 months ago (2018-10-12 09:31:32 UTC) #3
anton
https://codereview.adblockplus.org/29908555/diff/29908556/adblock-android/jni/JniFilterEngine.cpp File adblock-android/jni/JniFilterEngine.cpp (right): https://codereview.adblockplus.org/29908555/diff/29908556/adblock-android/jni/JniFilterEngine.cpp#newcode176 adblock-android/jni/JniFilterEngine.cpp:176: On 2018/10/12 09:31:32, k.zlomek wrote: > On 2018/10/12 09:27:11, ...
1 year, 4 months ago (2018-10-12 09:34:23 UTC) #4
sergei
On 2018/10/12 09:17:34, k.zlomek wrote: > I guess I should create also JniUpdater.cpp for changed ...
1 year, 4 months ago (2018-10-12 09:34:38 UTC) #5
anton
On 2018/10/12 09:34:38, sergei wrote: > On 2018/10/12 09:17:34, k.zlomek wrote: > > I guess ...
1 year, 4 months ago (2018-10-12 09:37:38 UTC) #6
krystian
On 2018/10/12 09:37:38, anton wrote: > On 2018/10/12 09:34:38, sergei wrote: > > On 2018/10/12 ...
1 year, 4 months ago (2018-10-12 09:41:26 UTC) #7
sergei
On 2018/10/12 09:37:38, anton wrote: > On 2018/10/12 09:34:38, sergei wrote: > > On 2018/10/12 ...
1 year, 4 months ago (2018-10-12 09:41:37 UTC) #8
sergei
https://codereview.adblockplus.org/29908555/diff/29908556/adblock-android/jni/JniFilterEngine.cpp File adblock-android/jni/JniFilterEngine.cpp (left): https://codereview.adblockplus.org/29908555/diff/29908556/adblock-android/jni/JniFilterEngine.cpp#oldcode523 adblock-android/jni/JniFilterEngine.cpp:523: { (char*)"setUpdateAvailableCallback", (char*)"(JJ)V", (void*)JniSetUpdateAvailableCallback }, BTW, what about Java ...
1 year, 4 months ago (2018-10-12 09:42:29 UTC) #9
anton
On 2018/10/12 09:41:37, sergei wrote: > On 2018/10/12 09:37:38, anton wrote: > > On 2018/10/12 ...
1 year, 4 months ago (2018-10-12 09:42:34 UTC) #10
sergei
On 2018/10/12 09:42:34, anton wrote: > Then we should not spend time on this task ...
1 year, 4 months ago (2018-10-12 09:48:39 UTC) #11
anton
On 2018/10/12 09:48:39, sergei wrote: > On 2018/10/12 09:42:34, anton wrote: > > Then we ...
1 year, 4 months ago (2018-10-12 09:53:46 UTC) #12
sergei
On 2018/10/12 09:53:46, anton wrote: > It means this is not right task if you ...
1 year, 4 months ago (2018-10-12 10:43:38 UTC) #13
krystian
On 2018/10/12 10:43:38, sergei wrote: > On 2018/10/12 09:53:46, anton wrote: > > It means ...
1 year, 4 months ago (2018-10-15 07:07:25 UTC) #14
krystian
https://codereview.adblockplus.org/29908555/diff/29911555/adblock-android-tests/src/org/adblockplus/libadblockplus/tests/UpdaterCallbacksTest.java File adblock-android-tests/src/org/adblockplus/libadblockplus/tests/UpdaterCallbacksTest.java (right): https://codereview.adblockplus.org/29908555/diff/29911555/adblock-android-tests/src/org/adblockplus/libadblockplus/tests/UpdaterCallbacksTest.java#newcode26 adblock-android-tests/src/org/adblockplus/libadblockplus/tests/UpdaterCallbacksTest.java:26: public class UpdaterCallbacksTest extends UpdaterTest This class is added ...
1 year, 4 months ago (2018-10-15 09:59:47 UTC) #15
krystian
On 2018/10/15 09:59:47, krystian wrote: > https://codereview.adblockplus.org/29908555/diff/29911555/adblock-android-tests/src/org/adblockplus/libadblockplus/tests/UpdaterCallbacksTest.java > File > adblock-android-tests/src/org/adblockplus/libadblockplus/tests/UpdaterCallbacksTest.java > (right): > > ...
1 year, 4 months ago (2018-10-15 10:21:50 UTC) #16
anton
On 2018/10/15 10:21:50, krystian wrote: > On 2018/10/15 09:59:47, krystian wrote: > > > https://codereview.adblockplus.org/29908555/diff/29911555/adblock-android-tests/src/org/adblockplus/libadblockplus/tests/UpdaterCallbacksTest.java ...
1 year, 4 months ago (2018-10-16 13:21:56 UTC) #17
krystian
On 2018/10/16 13:21:56, anton wrote: > On 2018/10/15 10:21:50, krystian wrote: > > On 2018/10/15 ...
1 year, 4 months ago (2018-10-16 13:51:13 UTC) #18
anton
On 2018/10/16 13:51:13, krystian wrote: > On 2018/10/16 13:21:56, anton wrote: > > On 2018/10/15 ...
1 year, 4 months ago (2018-10-16 17:02:20 UTC) #19
krystian
On 2018/10/16 17:02:20, anton wrote: > On 2018/10/16 13:51:13, krystian wrote: > > On 2018/10/16 ...
1 year, 4 months ago (2018-10-17 07:09:25 UTC) #20
anton
1 year, 4 months ago (2018-10-17 07:10:30 UTC) #21
On 2018/10/17 07:09:25, krystian wrote:
> On 2018/10/16 17:02:20, anton wrote:
> > On 2018/10/16 13:51:13, krystian wrote:
> > > On 2018/10/16 13:21:56, anton wrote:
> > > > On 2018/10/15 10:21:50, krystian wrote:
> > > > > On 2018/10/15 09:59:47, krystian wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.adblockplus.org/29908555/diff/29911555/adblock-android-tes...
> > > > > > File
> > > > > >
> > > > >
> > > >
> > >
> >
>
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/UpdaterCallbacksTest.java
> > > > > > (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.adblockplus.org/29908555/diff/29911555/adblock-android-tes...
> > > > > >
> > > > >
> > > >
> > >
> >
>
adblock-android-tests/src/org/adblockplus/libadblockplus/tests/UpdaterCallbacksTest.java:26:
> > > > > > public class UpdaterCallbacksTest extends UpdaterTest
> > > > > > This class is added instead of FilterEngineUpdaterTest class.
> > > > > 
> > > > > I just noticed I need to patch AdblockEngine, please wait for commit
#4.
> > > > 
> > > > Wow, that was fast!
> > > > However we need to land https://codereview.adblockplus.org/29819555/.
> > > > I'm afraid it can conflict with your changes.
> > > > 
> > > > Can we please do in the following way: remove this functionality as it
was
> > in
> > > > patch set #2 (without according Update java class) but also remove the
> > methods
> > > > from Java classes too?
> > > > And later as step #2 i will land the other part of changes.
> > > 
> > > OK, I'll revert to patch #2 adding cleanup in Java layer. I will push that
> > today
> > > or tomorrow morning.
> > > In case https://codereview.adblockplus.org/29819555/. gets merged first,
> I'll
> > > rebase my changes.
> > 
> > I've found some issues with that code review, so i think it makes sense to
> land
> > your code first.
> > Please just remove c++ and java code, no tests required.
> 
> OK, I think all is removed now.

Thanks!  LGTM
Sign in to reply to this message.

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