New JNI bindings

New JNI bindings

[René Jeschke, 2014-03-20 12:17:56]

Fixed wrong signature for getElementHidingSelectors.

[René Jeschke, 2014-03-20 15:12:18]

Added LICENSE, moved callback impls into cpp files.

[René Jeschke, 2014-03-20 15:17:10]

Cleaned up namespace usage in cpp files.

[Felix Dahlke, 2014-03-28 08:29:00]

This is a huge review and I've been keeping you waiting long enough as it is, so
I decided to do it in multiple passes.

1. Style issues only. There are a couple of those, literally all in the C++
code. Note that we try hard to achieve a consistent style, which is particularly
hard in C++. We do this based on the Mozilla and ABP coding styles, not personal
tastes. If you'd like to discuss our coding style, let's do it on IRC or
Discourse, this is not the place. Also note that I've pointed out some issues in
just one place, assuming you'll address other occurrences too (e.g. the include
guards.)
2. The high-level API - looks pretty good so far.
3. All the detail code.

One note about TODOs: I've seen a few of those. We should try to either fix or
remove most of them before we push. Creating follow-up issues for nice-to-have
things is better IMO.

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/LICE...
File LICENSE.txt (right):

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/LICE...
LICENSE.txt:1: GNU GENERAL PUBLIC LICENSE
Would rather have this in a separate commit - this one's big enough as it is :)

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
File jni/Android.mk (right):

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/Android.mk:28: LOCAL_SRC_FILES += JniJsEngine.cpp JniFilterEngine.cpp
Nit: A bit inconsistent how files are split up here, I thought it's to keep
lines short, but the last statement below is quite long, the first one above
too. Can you use some kind of system here, e.g. wrapping at 80 columns (this
line would get longer in the process I think).

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
File jni/JniCallbacks.h (right):

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/JniCallbacks.h:18: #ifndef JNICALLBACKS_H_
Our convention is: JNI_CALLBACKS_H (no trailing _, underscores as a word
separator)

Also note that we prepend the namespace, e.g. in this case it'd be:

ADBLOCK_PLUS_ANDROID_JNI_CALLBACKS_H

But, as I wrote below, I think it'd be better not to have a namespace here.

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/JniCallbacks.h:27: namespace Android
Namespace bodies are to be indented:
https://adblockplus.org/en/coding-style

However, since this is part of the application code and not a library, I'd vote
for not having a namespace at all.

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/JniCallbacks.h:35: env->GetJavaVM(&m_JavaVM);
IMO, we should move all the function definitions from JniCallbackBase to an
implementation file. It's not much, but beyond trivial.

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/JniCallbacks.h:44: inline JavaVM* GetJavaVM() const
inline is redundant for member functions. Same below.

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/JniCallbacks.h:74: JavaVM* m_JavaVM;
From https://adblockplus.org/en/coding-style:
"No hungarian notation, no special variable name prefixes or suffixes denoting
type or scope."

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/JniCallbacks.h:79: class JniEventCallback: public JniCallbackBase
Space before ":" please :) Likewise below.

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/JniCallbacks.h:109: private:
Whitespace before "private"?

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/JniCallbacks.h:119: private:
Whitespace before "private"?

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
File jni/JniEventCallback.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/JniEventCallback.cpp:20:
AdblockPlus::Android::JniEventCallback::JniEventCallback(JNIEnv* env, jobject
callbackObject) :
Should be broken before ":",  not after it, according to Mozilla's rules.

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/JniEventCallback.cpp:31: AdblockPlus::Android::JNIEnvAcquire
env(GetJavaVM());
Capitalisation is inconsistent here, "JniEventCallback" vs "JNIEnvAcquire" and
"GetJavaVM". Don't have a strong preference, but I think it should be
consistent.

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/JniEventCallback.cpp:43: static jlong JNICALL JniCtor(JNIEnv* env, jclass
clazz, jobject callbackObject)
We mostly use unnamed namespaces instead of static in new code (added a hint to
that to the coding style), but it doesn't matter too much as long as its
consistent on a per-file basis.

That said, I think we should move all the static locals up, before the member
function definitions etc. That's where the anonymous namespace usually goes,
should help avoid confusion.

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/JniEventCallback.cpp:54: delete
AdblockPlus::Android::JniLong2TypePtr<AdblockPlus::Android::JniEventCallback>(ptr);
I'd much prefer "JniLongToTypePtr" since this had me puzzled for a while :)

