Use jni_generator in Chromoting Android client

Use jni_generator in Chromoting Android client

This removes all manually-written JNI interface code and uses the
automatically generated JNI stubs instead.

BUG=304225
R=digit@chromium.org, solb@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231600

[Lambros, 2013-10-18 19:48:09]

@digit: I'm having trouble getting this to work - please could you take a look?

The program exits immediately when run - here's the log:

10-18 11:42:56.053: I/ActivityManager(532): START u0
{act=android.intent.action.MAIN cat=[android.intent.category.LAUNCHER]
flg=0x10200000 cmp=org.chromium.chromoting/.Chromoting} from pid 1720
10-18 11:42:56.083: I/ActivityManager(532): Start proc org.chromium.chromoting
for activity org.chromium.chromoting/.Chromoting: pid=871 uid=10073 gids={50073,
3003, 1028}
10-18 11:42:56.083: D/dalvikvm(871): Late-enabling CheckJNI
10-18 11:42:56.183: D/dalvikvm(871): Trying to load lib
/data/app-lib/org.chromium.chromoting-2/libremoting_client_jni.so 0x420d6db8
10-18 11:42:56.183: D/dalvikvm(871): Added shared lib
/data/app-lib/org.chromium.chromoting-2/libremoting_client_jni.so 0x420d6db8
10-18 11:42:56.193: E/dalvikvm(871): ERROR: couldn't find native method
10-18 11:42:56.193: E/dalvikvm(871): Requested:
Lorg/chromium/chromoting/jni/JniInterface;.nativeloadNative:(Landroid/content/Context;)V
10-18 11:42:56.193: E/chromium(871): [1018/114256:ERROR:JniInterface_jni.h(312)]
RegisterNatives failed in gen/remoting/jni/JniInterface_jni.h
10-18 11:42:56.193: E/chromium(871): [1018/114256:ERROR:jni_registrar.cc(21)]
remoting failed registration!
10-18 11:42:56.193: W/dalvikvm(871): JNI_OnLoad returned bad version (-1) in
/data/app-lib/org.chromium.chromoting-2/libremoting_client_jni.so 0x420d6db8
10-18 11:42:56.193: D/AndroidRuntime(871): Shutting down VM
10-18 11:42:56.193: W/dalvikvm(871): threadid=1: thread exiting with uncaught
exception (group=0x41828700)
10-18 11:42:56.193: E/AndroidRuntime(871): FATAL EXCEPTION: main
10-18 11:42:56.193: E/AndroidRuntime(871): java.lang.UnsatisfiedLinkError:
unknown failure
10-18 11:42:56.193: E/AndroidRuntime(871): 	at
java.lang.Runtime.loadLibrary(Runtime.java:361)
10-18 11:42:56.193: E/AndroidRuntime(871): 	at
java.lang.System.loadLibrary(System.java:525)
10-18 11:42:56.193: E/AndroidRuntime(871): 	at
org.chromium.chromoting.jni.JniInterface.loadLibrary(JniInterface.java:62)
10-18 11:42:56.193: E/AndroidRuntime(871): 	at
org.chromium.chromoting.Chromoting.onCreate(Chromoting.java:101)
10-18 11:42:56.193: E/AndroidRuntime(871): 	at
android.app.Activity.performCreate(Activity.java:5133)
10-18 11:42:56.193: E/AndroidRuntime(871): 	at
android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1087)
10-18 11:42:56.193: E/AndroidRuntime(871): 	at
android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2175)
10-18 11:42:56.193: E/AndroidRuntime(871): 	at
android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2261)
10-18 11:42:56.193: E/AndroidRuntime(871): 	at
android.app.ActivityThread.access$600(ActivityThread.java:141)
10-18 11:42:56.193: E/AndroidRuntime(871): 	at
android.app.ActivityThread$H.handleMessage(ActivityThread.java:1256)
10-18 11:42:56.193: E/AndroidRuntime(871): 	at
android.os.Handler.dispatchMessage(Handler.java:99)
10-18 11:42:56.193: E/AndroidRuntime(871): 	at
android.os.Looper.loop(Looper.java:137)
10-18 11:42:56.193: E/AndroidRuntime(871): 	at
android.app.ActivityThread.main(ActivityThread.java:5103)
10-18 11:42:56.193: E/AndroidRuntime(871): 	at
java.lang.reflect.Method.invokeNative(Native Method)
10-18 11:42:56.193: E/AndroidRuntime(871): 	at
java.lang.reflect.Method.invoke(Method.java:525)
10-18 11:42:56.193: E/AndroidRuntime(871): 	at
com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:737)
10-18 11:42:56.193: E/AndroidRuntime(871): 	at
com.android.internal.os.ZygoteInit.main(ZygoteInit.java:553)
10-18 11:42:56.193: E/AndroidRuntime(871): 	at
dalvik.system.NativeStart.main(Native Method)

[digit1, 2013-10-19 09:50:04]

Sure, I'd be happy to help :)

https://codereview.chromium.org/29583003/diff/30001/remoting/android/java/src...
File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
(right):

https://codereview.chromium.org/29583003/diff/30001/remoting/android/java/src...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:71:
private static native void loadNative(Context context);
Hint: You must call this function "nativeLoadNative()" for the JNI generator to
process it and generate a stub for it. The auot-generated
"jni/JniInteface_jni.h" will contain the following:

- A function named "RegisterNativesImpl(JNIEnv*)" which you must call explicitly
at JNI_OnLoad() time.
- An auto-generated stub that will call a function named "LoadNative()" that
must be defined in the source code that includes the header.

This patch is currently crashing because you never register a native method hook
for this 'loadNative()' in the current implementation.

