Closed Bug 1291375 Opened 3 years ago Closed 3 years ago

[geckoview] Remove all JNI entry points from outside GeckoView

Categories

(GeckoView :: General, defect)

defect
Not set

Tracking

(firefox51 fixed)

RESOLVED FIXED
Tracking Status
firefox51 --- fixed

People

(Reporter: nalexander, Assigned: jchen)

References

Details

Attachments

(6 files)

There are not many JNI entry points outside, but they need to go.
After the file moves in Bug 1291363, all uses of
@WrapForJNI/@JNITarget/etc should be in mobile/android/geckoview.  In
fact, we probably want to split the o.m.g.annotations into GeckoView
and Fennec annotations.  If we can, we want @RobocopTarget to be a
Fennec annotation, since library consumers should never assume
anything about the ProGuard behaviour of the libraries they consume.
jchen: I think this is the first thing to address.  The set of GeckoView files is given in https://reviewboard.mozilla.org/r/60476/diff/1#279, so any JNI interaction needs to be confined to those files.
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Talked with snorp about this. We don't think we should eliminate all JNI entry points outside of /geckoview, but just to make sure those outside entry points don't get used in a non-Fennec environment. We still need JNI for things like telemetry that exist outside of /geckoview.
(In reply to Jim Chen [:jchen] [:darchons] from comment #2)
> Talked with snorp about this. We don't think we should eliminate all JNI
> entry points outside of /geckoview, but just to make sure those outside
> entry points don't get used in a non-Fennec environment. We still need JNI
> for things like telemetry that exist outside of /geckoview.

I disagree with this: no non-Mozilla consumer will ever use JNI in this way.  (At least, they won't do it using our @WrapForJNI/@JNITarget/etc annotations.)  GeckoView should try to make Fennec as close to a non-Mozilla consumer as possible (to reduce special cases, to test what others use, etc), and therefore, we should aim to remove non-standard things when we can.

However, in the here in now, I guess we can keep this build system wart.
Well, Fennec code requires JNI; no JNI is not really an option and @WrapForJNI is the best system we have. If your concern is with Fennec and GeckoView using the same JNI annotation and build steps, then I think we'd be okay with splitting the two so they use separate JNI annotations and build steps.
It's a lot easier to split JNI binding generation when GeckoView code can be compiled separately from Fennec code, so I'll be focusing on that in the meantime.
Once bug 1291385 et al land, we'll be able to compile geckoview sources
separately from Fennec sources. This patch puts compiled geckoview into a
separate gecko-view.jar file, apart from gecko-browser.jar. Furthermore, having
a gecko-view.jar lets us generate separate JNI bindings for geckoview and
Fennec.
Attachment #8790871 - Flags: review?(nalexander)
Let AnnotationProcessor accept an output prefix argument, so that we can
generate two different sets of bindings for different jars - one set for
GeckoView code and one set for Fennec code.
Attachment #8791308 - Flags: review?(snorp)
Separate compiled JARs into GECKOVIEW_JARS and FENNEC_JARS, and run
AnnotationProcessor separately on each set. The GeckoView bindings are
put into widget/android/GeneratedJNI* (same as before), while the
Fennec-specific bindings are put into widget/android/fennec/FennecJNI*.
Attachment #8791312 - Flags: review?(snorp)
Attachment #8791312 - Flags: review?(nalexander)
Switch include statements from GeckoView bindings to Fennec bindings
where needed.
Attachment #8791314 - Flags: review?(snorp)
Move Fennec-specific JNI headers to widget/android/fennec.
Attachment #8791315 - Flags: review?(snorp)
Comment on attachment 8790871 [details] [diff] [review]
1. Compile separate gecko-view.jar (v1)

Review of attachment 8790871 [details] [diff] [review]:
-----------------------------------------------------------------

You did it!  Well done.

::: mobile/android/base/moz.build
@@ +201,1 @@
>          'gecko-mozglue.jar',

I can think of little advantage at this point to keeping util and mozglue separate from view.  Do you agree?  Follow-up to coalesce?

@@ +292,5 @@
> +gvjar.sources += [ thirdparty_source_dir + f for f in [
> +    'com/googlecode/eyesfree/braille/selfbraille/ISelfBrailleService.java',
> +    'com/googlecode/eyesfree/braille/selfbraille/SelfBrailleClient.java',
> +    'com/googlecode/eyesfree/braille/selfbraille/WriteData.java',
> +] ]

nit: ]], no space.
Attachment #8790871 - Flags: review?(nalexander) → review+
Comment on attachment 8791312 [details] [diff] [review]
3. Separate Fennec JNI binding generation (v1)

Review of attachment 8791312 [details] [diff] [review]:
-----------------------------------------------------------------

I don't want to grow the Makefile.in, but we're in service of a greater goal here.  This should be good.
Attachment #8791312 - Flags: review?(nalexander) → review+
Attachment #8791308 - Flags: review?(snorp) → review+
Attachment #8791312 - Flags: review?(snorp) → review+
Attachment #8791314 - Flags: review?(snorp) → review+
Attachment #8791315 - Flags: review?(snorp) → review+
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/797a4dbc48d8
1. Compile separate gecko-view.jar; r=nalexander
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d98d13567fc
2. Let AnnotationProcessor generate multiple sets of bindings; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e9c3d6cb11b
3. Separate Fennec JNI binding generation; r=nalexander r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e6dd69abc28
4. Update auto-generated bindings; r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cf1197dbe96
5. Use Fennec bindings where needed; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/29dc959e853c
6. Move Fennec JNI headers; r=snorp
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 51 → ---
You need to log in before you can comment on or make changes to this bug.