Issue 4948 - add possibility to not send data depending on connection properties

Issue 4948 - add possibility to not send data depending on connection properties

#depends on:
   https://codereview.adblockplus.org/29377570/ 
   https://codereview.adblockplus.org/29377054/

requires binaries to be updated":
   https://codereview.adblockplus.org/29391555
   https://codereview.adblockplus.org/29397642/

it requires libadblockplus-binaries and they depends on:
   https://codereview.adblockplus.org/29389580/

[anton, 2017-03-10 06:50:40]

https://codereview.adblockplus.org/29379647/diff/29379648/gradle/wrapper/grad...
File gradle/wrapper/gradle-wrapper.properties (right):

https://codereview.adblockplus.org/29379647/diff/29379648/gradle/wrapper/grad...
gradle/wrapper/gradle-wrapper.properties:5:
distributionUrl=https\://services.gradle.org/distributions/gradle-3.3-all.zip
actually it's not required to the task but for the IDE: the latest AndroidStudio
2.3 requires gradle 3.3 only.
I think it will not hurt to require for the latest version here

https://codereview.adblockplus.org/29379647/diff/29379648/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java
(right):

https://codereview.adblockplus.org/29379647/diff/29379648/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:118:
engine = AdblockEngine.create(
i'm thinking of refactoring in AdblockEngine: moving from ctor with lots of
arguments to Builder pattern as arguments count is growing all the time

https://codereview.adblockplus.org/29379647/diff/29379648/libadblockplus-andr...
File libadblockplus-android/jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29379647/diff/29379648/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:71: return
JniPtrToLong(filterEnginePtr.get());
for some (unknown) reason new static methods (instead of ctor previously)
returns incorrect pointer. iw as unable to test because of this

https://codereview.adblockplus.org/29379647/diff/29379648/libadblockplus-andr...
File libadblockplus-android/jni/JniIsAllowedConnectionTypeCallback.cpp (right):

https://codereview.adblockplus.org/29379647/diff/29379648/libadblockplus-andr...
libadblockplus-android/jni/JniIsAllowedConnectionTypeCallback.cpp:20: 
is seems to be Rietveld bug here: the file was absent but somehow it compares it
with another initial file

[sergei, 2017-03-15 10:01:38]

I have taken a look only at a small part of the code and would like to leave
some comments regarding it.

https://codereview.adblockplus.org/29379647/diff/29379648/libadblockplus-andr...
File libadblockplus-android/jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29379647/diff/29379648/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:52: AdblockPlus::FilterEnginePtr
filterEnginePtr = NULL;
smart pointer is already initialized to nullptr, so no need for " = NULL" here.

https://codereview.adblockplus.org/29379647/diff/29379648/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:52: AdblockPlus::FilterEnginePtr
filterEnginePtr = NULL;
I would just call the variable as filterEngine, without Ptr suffix, especially
there is no need for it, like collision with another variable.

https://codereview.adblockplus.org/29379647/diff/29379648/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:71: return
JniPtrToLong(filterEnginePtr.get());
TL;DR: Actually here is a bug, but my vote is to accept it as is for present.

Long story:
The bug is that instance of FilterEngine is destroyed after finishing of the
method but we continue to use the dangling pointer. It is destroyed because we
exit from the scope, no one keeps the reference to FilterEngine and
FilterEnginePtr destroys it.

There are several ways to figure it out but I think it should be firstly
addressed in libadblockplus and we should not disturb the code of
libadblockplus-android because of bugs in libadblockplus. On the interface level
I plan to not force the user of libadblockplus to use shared_ptr<FilterEngine>
and change it to std::unique_ptr<FilterEngine> (or even to FilterEngine, though
it's unlikely), it will require to change only this particular part.

I'm ready to accept it because currently we have a circular reference and
FilterEngine is not destroyed, so technically it does work now, though it's
incorrect. Secondly, I think that fixing it now in libadblockplus-android will
introduce only numerous changes most of which will be reverted afterwards, and
this actions are unnecessary and error-prone. So for the sake of simplicity I
think we can accept it for present, then fix in libadblockplus and then change
probably only one line here.

[anton, 2017-03-15 10:13:52]

https://codereview.adblockplus.org/29379647/diff/29379648/libadblockplus-andr...
File libadblockplus-android/jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29379647/diff/29379648/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:52: AdblockPlus::FilterEnginePtr
filterEnginePtr = NULL;
On 2017/03/15 10:01:38, sergei wrote:
> smart pointer is already initialized to nullptr, so no need for " = NULL"
here.

Acknowledged.

https://codereview.adblockplus.org/29379647/diff/29379648/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:71: return
JniPtrToLong(filterEnginePtr.get());
On 2017/03/15 10:01:38, sergei wrote:
> TL;DR: Actually here is a bug, but my vote is to accept it as is for present.
> 
> Long story:
> The bug is that instance of FilterEngine is destroyed after finishing of the
> method but we continue to use the dangling pointer. It is destroyed because we
> exit from the scope, no one keeps the reference to FilterEngine and
> FilterEnginePtr destroys it.
> 
> There are several ways to figure it out but I think it should be firstly
> addressed in libadblockplus and we should not disturb the code of
> libadblockplus-android because of bugs in libadblockplus. On the interface
level
> I plan to not force the user of libadblockplus to use shared_ptr<FilterEngine>
> and change it to std::unique_ptr<FilterEngine> (or even to FilterEngine,
though
> it's unlikely), it will require to change only this particular part.
> 
> I'm ready to accept it because currently we have a circular reference and
> FilterEngine is not destroyed, so technically it does work now, though it's
> incorrect. Secondly, I think that fixing it now in libadblockplus-android will
> introduce only numerous changes most of which will be reverted afterwards, and
> this actions are unnecessary and error-prone. So for the sake of simplicity I
> think we can accept it for present, then fix in libadblockplus and then change
> probably only one line here.

Acknowledged.

[anton, 2017-03-15 12:25:07]

https://codereview.adblockplus.org/29379647/diff/29379648/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java
(right):

https://codereview.adblockplus.org/29379647/diff/29379648/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:129:
engine.getFilterEngine().setAllowedConnectionType("new_type");
to be removed!

[anton, 2017-03-15 12:52:54]

https://codereview.adblockplus.org/29379647/diff/29384731/libadblockplus-andr...
File libadblockplus-android-settings/AndroidManifest.xml (right):

https://codereview.adblockplus.org/29379647/diff/29384731/libadblockplus-andr...
libadblockplus-android-settings/AndroidManifest.xml:12: <uses-permission
android:name="android.permission.ACCESS_NETWORK_STATE"/>
tested and it's required for ConnectivityManager access

https://codereview.adblockplus.org/29379647/diff/29384731/libadblockplus-andr...
File libadblockplus-android/jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29379647/diff/29384731/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:73: return
JniPtrToLong(filterEngine);
i had to migrate from FilterEngine* to FilterEnginePtr* (which is actually
shared_ptr) as otherwise filter engine instance was released somehow in
libadblockplus internals - tested with Sergei

[sergei, 2017-03-15 13:41:49]

https://codereview.adblockplus.org/29379647/diff/29384731/build.gradle
File build.gradle (right):

https://codereview.adblockplus.org/29379647/diff/29384731/build.gradle#newcode8
build.gradle:8: classpath 'com.android.tools.build:gradle:2.3.0'
I don't know you policies but should not it be in another commit?

https://codereview.adblockplus.org/29379647/diff/29384731/libadblockplus-andr...
File libadblockplus-android-settings/build.gradle (right):

https://codereview.adblockplus.org/29379647/diff/29384731/libadblockplus-andr...
libadblockplus-android-settings/build.gradle:5: buildToolsVersion '25.0.0'
I don't know you policies but should not it be in another commit?

https://codereview.adblockplus.org/29379647/diff/29384731/libadblockplus-andr...
File libadblockplus-android-tests/build.gradle (right):

https://codereview.adblockplus.org/29379647/diff/29384731/libadblockplus-andr...
libadblockplus-android-tests/build.gradle:17: buildToolsVersion '25.0.0'
I don't know you policies but should not it be in another commit?

https://codereview.adblockplus.org/29379647/diff/29384731/libadblockplus-andr...
File libadblockplus-android-webview/build.gradle (right):

https://codereview.adblockplus.org/29379647/diff/29384731/libadblockplus-andr...
libadblockplus-android-webview/build.gradle:5: buildToolsVersion '25.0.0'
I don't know you policies but should not it be in another commit?

https://codereview.adblockplus.org/29379647/diff/29384731/libadblockplus-andr...
File libadblockplus-android-webviewapp/build.gradle (right):

https://codereview.adblockplus.org/29379647/diff/29384731/libadblockplus-andr...
libadblockplus-android-webviewapp/build.gradle:17: buildToolsVersion '25.0.0'
I don't know you policies but should not it be in another commit?

https://codereview.adblockplus.org/29379647/diff/29384731/libadblockplus-andr...
File libadblockplus-android/build.gradle (right):

https://codereview.adblockplus.org/29379647/diff/29384731/libadblockplus-andr...
libadblockplus-android/build.gradle:5: buildToolsVersion '25.0.0'
I don't know you policies but should not it be in another commit?

https://codereview.adblockplus.org/29379647/diff/29384731/libadblockplus-andr...
File libadblockplus-android/jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29379647/diff/29384731/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:73: return
JniPtrToLong(filterEngine);
On 2017/03/15 12:52:54, anton wrote:
> i had to migrate from FilterEngine* to FilterEnginePtr* (which is actually
> shared_ptr) as otherwise filter engine instance was released somehow in
> libadblockplus internals - tested with Sergei

It's not removed in internals of libadblockplus, it was indeed the case I have
described in
https://codereview.adblockplus.org/29379647/diff/29379648/libadblockplus-andr....
There are no references to FilterEngine, it's destroyed and we have a dangling
pointer.

https://codereview.adblockplus.org/29379647/diff/29384731/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:466: value = &stdValue;
here is a bug:
you assign a pointer to stdValue which is allocated on the stack and which is
about to be destroyed because the scope ends on the next line. The value of
`value` variable cannot be used later.

I would just create std::string on the stack and say something like:
auto value = jvalue ? &(stdStringValue = JniJavaToStdString(env, jvalue)) :
nullptr;

[anton, 2017-03-15 13:57:25]

https://codereview.adblockplus.org/29379647/diff/29384731/build.gradle
File build.gradle (right):

https://codereview.adblockplus.org/29379647/diff/29384731/build.gradle#newcode8
build.gradle:8: classpath 'com.android.tools.build:gradle:2.3.0'
On 2017/03/15 13:41:47, sergei wrote:
> I don't know you policies but should not it be in another commit?

yes, it should. i will fix it

[sergei, 2017-03-16 12:15:54]

C++ part LGTM

[diegocarloslima, 2017-03-20 14:05:41]

https://codereview.adblockplus.org/29379647/diff/29384788/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java
(right):

https://codereview.adblockplus.org/29379647/diff/29384788/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:127:
Log.d(TAG, "Adblock engine created");
Why don't you use the other constructor here, which would avoid passing 4 null
arguments?

[diegocarloslima, 2017-03-20 14:42:06]

LGTM, but please take a look on my comment regarding the creation of
AdblockEngine in AdblockHelper

https://codereview.adblockplus.org/29379647/diff/29384788/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java
(right):

https://codereview.adblockplus.org/29379647/diff/29384788/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:127:
Log.d(TAG, "Adblock engine created");
On 2017/03/20 14:05:40, diegocarloslima wrote:
> Why don't you use the other constructor here, which would avoid passing 4 null
> arguments?

Just saw that on Issue 5010 you added the Builder pattern to the AdblockEngine.
Since we'll be using a builder, you can ignore my previous comment. I think that
you should add this issue as blocking 5010 or make the Builder refactoring in a
follow up issue . Otherwise it will be missing the code change here for adding
the IsAllowedConnectionCallback using the Builder pattern.

[anton, 2017-03-21 05:48:23]

On 2017/03/20 14:42:06, diegocarloslima wrote:
> LGTM, but please take a look on my comment regarding the creation of
> AdblockEngine in AdblockHelper
> 
>
https://codereview.adblockplus.org/29379647/diff/29384788/libadblockplus-andr...
> File
>
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java
> (right):
> 
>
https://codereview.adblockplus.org/29379647/diff/29384788/libadblockplus-andr...
>
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:127:
> Log.d(TAG, "Adblock engine created");
> On 2017/03/20 14:05:40, diegocarloslima wrote:
> > Why don't you use the other constructor here, which would avoid passing 4
null
> > arguments?
> 
> Just saw that on Issue 5010 you added the Builder pattern to the
AdblockEngine.
> Since we'll be using a builder, you can ignore my previous comment. I think
that
> you should add this issue as blocking 5010 or make the Builder refactoring in
a
> follow up issue . Otherwise it will be missing the code change here for adding
> the IsAllowedConnectionCallback using the Builder pattern.

Since the tasks are independent it's overhead of having multiple code reviews
that change the same file.
I will merge the changes for both (as new builder method)

[anton, 2017-03-21 05:48:30]

https://codereview.adblockplus.org/29379647/diff/29384788/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java
(right):

https://codereview.adblockplus.org/29379647/diff/29384788/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:127:
Log.d(TAG, "Adblock engine created");
On 2017/03/20 14:42:04, diegocarloslima wrote:
> On 2017/03/20 14:05:40, diegocarloslima wrote:
> > Why don't you use the other constructor here, which would avoid passing 4
null
> > arguments?
> 
> Just saw that on Issue 5010 you added the Builder pattern to the
AdblockEngine.
> Since we'll be using a builder, you can ignore my previous comment. I think
that
> you should add this issue as blocking 5010 or make the Builder refactoring in
a
> follow up issue . Otherwise it will be missing the code change here for adding
> the IsAllowedConnectionCallback using the Builder pattern.

yes, ctor is getting more and more complex with different sets of independent
arguments so builder is the solution most likely

[anton, 2017-03-21 08:14:15]

On 2017/03/16 12:15:54, sergei wrote:
> C++ part LGTM

Please review C++ part of
https://codereview.adblockplus.org/29379647/#ps29390555

[sergei, 2017-03-21 09:26:45]

On 2017/03/21 08:14:15, anton wrote:
> On 2017/03/16 12:15:54, sergei wrote:
> > C++ part LGTM
> 
> Please review C++ part of
> https://codereview.adblockplus.org/29379647/#ps29390555

LGTM

[Felix Dahlke, 2017-03-24 07:37:25]

Only did a high level review, but looks good by and large. Some minor comments.

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java
(right):

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:123:
null,
How I saw it, there is a version of create where you wouldn't have to pass in
those four `null`s here, or did I miss something?

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:126:
null); // `true` as we need element hiding
Nit: Seems like this comment should move up now, so it's next to the `true`
being passed.

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/ConnectionType.java
(right):

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/ConnectionType.java:3:
* Copyright (C) 2006-2016 Eyeo GmbH
2017 :)

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java
(right):

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java:3:
* Copyright (C) 2006-2016 Eyeo GmbH
2017 :)

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
File libadblockplus-android/jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:87: AdblockPlus::FilterEnginePtr&
engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
I might be missing something, but is this change actually needed for #4948? It
seems unrelated at first glance. Same goes for the similar changes below.

