Closed Bug 787271 Opened 9 years ago Closed 9 years ago

Expose some JNI to js through js-ctypes

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(firefox17 fixed)

RESOLVED FIXED
Firefox 18
Tracking Status
firefox17 --- fixed

People

(Reporter: wesj, Assigned: wesj)

References

Details

Attachments

(2 files)

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.
Attached patch PatchSplinter Review
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.
Assignee: nobody → wjohnston
Attachment #657080 - Flags: review?(mark.finkle)
Attachment #657080 - Flags: review?(blassey.bugs)
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.
Attachment #657080 - Flags: review?(mark.finkle) → review+
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.
Attachment #657080 - Flags: review?(blassey.bugs) → review-
Attached patch Patch 2/2Splinter Review
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.
Attachment #657495 - Flags: review?(blassey.bugs)
Attachment #657495 - Flags: review?(blassey.bugs) → review+
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
Attachment #657080 - Flags: review- → review+
https://github.com/cscott/skeleton-addon-fxandroid/tree/jni has an alternative pure-js approach.
https://hg.mozilla.org/mozilla-central/rev/eb94b09949ca
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
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.
Attachment #657080 - Flags: approval-mozilla-aurora?
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.
Attachment #657495 - Flags: approval-mozilla-aurora?
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.
Attachment #657080 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #657495 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 799015
Flags: sec-review?(sarentz)
Flags: sec-review?(sarentz) → sec-review?
Flags: needinfo?(dveditz)
Flags: needinfo?(dveditz)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.