Closed
Bug 1178850
Opened 9 years ago
Closed 9 years ago
Direct native Java method calls to C++ classes
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox42 fixed)
RESOLVED
FIXED
Firefox 42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: jchen, Assigned: jchen)
References
Details
Attachments
(5 files, 1 obsolete file)
17.99 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
19.24 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
12.87 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
125.89 KB,
patch
|
jchen
:
review+
|
Details | Diff | Splinter Review |
4.80 KB,
patch
|
jchen
:
review+
|
Details | Diff | Splinter Review |
Right now most native JNI methods go through mozglue stubs to AndroidJNI.cpp. It would be great to eliminate these stubs using JNIEnv::RegisterNatives. We should also direct JNI calls to appropriate classes, e.g. a GeckoView native method would call a corresponding function in nsWindow.
Assignee | ||
Comment 1•9 years ago
|
||
Side patch to change mozilla::jni::Param to be usable directly, instead of having to use mozilla::jni::Param::Type.
Attachment #8630805 -
Flags: review?(snorp)
Assignee | ||
Comment 2•9 years ago
|
||
Add a new autogenerated file, GeneratedJNINatives.h, that contains code to generate stubs for JNI native methods and register the stubs.
Attachment #8630838 -
Flags: review?(snorp)
Assignee | ||
Comment 3•9 years ago
|
||
Add the template classes that
Attachment #8630839 -
Flags: review?(snorp)
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to Jim Chen [:jchen] [:darchons] from comment #3) > Created attachment 8630839 [details] [diff] [review] > Add supporting classes for native JNI calls (v1) > > Add the template classes that Oops. Add the template classes that generate stubs and perform registration.
Assignee | ||
Comment 5•9 years ago
|
||
Copy the new natives header from the objdir to the source directory, along with the generated wrapper files from before. The patch also removes saving the originals to .old files to avoid producing untracked files in the source tree. The .old files were not useful anyways.
Attachment #8630841 -
Flags: review?(nalexander)
Attachment #8630805 -
Flags: review?(snorp) → review+
Comment on attachment 8630838 [details] [diff] [review] Generate naive method bindings in annotation processor (v1) Review of attachment 8630838 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/annotationProcessors/AnnotationProcessor.java @@ +175,5 @@ > } > } > } > + > + FileOutputStream nativesStream = null; We have a few blocks like this that are mostly copy/paste right? Consider factoring it out into some helper method.
Attachment #8630838 -
Flags: review?(snorp) → review+
Comment on attachment 8630839 [details] [diff] [review] Add supporting classes for native JNI calls (v1) Review of attachment 8630839 [details] [diff] [review]: ----------------------------------------------------------------- This is mostly black magic to me.
Attachment #8630839 -
Flags: review?(snorp) → review+
Comment 9•9 years ago
|
||
Comment on attachment 8630841 [details] [diff] [review] Copy generated natives header (v1) Review of attachment 8630841 [details] [diff] [review]: ----------------------------------------------------------------- There's code to diff files that has rotted at the end of the Makefile.in: see the libs:: target. Explain why you're selectively diffing or update that code too. ::: mobile/android/base/Makefile.in @@ -469,2 @@ > @cp $(CURDIR)/jni-stubs.inc $(topsrcdir)/mozglue/android > - @cp $(CURDIR)/GeneratedJNIWrappers.* $(topsrcdir)/widget/android nit: Prefer not wild-carding. Why is this not copying GeneratedJNINatives.h?
Attachment #8630841 -
Flags: review?(nalexander) → feedback+
Comment 10•9 years ago
|
||
Comment on attachment 8630841 [details] [diff] [review] Copy generated natives header (v1) Review of attachment 8630841 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/Makefile.in @@ -469,2 @@ > @cp $(CURDIR)/jni-stubs.inc $(topsrcdir)/mozglue/android > - @cp $(CURDIR)/GeneratedJNIWrappers.* $(topsrcdir)/widget/android Crap, I mis-intepreted this diff. This is fine. Still, prefer not wild-carding.
Attachment #8630841 -
Flags: review+
Comment 11•9 years ago
|
||
Comment on attachment 8630838 [details] [diff] [review] Generate naive method bindings in annotation processor (v1) Review of attachment 8630838 [details] [diff] [review]: ----------------------------------------------------------------- Do you get the deps on the new Natives.h for free (i.e., implicit from the existing generated .cpp file)? If so, please comment in the appropiate Makefile.in (mobile/android/base, I guess).
Assignee | ||
Comment 12•9 years ago
|
||
(In reply to Nick Alexander :nalexander from comment #9) > Comment on attachment 8630841 [details] [diff] [review] > Copy generated natives header (v1) > > Review of attachment 8630841 [details] [diff] [review]: > ----------------------------------------------------------------- > > There's code to diff files that has rotted at the end of the Makefile.in: > see the libs:: target. Explain why you're selectively diffing or update > that code too. We were selectively diffing before. This new patch changes it to diffing all the files. (In reply to Nick Alexander :nalexander from comment #10) > Comment on attachment 8630841 [details] [diff] [review] > Copy generated natives header (v1) > > Review of attachment 8630841 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/Makefile.in > @@ -469,2 @@ > > @cp $(CURDIR)/jni-stubs.inc $(topsrcdir)/mozglue/android > > - @cp $(CURDIR)/GeneratedJNIWrappers.* $(topsrcdir)/widget/android > > Crap, I mis-intepreted this diff. This is fine. Still, prefer not > wild-carding. Fixed the wildcard. (In reply to Nick Alexander :nalexander from comment #11) > Comment on attachment 8630838 [details] [diff] [review] > Generate naive method bindings in annotation processor (v1) > > Review of attachment 8630838 [details] [diff] [review]: > ----------------------------------------------------------------- > > Do you get the deps on the new Natives.h for free (i.e., implicit from the > existing generated .cpp file)? If so, please comment in the appropiate > Makefile.in (mobile/android/base, I guess). Added a few comments to mobile/android/base/Makefile.in
Attachment #8630841 -
Attachment is obsolete: true
Attachment #8632230 -
Flags: review+
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/821b22ce79e1 https://hg.mozilla.org/integration/mozilla-inbound/rev/8ee5809f349b https://hg.mozilla.org/integration/mozilla-inbound/rev/d6dab7810669 https://hg.mozilla.org/integration/mozilla-inbound/rev/c02b603104ea https://hg.mozilla.org/integration/mozilla-inbound/rev/79085d3894e8
Backed out for android build bustage in https://hg.mozilla.org/integration/mozilla-inbound/rev/0084c286fd30 https://treeherder.mozilla.org/logviewer.html#?job_id=11569257&repo=mozilla-inbound
Flags: needinfo?(nchen)
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f032df69e2ed https://hg.mozilla.org/integration/mozilla-inbound/rev/ad3cecc11ead https://hg.mozilla.org/integration/mozilla-inbound/rev/a85c903f9181 https://hg.mozilla.org/integration/mozilla-inbound/rev/4f37bdd44838 https://hg.mozilla.org/integration/mozilla-inbound/rev/0a4c833f2b0e https://hg.mozilla.org/integration/mozilla-inbound/rev/24e959bb47f5
Comment 16•9 years ago
|
||
You filed a bug for that clobber, right?
Comment 18•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f032df69e2ed https://hg.mozilla.org/mozilla-central/rev/ad3cecc11ead https://hg.mozilla.org/mozilla-central/rev/a85c903f9181 https://hg.mozilla.org/mozilla-central/rev/4f37bdd44838 https://hg.mozilla.org/mozilla-central/rev/0a4c833f2b0e https://hg.mozilla.org/mozilla-central/rev/24e959bb47f5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
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
•