Usage of new API, cleanups (reduced)

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

[Felix Dahlke, 2014-04-16 15:24:25]

Not finished, but I think it makes sense to address what I found so far first.
Mostly about conventions and unrelated changes. I haven't pointed every
occurrence of those individually.

Things like mandatory braces, optional "this." etc. are not documented
conventions and hence left to the author. We shouldn't make bulk changes that
adjust these things to our personal tastes.

Note that even the "final" bulk change is something we normally don't do. But
it's something we discussed and agreed on. (And we agreed to do it intermixed
with actual changes, which is also something we normally don't do.)

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/jni/...
File jni/Android.mk (right):

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/jni/...
jni/Android.mk:32: LOCAL_CPP_FEATURES := exceptions # rtti
I assume we don't need RTTI anymore? Might as well get rid of the comment.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/jni/...
File jni/Application.mk (right):

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/jni/...
jni/Application.mk:3: APP_ABI := armeabi #-v7a
What's this comment for?

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/jni/...
File jni/JniCallbacks.h (right):

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/jni/...
jni/JniCallbacks.h:70: JniGlobalReference<jclass>::Ptr jsValueClass;
You seem to make everything const that can be, so why not this?

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/jni/...
File jni/JniFilterChangeCallback.cpp (right):

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/jni/...
jni/JniFilterChangeCallback.cpp:37: jsValueClass.reset(new
JniGlobalReference<jclass>(env, env->FindClass(PKG("JsValue"))));
Should be initialised in the initialisation list.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
File src/org/adblockplus/ChunkedOutputStream.java (right):

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/ChunkedOutputStream.java:43: // TODO: Huh? No sync, no
'this.wroteFinalChunk = false'?
Can you make this TODO more clear? If you think this needs to be synchronized,
please say why. And why do you think wroteFinalChunk needs to be set to false?

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/ChunkedOutputStream.java:44: if (!this.wroteFinalChunk)
I agree to adding final's everywhere, but let's please keep unrelated changes to
a minimum otherwise. Always prefixing instance variables with "this" is an
example of this.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/ChunkedOutputStream.java:45: {
This is also an unrelated change.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/ChunkedOutputStream.java:86: // Zero sized buffers are ok on
slow connections but not in our case - zero chunk is used to indicate the end of
transfer.
Our rule is to wrap at 100 columns, which is what this does. We're not too
strict about enforcing it, but we should definitely not change code that adheres
to it.

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

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/ABPEngine.java:45: private volatile JsEngine
jsEngine;
Why do these have to be volatile? How I understand it, volatile in Java only
ensures that no cached values of a variable are used. And since they are set
only once, in "create", it doesn't seem to me like there's more than one thread
involved at that point. Did I get that right?

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/ABPEngine.java:78:
.setDevelopmentBuild(developmentBuild)
Looks like trailing whitespace, can you make sure not to add trailing
whitespace? Also below.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/ABPEngine.java:160: final
org.adblockplus.android.Subscription ret = new
org.adblockplus.android.Subscription();
How about calling this "subscription" (or "sub") and the parameters
"jsSubscription" (or "jsSub")?

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

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/AboutDialog.java:43:
this.requestWindowFeature(Window.FEATURE_NO_TITLE);
As before, I'm fine with the "final" change, but not with the "this." one.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
File src/org/adblockplus/android/AdblockPlus.java (left):

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/AdblockPlus.java:208: ConnectivityManager
connectivityManager = (ConnectivityManager)
context.getSystemService(Context.CONNECTIVITY_SERVICE);
Why remove the space after the cast? More common/clear that way IMO. Same below.

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

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/AdblockPlus.java:71: public final static int
UPDATE_NOTIFICATION_ID = R.string.app_name + 1;
Should be "static final", not "final" static, according to the Android
conventions (and Sun too IIRC). Also below.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/AdblockPlus.java:110: return this.size() > MAX_SIZE;
Same as in other files, also below.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/AdblockPlus.java:165: {
Same as in other files, also below.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/AdblockPlus.java:288: *          Subscription url
Why this weird break? Same below.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/AdblockPlus.java:341: * Starting with 1.1.2, we
ignored element hiding rules after download anyway, to keep the memory usage
down. Doing this with libadblockplus is
As before, please don't make code that adheres to the 100 column rule violate
it. Same below.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/AdblockPlus.java:345: /*
We don't want to leave commented out code around, let's either leave it alone or
get rid of it.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/AdblockPlus.java:537: final char[] buffer = new
char[4096];
Why this? Hex is not that hard to read.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/AdblockPlus.java:540: 
Empty lines are also arguably an unrelated change, same below.

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

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/AdvancedPreferences.java:61:
this.addPreferencesFromResource(R.xml.preferences_advanced);
ALso a bunch of unrelated changes here, same as before.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/AdvancedPreferences.java:83: @Override
Does that mean we're on Java 6 now?

[René Jeschke, 2014-04-16 17:51:47]

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/jni/...
File jni/Android.mk (right):

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/jni/...
jni/Android.mk:32: LOCAL_CPP_FEATURES := exceptions # rtti
On 2014/04/16 15:24:25, Felix H. Dahlke wrote:
> I assume we don't need RTTI anymore? Might as well get rid of the comment.

Done.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/jni/...
File jni/Application.mk (right):

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/jni/...
jni/Application.mk:3: APP_ABI := armeabi #-v7a
On 2014/04/16 15:24:25, Felix H. Dahlke wrote:
> What's this comment for?
Removed.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/jni/...
File jni/JniCallbacks.h (right):

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/jni/...
jni/JniCallbacks.h:70: JniGlobalReference<jclass>::Ptr jsValueClass;
On 2014/04/16 15:24:25, Felix H. Dahlke wrote:
> You seem to make everything const that can be, so why not this?

Done.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/jni/...
File jni/JniFilterChangeCallback.cpp (right):

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/jni/...
jni/JniFilterChangeCallback.cpp:37: jsValueClass.reset(new
JniGlobalReference<jclass>(env, env->FindClass(PKG("JsValue"))));
On 2014/04/16 15:24:25, Felix H. Dahlke wrote:
> Should be initialised in the initialisation list.

Done.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
File src/org/adblockplus/ChunkedOutputStream.java (right):

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/ChunkedOutputStream.java:43: // TODO: Huh? No sync, no
'this.wroteFinalChunk = false'?
On 2014/04/16 15:24:25, Felix H. Dahlke wrote:
> Can you make this TODO more clear? If you think this needs to be synchronized,
> please say why. And why do you think wroteFinalChunk needs to be set to false?
Removed. It will throw an exception anyway if it is called twice.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/ChunkedOutputStream.java:44: if (!this.wroteFinalChunk)
On 2014/04/16 15:24:25, Felix H. Dahlke wrote:
> I agree to adding final's everywhere, but let's please keep unrelated changes
to
> a minimum otherwise. Always prefixing instance variables with "this" is an
> example of this.

Done.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/ChunkedOutputStream.java:45: {
On 2014/04/16 15:24:25, Felix H. Dahlke wrote:
> This is also an unrelated change.

Done.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/ChunkedOutputStream.java:86: // Zero sized buffers are ok on
slow connections but not in our case - zero chunk is used to indicate the end of
transfer.
On 2014/04/16 15:24:25, Felix H. Dahlke wrote:
> Our rule is to wrap at 100 columns, which is what this does. We're not too
> strict about enforcing it, but we should definitely not change code that
adheres
> to it.

Done.

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

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/ABPEngine.java:45: private volatile JsEngine
jsEngine;
On 2014/04/16 15:24:25, Felix H. Dahlke wrote:
> Why do these have to be volatile? How I understand it, volatile in Java only
> ensures that no cached values of a variable are used. And since they are set
> only once, in "create", it doesn't seem to me like there's more than one
thread
> involved at that point. Did I get that right?

I have encountered problems with JNI backed Java objects and the JIT compiler.
Seemed like the JIT removed the object underneath and inlined the rest. The
solution to this is to declare this stuff 'volatile' (which somehow prevents the
JIT from removing the objects).

So, please, let's keep those volatile for now.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/ABPEngine.java:78:
.setDevelopmentBuild(developmentBuild)
On 2014/04/16 15:24:25, Felix H. Dahlke wrote:
> Looks like trailing whitespace, can you make sure not to add trailing
> whitespace? Also below.

Don't have any trailing whitespaces in my editor for those lines.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/ABPEngine.java:160: final
org.adblockplus.android.Subscription ret = new
org.adblockplus.android.Subscription();
On 2014/04/16 15:24:25, Felix H. Dahlke wrote:
> How about calling this "subscription" (or "sub") and the parameters
> "jsSubscription" (or "jsSub")?

Done.

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

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/AboutDialog.java:43:
this.requestWindowFeature(Window.FEATURE_NO_TITLE);
On 2014/04/16 15:24:25, Felix H. Dahlke wrote:
> As before, I'm fine with the "final" change, but not with the "this." one.

Done.

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

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/AdblockPlus.java:71: public final static int
UPDATE_NOTIFICATION_ID = R.string.app_name + 1;
On 2014/04/16 15:24:25, Felix H. Dahlke wrote:
> Should be "static final", not "final" static, according to the Android
> conventions (and Sun too IIRC). Also below.

Done.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/AdblockPlus.java:110: return this.size() > MAX_SIZE;
On 2014/04/16 15:24:25, Felix H. Dahlke wrote:
> Same as in other files, also below.

Done.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/AdblockPlus.java:165: {
On 2014/04/16 15:24:25, Felix H. Dahlke wrote:
> Same as in other files, also below.

Done.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/AdblockPlus.java:341: * Starting with 1.1.2, we
ignored element hiding rules after download anyway, to keep the memory usage
down. Doing this with libadblockplus is
On 2014/04/16 15:24:25, Felix H. Dahlke wrote:
> As before, please don't make code that adheres to the 100 column rule violate
> it. Same below.

Done.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/AdblockPlus.java:345: /*
On 2014/04/16 15:24:25, Felix H. Dahlke wrote:
> We don't want to leave commented out code around, let's either leave it alone
or
> get rid of it.

Done.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/AdblockPlus.java:537: final char[] buffer = new
char[4096];
On 2014/04/16 15:24:25, Felix H. Dahlke wrote:
> Why this? Hex is not that hard to read.

Because is totally does not make any sense here. Well, still.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/AdblockPlus.java:540: 
On 2014/04/16 15:24:25, Felix H. Dahlke wrote:
> Empty lines are also arguably an unrelated change, same below.

Done.

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

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/AdvancedPreferences.java:61:
this.addPreferencesFromResource(R.xml.preferences_advanced);
On 2014/04/16 15:24:25, Felix H. Dahlke wrote:
> ALso a bunch of unrelated changes here, same as before.

Done.

http://codereview.adblockplus.org/5697499218051072/diff/5629499534213120/src/...
src/org/adblockplus/android/AdvancedPreferences.java:83: @Override
On 2014/04/16 15:24:25, Felix H. Dahlke wrote:
> Does that mean we're on Java 6 now?

The whole code base (please have a look) is a mixture of missing and used
@override annotations, so either we always use them, or we don't.

[Felix Dahlke, 2014-04-28 07:29:01]

Phew, that was a really huge one! :)

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
File src/org/adblockplus/ChunkedOutputStream.java (right):

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/ChunkedOutputStream.java:32: private static final byte[]
FINAL_CHUNK = new byte[] { '0', '\r', '\n', '\r', '\n' };
Space after { and before } is optional in the Java conventions, so please don't
change it here.

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

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/ABPEngine.java:45: private volatile JsEngine
jsEngine;
I know we discussed this in the last path set, but I still think it's not really
obvious why these are volatile. Maybe it makes sense to add a comment describing
the problem?

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/ABPEngine.java:78:
.setDevelopmentBuild(developmentBuild)
Is this trailing whitespace? I can't really tell from the diff, looks weird.
Same below.

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/ABPEngine.java:168: private static
org.adblockplus.android.Subscription[] convertJsSubscriptions(final
List<Subscription> subs)
How about renaming "subs" to "jsSubscription" and "ret" to "subscription" like
above?

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

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/AdblockPlus.java:335: 
Unrelated whitespace change - as discussed before, I'd like to keep unrelated
changes to a minimum with such a big patch.

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/AdblockPlus.java:342: */
Same as above, let's leave this. Plus the indentation this had is actually more
typical when comment lines are not preceded by *.

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/AdblockPlus.java:434: * Starts ABP engine. It also
initiates subscription refresh if it is enabled in user settings.
This unrelated change seems fine. Looks like 100 columns, and you're touching
the function anyway.

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

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/AdvancedPreferences.java:48:
@SuppressWarnings("deprecation")
I don't think we should ignore this, in general pretty much. There are a bunch
of warnings anyway, and deprecation warnings can usually be fixed.

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/AdvancedPreferences.java:208:
items.add(String.format("Host: [%s] Port: [%s] Excl: [%s]",
proxyService.proxy.props.getProperty("adblock.proxyHost"),
Seems like this adds a lot of trailing whitespace, based on the diff. Shouldn't
be changed really.

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/AdvancedPreferences.java:240: @Override
I remarked about this before, but IIRC, the situation with @Override was this:

1. Mandatory for overriding base class methods since Java 5
2. Mandatory for implementing abstract/interface methods since Java 6
3. Not allowed (!) for implementing interface methods in Java 5

OnClickListener.onClick is abstract, so this means this wouldn't work in Java 5,
right? Aren't we still on Java 5?

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

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/AndroidWebRequest.java:71: out = new byte[out.length
+ 65536];
I think 65536 should be a constant here.

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

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/Preferences.java:75:
@SuppressWarnings("deprecation")
As before, not a fan. Also below.

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

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/ProxyService.java:67: private static final String
TAG = Utils.getTag(ProxyService.class);
What's the logic behind reordering these?

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

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/SummarizedPreferences.java:36:
@SuppressWarnings("deprecation")
As before, not a fan. Also below.

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

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/Utils.java:35: //
What's this for?

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
File src/org/adblockplus/android/updater/AlarmReceiver.java (right):

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/updater/AlarmReceiver.java:65:
@SuppressWarnings("deprecation")
As before, not a fan.

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
File src/org/adblockplus/android/updater/UpdaterActivity.java (right):

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/updater/UpdaterActivity.java:56: new
AlertDialog.Builder(this).setTitle(R.string.msg_update_available).setMessage(getString(R.string.msg_update_description))
Looks like weird whitespace changes again, the code shouldn't really change
apart from the final.

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
File src/org/adblockplus/android/updater/UpdaterService.java (right):

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/updater/UpdaterService.java:59:
@SuppressWarnings("deprecation")
As before, not a fan. Also below.

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
File src/org/adblockplus/libadblockplus/Filter.java (right):

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/libadblockplus/Filter.java:20: public final class Filter
extends JsValue
Are you making all classes final as well while final-ing everything? I don't
mind, just curious why: Is there any benefit beyond ensuring it cannot be
subclassed?

[René Jeschke, 2014-04-28 08:34:31]

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
File src/org/adblockplus/ChunkedOutputStream.java (right):

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/ChunkedOutputStream.java:32: private static final byte[]
FINAL_CHUNK = new byte[] { '0', '\r', '\n', '\r', '\n' };
On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> Space after { and before } is optional in the Java conventions, so please
don't
> change it here.

Done.

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

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/ABPEngine.java:45: private volatile JsEngine
jsEngine;
On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> I know we discussed this in the last path set, but I still think it's not
really
> obvious why these are volatile. Maybe it makes sense to add a comment
describing
> the problem?

Done.

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/ABPEngine.java:78:
.setDevelopmentBuild(developmentBuild)
On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> Is this trailing whitespace? I can't really tell from the diff, looks weird.
> Same below.

Nope, Editor->Save actions->Remove trailing whitespace ... if I touched the
file, there are no trailing whitespace anymore^^

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/ABPEngine.java:168: private static
org.adblockplus.android.Subscription[] convertJsSubscriptions(final
List<Subscription> subs)
On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> How about renaming "subs" to "jsSubscription" and "ret" to "subscription" like
> above?

Done.

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

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/AdblockPlus.java:434: * Starts ABP engine. It also
initiates subscription refresh if it is enabled in user settings.
On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> This unrelated change seems fine. Looks like 100 columns, and you're touching
> the function anyway.

Done.

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

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/AdvancedPreferences.java:48:
@SuppressWarnings("deprecation")
On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> I don't think we should ignore this, in general pretty much. There are a bunch
> of warnings anyway, and deprecation warnings can usually be fixed.

Those deprecation warnings can only be resolved by raising the minimum API level
to 11 and refactoring the whole app to use the fragment API.

Still, removed (also this will introduce a lot of additional noise to any code
improvement tries as one has to filter out which warnings are serious and which
are not).

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/AdvancedPreferences.java:208:
items.add(String.format("Host: [%s] Port: [%s] Excl: [%s]",
proxyService.proxy.props.getProperty("adblock.proxyHost"),
On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> Seems like this adds a lot of trailing whitespace, based on the diff.
Shouldn't
> be changed really.

Nope, just the diff view gone mad.

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/AdvancedPreferences.java:240: @Override
On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> I remarked about this before, but IIRC, the situation with @Override was this:
> 
> 1. Mandatory for overriding base class methods since Java 5
> 2. Mandatory for implementing abstract/interface methods since Java 6
> 3. Not allowed (!) for implementing interface methods in Java 5
> 
> OnClickListener.onClick is abstract, so this means this wouldn't work in Java
5,
> right? Aren't we still on Java 5?

The whole Adblock Plus Android gets checked/compiled agains Java6 compliance
level. This is hardcoded in the project properties, so we should be fine.

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

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/ProxyService.java:67: private static final String
TAG = Utils.getTag(ProxyService.class);
On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> What's the logic behind reordering these?

Reverted.

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

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/SummarizedPreferences.java:36:
@SuppressWarnings("deprecation")
On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> As before, not a fan. Also below.

Done.

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

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/Utils.java:35: //
On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> What's this for?

Commented.

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
File src/org/adblockplus/android/updater/UpdaterActivity.java (right):

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/updater/UpdaterActivity.java:56: new
AlertDialog.Builder(this).setTitle(R.string.msg_update_available).setMessage(getString(R.string.msg_update_description))
On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> Looks like weird whitespace changes again, the code shouldn't really change
> apart from the final.

Done.

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
File src/org/adblockplus/libadblockplus/Filter.java (right):

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/libadblockplus/Filter.java:20: public final class Filter
extends JsValue
On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> Are you making all classes final as well while final-ing everything? I don't
> mind, just curious why: Is there any benefit beyond ensuring it cannot be
> subclassed?

Filter and Subscription are both not intended to be sub-classed, so I tend to
forbid this.

[Felix Dahlke, 2014-04-28 10:09:41]

Almost there!

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

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/ABPEngine.java:78:
.setDevelopmentBuild(developmentBuild)
On 2014/04/28 08:34:32, René Jeschke wrote:
> On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> > Is this trailing whitespace? I can't really tell from the diff, looks weird.
> > Same below.
> 
> Nope, Editor->Save actions->Remove trailing whitespace ... if I touched the
> file, there are no trailing whitespace anymore^^

Ah, so I guess the diff is telling me that there WAS trailing whitespace then.
Pretty weird, but OK :)

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

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/AdblockPlus.java:434: * Starts ABP engine. It also
initiates subscription refresh if it is enabled in user settings.
On 2014/04/28 08:34:32, René Jeschke wrote:
> On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> > This unrelated change seems fine. Looks like 100 columns, and you're
touching
> > the function anyway.
> 
> Done.

I actually meant it's OK, because it's still 100 columns :) But I don't mind
either way.

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

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/AdvancedPreferences.java:48:
@SuppressWarnings("deprecation")
On 2014/04/28 08:34:32, René Jeschke wrote:
> On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> > I don't think we should ignore this, in general pretty much. There are a
bunch
> > of warnings anyway, and deprecation warnings can usually be fixed.
> 
> Those deprecation warnings can only be resolved by raising the minimum API
level
> to 11 and refactoring the whole app to use the fragment API.
> 
> Still, removed (also this will introduce a lot of additional noise to any code
> improvement tries as one has to filter out which warnings are serious and
which
> are not).

It may make sense then, however, I think it belongs in a separate review then,
this one is pretty big already.

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/AdvancedPreferences.java:240: @Override
On 2014/04/28 08:34:32, René Jeschke wrote:
> On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> > I remarked about this before, but IIRC, the situation with @Override was
this:
> > 
> > 1. Mandatory for overriding base class methods since Java 5
> > 2. Mandatory for implementing abstract/interface methods since Java 6
> > 3. Not allowed (!) for implementing interface methods in Java 5
> > 
> > OnClickListener.onClick is abstract, so this means this wouldn't work in
Java
> 5,
> > right? Aren't we still on Java 5?
> 
> The whole Adblock Plus Android gets checked/compiled agains Java6 compliance
> level. This is hardcoded in the project properties, so we should be fine.

Alright, thought it was Java 5 for some reason.

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

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/Utils.java:35: //
On 2014/04/28 08:34:32, René Jeschke wrote:
> On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> > What's this for?
> 
> Commented.

Well that comment is kinda obvious. I meant what the empty comment is for, not
the private constructor. Do we need a comment here at all costs? Warnings or
something? (Didn't get one when quickly adding a private constructor somewhere.)

If we absolutely need it, I prefer the empty comment. If we don't need it, I'd
rather have none.

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
File src/org/adblockplus/libadblockplus/Filter.java (right):

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/libadblockplus/Filter.java:20: public final class Filter
extends JsValue
On 2014/04/28 08:34:32, René Jeschke wrote:
> On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> > Are you making all classes final as well while final-ing everything? I don't
> > mind, just curious why: Is there any benefit beyond ensuring it cannot be
> > subclassed?
> 
> Filter and Subscription are both not intended to be sub-classed, so I tend to
> forbid this.

Ah. Thought there may be some tangiable benefits maybe, in C++ e.g. it's a huge
win if you don't need a vtable, but that's not how the JVM works of course.

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

http://codereview.adblockplus.org/5697499218051072/diff/5711312218750976/src/...
src/org/adblockplus/android/ABPEngine.java:45: /**
If this is a javadoc comment, it'll show up as the docs for "jsEngine", while it
actually applies to the whole block. Shouldn't it be "/*" then?

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];
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.

[René Jeschke, 2014-04-28 10:18:34]

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

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/ABPEngine.java:78:
.setDevelopmentBuild(developmentBuild)
On 2014/04/28 10:09:41, Felix H. Dahlke wrote:
> On 2014/04/28 08:34:32, René Jeschke wrote:
> > On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> > > Is this trailing whitespace? I can't really tell from the diff, looks
weird.
> > > Same below.
> > 
> > Nope, Editor->Save actions->Remove trailing whitespace ... if I touched the
> > file, there are no trailing whitespace anymore^^
> 
> Ah, so I guess the diff is telling me that there WAS trailing whitespace then.
> Pretty weird, but OK :)

Done.

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

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/AdblockPlus.java:434: * Starts ABP engine. It also
initiates subscription refresh if it is enabled in user settings.
On 2014/04/28 10:09:41, Felix H. Dahlke wrote:
> On 2014/04/28 08:34:32, René Jeschke wrote:
> > On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> > > This unrelated change seems fine. Looks like 100 columns, and you're
> touching
> > > the function anyway.
> > 
> > Done.
> 
> I actually meant it's OK, because it's still 100 columns :) But I don't mind
> either way.

Done.

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

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/AdvancedPreferences.java:48:
@SuppressWarnings("deprecation")
On 2014/04/28 10:09:41, Felix H. Dahlke wrote:
> On 2014/04/28 08:34:32, René Jeschke wrote:
> > On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> > > I don't think we should ignore this, in general pretty much. There are a
> bunch
> > > of warnings anyway, and deprecation warnings can usually be fixed.
> > 
> > Those deprecation warnings can only be resolved by raising the minimum API
> level
> > to 11 and refactoring the whole app to use the fragment API.
> > 
> > Still, removed (also this will introduce a lot of additional noise to any
code
> > improvement tries as one has to filter out which warnings are serious and
> which
> > are not).
> 
> It may make sense then, however, I think it belongs in a separate review then,
> this one is pretty big already.

Done.

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/AdvancedPreferences.java:240: @Override
On 2014/04/28 10:09:41, Felix H. Dahlke wrote:
> On 2014/04/28 08:34:32, René Jeschke wrote:
> > On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> > > I remarked about this before, but IIRC, the situation with @Override was
> this:
> > > 
> > > 1. Mandatory for overriding base class methods since Java 5
> > > 2. Mandatory for implementing abstract/interface methods since Java 6
> > > 3. Not allowed (!) for implementing interface methods in Java 5
> > > 
> > > OnClickListener.onClick is abstract, so this means this wouldn't work in
> Java
> > 5,
> > > right? Aren't we still on Java 5?
> > 
> > The whole Adblock Plus Android gets checked/compiled agains Java6 compliance
> > level. This is hardcoded in the project properties, so we should be fine.
> 
> Alright, thought it was Java 5 for some reason.

Done.

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

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/android/Utils.java:35: //
On 2014/04/28 10:09:41, Felix H. Dahlke wrote:
> On 2014/04/28 08:34:32, René Jeschke wrote:
> > On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> > > What's this for?
> > 
> > Commented.
> 
> Well that comment is kinda obvious. I meant what the empty comment is for, not
> the private constructor. Do we need a comment here at all costs? Warnings or
> something? (Didn't get one when quickly adding a private constructor
somewhere.)
> 
> If we absolutely need it, I prefer the empty comment. If we don't need it, I'd
> rather have none.

Empty blocks should be commented to show that this is on purpose, depends on the
warning settings, I use pretty strict warnings. So I change it back to an empty
comment.

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
File src/org/adblockplus/libadblockplus/Filter.java (right):

http://codereview.adblockplus.org/5697499218051072/diff/5641332169113600/src/...
src/org/adblockplus/libadblockplus/Filter.java:20: public final class Filter
extends JsValue
On 2014/04/28 10:09:41, Felix H. Dahlke wrote:
> On 2014/04/28 08:34:32, René Jeschke wrote:
> > On 2014/04/28 07:29:01, Felix H. Dahlke wrote:
> > > Are you making all classes final as well while final-ing everything? I
don't
> > > mind, just curious why: Is there any benefit beyond ensuring it cannot be
> > > subclassed?
> > 
> > Filter and Subscription are both not intended to be sub-classed, so I tend
to
> > forbid this.
> 
> Ah. Thought there may be some tangiable benefits maybe, in C++ e.g. it's a
huge
> win if you don't need a vtable, but that's not how the JVM works of course.

Done.

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

http://codereview.adblockplus.org/5697499218051072/diff/5711312218750976/src/...
src/org/adblockplus/android/ABPEngine.java:45: /**
On 2014/04/28 10:09:41, Felix H. Dahlke wrote:
> If this is a javadoc comment, it'll show up as the docs for "jsEngine", while
it
> actually applies to the whole block. Shouldn't it be "/*" then?

Done.

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: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.

[Felix Dahlke, 2014-04-28 10:20:57]

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.