Issue 7035 - Update libadblockplus dependency

Issue 7035 - Update libadblockplus dependency

[krystian, 2018-10-12 09:17:34]

I guess I should create also JniUpdater.cpp for changed API in libadblockplus.
Unless it is not needed as it seems to be not used - please let me know.

[anton, 2018-10-12 09:27:11]

https://codereview.adblockplus.org/29908555/diff/29908556/adblock-android/jni...
File adblock-android/jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29908555/diff/29908556/adblock-android/jni...
adblock-android/jni/JniFilterEngine.cpp:176: 
does it mean we just dropped this functionality?

https://codereview.adblockplus.org/29908555/diff/29908556/dependencies
File dependencies (right):

https://codereview.adblockplus.org/29908555/diff/29908556/dependencies#newcode1
dependencies:1: _root = hg:https://hg.adblockplus.org/
git:https://github.com/adblockplus/
isn't better to use gitlab repository instead (since it's primary repository)?

[krystian, 2018-10-12 09:31:32]

https://codereview.adblockplus.org/29908555/diff/29908556/adblock-android/jni...
File adblock-android/jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29908555/diff/29908556/adblock-android/jni...
adblock-android/jni/JniFilterEngine.cpp:176: 
On 2018/10/12 09:27:11, anton wrote:
> does it mean we just dropped this functionality?

It was just moved to different component - libadblockplus/src/Updater.cpp.

[anton, 2018-10-12 09:34:23]

https://codereview.adblockplus.org/29908555/diff/29908556/adblock-android/jni...
File adblock-android/jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29908555/diff/29908556/adblock-android/jni...
adblock-android/jni/JniFilterEngine.cpp:176: 
On 2018/10/12 09:31:32, k.zlomek wrote:
> On 2018/10/12 09:27:11, anton wrote:
> > does it mean we just dropped this functionality?
> 
> It was just moved to different component - libadblockplus/src/Updater.cpp.

then we need to have according java code. Are you going to do it?

[sergei, 2018-10-12 09:34:38]

On 2018/10/12 09:17:34, k.zlomek wrote:
> I guess I should create also JniUpdater.cpp for changed API in libadblockplus.
> Unless it is not needed as it seems to be not used - please let me know.

I think we don't need it for present, so just dropping it is fine.

[anton, 2018-10-12 09:37:38]

On 2018/10/12 09:34:38, sergei wrote:
> On 2018/10/12 09:17:34, k.zlomek wrote:
> > I guess I should create also JniUpdater.cpp for changed API in
libadblockplus.
> > Unless it is not needed as it seems to be not used - please let me know.
> 
> I think we don't need it for present, so just dropping it is fine.

I think it's strange half-decision.
IMHO if we drop it we need to create java code for moved functionality.
Otherwise i don't see any point.

[krystian, 2018-10-12 09:41:26]

On 2018/10/12 09:37:38, anton wrote:
> On 2018/10/12 09:34:38, sergei wrote:
> > On 2018/10/12 09:17:34, k.zlomek wrote:
> > > I guess I should create also JniUpdater.cpp for changed API in
> libadblockplus.
> > > Unless it is not needed as it seems to be not used - please let me know.
> > 
> > I think we don't need it for present, so just dropping it is fine.
> 
> I think it's strange half-decision.
> IMHO if we drop it we need to create java code for moved functionality.
> Otherwise i don't see any point.

OK, I can provide JniUpdater.cpp in next patch.

[sergei, 2018-10-12 09:41:37]

On 2018/10/12 09:37:38, anton wrote:
> On 2018/10/12 09:34:38, sergei wrote:
> > On 2018/10/12 09:17:34, k.zlomek wrote:
> > > I guess I should create also JniUpdater.cpp for changed API in
> libadblockplus.
> > > Unless it is not needed as it seems to be not used - please let me know.
> > 
> > I think we don't need it for present, so just dropping it is fine.
> 
> I think it's strange half-decision.
> IMHO if we drop it we need to create java code for moved functionality.
> Otherwise i don't see any point.

It's not used and seems not going to be used. We can file a ticket for that but
we should not spend time on it now.

[sergei, 2018-10-12 09:42:29]

https://codereview.adblockplus.org/29908555/diff/29908556/adblock-android/jni...
File adblock-android/jni/JniFilterEngine.cpp (left):

https://codereview.adblockplus.org/29908555/diff/29908556/adblock-android/jni...
adblock-android/jni/JniFilterEngine.cpp:523: {
(char*)"setUpdateAvailableCallback", (char*)"(JJ)V",
(void*)JniSetUpdateAvailableCallback },
BTW, what about Java code? Should it be removed too?

https://codereview.adblockplus.org/29908555/diff/29908556/dependencies
File dependencies (right):