[anton, 2017-03-24 09:43:12]

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java
(right):

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:123:
null,
On 2017/03/24 07:37:24, Felix Dahlke wrote:
> How I saw it, there is a version of create where you wouldn't have to pass in
> those four `null`s here, or did I miss something?

Actually you're right and i can just use the version without 4 nulls.
But since i know about upcoming changes (refactoring of the code to use Builder
instead of method with independent argument sets
i did not take care of it too much here. I can change to use method without 4
nulls passed if you insist, do you?

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:126:
null); // `true` as we need element hiding
On 2017/03/24 07:37:24, Felix Dahlke wrote:
> Nit: Seems like this comment should move up now, so it's next to the `true`
> being passed.

Acknowledged.

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/ConnectionType.java
(right):

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/ConnectionType.java:3:
* Copyright (C) 2006-2016 Eyeo GmbH
On 2017/03/24 07:37:24, Felix Dahlke wrote:
> 2017 :)

Yes, i've left it 2016 as i will update 2016 to 2017 in all the files at once in
separate task.

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java
(right):

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java:3:
* Copyright (C) 2006-2016 Eyeo GmbH
On 2017/03/24 07:37:25, Felix Dahlke wrote:
> 2017 :)

Same here - is it ok to create separate task or even "noissue" to update the
year?

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
File libadblockplus-android/jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:87: AdblockPlus::FilterEnginePtr&
engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
On 2017/03/24 07:37:25, Felix Dahlke wrote:
> I might be missing something, but is this change actually needed for #4948? It
> seems unrelated at first glance. Same goes for the similar changes below.

