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

Issue 5697499218051072: Usage of new API, cleanups (reduced) (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 3 months ago by René Jeschke
Modified:
5 years, 2 months ago
Reviewers:
Felix Dahlke
CC:
Wladimir Palant
Visibility:
Public.

Description

This is a monster change, sorry. This version now only uses the new API, ad blocking and updates are working. Short summary of the changes: - Whitespace, indentation fixed - 'final' where appropriate - Removed all old JNI code - Replaced ABPEngine - Cleanups What's still to do (I will create issues for that): - ensure API7 compatibility - improve robustness of JNI code - various small Android related fixes

Patch Set 1 #

Total comments: 44

Patch Set 2 : Adressed first batch of review issues. #

Patch Set 3 : Fixed leftover first-batch review issues. #

Patch Set 4 : Removed newly added neetutils code #

Total comments: 44

Patch Set 5 : Addressed new review issues #

Patch Set 6 : Removed another whitespace change #

Patch Set 7 : Removed even more unrelated changes #

Total comments: 5

Patch Set 8 : Even more review issues fixed. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1274 lines, -1994 lines) Patch
M .hgignore View 1 chunk +1 line, -1 line 0 comments Download
D jni/AbpEngine.cpp View 1 chunk +0 lines, -558 lines 0 comments Download
M jni/Android.mk View 1 1 chunk +3 lines, -5 lines 0 comments Download
D jni/AndroidLogSystem.h View 1 chunk +0 lines, -30 lines 0 comments Download
D jni/AndroidLogSystem.cpp View 1 chunk +0 lines, -51 lines 0 comments Download
D jni/AndroidWebRequest.h View 1 chunk +0 lines, -57 lines 0 comments Download
D jni/AndroidWebRequest.cpp View 1 chunk +0 lines, -323 lines 0 comments Download
M jni/Application.mk View 1 1 chunk +1 line, -1 line 0 comments Download
D jni/Debug.h View 1 chunk +0 lines, -18 lines 0 comments Download
M jni/JniCallbacks.h View 1 4 chunks +8 lines, -5 lines 0 comments Download
M jni/JniCallbacks.cpp View 1 1 chunk +2 lines, -2 lines 0 comments Download
M jni/JniFilterChangeCallback.cpp View 1 2 chunks +2 lines, -2 lines 0 comments Download
M jni/JniJsValue.h View 1 chunk +1 line, -1 line 0 comments Download
M jni/JniJsValue.cpp View 4 chunks +17 lines, -9 lines 0 comments Download
M jni/JniLogSystem.cpp View 1 1 chunk +1 line, -2 lines 0 comments Download
M jni/JniWebRequest.cpp View 1 2 3 2 chunks +4 lines, -4 lines 0 comments Download
M jni/Utils.h View 3 chunks +12 lines, -14 lines 0 comments Download
M jni/Utils.cpp View 1 chunk +0 lines, -20 lines 0 comments Download
D src/com/github/rjeschke/neetutils/Objects.java View 1 2 3 1 chunk +0 lines, -135 lines 0 comments Download
D src/com/github/rjeschke/neetutils/collections/Tuple.java View 1 2 3 1 chunk +0 lines, -72 lines 0 comments Download
M src/org/adblockplus/ChunkedOutputStream.java View 1 2 3 4 5 5 chunks +6 lines, -6 lines 0 comments Download
M src/org/adblockplus/android/ABPEngine.java View 1 2 3 4 5 6 7 1 chunk +203 lines, -67 lines 0 comments Download
M src/org/adblockplus/android/AboutDialog.java View 1 3 chunks +6 lines, -6 lines 0 comments Download
M src/org/adblockplus/android/AdblockPlus.java View 1 2 3 4 5 6 24 chunks +64 lines, -80 lines 0 comments Download
M src/org/adblockplus/android/AdvancedPreferences.java View 1 2 3 4 5 6 11 chunks +40 lines, -35 lines 0 comments Download
A + src/org/adblockplus/android/AndroidFilterChangeCallback.java View 1 chunk +14 lines, -18 lines 0 comments Download
A + src/org/adblockplus/android/AndroidLogSystem.java View 1 chunk +27 lines, -9 lines 0 comments Download
A src/org/adblockplus/android/AndroidUpdateAvailableCallback.java View 1 2 3 1 chunk +50 lines, -0 lines 0 comments Download
A src/org/adblockplus/android/AndroidUpdaterCallback.java View 1 2 3 1 chunk +57 lines, -0 lines 0 comments Download
A src/org/adblockplus/android/AndroidWebRequest.java View 1 2 3 4 5 6 7 1 chunk +97 lines, -0 lines 0 comments Download
M src/org/adblockplus/android/ConfigurationActivity.java View 1 1 chunk +7 lines, -7 lines 0 comments Download
M src/org/adblockplus/android/CrashHandler.java View 1 2 3 4 5 6 9 chunks +17 lines, -17 lines 0 comments Download
M src/org/adblockplus/android/CrashReportDialog.java View 1 2 3 4 5 6 6 chunks +22 lines, -22 lines 0 comments Download
M src/org/adblockplus/android/HelpfulCheckBoxPreference.java View 1 3 chunks +7 lines, -7 lines 0 comments Download
M src/org/adblockplus/android/Preferences.java View 1 2 3 4 23 chunks +97 lines, -95 lines 0 comments Download
M src/org/adblockplus/android/ProxyConfigurationActivity.java View 1 1 chunk +9 lines, -9 lines 0 comments Download
M src/org/adblockplus/android/ProxyService.java View 1 2 3 4 5 6 36 chunks +111 lines, -110 lines 0 comments Download
M src/org/adblockplus/android/ProxySettings.java View 1 2 3 4 5 6 11 chunks +33 lines, -33 lines 0 comments Download
M src/org/adblockplus/android/RefreshableListPreference.java View 1 2 3 chunks +7 lines, -7 lines 0 comments Download
M src/org/adblockplus/android/Starter.java View 1 2 3 4 5 6 1 chunk +8 lines, -8 lines 0 comments Download
M src/org/adblockplus/android/SubscriptionParser.java View 1 3 chunks +6 lines, -6 lines 0 comments Download
M src/org/adblockplus/android/SummarizedPreferences.java View 1 2 3 4 3 chunks +7 lines, -7 lines 0 comments Download
A src/org/adblockplus/android/Utils.java View 1 2 3 5 6 7 1 chunk +120 lines, -0 lines 0 comments Download
M src/org/adblockplus/android/updater/AlarmReceiver.java View 1 2 3 4 7 chunks +34 lines, -33 lines 0 comments Download
M src/org/adblockplus/android/updater/UpdaterActivity.java View 1 2 3 4 5 6 4 chunks +8 lines, -8 lines 0 comments Download
M src/org/adblockplus/android/updater/UpdaterService.java View 1 2 3 4 9 chunks +25 lines, -24 lines 0 comments Download
M src/org/adblockplus/brazil/BaseRequestHandler.java View 1 2 3 4 5 6 2 chunks +6 lines, -6 lines 0 comments Download
M src/org/adblockplus/brazil/RequestHandler.java View 1 2 3 4 5 6 16 chunks +23 lines, -23 lines 0 comments Download
M src/org/adblockplus/brazil/SSLConnectionHandler.java View 1 2 3 4 5 6 9 chunks +13 lines, -13 lines 0 comments Download
M src/org/adblockplus/brazil/TransparentProxyHandler.java View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download
M src/org/adblockplus/libadblockplus/Filter.java View 1 chunk +1 line, -1 line 0 comments Download
M src/org/adblockplus/libadblockplus/FilterChangeCallback.java View 1 chunk +1 line, -1 line 0 comments Download
A src/org/adblockplus/libadblockplus/HeaderEntry.java View 1 2 3 1 chunk +68 lines, -0 lines 0 comments Download
M src/org/adblockplus/libadblockplus/JniExceptionHandler.java View 2 chunks +7 lines, -2 lines 0 comments Download
M src/org/adblockplus/libadblockplus/JsEngine.java View 1 chunk +1 line, -1 line 0 comments Download
M src/org/adblockplus/libadblockplus/JsValue.java View 2 chunks +3 lines, -3 lines 0 comments Download
M src/org/adblockplus/libadblockplus/ServerResponse.java View 1 2 3 4 chunks +12 lines, -14 lines 0 comments Download
M src/org/adblockplus/libadblockplus/Subscription.java View 1 chunk +1 line, -1 line 0 comments Download
M src/org/adblockplus/libadblockplus/UpdaterCallback.java View 1 chunk +1 line, -1 line 0 comments Download
M src/org/adblockplus/libadblockplus/WebRequest.java View 1 2 3 2 chunks +1 line, -2 lines 0 comments Download

