1291375 - [geckoview] Remove all JNI entry points from outside GeckoView

Status: RESOLVED FIXED

(GeckoView :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

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.

Comment 1

[Nick Alexander :nalexander [he/him]]

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.

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 3

[Nick Alexander :nalexander [he/him]]

(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.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 1. Compile separate gecko-view.jar (v1)

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.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 2. Let AnnotationProcessor generate multiple sets of bindings (v1)

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.

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 3. Separate Fennec JNI binding generation (v1)

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*.

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 4. Update auto-generated bindings (v1)

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 5. Use Fennec bindings where needed (v1)

Switch include statements from GeckoView bindings to Fennec bindings
where needed.

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 6. Move Fennec JNI headers (v1)

Move Fennec-specific JNI headers to widget/android/fennec.

Comment 12

[Nick Alexander :nalexander [he/him]]

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.

Comment 13

[Nick Alexander :nalexander [he/him]]

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.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/797a4dbc48d8
https://hg.mozilla.org/mozilla-central/rev/3d98d13567fc
https://hg.mozilla.org/mozilla-central/rev/9e9c3d6cb11b
https://hg.mozilla.org/mozilla-central/rev/8e6dd69abc28
https://hg.mozilla.org/mozilla-central/rev/3cf1197dbe96
https://hg.mozilla.org/mozilla-central/rev/29dc959e853c