Yes, it's required as it comes from libadblockplus API changes: now it returns
smart pointer to engine instead of just engine pointer.
So we had to store smart pointer too and change type casting.

[anton, 2017-03-24 09:48:20]

On 2017/03/24 07:37:25, Felix Dahlke wrote:
> Only did a high level review, but looks good by and large. Some minor
comments.
> 
>
https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
> File
>
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java
> (right):
> 
>
https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
>
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:123:
> null,
> How I saw it, there is a version of create where you wouldn't have to pass in
> those four `null`s here, or did I miss something?
> 
>
https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
>
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:126:
> null); // `true` as we need element hiding
> Nit: Seems like this comment should move up now, so it's next to the `true`
> being passed.
> 
>
https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
> File
>
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/ConnectionType.java
> (right):
> 
>
https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
>
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/ConnectionType.java:3:
> * Copyright (C) 2006-2016 Eyeo GmbH
> 2017 :)
> 
>
https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
> File
>
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java
> (right):
> 
>
https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
>
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java:3:
> * Copyright (C) 2006-2016 Eyeo GmbH
> 2017 :)
> 
>
https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
> File libadblockplus-android/jni/JniFilterEngine.cpp (right):
> 
>
https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
> libadblockplus-android/jni/JniFilterEngine.cpp:87:
AdblockPlus::FilterEnginePtr&
> engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
> I might be missing something, but is this change actually needed for #4948? It
> seems unrelated at first glance. Same goes for the similar changes below.

