1351000 - Generate and use separate omni.ja for GeckoView

Status: RESOLVED FIXED

(GeckoView :: General, enhancement)

Description

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

Having a different omni.ja than Fennec for GeckoView will save us at least 1MB in apk size, and is necessary for us to not use some Fennec components such as the existing PromptService, which is not suitable for GeckoView use.

Comment 1

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

Attachment: 1. Generate separate omni.ja for GeckoView (v1)

Generate a separate omni.ja for GeckoView during the packaging step,
under dist/geckoview. Define a MOZ_GECKOVIEW_JAR flag to optionally
include/exclude files in the package manifest.

Comment 2

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

Attachment: 2. Separate out GeckoView chrome/JS files (v1)

Put GeckoView chrome content and JS module files into their own
GeckoView-specific directories, to make it easier to build separate JARs
for GeckoView and Fennec.

Comment 3

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8852616 [details] [diff] [review]
1. Generate separate omni.ja for GeckoView (v1)

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

I believe this will achieve what you are intending, but it's very likely to break as the existing packager changes (for example, you needed to turn an `=` into an `?=` in at least one place) and it will make it that much more difficult to move the packager forward over time.

We have not pursued this direction for expediency, but I always envisioned something like https://bugzilla.mozilla.org/show_bug.cgi?id=1291367 being how we would handle this kind of GeckoView-specific configuration.  Such an approach is frustrating now because so much code is shared, but over time this allows us to dramatically simplify GeckoView's build (and built outputs!) in ways that forking does not.

So I would like to get your thinking on the relative merits of the two approaches before I mark as r+.  In addition, glandium understands the internals of the packager and assumptions about omni.ja much better than I do.  I'd like him to review this approach, comment on the relative merits, and perhaps counter with alternate options.  Glandium?

Comment 4

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8852617 [details] [diff] [review]
2. Separate out GeckoView chrome/JS files (v1)

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

FWIW, I did exactly this in my proof-of-concept GeckoView branch.  I think the advantage of the approach suggested by https://bugzilla.mozilla.org/show_bug.cgi?id=1291367 is even more clear here; rather than overloading a single `package-manifest.in` to mean rapidly diverging things, you extract a shared include and build upon it.  This separation of platform (Gecko) and application looks better and better as things like GeckoView and https://github.com/mozilla/qbrt move forward.

Comment 5

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

(In reply to Nick Alexander :nalexander from comment #3)
> Comment on attachment 8852616 [details] [diff] [review]
> 1. Generate separate omni.ja for GeckoView (v1)
> 
> Review of attachment 8852616 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We have not pursued this direction for expediency, but I always envisioned
> something like https://bugzilla.mozilla.org/show_bug.cgi?id=1291367 being
> how we would handle this kind of GeckoView-specific configuration.  Such an
> approach is frustrating now because so much code is shared, but over time
> this allows us to dramatically simplify GeckoView's build (and built
> outputs!) in ways that forking does not.

I think this is the best approach in the long term. In the short term, we have a pressing need for separate omni.ja because of prompts (we need different prompt services registered for GeckoView versus Fennec), and I believe this patch is a reasonable short-term solution that doesn't involve massive changes across the board.

(In reply to Nick Alexander :nalexander from comment #4)
> Comment on attachment 8852617 [details] [diff] [review]
> 2. Separate out GeckoView chrome/JS files (v1)
> 
> Review of attachment 8852617 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> FWIW, I did exactly this in my proof-of-concept GeckoView branch.  I think
> the advantage of the approach suggested by
> https://bugzilla.mozilla.org/show_bug.cgi?id=1291367 is even more clear
> here; rather than overloading a single `package-manifest.in` to mean rapidly
> diverging things, you extract a shared include and build upon it.  This
> separation of platform (Gecko) and application looks better and better as
> things like GeckoView and https://github.com/mozilla/qbrt move forward.

I originally had a separate GeckoView manifest, which the Fennec manifest included, but the Fennec-only set is small enough that I decided a single manifest would work just as well. I think eventually "separation of platform and application" will look like separate omni.ja and app.ja files. Building GeckoView would build omni.ja, and building Fennec on top of GeckoView would build an app.ja alongside the omni.ja from GeckoView.

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #5)
> (In reply to Nick Alexander :nalexander from comment #3)
> > Comment on attachment 8852616 [details] [diff] [review]
> > 1. Generate separate omni.ja for GeckoView (v1)
> > 
> > Review of attachment 8852616 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > We have not pursued this direction for expediency, but I always envisioned
> > something like https://bugzilla.mozilla.org/show_bug.cgi?id=1291367 being
> > how we would handle this kind of GeckoView-specific configuration.  Such an
> > approach is frustrating now because so much code is shared, but over time
> > this allows us to dramatically simplify GeckoView's build (and built
> > outputs!) in ways that forking does not.
> 
> I think this is the best approach in the long term. In the short term, we
> have a pressing need for separate omni.ja because of prompts (we need
> different prompt services registered for GeckoView versus Fennec), and I
> believe this patch is a reasonable short-term solution that doesn't involve
> massive changes across the board.

OK.  If glandium accepts this, then I can live with it.  These short-term solutions never seem to be short, though...

> (In reply to Nick Alexander :nalexander from comment #4)
> > Comment on attachment 8852617 [details] [diff] [review]
> > 2. Separate out GeckoView chrome/JS files (v1)
> > 
> > Review of attachment 8852617 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > FWIW, I did exactly this in my proof-of-concept GeckoView branch.  I think
> > the advantage of the approach suggested by
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1291367 is even more clear
> > here; rather than overloading a single `package-manifest.in` to mean rapidly
> > diverging things, you extract a shared include and build upon it.  This
> > separation of platform (Gecko) and application looks better and better as
> > things like GeckoView and https://github.com/mozilla/qbrt move forward.
> 
> I originally had a separate GeckoView manifest, which the Fennec manifest
> included, but the Fennec-only set is small enough that I decided a single
> manifest would work just as well. I think eventually "separation of platform
> and application" will look like separate omni.ja and app.ja files. Building
> GeckoView would build omni.ja, and building Fennec on top of GeckoView would
> build an app.ja alongside the omni.ja from GeckoView.

I once posted to mobile-firefox-dev about this, and I conclude that the horse has left the barn.  Without significant changes to the way that resources are located in the omnijars (i.e., matching gre:// first in app.ja and then in omni.ja, even though it's supposed to be app:// in app.ja) this will never be possible.  And existing add-ons will always be refering to gre://..., so we can't even update Fennec and just move to app:// internally.  glandium may have other ideas about this layering.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8852617 [details] [diff] [review]
2. Separate out GeckoView chrome/JS files (v1)

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

I think you need to also ifdef stuff out in the MobileComponents.manifest so we don't have useless xpcom category notifications being run.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8852616 [details] [diff] [review]
1. Generate separate omni.ja for GeckoView (v1)

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

The patch is not as bad as I thought it would be from the discussion in the bug.

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8852617 [details] [diff] [review]
2. Separate out GeckoView chrome/JS files (v1)

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

Oops, I'm wrong.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/729859cd4cde
1. Generate separate omni.ja for GeckoView; r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/a54617dc98c1
2. Separate out GeckoView chrome/JS files; r=snorp

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/729859cd4cde
https://hg.mozilla.org/mozilla-central/rev/a54617dc98c1