Same for the others.

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/JniEventCallback.cpp:59: { (char*)"ctor", (char*)"(Ljava/lang/Object;)J",
(void*)JniCtor },
1. Note that as per our coding style, new code should only use C-style casts
when we need its exact semantics, which is not the case here. However, in this
particular case I think it's obvious enough what happens (casting literals and
casting stuff to void*), and it'd be awkwardly long with C++-style casts, so I'm
quite fine with this.
2. I think it'd be more in line with our code style if this and the following
line where indented, and if the closing }; was on its own line.

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
File jni/JniFilterEngine.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/JniFilterEngine.cpp:116: // FIXME: Code duplication
That should probably be fixed :)

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
File jni/JniJsEngine.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/JniJsEngine.cpp:231: //JsValuePtr NewObject();
Old code? Not a fan of leaving commented out code around.

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
File jni/JniJsValue.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/JniJsValue.cpp:163: //std::vector<std::string> GetOwnPropertyNames() const;
As before, looks like this is just old code, let's remove it.

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
File jni/Utils.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/Utils.cpp:3: * Copyright (C) 2006-2013 Eyeo GmbH
Why this?

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
File jni/Utils.h (right):

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/Utils.h:114: typedef std::tr1::shared_ptr<JniGlobalReference<T> > PTR;
Looks like a macro this way, should be "Ptr" IMO.

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/Utils.h:131: // TODO(rje): It's feeling a bit dirty casting to shared_ptr<T:
JsValue> directly, so maybe
I'd ditch the (rje) part - it's apparent from hg log who wrote this.

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/src/...
File src/org/adblockplus/android/api/AdblockPlusException.java (right):

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/src/...
src/org/adblockplus/android/api/AdblockPlusException.java:17: package
org.adblockplus.android.api;
We should be consistent with having a blank line below the license header or
not. Leaning towards the former but I don't care much.

[René Jeschke, 2014-03-28 10:59:14]

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
File jni/Android.mk (right):

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/Android.mk:28: LOCAL_SRC_FILES += JniJsEngine.cpp JniFilterEngine.cpp
On 2014/03/28 08:29:00, Felix H. Dahlke wrote:
> Nit: A bit inconsistent how files are split up here, I thought it's to keep
> lines short, but the last statement below is quite long, the first one above
> too. Can you use some kind of system here, e.g. wrapping at 80 columns (this
> line would get longer in the process I think).

Has been a semantic grouping, but I'll change this to line wrapping.

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
File jni/JniCallbacks.h (right):

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/JniCallbacks.h:74: JavaVM* m_JavaVM;
On 2014/03/28 08:29:00, Felix H. Dahlke wrote:
> From https://adblockplus.org/en/coding-style:
> "No hungarian notation, no special variable name prefixes or suffixes denoting
> type or scope."

I'll change this, but we should think this over again, as having some prefix for
member variables avoids shadowing (which will otherwise result in strange
function argument names or the usage of 'this').

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
File jni/JniEventCallback.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/JniEventCallback.cpp:31: AdblockPlus::Android::JNIEnvAcquire
env(GetJavaVM());
On 2014/03/28 08:29:00, Felix H. Dahlke wrote:
> Capitalisation is inconsistent here, "JniEventCallback" vs "JNIEnvAcquire" and
> "GetJavaVM". Don't have a strong preference, but I think it should be
> consistent.

It is called JNIEnvAcquire because it acquires a thing of type 'JNIEnv*', so the
name is correct IMHO. 'GetJavaVM' also returns a 'JavaVM*'.

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
File jni/JniJsEngine.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/JniJsEngine.cpp:231: //JsValuePtr NewObject();
On 2014/03/28 08:29:00, Felix H. Dahlke wrote:
> Old code? Not a fan of leaving commented out code around.

Not old code, but 'not-yet-implemented' functionality for JsValue. I did not (as
we discussed) wrote a wrapper for every functionality. I can remove this if you
want but I would like to keep it around to see which functions are missing.

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
File jni/Utils.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/Utils.cpp:3: * Copyright (C) 2006-2013 Eyeo GmbH
On 2014/03/28 08:29:00, Felix H. Dahlke wrote:
> Why this?

Ah, this was definitely a mistake. (Merge went wrong)

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/src/...
File src/org/adblockplus/android/api/AdblockPlusException.java (right):

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/src/...
src/org/adblockplus/android/api/AdblockPlusException.java:17: package
org.adblockplus.android.api;
On 2014/03/28 08:29:00, Felix H. Dahlke wrote:
> We should be consistent with having a blank line below the license header or
> not. Leaning towards the former but I don't care much.

Yes, a blank line here should be.

[Felix Dahlke, 2014-03-28 11:27:17]

Great, looks like that's out of the way! Found a bit more minor stuff, but I'll
get started on reviewing the API now.

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
File jni/JniEventCallback.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/JniEventCallback.cpp:31: AdblockPlus::Android::JNIEnvAcquire
env(GetJavaVM());
On 2014/03/28 10:59:15, René Jeschke wrote:
> On 2014/03/28 08:29:00, Felix H. Dahlke wrote:
> > Capitalisation is inconsistent here, "JniEventCallback" vs "JNIEnvAcquire"
and
> > "GetJavaVM". Don't have a strong preference, but I think it should be
> > consistent.
> 
> It is called JNIEnvAcquire because it acquires a thing of type 'JNIEnv*', so
the
> name is correct IMHO. 'GetJavaVM' also returns a 'JavaVM*'.

Yeah you're right, that's better.

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
File jni/JniJsEngine.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5955934228578304/jni/...
jni/JniJsEngine.cpp:231: //JsValuePtr NewObject();
On 2014/03/28 10:59:15, René Jeschke wrote:
> On 2014/03/28 08:29:00, Felix H. Dahlke wrote:
> > Old code? Not a fan of leaving commented out code around.
> 
> Not old code, but 'not-yet-implemented' functionality for JsValue. I did not
(as
> we discussed) wrote a wrapper for every functionality. I can remove this if
you
> want but I would like to keep it around to see which functions are missing.

No that's perfectly fine, would be helpful to add an explanatory comment on top
though so it's obvious.

http://codereview.adblockplus.org/6606493159784448/diff/5689792285114368/jni/...
File jni/JniCallbacks.h (right):