Updated using method without nulls in patch set 7

[Felix Dahlke, 2017-03-27 14:18:12]

LGTM

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/ConnectionType.java
(right):

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/ConnectionType.java:3:
* Copyright (C) 2006-2016 Eyeo GmbH
On 2017/03/24 09:43:11, anton wrote:
> On 2017/03/24 07:37:24, Felix Dahlke wrote:
> > 2017 :)
> 
> Yes, i've left it 2016 as i will update 2016 to 2017 in all the files at once
in
> separate task.

Acknowledged.

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java
(right):

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java:3:
* Copyright (C) 2006-2016 Eyeo GmbH
On 2017/03/24 09:43:11, anton wrote:
> On 2017/03/24 07:37:25, Felix Dahlke wrote:
> > 2017 :)
> 
> Same here - is it ok to create separate task or even "noissue" to update the
> year?

Acknowledged.

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
File libadblockplus-android/jni/JniFilterEngine.cpp (right):

https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-andr...
libadblockplus-android/jni/JniFilterEngine.cpp:87: AdblockPlus::FilterEnginePtr&
engine = *JniLongToTypePtr<AdblockPlus::FilterEnginePtr>(ptr);
On 2017/03/24 09:43:11, anton wrote:
> On 2017/03/24 07:37:25, Felix Dahlke wrote:
> > I might be missing something, but is this change actually needed for #4948?
It
> > seems unrelated at first glance. Same goes for the similar changes below.
> 
> Yes, it's required as it comes from libadblockplus API changes: now it returns
> smart pointer to engine instead of just engine pointer.
> So we had to store smart pointer too and change type casting.

