Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29379647: Issue 4948 - add possibility to not send data depending on connection properties (Closed)

Created:
March 10, 2017, 6:40 a.m. by anton
Modified:
March 30, 2017, 8:42 p.m.
CC:
René Jeschke
Visibility:
Public.

Description

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/

Patch Set 1 #

Total comments: 10

Patch Set 2 : after Sergei's comments minor clean-up #

Patch Set 3 : migrated from FilterEngine* to FilterEnginePtr*, minor improvements #

Total comments: 11

Patch Set 4 : reverted using latest tools, sergei's comments #

Total comments: 3

Patch Set 5 : removed ->get() to be consistent with JniJsEngine #

Patch Set 6 : updated to 4931 code review patch set 3 and later #

Total comments: 13

Patch Set 7 : felix's comments #

Patch Set 8 : updated dependency to libadblockplus-binaries #

Patch Set 9 : simplified ConnectionType, added android permissions note in README #

Total comments: 2

Patch Set 10 : updated dependency to binaries, updated comment for allowed connection type #

Unified diffs Side-by-side diffs Delta from patch set Stats (+441 lines, -65 lines) Patch
M README.md View 1 2 3 4 5 6 7 8 1 chunk +8 lines, -0 lines 0 comments Download
M dependencies View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M libadblockplus-android-settings/AndroidManifest.xml View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M libadblockplus-android-settings/res/values/adblock_settings_strings.xml View 1 chunk +4 lines, -0 lines 0 comments Download
M libadblockplus-android-settings/res/values/adblock_settings_strings_internal.xml View 1 chunk +1 line, -0 lines 0 comments Download
M libadblockplus-android-settings/res/xml/preference_adblock_general.xml View 1 chunk +6 lines, -0 lines 0 comments Download
M libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java View 1 2 3 4 5 6 7 8 9 6 chunks +23 lines, -4 lines 0 comments Download
M libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettings.java View 3 chunks +12 lines, -0 lines 0 comments Download
M libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockSettingsStorage.java View 1 chunk +2 lines, -0 lines 0 comments Download
A libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/ConnectionType.java View 1 2 3 4 5 6 7 8 1 chunk +89 lines, -0 lines 0 comments Download
M libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/GeneralSettingsFragment.java View 1 2 8 chunks +55 lines, -0 lines 0 comments Download
A libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java View 1 2 3 4 5 6 7 8 1 chunk +73 lines, -0 lines 0 comments Download
M libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/SharedPrefsStorage.java View 3 chunks +8 lines, -0 lines 0 comments Download
M libadblockplus-android-webview/src/org/adblockplus/libadblockplus/android/webview/AdblockWebView.java View 1 chunk +3 lines, -1 line 0 comments Download
M libadblockplus-android/jni/Android.mk View 1 chunk +1 line, -0 lines 0 comments Download
M libadblockplus-android/jni/JniCallbacks.h View 1 chunk +7 lines, -0 lines 0 comments Download
M libadblockplus-android/jni/JniFilterEngine.cpp View 1 2 3 4 5 24 chunks +91 lines, -35 lines 0 comments Download
A + libadblockplus-android/jni/JniIsAllowedConnectionTypeCallback.cpp View 2 chunks +18 lines, -15 lines 0 comments Download
M libadblockplus-android/src/org/adblockplus/libadblockplus/FilterEngine.java View 4 chunks +26 lines, -3 lines 0 comments Download
A + libadblockplus-android/src/org/adblockplus/libadblockplus/IsAllowedConnectionCallback.java View 2 chunks +3 lines, -3 lines 0 comments Download
M libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java View 4 chunks +7 lines, -3 lines 0 comments Download

Messages

