Last Comment Bug 787271 - Expose some JNI to js through js-ctypes
: Expose some JNI to js through js-ctypes
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 18
Assigned To: Wesley Johnston (:wesj)
:
Mentors:
Depends on: 799015 813985
Blocks: 783921
  Show dependency treegraph
 
Reported: 2012-08-30 17:00 PDT by Wesley Johnston (:wesj)
Modified: 2013-12-18 14:10 PST (History)
9 users (show)
yvanboily+mozbugmail: needinfo? (dveditz)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Patch (7.63 KB, patch)
2012-08-30 17:03 PDT, Wesley Johnston (:wesj)
blassey.bugs: review+
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Patch 2/2 (10.60 KB, patch)
2012-08-31 17:12 PDT, Wesley Johnston (:wesj)
blassey.bugs: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Wesley Johnston (:wesj) 2012-08-30 17:00:31 PDT
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) 2012-08-30 17:03:08 PDT
Created attachment 657080 [details] [diff] [review]
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?) 2012-08-31 12:36:29 PDT
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?) 2012-08-31 13:14:46 PDT
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) 2012-08-31 17:12:25 PDT
Created attachment 657495 [details] [diff] [review]
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?) 2012-09-04 20:25:56 PDT
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 2012-09-11 15:40:32 PDT
https://github.com/cscott/skeleton-addon-fxandroid/tree/jni has an alternative pure-js approach.
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-09-12 18:16:50 PDT
https://hg.mozilla.org/mozilla-central/rev/eb94b09949ca
Comment 9 Wesley Johnston (:wesj) 2012-09-18 10:29:20 PDT
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) 2012-09-18 10:29:28 PDT
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] 2012-09-19 17:24:36 PDT
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) 2012-09-20 09:07:27 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/46c7396b1622

Note You need to log in before you can comment on or make changes to this bug.