787271 - Expose some JNI to js through js-ctypes

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

This is useful for the frontend (avoid having to pass messages or write special bridge or interface messages for everything) and potentially for addons. Going to be used for bug 783921.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

Patch. This only exposes a few JNI methods:

jclass FindClass(const char *className);
jmethodID GetStaticMethodID(jclass methodClass,
                            const char *methodName,
                            const char *signature);
bool ExceptionCheck();
void CallStaticVoidMethodA(jclass cls, jmethodID method, jvalue *values);
int CallStaticIntMethodA(jclass cls, jmethodID method, jvalue *values);

That's all (more actually with the staticvoid one) that I need right now. Adding more is fairly easy.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 657080 [details] [diff] [review]
Patch


>+  },
>+  findClass: function(name) {

nit: Add a blank line

>+    var ret = this._findClass(name);

let ?

>+  },
>+  getStaticMethodID: function(aClass, aName, aSignature) {

nit: same

>+    var ret = this._getStaticMethodID(aClass, aName, aSignature);

nit: same

>+  },
>+  exceptionCheck: function() {

nit: same

>+  },
>+  callStaticVoidMethod: function(aClass, aName, args) {

nit: same

>+    var ret = this._callStaticVoidMethod(aClass, aName, args);

nit: same

>+  },
>+  callStaticIntMethod: function(aClass, aName, args) {

nit: same

>+    var ret = this._callStaticIntMethod(aClass, aName, args);

nit: same

>diff --git a/mobile/android/modules/Makefile.in b/mobile/android/modules/Makefile.in

> EXTRA_JS_MODULES = \
>   LocaleRepository.jsm \
>   linuxTypes.jsm \
>   video.jsm \

Hmm, do we really use these 3 anymore? Followup to remove them?


> EXTRA_PP_JS_MODULES = \
>   contacts.jsm \

Same

>diff --git a/widget/android/AndroidBridge.cpp b/widget/android/AndroidBridge.cpp

>+extern "C" {

>+  FindClass(const char *className) {

>+  GetStaticMethodID(jclass methodClass,

>+  ExceptionCheck() {

>+  CallStaticVoidMethodA(jclass cls,

>+  CallStaticIntMethodA(jclass cls,

Should we use some prefix like "jsjni_" for these functions? I am worried we could cause some collisions with other generically named functions.

I think we should file a followup bug to add more JNI support. Right now we only support enough to handle the uses we have planned for now. If we can't get solid, general support, we should consider scaling back to simple jsctypes-only approaches.

r+ from me on the general idea. fix the nits. file the followups.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 657080 [details] [diff] [review]
Patch

Review of attachment 657080 [details] [diff] [review]:
-----------------------------------------------------------------

I would rather hide the ctype details from callers. The methodID object returned from the findClass wrapper can hold onto the signature such that the callXXXMethod() methods can handle all of the argument casting internally.

This would also allow us to preserve this interface even if we later changed the underlying implementation away from ctypes.

Otherwise, good stuff.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

This applies on top of the first patch. It gives us a class, jMethod, and the ability to parse JNI method signatures (tested the regex's on a various examples I found around the web, and tested by putting some dumb methods in GeckoAppShel for testing). 

You can now do something like:
 *   let jni = new JNI();
 *   cls = jni.findClass("org.mozilla.gecko.GeckoAppShell");
 *   method = jni.getStaticMethodID(cls, "getPreferredIconSize", "(IZF)I");
 *
 *   let val = jni.callStaticIntMethod(cls, method, 3, true, 3.05);
 *   // close the jni library when you are done
 *   jni.close();

Support for arrays and strings will mean exposing a bit more JNI to JS and probably wrapping it in some nice wrapper functions. I'll file some bugs.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 657080 [details] [diff] [review]
Patch

Review of attachment 657080 [details] [diff] [review]:
-----------------------------------------------------------------

I would rather hide the ctype details from callers. The methodID object returned from the findClass wrapper can hold onto the signature such that the callXXXMethod() methods can handle all of the argument casting internally.

This would also allow us to preserve this interface even if we later changed the underlying implementation away from ctypes.

Otherwise, good stuff.

::: widget/android/AndroidBridge.cpp
@@ +2534,5 @@
> +extern "C" {
> +  __attribute__ ((visibility("default")))
> +  jclass
> +  FindClass(const char *className) {
> +    JNIEnv *env = GetJNIForThread();

this should always be called on the main thread, right? Just call the cheaper GetJNIEnv();

@@ +2562,5 @@
> +  void
> +  CallStaticVoidMethodA(jclass cls,
> +                        jmethodID method,
> +                        jvalue *values) {
> +    JNIEnv *env = GetJNIForThread();

you should have a jniframe here

@@ +2572,5 @@
> +  int
> +  CallStaticIntMethodA(jclass cls,
> +                       jmethodID method,
> +                       jvalue *values) {
> +    JNIEnv *env = GetJNIForThread();

and here too

Comment 6

[cscott]

https://github.com/cscott/skeleton-addon-fxandroid/tree/jni has an alternative pure-js approach.

Comment 7

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb94b09949ca

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eb94b09949ca

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 657080 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This is required for bug 783921 to be uplifted
User impact if declined: Shortcuts looks kinda crappy
Testing completed (on m-c, etc.): Landed on mc this week
Risk to taking this patch (and alternatives if risky): I'm working on a refactoring that lessens the C changes here (but involves more JS). Its not quite ready yet.
String or UUID changes made by this patch: None.

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 657495 [details] [diff] [review]
Patch 2/2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This is required for bug 783921 to be uplifted
User impact if declined: Shortcuts looks kinda crappy
Testing completed (on m-c, etc.): Landed on mc this week
Risk to taking this patch (and alternatives if risky): I'm working on a refactoring that lessens the C changes here (but involves more JS). Its not quite ready yet.
String or UUID changes made by this patch: None.

Comment 11

[Alex Keybl [:akeybl]]

Comment on attachment 657080 [details] [diff] [review]
Patch

[Triage Comment]
This has had some time to bake on Nightly, so let's get this mobile-only polish uplifted in preparation for the Marketplace Beta. Given this landing, let's not diverge Nightly/Aurora with a refactoring in the short term, if at all possible.

Comment 12

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/46c7396b1622