Total messages: 22
anton
https://codereview.adblockplus.org/29379647/diff/29379648/gradle/wrapper/gradle-wrapper.properties File gradle/wrapper/gradle-wrapper.properties (right): https://codereview.adblockplus.org/29379647/diff/29379648/gradle/wrapper/gradle-wrapper.properties#newcode5 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 ...
March 10, 2017, 6:50 a.m. (2017-03-10 06:50:40 UTC) #1
sergei
I have taken a look only at a small part of the code and would ...
March 15, 2017, 10:01 a.m. (2017-03-15 10:01:38 UTC) #2
anton
https://codereview.adblockplus.org/29379647/diff/29379648/libadblockplus-android/jni/JniFilterEngine.cpp File libadblockplus-android/jni/JniFilterEngine.cpp (right): https://codereview.adblockplus.org/29379647/diff/29379648/libadblockplus-android/jni/JniFilterEngine.cpp#newcode52 libadblockplus-android/jni/JniFilterEngine.cpp:52: AdblockPlus::FilterEnginePtr filterEnginePtr = NULL; On 2017/03/15 10:01:38, sergei wrote: ...
March 15, 2017, 10:13 a.m. (2017-03-15 10:13:52 UTC) #3
anton
https://codereview.adblockplus.org/29379647/diff/29379648/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29379647/diff/29379648/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java#newcode129 libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:129: engine.getFilterEngine().setAllowedConnectionType("new_type"); to be removed!
March 15, 2017, 12:25 p.m. (2017-03-15 12:25:07 UTC) #4
anton
https://codereview.adblockplus.org/29379647/diff/29384731/libadblockplus-android-settings/AndroidManifest.xml File libadblockplus-android-settings/AndroidManifest.xml (right): https://codereview.adblockplus.org/29379647/diff/29384731/libadblockplus-android-settings/AndroidManifest.xml#newcode12 libadblockplus-android-settings/AndroidManifest.xml:12: <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE"/> tested and it's required for ConnectivityManager access ...
March 15, 2017, 12:52 p.m. (2017-03-15 12:52:54 UTC) #5
sergei
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 ...
March 15, 2017, 1:41 p.m. (2017-03-15 13:41:49 UTC) #6
anton
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 ...
March 15, 2017, 1:57 p.m. (2017-03-15 13:57:25 UTC) #7
sergei
C++ part LGTM
March 16, 2017, 12:15 p.m. (2017-03-16 12:15:54 UTC) #8
diegocarloslima
https://codereview.adblockplus.org/29379647/diff/29384788/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29379647/diff/29384788/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java#newcode127 libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:127: Log.d(TAG, "Adblock engine created"); Why don't you use the ...
March 20, 2017, 2:05 p.m. (2017-03-20 14:05:41 UTC) #9
diegocarloslima
LGTM, but please take a look on my comment regarding the creation of AdblockEngine in ...
March 20, 2017, 2:42 p.m. (2017-03-20 14:42:06 UTC) #10
anton
On 2017/03/20 14:42:06, diegocarloslima wrote: > LGTM, but please take a look on my comment ...
March 21, 2017, 5:48 a.m. (2017-03-21 05:48:23 UTC) #11
anton
https://codereview.adblockplus.org/29379647/diff/29384788/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29379647/diff/29384788/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java#newcode127 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: ...
March 21, 2017, 5:48 a.m. (2017-03-21 05:48:30 UTC) #12
anton
On 2017/03/16 12:15:54, sergei wrote: > C++ part LGTM Please review C++ part of https://codereview.adblockplus.org/29379647/#ps29390555
March 21, 2017, 8:14 a.m. (2017-03-21 08:14:15 UTC) #13
sergei
On 2017/03/21 08:14:15, anton wrote: > On 2017/03/16 12:15:54, sergei wrote: > > C++ part ...
March 21, 2017, 9:26 a.m. (2017-03-21 09:26:45 UTC) #14
Felix Dahlke
Only did a high level review, but looks good by and large. Some minor comments. ...
March 24, 2017, 7:37 a.m. (2017-03-24 07:37:25 UTC) #15
anton
https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java (right): https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java#newcode123 libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/AdblockHelper.java:123: null, On 2017/03/24 07:37:24, Felix Dahlke wrote: > How ...
March 24, 2017, 9:43 a.m. (2017-03-24 09:43:12 UTC) #16
anton
On 2017/03/24 07:37:25, Felix Dahlke wrote: > Only did a high level review, but looks ...
March 24, 2017, 9:48 a.m. (2017-03-24 09:48:20 UTC) #17
Felix Dahlke
LGTM https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/ConnectionType.java File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/ConnectionType.java (right): https://codereview.adblockplus.org/29379647/diff/29390630/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/ConnectionType.java#newcode3 libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/ConnectionType.java:3: * Copyright (C) 2006-2016 Eyeo GmbH On 2017/03/24 ...
March 27, 2017, 2:18 p.m. (2017-03-27 14:18:12 UTC) #18
anton
https://codereview.adblockplus.org/29379647/diff/29397555/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java (right): https://codereview.adblockplus.org/29379647/diff/29397555/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java#newcode67 libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java:67: Log.w(TAG, "Current connection type is not allowed for web ...
March 29, 2017, 6:05 a.m. (2017-03-29 06:05:35 UTC) #19
Felix Dahlke
https://codereview.adblockplus.org/29379647/diff/29397555/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java File libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java (right): https://codereview.adblockplus.org/29379647/diff/29397555/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java#newcode67 libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java:67: Log.w(TAG, "Current connection type is not allowed for web ...
March 30, 2017, 1:18 p.m. (2017-03-30 13:18:03 UTC) #20
sergei
On 2017/03/30 13:18:03, Felix Dahlke wrote: > https://codereview.adblockplus.org/29379647/diff/29397555/libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java > File > libadblockplus-android-settings/src/org/adblockplus/libadblockplus/android/settings/IsAllowedConnectionCallbackImpl.java > (right): > ...
March 30, 2017, 1:20 p.m. (2017-03-30 13:20:57 UTC) #21
Felix Dahlke
March 30, 2017, 1:44 p.m. (2017-03-30 13:44:10 UTC) #22
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.

Powered by Google App Engine
This is Rietveld