Don't worry if you didn't get it on the first try, it means we must better
document this whole process, that's all :)

https://codereview.chromium.org/29583003/diff/30001/remoting/client/jni/chrom...
File remoting/client/jni/chromoting_jni_runtime.cc (right):

https://codereview.chromium.org/29583003/diff/30001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_runtime.cc:43: { "remoting",
remoting::RegisterNativesImpl },
nit: I would suggest moving this into its own source file (e.g.
"chromoting_jni_onload.cc").

The typical convention is the following:

  - If you're writing a "component" library (e.g. base/, net/, etc...), then do
not put a JNI_OnLoad()
     function at all. Instead, export a registration function like
'base::android::RegisterJni' above)

  - If you're writing an "embedder" library (i.e. content_shell, or a shared
library for any other kind
     of actual Android application), then put  JNI_OnLoad() that calls the
registar functions of each
     component library it uses.

I don't know exactly what you're doing here, but in all cases, if you need a
JNI_OnLoad() here, it's much better to put it in its own source file instead.

This also means defining here a function like:

bool remoting::RegisterJni(JNIEnv* env) {
  return remoting::RegisterNativesImpl(env);
}

and declare it in "chromoting_jni_runtime.h" as well, to be included by
"chromoting_jni_onload.cc" or whatever you decide to call it.

Hope this is clear enough :) If not, please keep asking!

https://codereview.chromium.org/29583003/diff/30001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_runtime.cc:61: static void loadNative(JNIEnv*
env, jclass clazz, jobject context) {
After renaming the Java method to "nativeLoadNative", try renaming this to
LoadNative() so it can be called by the stub that will be generated by
jni_generator.

[Lambros, 2013-10-23 17:35:23]

Got it working now, thanks for your help!

digit: ptal
solb: This is more FYI, but I'd like OWNERS approval.

I get a presubmit error from the DEPS change on this. I guess it's OK to ignore
this and land manually?

** Presubmit Messages **
Missing LGTM from OWNERS of directories added to DEPS:
    '+jni',

Suggested missing target path OWNERS:
    darin@chromium.org

https://codereview.chromium.org/29583003/diff/30001/remoting/android/java/src...
File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
(right):

https://codereview.chromium.org/29583003/diff/30001/remoting/android/java/src...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:71:
private static native void loadNative(Context context);
On 2013/10/19 09:50:04, digit1 wrote:
> Hint: You must call this function "nativeLoadNative()" for the JNI generator
to
> process it and generate a stub for it. The auot-generated
> "jni/JniInteface_jni.h" will contain the following:
> 
> - A function named "RegisterNativesImpl(JNIEnv*)" which you must call
explicitly
> at JNI_OnLoad() time.
> - An auto-generated stub that will call a function named "LoadNative()" that
> must be defined in the source code that includes the header.
> 
> This patch is currently crashing because you never register a native method
hook
> for this 'loadNative()' in the current implementation.
> 
> Don't worry if you didn't get it on the first try, it means we must better
> document this whole process, that's all :)

Done.

https://codereview.chromium.org/29583003/diff/30001/remoting/client/jni/chrom...
File remoting/client/jni/chromoting_jni_runtime.cc (right):

https://codereview.chromium.org/29583003/diff/30001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_runtime.cc:43: { "remoting",
remoting::RegisterNativesImpl },
On 2013/10/19 09:50:04, digit1 wrote:
> nit: I would suggest moving this into its own source file (e.g.
> "chromoting_jni_onload.cc").
> 
> The typical convention is the following:
> 
>   - If you're writing a "component" library (e.g. base/, net/, etc...), then
do
> not put a JNI_OnLoad()
>      function at all. Instead, export a registration function like
> 'base::android::RegisterJni' above)
> 
>   - If you're writing an "embedder" library (i.e. content_shell, or a shared
> library for any other kind
>      of actual Android application), then put  JNI_OnLoad() that calls the
> registar functions of each
>      component library it uses.
> 
> I don't know exactly what you're doing here, but in all cases, if you need a
> JNI_OnLoad() here, it's much better to put it in its own source file instead.
> 
> This also means defining here a function like:
> 
> bool remoting::RegisterJni(JNIEnv* env) {
>   return remoting::RegisterNativesImpl(env);
> }
> 
> and declare it in "chromoting_jni_runtime.h" as well, to be included by
> "chromoting_jni_onload.cc" or whatever you decide to call it.
> 
> Hope this is clear enough :) If not, please keep asking!

Done.

https://codereview.chromium.org/29583003/diff/30001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_runtime.cc:61: static void loadNative(JNIEnv*
env, jclass clazz, jobject context) {
On 2013/10/19 09:50:04, digit1 wrote:
> After renaming the Java method to "nativeLoadNative", try renaming this to
> LoadNative() so it can be called by the stub that will be generated by
> jni_generator.

Done.

[solb, 2013-10-25 15:44:23]

lgtm

Sorry I didn't take the time to learn about the generator when I was first
committing the project!

[Lambros, 2013-10-25 20:21:10]

On 2013/10/25 15:44:23, solb wrote:
> lgtm
> 
> Sorry I didn't take the time to learn about the generator when I was first
> committing the project!

No worries - it's given me the chance to learn about this stuff! :)

@digit: OK for me to land this manually after trybot passes? I don't need extra
approval for adding +jni to DEPS, do I?

[commit-bot: I haz the power, 2013-10-25 20:22:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/29583003/9...

[digit1, 2013-10-29 10:26:09]

lgtm - very nice change, thanks :)

[Lambros, 2013-10-29 17:57:01]

Committed patchset #2 manually as r231600.