Messages

Total messages: 8
René Jeschke
5 years, 3 months ago (2014-04-11 13:33:24 UTC) #1
Felix Dahlke
Not finished, but I think it makes sense to address what I found so far ...
5 years, 3 months ago (2014-04-16 15:24:25 UTC) #2
René Jeschke
http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/jni/Android.mk File jni/Android.mk (right): http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/jni/Android.mk#newcode32 jni/Android.mk:32: LOCAL_CPP_FEATURES := exceptions # rtti On 2014/04/16 15:24:25, Felix ...
5 years, 3 months ago (2014-04-16 17:51:47 UTC) #3
Felix Dahlke
Phew, that was a really huge one! :) http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/org/adblockplus/ChunkedOutputStream.java File src/org/adblockplus/ChunkedOutputStream.java (right): http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/org/adblockplus/ChunkedOutputStream.java#newcode32 src/org/adblockplus/ChunkedOutputStream.java:32: private ...
5 years, 2 months ago (2014-04-28 07:29:01 UTC) #4
René Jeschke
http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/org/adblockplus/ChunkedOutputStream.java File src/org/adblockplus/ChunkedOutputStream.java (right): http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/org/adblockplus/ChunkedOutputStream.java#newcode32 src/org/adblockplus/ChunkedOutputStream.java:32: private static final byte[] FINAL_CHUNK = new byte[] { ...
5 years, 2 months ago (2014-04-28 08:34:31 UTC) #5
Felix Dahlke
Almost there! http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/org/adblockplus/android/ABPEngine.java File src/org/adblockplus/android/ABPEngine.java (right): http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/org/adblockplus/android/ABPEngine.java#newcode78 src/org/adblockplus/android/ABPEngine.java:78: .setDevelopmentBuild(developmentBuild) On 2014/04/28 08:34:32, René Jeschke wrote: ...
5 years, 2 months ago (2014-04-28 10:09:41 UTC) #6
René Jeschke
http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/org/adblockplus/android/ABPEngine.java File src/org/adblockplus/android/ABPEngine.java (right): http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/org/adblockplus/android/ABPEngine.java#newcode78 src/org/adblockplus/android/ABPEngine.java:78: .setDevelopmentBuild(developmentBuild) On 2014/04/28 10:09:41, Felix H. Dahlke wrote: > ...
5 years, 2 months ago (2014-04-28 10:18:34 UTC) #7
Felix Dahlke
5 years, 2 months ago (2014-04-28 10:20:57 UTC) #8
LGTM!

http://codereview.adblockplus.org/5697499218051072/diff/5711312218750976/src/...
File src/org/adblockplus/android/AndroidWebRequest.java (right):

http://codereview.adblockplus.org/5697499218051072/diff/5711312218750976/src/...
src/org/adblockplus/android/AndroidWebRequest.java:60: byte[] out = new
byte[65536];
On 2014/04/28 10:18:34, René Jeschke wrote:
> On 2014/04/28 10:09:41, Felix H. Dahlke wrote:
> > Also use BUFFER_GROWTH_DELTA here? That's why I suggested a constant -
because
> > it's the same value twice. But if you argue we logically need two constants
> > here, then I'm fine with how it was.
> 
> Ah, well, this is technically another constant.

Feel free to hard code both values then. Whatever you prefer.
Sign in to reply to this message.

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