Expose some JNI to js through js-ctypes

RESOLVED FIXED in Firefox 17

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
6 months ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

unspecified
Firefox 18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox17 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
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-
(Assignee)

Comment 4

5 years ago
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.
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+
Blocks: 783921

Comment 6

5 years ago
https://github.com/cscott/skeleton-addon-fxandroid/tree/jni has an alternative pure-js approach.
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb94b09949ca
https://hg.mozilla.org/mozilla-central/rev/eb94b09949ca
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
(Assignee)

Comment 9

5 years ago
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?
(Assignee)

Comment 10

5 years ago
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+

Updated

5 years ago
Attachment #657495 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 12

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

Updated

5 years ago
status-firefox17: --- → fixed
Depends on: 799015
Flags: sec-review?(sarentz)
Depends on: 813985
Flags: sec-review?(sarentz) → sec-review?
Flags: needinfo?(dveditz)
Flags: sec-review?
Flags: needinfo?(dveditz)
You need to log in before you can comment on or make changes to this bug.