http://codereview.adblockplus.org/6606493159784448/diff/5689792285114368/jni/...
jni/JniCallbacks.h:18: #ifndef JNICALLBACKS_H
We also typically use underscores for word separation, but not fully
consistently, so nevermind.

http://codereview.adblockplus.org/6606493159784448/diff/5689792285114368/jni/...
File jni/JniJsEngine.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5689792285114368/jni/...
jni/JniJsEngine.cpp:59: std::string eventName = JniJava2StdString(env,
jEventName);
Would also prefer a s/2/To/ for this function.

http://codereview.adblockplus.org/6606493159784448/diff/5689792285114368/jni/...
File jni/Utils.h (right):

http://codereview.adblockplus.org/6606493159784448/diff/5689792285114368/jni/...
jni/Utils.h:45: JNIEnvAcquire(JavaVM* javaVM) :
Colon on next line s'il vous plaît.

http://codereview.adblockplus.org/6606493159784448/diff/5689792285114368/jni/...
jni/Utils.h:48: attachmentStatus = javaVM->GetEnv((void **)&jniEnv,
ABP_JNI_VERSION);
Same as JniCallbacks.h, non-trivial non-template functions should be defined in
implementation files.

[Felix Dahlke, 2014-03-28 15:52:51]

Something seems to be wrong with Patch Set 7, it doesn't contain all the changes
here, just the changes to Patch Set 6. Can you delete set 7 and reupload the
full diff?

[René Jeschke, 2014-03-28 15:58:05]

On 2014/03/28 15:52:51, Felix H. Dahlke wrote:
> Something seems to be wrong with Patch Set 7, it doesn't contain all the
changes
> here, just the changes to Patch Set 6. Can you delete set 7 and reupload the
> full diff?

Done.

[Felix Dahlke, 2014-03-28 17:28:41]