https://codereview.adblockplus.org/29908555/diff/29908556/dependencies#newcode1
dependencies:1: _root = hg:https://hg.adblockplus.org/
git:https://github.com/adblockplus/
On 2018/10/12 09:27:11, anton wrote:
> isn't better to use gitlab repository instead (since it's primary repository)?

buildtools are still not on gitlab, I would propose for the sake of simplicity
just keep github. We could rewrite this file in favor of switching to git in a
separate commit. I would propose

_root = git:https://gitlab.com/eyeo/adblockplus/
_self = buildtools/ensure_dependencies.py
buildtools = git:https://github.com/adblockplus/buildtools@96b22c9
libadblockplus = libadblockplus git:f2df538b1d66b982fba18a0bc36cf27c1b3bc716

[anton, 2018-10-12 09:42:34]

On 2018/10/12 09:41:37, sergei wrote:
> On 2018/10/12 09:37:38, anton wrote:
> > On 2018/10/12 09:34:38, sergei wrote:
> > > On 2018/10/12 09:17:34, k.zlomek wrote:
> > > > I guess I should create also JniUpdater.cpp for changed API in
> > libadblockplus.
> > > > Unless it is not needed as it seems to be not used - please let me know.
> > > 
> > > I think we don't need it for present, so just dropping it is fine.
> > 
> > I think it's strange half-decision.
> > IMHO if we drop it we need to create java code for moved functionality.
> > Otherwise i don't see any point.
> 
> It's not used and seems not going to be used. We can file a ticket for that
but
> we should not spend time on it now.

Then we should not spend time on this task now at all.

[sergei, 2018-10-12 09:48:39]

On 2018/10/12 09:42:34, anton wrote:
> Then we should not spend time on this task now at all.

It's not clear what task. We need to update libadblockplus in
libadblockplus-android because core has been updated in libadblockplus and it
has better memory usage, and now libadblockplus does not send the requests to
update.json by default. There were some complains about the latter, and BTW it's
that part which is refactored. Additionally the task is a part of onboarding, in
particular learning of the process.

[anton, 2018-10-12 09:53:46]

On 2018/10/12 09:48:39, sergei wrote:
> On 2018/10/12 09:42:34, anton wrote:
> > Then we should not spend time on this task now at all.
> 
> It's not clear what task.

Currently we have task to update to more recent version of libadblockplus.
It means saving the same functionality, not just "make it compilable".
If it's moved in libadblockplus then it should be moved in
libadblockplus-android too.
Just removing some methods and not creating in in other file means absolutely
meaningless to me.

> We need to update libadblockplus in
> libadblockplus-android because core has been updated in libadblockplus and it
> has better memory usage, and now libadblockplus does not send the requests to
> update.json by default. There were some complains about the latter, and BTW
it's
> that part which is refactored.

That's clear but it just means we need to write additional code in Java to still
have it (as we have it in libadblockplus).

> Additionally the task is a part of onboarding, in
> particular learning of the process.

It means this is not right task if you don't have time to do it properly.
Either we do it properly or not do it at all.

NOT LGTM

[sergei, 2018-10-12 10:43:38]

On 2018/10/12 09:53:46, anton wrote:
> It means this is not right task if you don't have time to do it properly.
> Either we do it properly or not do it at all.

I wrote
> It's not used and seems not going to be used. We can file a ticket for that
but
we should not spend time on it now.

So, it's not about not having the time, it's about using of the time in more
efficient manner, we can have a progress in terms of the memory usage, the round
of updating of libadblockplus in libadblockplus-android without the questionable
functionality, which is not used anyway, and stop questions about that
update.json or have no progress at all.

[krystian, 2018-10-15 07:07:25]

On 2018/10/12 10:43:38, sergei wrote:
> On 2018/10/12 09:53:46, anton wrote:
> > It means this is not right task if you don't have time to do it properly.
> > Either we do it properly or not do it at all.
> 
> I wrote
> > It's not used and seems not going to be used. We can file a ticket for that
> but
> we should not spend time on it now.
> 
> So, it's not about not having the time, it's about using of the time in more
> efficient manner, we can have a progress in terms of the memory usage, the
round
> of updating of libadblockplus in libadblockplus-android without the
questionable
> functionality, which is not used anyway, and stop questions about that
> update.json or have no progress at all.

Guys, I forgot to update Java code in my 2nd patch, I'll push patch #3 very
soon.

[krystian, 2018-10-15 09:59:47]

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.

[krystian, 2018-10-15 10:21:50]

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.

[anton, 2018-10-16 13:21:56]

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.

[krystian, 2018-10-16 13:51:13]

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.

[anton, 2018-10-16 17:02:20]

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.

[krystian, 2018-10-17 07:09:25]

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.

[anton, 2018-10-17 07:10:30]

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