Acknowledged.

[anton, 2017-03-29 06:05:35]

https://codereview.adblockplus.org/29379647/diff/29397555/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java
(right):

https://codereview.adblockplus.org/29379647/diff/29397555/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java:67:
Log.w(TAG, "Current connection type is not allowed for web requests");
BTW is allows/rejects all web requests (not only subscriptions, but
notifications and available updates). is it ok?

[Felix Dahlke, 2017-03-30 13:18:03]

https://codereview.adblockplus.org/29379647/diff/29397555/libadblockplus-andr...
File
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java
(right):

https://codereview.adblockplus.org/29379647/diff/29397555/libadblockplus-andr...
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java:67:
Log.w(TAG, "Current connection type is not allowed for web requests");
On 2017/03/29 06:05:34, anton wrote:
> BTW is allows/rejects all web requests (not only subscriptions, but
> notifications and available updates). is it ok?

No, I think notifications and update manifests should always be downloaded,
those are pretty small files.

[sergei, 2017-03-30 13:20:57]

On 2017/03/30 13:18:03, Felix Dahlke wrote:
>
https://codereview.adblockplus.org/29379647/diff/29397555/libadblockplus-andr...
> File
>
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java
> (right):
> 
>
https://codereview.adblockplus.org/29379647/diff/29397555/libadblockplus-andr...
>
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java:67:
> Log.w(TAG, "Current connection type is not allowed for web requests");
> On 2017/03/29 06:05:34, anton wrote:
> > BTW is allows/rejects all web requests (not only subscriptions, but
> > notifications and available updates). is it ok?
> 
> No, I think notifications and update manifests should always be downloaded,
> those are pretty small files.

I would say they should be downloaded for many reasons. It will be fixed later
and it should be done in adblockpluscore without hacks on the upper/lower
levels.

[Felix Dahlke, 2017-03-30 13:44:10]

On 2017/03/30 13:20:57, sergei wrote:
> On 2017/03/30 13:18:03, Felix Dahlke wrote:
> >
>
https://codereview.adblockplus.org/29379647/diff/29397555/libadblockplus-andr...
> > File
> >
>
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java
> > (right):
> > 
> >
>
https://codereview.adblockplus.org/29379647/diff/29397555/libadblockplus-andr...
> >
>
libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java:67:
> > Log.w(TAG, "Current connection type is not allowed for web requests");
> > On 2017/03/29 06:05:34, anton wrote:
> > > BTW is allows/rejects all web requests (not only subscriptions, but
> > > notifications and available updates). is it ok?
> > 
> > No, I think notifications and update manifests should always be downloaded,
> > those are pretty small files.
> 
> I would say they should be downloaded for many reasons. It will be fixed later
> and it should be done in adblockpluscore without hacks on the upper/lower
> levels.

Agreed.