Okay, looking pretty good, didn't find a lot, considering how big the patch is.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/JniCallbacks.h (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/JniCallbacks.h:53: virtual ~JniEventCallback();
A virtual destructor is only required for base classes. AFAIK, nothing derives
from JniEventCallback (or any of the callbacks below), so we can get rid of the
virtual, and the whole destructor if there's no destruction code.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/JniEventCallback.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/JniEventCallback.cpp:43: static jlong JNICALL JniCtor(JNIEnv* env, jclass
clazz, jobject callbackObject)
Static to the top as well here please.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/JniFilterChangeCallback.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/JniFilterChangeCallback.cpp:46: static jlong JNICALL JniCtor(JNIEnv* env,
jclass clazz, jobject callbackObject)
Static stuff to the top here as well please.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/JniFilterEngine.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/JniFilterEngine.cpp:245: AdblockPlus::FilterEngine* engine =
JniLongToTypePtr < AdblockPlus::FilterEngine > (ptr);
No whitespace around < or >

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/JniJsValue.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/JniJsValue.cpp:52: static jboolean JNICALL JniIsUndefined(JNIEnv* env,
jclass clazz, jlong ptr)
Static stuff to the top here as well please.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/JniJsValue.h (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/JniJsValue.h:26: jobject JniJsValueList2ArrayList(JNIEnv* env,
AdblockPlus::JsValueList& list);
Ha, another function where I vote for a s/2/To/!

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/JniLogSystem.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/JniLogSystem.cpp:79: static jlong JNICALL JniCtor(JNIEnv* env, jclass clazz,
jobject callbackObject)
Static stuff to the top here as well please.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/JniUpdaterCallback.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/JniUpdaterCallback.cpp:45: static jlong JNICALL JniCtor(JNIEnv* env, jclass
clazz, jobject callbackObject)
Static stuff to the top here as well please.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/JniWebRequest.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/JniWebRequest.cpp:72: static jlong JNICALL JniCtor(JNIEnv* env, jclass
clazz, jobject callbackObject)
Static stuff to the top here as well please.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/Utils.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.cpp:71: jclass clazz = env->FindClass(PKG("AdblockPlusException"));
We're only using AdblockPlusException for native exceptions (which should only
come from libadblockplus), right? I'd vote for a more telling name, maybe
("LibAdblockPlusException") or ("NativeAdblockPlusException"), not sure. What do
you think?

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.cpp:82: JniThrowException(env, "Unknown exception from
libAdblockPlus");
We usually call it "libadblockplus".

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.cpp:94: // throwing a runtime_exception in a ctor can be tolerated
here IMHO
Throwing a std::runtime_error from a constructor is perfectly fine IMO, doesn't
need reasoning - std::runtime_error is really just a general purpose exception
base class, and exceptions are how error handling with constructors is supposed
to work.

You seem to think differently?

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/Utils.h (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.h:31: #define PKG(x) "org/adblockplus/android/api/" x
PKG is fine, but I'd personally vote for "PACKAGE" since we tend to not
abbreviate stuff much. But it's up to you.

More importantly, I'm wondering if we need some kind of prefix for all macros,
Mozilla does it. "ABP_" comes to mind. What do you think?

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.h:32: #define TYP(x) "L" PKG(x) ";"
Correct me if I'm wrong, but it seems the proper name for the "Lfoo/Bar;" thing
seems to be "name", that's how I understand it here:

http://docs.oracle.com/javase/6/docs/api/java/lang/Class.html#getName%28%29

Based on that, I think we should call this macro NAME.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.h:84: T get()
All functions should start with upper case letters.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.h:156: #define TRY try
I believe we can live without this one :D

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.h:158: #define CATCH_AND_THROW(jEnv) \
How about "CATCH_AND_RETHROW"?

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/src/...
File src/org/adblockplus/android/api/AppInfo.java (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/src/...
src/org/adblockplus/android/api/AppInfo.java:17: package
org.adblockplus.android.api;
How about org.adblockplus.libadblockplus for the package name? How I see it,
nothing in here should have anything to do with Android.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/src/...
src/org/adblockplus/android/api/AppInfo.java:29: // TODO: Builder or not?
IMHO this should mirror the C++ API, that is: Just the default constructor and
non-final public members.

[Felix Dahlke, 2014-03-28 17:36:22]

Another comment, not really anything that needs changing, just something I'm
wondering about.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/Android.mk (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Android.mk:26: LOCAL_MODULE := abpEngine
Me being pedantic, but I don't think "abpEngine" makes a lot of sense once we
got rid of AbpEngine.cpp et. al.

Let's keep it like this for now, but doesn't hurt to think of a suitable name. I
was thinking of something along the lines of "libadblockplusWrappers", but I'm
not sure if that's any good. What do you think?

[René Jeschke, 2014-03-31 09:10:38]

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/Android.mk (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Android.mk:26: LOCAL_MODULE := abpEngine
On 2014/03/28 17:36:23, Felix H. Dahlke wrote:
> Me being pedantic, but I don't think "abpEngine" makes a lot of sense once we
> got rid of AbpEngine.cpp et. al.
> 
> Let's keep it like this for now, but doesn't hurt to think of a suitable name.
I
> was thinking of something along the lines of "libadblockplusWrappers", but I'm
> not sure if that's any good. What do you think?

I think we should just call it 'libadblockplus' as this is exactly what it is
for Android/Java IMO (even if the wrapping part is quite heavy here).

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/JniEventCallback.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/JniEventCallback.cpp:43: static jlong JNICALL JniCtor(JNIEnv* env, jclass
clazz, jobject callbackObject)
On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> Static to the top as well here please.

Hm, those static methods are exported through JNI and don't seem to fit the
'normal' static case, so are you sure I should put those on the top?

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/Utils.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.cpp:71: jclass clazz = env->FindClass(PKG("AdblockPlusException"));
On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> We're only using AdblockPlusException for native exceptions (which should only
> come from libadblockplus), right? I'd vote for a more telling name, maybe
> ("LibAdblockPlusException") or ("NativeAdblockPlusException"), not sure. What
do
> you think?

I would also like to use the "AdblockPlusException" for all exceptions thrown by
the Java code. This way it's easier to say if it's a system error or something
we don't like. So I would like to keep the name.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.cpp:94: // throwing a runtime_exception in a ctor can be tolerated
here IMHO
On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> Throwing a std::runtime_error from a constructor is perfectly fine IMO,
doesn't
> need reasoning - std::runtime_error is really just a general purpose exception
> base class, and exceptions are how error handling with constructors is
supposed
> to work.
> 
> You seem to think differently?

When using 'new' and exceptions from the ctor you get an allocated but not
initialized object. So I really don't like throwing exceptions in a ctor and
prefer static factory methods that include cleanup on failure.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/Utils.h (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.h:31: #define PKG(x) "org/adblockplus/android/api/" x
On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> PKG is fine, but I'd personally vote for "PACKAGE" since we tend to not
> abbreviate stuff much. But it's up to you.
> 
> More importantly, I'm wondering if we need some kind of prefix for all macros,
> Mozilla does it. "ABP_" comes to mind. What do you think?

I prefer PKG because it's only 3 chars. As there are only 2 macros that uses
this short naming schema (TYP & PKG) and as both are intended to be used
'inline' with/for string creation I really would like to keep them as short as
possible.

As for the "ABP_" extension: There are currently no name clashes and we're also
not exporting/reusing any of the code elsewhere, so we should be fine without
the prefix IMO.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.h:32: #define TYP(x) "L" PKG(x) ";"
On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> Correct me if I'm wrong, but it seems the proper name for the "Lfoo/Bar;"
thing
> seems to be "name", that's how I understand it here:
> 
>
http://docs.oracle.com/javase/6/docs/api/java/lang/Class.html#getName%2528%2529
> 
> Based on that, I think we should call this macro NAME.

That's not quite correct. The name of the class consists of the class name and
package name(s) joined by dots, so "foo.Bar". "Lfoo/Bar;" is a type signature as
is I, B, Z, J, ...

I'll explain what the macros do and why I called them PKG and TYP:

PKG("Bar") will return "foo/Bar", so it prefixes the given simple class name
with a fixed package and returns a FindClass compatible class name.

TYP("Bar") will return "Lfoo/Bar;", so it prefixes the package and wraps
everything in a type definition for method signatures.

First I was thinking about naming them PACKAGE and TYPE, but decided against
this for several reasons:

1. TYPE will definitely clash with something ... definitely
2. TYPE is 4 letters, PACKAGE is 7 letters, I hate asymmetry 
3. As because of #1 I must change TYPE to anything and didn't want to make the
macro longer, I came up with PKG and TYP

I would really like to keep both macros unchanged but it's up to you to make the
final decision.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.h:156: #define TRY try
On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> I believe we can live without this one :D

It is just for symmetry reasons as:
try
{
  // ....
}
CATCH_AND_THROW(...)

looks awkward compared to having TRY on top.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.h:158: #define CATCH_AND_THROW(jEnv) \
On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> How about "CATCH_AND_RETHROW"?

Rethrow would imply that I would catch the C++ exception and rethrow it after
doing something, but I am transforming a C++ exception into a Java exception and
throwing this one into the JVM, so I vote for _THROW.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/src/...
File src/org/adblockplus/android/api/AppInfo.java (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/src/...
src/org/adblockplus/android/api/AppInfo.java:29: // TODO: Builder or not?
On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> IMHO this should mirror the C++ API, that is: Just the default constructor and
> non-final public members.

Why do we want to have the object non-final? We only initialize it once, then we
pass it onto some JNI methods. I might provide a builder if this suits you more,
but public, non-final is no good and also not needed/wanted here.

[Felix Dahlke, 2014-03-31 10:43:23]

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/Android.mk (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Android.mk:26: LOCAL_MODULE := abpEngine
On 2014/03/31 09:10:38, René Jeschke wrote:
> On 2014/03/28 17:36:23, Felix H. Dahlke wrote:
> > Me being pedantic, but I don't think "abpEngine" makes a lot of sense once
we
> > got rid of AbpEngine.cpp et. al.
> > 
> > Let's keep it like this for now, but doesn't hurt to think of a suitable
name.
> I
> > was thinking of something along the lines of "libadblockplusWrappers", but
I'm
> > not sure if that's any good. What do you think?
> 
> I think we should just call it 'libadblockplus' as this is exactly what it is
> for Android/Java IMO (even if the wrapping part is quite heavy here).

Trouble is, we actually have a module called "libadblockplus" (the actual
libadblockplus), see above. The code in the abpEngine module is really just
(going to be) the wrappers.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/JniEventCallback.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/JniEventCallback.cpp:43: static jlong JNICALL JniCtor(JNIEnv* env, jclass
clazz, jobject callbackObject)
On 2014/03/31 09:10:38, René Jeschke wrote:
> On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> > Static to the top as well here please.
> 
> Hm, those static methods are exported through JNI and don't seem to fit the
> 'normal' static case, so are you sure I should put those on the top?

IMO yes, unnamed namespace should work all the same after all.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/Utils.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.cpp:71: jclass clazz = env->FindClass(PKG("AdblockPlusException"));
On 2014/03/31 09:10:38, René Jeschke wrote:
> On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> > We're only using AdblockPlusException for native exceptions (which should
only
> > come from libadblockplus), right? I'd vote for a more telling name, maybe
> > ("LibAdblockPlusException") or ("NativeAdblockPlusException"), not sure.
What
> do
> > you think?
> 
> I would also like to use the "AdblockPlusException" for all exceptions thrown
by
> the Java code. This way it's easier to say if it's a system error or something
> we don't like. So I would like to keep the name.

Alright.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.cpp:94: // throwing a runtime_exception in a ctor can be tolerated
here IMHO
On 2014/03/31 09:10:38, René Jeschke wrote:
> On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> > Throwing a std::runtime_error from a constructor is perfectly fine IMO,
> doesn't
> > need reasoning - std::runtime_error is really just a general purpose
exception
> > base class, and exceptions are how error handling with constructors is
> supposed
> > to work.
> > 
> > You seem to think differently?
> 
> When using 'new' and exceptions from the ctor you get an allocated but not
> initialized object. So I really don't like throwing exceptions in a ctor and
> prefer static factory methods that include cleanup on failure.

Are we talking about C++ constructors? I'm absolutely sure that no memory is
getting allocated if the constructor fails.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/Utils.h (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.h:31: #define PKG(x) "org/adblockplus/android/api/" x
On 2014/03/31 09:10:38, René Jeschke wrote:
> On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> > PKG is fine, but I'd personally vote for "PACKAGE" since we tend to not
> > abbreviate stuff much. But it's up to you.
> > 
> > More importantly, I'm wondering if we need some kind of prefix for all
macros,
> > Mozilla does it. "ABP_" comes to mind. What do you think?
> 
> I prefer PKG because it's only 3 chars. As there are only 2 macros that uses
> this short naming schema (TYP & PKG) and as both are intended to be used
> 'inline' with/for string creation I really would like to keep them as short as
> possible.
> 
> As for the "ABP_" extension: There are currently no name clashes and we're
also
> not exporting/reusing any of the code elsewhere, so we should be fine without
> the prefix IMO.

OK, fine with having no prefix. Also fine with PKG if you prefer it.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.h:32: #define TYP(x) "L" PKG(x) ";"
On 2014/03/31 09:10:38, René Jeschke wrote:
> On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> > Correct me if I'm wrong, but it seems the proper name for the "Lfoo/Bar;"
> thing
> > seems to be "name", that's how I understand it here:
> > 
> >
>
http://docs.oracle.com/javase/6/docs/api/java/lang/Class.html#getName%252528%...
> > 
> > Based on that, I think we should call this macro NAME.
> 
> That's not quite correct. The name of the class consists of the class name and
> package name(s) joined by dots, so "foo.Bar". "Lfoo/Bar;" is a type signature
as
> is I, B, Z, J, ...
> 
> I'll explain what the macros do and why I called them PKG and TYP:
> 
> PKG("Bar") will return "foo/Bar", so it prefixes the given simple class name
> with a fixed package and returns a FindClass compatible class name.
> 
> TYP("Bar") will return "Lfoo/Bar;", so it prefixes the package and wraps
> everything in a type definition for method signatures.
> 
> First I was thinking about naming them PACKAGE and TYPE, but decided against
> this for several reasons:
> 
> 1. TYPE will definitely clash with something ... definitely
> 2. TYPE is 4 letters, PACKAGE is 7 letters, I hate asymmetry 
> 3. As because of #1 I must change TYPE to anything and didn't want to make the
> macro longer, I came up with PKG and TYP
> 
> I would really like to keep both macros unchanged but it's up to you to make
the
> final decision.

Na it's not, we should both be able to understand the code. I can live with TYP,
your call.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.h:156: #define TRY try
On 2014/03/31 09:10:38, René Jeschke wrote:
> On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> > I believe we can live without this one :D
> 
> It is just for symmetry reasons as:
> try
> {
>   // ....
> }
> CATCH_AND_THROW(...)
> 
> looks awkward compared to having TRY on top.

Using "TRY" suggests that some magic is going on here, while "try" makes it
clear that "CATCH_AND_THROW" just generates a normal catch block. I'd rather
have things look a bit asymmetric but be more clear personally.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.h:158: #define CATCH_AND_THROW(jEnv) \
On 2014/03/31 09:10:38, René Jeschke wrote:
> On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> > How about "CATCH_AND_RETHROW"?
> 
> Rethrow would imply that I would catch the C++ exception and rethrow it after
> doing something, but I am transforming a C++ exception into a Java exception
and
> throwing this one into the JVM, so I vote for _THROW.

Fair enough.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/src/...
File src/org/adblockplus/android/api/AppInfo.java (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/src/...
src/org/adblockplus/android/api/AppInfo.java:29: // TODO: Builder or not?
On 2014/03/31 09:10:38, René Jeschke wrote:
> On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> > IMHO this should mirror the C++ API, that is: Just the default constructor
and
> > non-final public members.
> 
> Why do we want to have the object non-final? We only initialize it once, then
we
> pass it onto some JNI methods. I might provide a builder if this suits you
more,
> but public, non-final is no good and also not needed/wanted here.

All I'm saying is that the Java API should mirror the C++ API as closely as
possible for simplicity's sake.

We could change both the C++ AppInfo object and this though.

[René Jeschke, 2014-04-11 12:25:53]

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/Android.mk (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Android.mk:26: LOCAL_MODULE := abpEngine
On 2014/03/31 10:43:23, Felix H. Dahlke wrote:
> On 2014/03/31 09:10:38, René Jeschke wrote:
> > On 2014/03/28 17:36:23, Felix H. Dahlke wrote:
> > > Me being pedantic, but I don't think "abpEngine" makes a lot of sense once
> we
> > > got rid of AbpEngine.cpp et. al.
> > > 
> > > Let's keep it like this for now, but doesn't hurt to think of a suitable
> name.
> > I
> > > was thinking of something along the lines of "libadblockplusWrappers", but
> I'm
> > > not sure if that's any good. What do you think?
> > 
> > I think we should just call it 'libadblockplus' as this is exactly what it
is
> > for Android/Java IMO (even if the wrapping part is quite heavy here).
> 
> Trouble is, we actually have a module called "libadblockplus" (the actual
> libadblockplus), see above. The code in the abpEngine module is really just
> (going to be) the wrappers.

Done.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/JniCallbacks.h (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/JniCallbacks.h:53: virtual ~JniEventCallback();
On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> A virtual destructor is only required for base classes. AFAIK, nothing derives
> from JniEventCallback (or any of the callbacks below), so we can get rid of
the
> virtual, and the whole destructor if there's no destruction code.

Done.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/JniEventCallback.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/JniEventCallback.cpp:43: static jlong JNICALL JniCtor(JNIEnv* env, jclass
clazz, jobject callbackObject)
On 2014/03/31 10:43:23, Felix H. Dahlke wrote:
> On 2014/03/31 09:10:38, René Jeschke wrote:
> > On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> > > Static to the top as well here please.
> > 
> > Hm, those static methods are exported through JNI and don't seem to fit the
> > 'normal' static case, so are you sure I should put those on the top?
> 
> IMO yes, unnamed namespace should work all the same after all.

Done.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/JniFilterChangeCallback.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/JniFilterChangeCallback.cpp:46: static jlong JNICALL JniCtor(JNIEnv* env,
jclass clazz, jobject callbackObject)
On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> Static stuff to the top here as well please.

Done.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/JniFilterEngine.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/JniFilterEngine.cpp:245: AdblockPlus::FilterEngine* engine =
JniLongToTypePtr < AdblockPlus::FilterEngine > (ptr);
On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> No whitespace around < or >

Done.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/JniJsValue.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/JniJsValue.cpp:52: static jboolean JNICALL JniIsUndefined(JNIEnv* env,
jclass clazz, jlong ptr)
On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> Static stuff to the top here as well please.

Done.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/JniJsValue.h (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/JniJsValue.h:26: jobject JniJsValueList2ArrayList(JNIEnv* env,
AdblockPlus::JsValueList& list);
On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> Ha, another function where I vote for a s/2/To/!

Done.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/JniLogSystem.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/JniLogSystem.cpp:79: static jlong JNICALL JniCtor(JNIEnv* env, jclass clazz,
jobject callbackObject)
On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> Static stuff to the top here as well please.

Done.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/JniUpdaterCallback.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/JniUpdaterCallback.cpp:45: static jlong JNICALL JniCtor(JNIEnv* env, jclass
clazz, jobject callbackObject)
On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> Static stuff to the top here as well please.

Done.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/JniWebRequest.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/JniWebRequest.cpp:72: static jlong JNICALL JniCtor(JNIEnv* env, jclass
clazz, jobject callbackObject)
On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> Static stuff to the top here as well please.

Done.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/Utils.cpp (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.cpp:71: jclass clazz = env->FindClass(PKG("AdblockPlusException"));
On 2014/03/31 10:43:23, Felix H. Dahlke wrote:
> On 2014/03/31 09:10:38, René Jeschke wrote:
> > On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> > > We're only using AdblockPlusException for native exceptions (which should
> only
> > > come from libadblockplus), right? I'd vote for a more telling name, maybe
> > > ("LibAdblockPlusException") or ("NativeAdblockPlusException"), not sure.
> What
> > do
> > > you think?
> > 
> > I would also like to use the "AdblockPlusException" for all exceptions
thrown
> by
> > the Java code. This way it's easier to say if it's a system error or
something
> > we don't like. So I would like to keep the name.
> 
> Alright.

Done.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.cpp:82: JniThrowException(env, "Unknown exception from
libAdblockPlus");
On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> We usually call it "libadblockplus".

Done.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.cpp:94: // throwing a runtime_exception in a ctor can be tolerated
here IMHO
On 2014/03/31 10:43:23, Felix H. Dahlke wrote:
> On 2014/03/31 09:10:38, René Jeschke wrote:
> > On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> > > Throwing a std::runtime_error from a constructor is perfectly fine IMO,
> > doesn't
> > > need reasoning - std::runtime_error is really just a general purpose
> exception
> > > base class, and exceptions are how error handling with constructors is
> > supposed
> > > to work.
> > > 
> > > You seem to think differently?
> > 
> > When using 'new' and exceptions from the ctor you get an allocated but not
> > initialized object. So I really don't like throwing exceptions in a ctor and
> > prefer static factory methods that include cleanup on failure.
> 
> Are we talking about C++ constructors? I'm absolutely sure that no memory is
> getting allocated if the constructor fails.

Done.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
File jni/Utils.h (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.h:31: #define PKG(x) "org/adblockplus/android/api/" x
On 2014/03/31 10:43:23, Felix H. Dahlke wrote:
> On 2014/03/31 09:10:38, René Jeschke wrote:
> > On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> > > PKG is fine, but I'd personally vote for "PACKAGE" since we tend to not
> > > abbreviate stuff much. But it's up to you.
> > > 
> > > More importantly, I'm wondering if we need some kind of prefix for all
> macros,
> > > Mozilla does it. "ABP_" comes to mind. What do you think?
> > 
> > I prefer PKG because it's only 3 chars. As there are only 2 macros that uses
> > this short naming schema (TYP & PKG) and as both are intended to be used
> > 'inline' with/for string creation I really would like to keep them as short
as
> > possible.
> > 
> > As for the "ABP_" extension: There are currently no name clashes and we're
> also
> > not exporting/reusing any of the code elsewhere, so we should be fine
without
> > the prefix IMO.
> 
> OK, fine with having no prefix. Also fine with PKG if you prefer it.

Done.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.h:32: #define TYP(x) "L" PKG(x) ";"
On 2014/03/31 10:43:23, Felix H. Dahlke wrote:
> On 2014/03/31 09:10:38, René Jeschke wrote:
> > On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> > > Correct me if I'm wrong, but it seems the proper name for the "Lfoo/Bar;"
> > thing
> > > seems to be "name", that's how I understand it here:
> > > 
> > >
> >
>
http://docs.oracle.com/javase/6/docs/api/java/lang/Class.html#getName%2525252...
> > > 
> > > Based on that, I think we should call this macro NAME.
> > 
> > That's not quite correct. The name of the class consists of the class name
and
> > package name(s) joined by dots, so "foo.Bar". "Lfoo/Bar;" is a type
signature
> as
> > is I, B, Z, J, ...
> > 
> > I'll explain what the macros do and why I called them PKG and TYP:
> > 
> > PKG("Bar") will return "foo/Bar", so it prefixes the given simple class name
> > with a fixed package and returns a FindClass compatible class name.
> > 
> > TYP("Bar") will return "Lfoo/Bar;", so it prefixes the package and wraps
> > everything in a type definition for method signatures.
> > 
> > First I was thinking about naming them PACKAGE and TYPE, but decided against
> > this for several reasons:
> > 
> > 1. TYPE will definitely clash with something ... definitely
> > 2. TYPE is 4 letters, PACKAGE is 7 letters, I hate asymmetry 
> > 3. As because of #1 I must change TYPE to anything and didn't want to make
the
> > macro longer, I came up with PKG and TYP
> > 
> > I would really like to keep both macros unchanged but it's up to you to make
> the
> > final decision.
> 
> Na it's not, we should both be able to understand the code. I can live with
TYP,
> your call.

Done.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.h:84: T get()
On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> All functions should start with upper case letters.

Done.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.h:156: #define TRY try
On 2014/03/31 10:43:23, Felix H. Dahlke wrote:
> On 2014/03/31 09:10:38, René Jeschke wrote:
> > On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> > > I believe we can live without this one :D
> > 
> > It is just for symmetry reasons as:
> > try
> > {
> >   // ....
> > }
> > CATCH_AND_THROW(...)
> > 
> > looks awkward compared to having TRY on top.
> 
> Using "TRY" suggests that some magic is going on here, while "try" makes it
> clear that "CATCH_AND_THROW" just generates a normal catch block. I'd rather
> have things look a bit asymmetric but be more clear personally.

Done.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/jni/...
jni/Utils.h:158: #define CATCH_AND_THROW(jEnv) \
On 2014/03/31 10:43:23, Felix H. Dahlke wrote:
> On 2014/03/31 09:10:38, René Jeschke wrote:
> > On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> > > How about "CATCH_AND_RETHROW"?
> > 
> > Rethrow would imply that I would catch the C++ exception and rethrow it
after
> > doing something, but I am transforming a C++ exception into a Java exception
> and
> > throwing this one into the JVM, so I vote for _THROW.
> 
> Fair enough.

Done.

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/src/...
File src/org/adblockplus/android/api/AppInfo.java (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/src/...
src/org/adblockplus/android/api/AppInfo.java:29: // TODO: Builder or not?
On 2014/03/31 10:43:23, Felix H. Dahlke wrote:
> On 2014/03/31 09:10:38, René Jeschke wrote:
> > On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> > > IMHO this should mirror the C++ API, that is: Just the default constructor
> and
> > > non-final public members.
> > 
> > Why do we want to have the object non-final? We only initialize it once,
then
> we
> > pass it onto some JNI methods. I might provide a builder if this suits you
> more,
> > but public, non-final is no good and also not needed/wanted here.
> 
> All I'm saying is that the Java API should mirror the C++ API as closely as
> possible for simplicity's sake.
> 
> We could change both the C++ AppInfo object and this though.

I don't think it's a good idea to change the C++ API here, still I think that an
immutable object is the best pattern on the Java side, so I would prefer leaving
this "as is".

[Felix Dahlke, 2014-04-11 13:01:29]

LGTM

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/src/...
File src/org/adblockplus/android/api/AppInfo.java (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/src/...
src/org/adblockplus/android/api/AppInfo.java:29: // TODO: Builder or not?
On 2014/04/11 12:25:53, René Jeschke wrote:
> On 2014/03/31 10:43:23, Felix H. Dahlke wrote:
> > On 2014/03/31 09:10:38, René Jeschke wrote:
> > > On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> > > > IMHO this should mirror the C++ API, that is: Just the default
constructor
> > and
> > > > non-final public members.
> > > 
> > > Why do we want to have the object non-final? We only initialize it once,
> then
> > we
> > > pass it onto some JNI methods. I might provide a builder if this suits you
> > more,
> > > but public, non-final is no good and also not needed/wanted here.
> > 
> > All I'm saying is that the Java API should mirror the C++ API as closely as
> > possible for simplicity's sake.
> > 
> > We could change both the C++ AppInfo object and this though.
> 
> I don't think it's a good idea to change the C++ API here, still I think that
an
> immutable object is the best pattern on the Java side, so I would prefer
leaving
> this "as is".

Discussed this on IRC. I agree now, an immutable object is better for stuff like
this, since Java doesn't support const values. I'd find this much more readable
as a builder, but feel free to do that in a follow-up, the TODO is there.

[René Jeschke, 2014-04-11 13:28:39]

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/src/...
File src/org/adblockplus/android/api/AppInfo.java (right):

http://codereview.adblockplus.org/6606493159784448/diff/5684049913839616/src/...
src/org/adblockplus/android/api/AppInfo.java:29: // TODO: Builder or not?
On 2014/04/11 13:01:29, Felix H. Dahlke wrote:
> On 2014/04/11 12:25:53, René Jeschke wrote:
> > On 2014/03/31 10:43:23, Felix H. Dahlke wrote:
> > > On 2014/03/31 09:10:38, René Jeschke wrote:
> > > > On 2014/03/28 17:28:41, Felix H. Dahlke wrote:
> > > > > IMHO this should mirror the C++ API, that is: Just the default
> constructor
> > > and
> > > > > non-final public members.
> > > > 
> > > > Why do we want to have the object non-final? We only initialize it once,
> > then
> > > we
> > > > pass it onto some JNI methods. I might provide a builder if this suits
you
> > > more,
> > > > but public, non-final is no good and also not needed/wanted here.
> > > 
> > > All I'm saying is that the Java API should mirror the C++ API as closely
as
> > > possible for simplicity's sake.
> > > 
> > > We could change both the C++ AppInfo object and this though.
> > 
> > I don't think it's a good idea to change the C++ API here, still I think
that
> an
> > immutable object is the best pattern on the Java side, so I would prefer
> leaving
> > this "as is".
> 
> Discussed this on IRC. I agree now, an immutable object is better for stuff
like
> this, since Java doesn't support const values. I'd find this much more
readable
> as a builder, but feel free to do that in a follow-up, the TODO is there.

Done.

[Felix Dahlke, 2014-04-11 13:41:16]

LGTM!