Closed
Bug 787271
Opened 11 years ago
Closed 11 years ago
Expose some JNI to js through js-ctypes
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(firefox17 fixed)
RESOLVED
FIXED
Firefox 18
Tracking | Status | |
---|---|---|
firefox17 | --- | fixed |
People
(Reporter: wesj, Assigned: wesj)
References
Details
Attachments
(2 files)
7.63 KB,
patch
|
blassey
:
review+
mfinkle
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
10.60 KB,
patch
|
blassey
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
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 2•11 years ago
|
||
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 3•11 years ago
|
||
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•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #657495 -
Flags: review?(blassey.bugs) → review+
Comment 5•11 years ago
|
||
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.
Assignee | ||
Comment 7•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb94b09949ca
Comment 8•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/eb94b09949ca
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Assignee | ||
Comment 9•11 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•11 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 11•11 years ago
|
||
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•11 years ago
|
Attachment #657495 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 12•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/46c7396b1622
Updated•11 years ago
|
status-firefox17:
--- → fixed
Updated•11 years ago
|
Flags: sec-review?(sarentz)
Updated•10 years ago
|
Flags: sec-review?(sarentz) → sec-review?
Updated•10 years ago
|
Flags: needinfo?(dveditz)
![]() |
||
Updated•10 years ago
|
Flags: sec-review?
Updated•7 years ago
|
Flags: needinfo?(dveditz)
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•