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)

All
Android
defect
Not set
normal

Tracking

(firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(5 files, 1 obsolete file)

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.
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)
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)
Add the template classes that
Attachment #8630839 - Flags: review?(snorp)
(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.
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)
Autogenerated files.
Attachment #8630842 - Flags: review+
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 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 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 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).
(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+
You filed a bug for that clobber, right?
Filed bug 1182840.
Flags: needinfo?(nchen)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: