Closed Bug 1291368 Opened 3 years ago Closed 3 years ago

[geckoview] Remove all use of resources from GeckoView

Categories

(GeckoView :: General, defect)

defect
Not set

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: nalexander, Assigned: jchen)

References

Details

Attachments

(3 files)

This ticket tracks ensuring that GeckoView can build without
org.mozilla.gecko.R.  Once we get this, we can follow-up Bug XXX to
build the GeckoView sources in moz.build as a JAR that Fennec's JARs
depend on, rather than requiring them to be built with the rest of the
Fennec source code.

In addition, this allows us to migrate to the org.mozilla.gecko
resource namespace, leaving org.mozilla.gecko to Fennec.  (It's not
feasible to move Fennec to a better resource namespace (say
org.mozilla.firefox) since we have Intents and short-cuts to
org.mozilla.gecko in the wild, and terrible things happen if we change
the manifest in this way).

At this point we have *truly* split the monolith between GeckoView and
Fennec, since Fennec will be consuming GeckoView.  After that, we can
choose to regrow resources in GeckoView's new namespace, etc.
(In reply to Nick Alexander :nalexander (Away Aug 9 - Aug 27) from comment #0)
> This ticket tracks ensuring that GeckoView can build without
> org.mozilla.gecko.R.  Once we get this, we can follow-up Bug XXX to
> build the GeckoView sources in moz.build as a JAR that Fennec's JARs
> depend on, rather than requiring them to be built with the rest of the
> Fennec source code.

Unsure which bug XXX is referencing, but this sounds like it blocks bug 1291367, which implements --enable-application=path/to/geckoview, since that suggests that we're building GeckoView without Fennec.


> In addition, this allows us to migrate to the org.mozilla.gecko
> resource namespace, leaving org.mozilla.gecko to Fennec.

Erm, s/org.mozilla.gecko/org.mozilla.geckoview/ (for the first occurrence)?


> (It's not
> feasible to move Fennec to a better resource namespace (say
> org.mozilla.firefox) since we have Intents and short-cuts to
> org.mozilla.gecko in the wild, and terrible things happen if we change
> the manifest in this way).

And also because that would make the Google Play Store consider org.mozilla.firefox to be a new, distinct app, as I understand it.
(In reply to Myk Melez [:myk] [@mykmelez] from comment #1)
> (In reply to Nick Alexander :nalexander (Away Aug 9 - Aug 27) from comment
> #0)
> > This ticket tracks ensuring that GeckoView can build without
> > org.mozilla.gecko.R.  Once we get this, we can follow-up Bug XXX to
> > build the GeckoView sources in moz.build as a JAR that Fennec's JARs
> > depend on, rather than requiring them to be built with the rest of the
> > Fennec source code.
> 
> Unsure which bug XXX is referencing, but this sounds like it blocks bug
> 1291367, which implements --enable-application=path/to/geckoview, since that
> suggests that we're building GeckoView without Fennec.

Gah!  Bug XXX is my marker to update when I'm bulk filing, and I missed this one.  This is follow-up to Bug 1098129.

> > In addition, this allows us to migrate to the org.mozilla.gecko
> > resource namespace, leaving org.mozilla.gecko to Fennec.
> 
> Erm, s/org.mozilla.gecko/org.mozilla.geckoview/ (for the first occurrence)?

Correct.

> > (It's not
> > feasible to move Fennec to a better resource namespace (say
> > org.mozilla.firefox) since we have Intents and short-cuts to
> > org.mozilla.gecko in the wild, and terrible things happen if we change
> > the manifest in this way).
> 
> And also because that would make the Google Play Store consider
> org.mozilla.firefox to be a new, distinct app, as I understand it.

No.  The resource namespace is distinct from the package namespace, precisely to allow one codebase (say org.mozilla.gecko) to target multiple Android packages (org.mozilla.firefox{_beta}).  Firefox did this by pre-processing before Android tools supported it well; we do it close to correctly now in Gradle, but we need to split the monolith to do it "correctly" in moz.build.
Assignee: nobody → nchen
Blocks: 1291363
Status: NEW → ASSIGNED
Depends on: 1304145
In BitmapUtils.getResources(), instead of using reflection on the Fennec
resources class to find a resource id, call Resources.getIdentifier and
pass in a package name. This eliminates the dependency on the Fennec
resources class, and potentially improves geckoview support for third
party consumers, because an application can now use the "drawable://"
URI to refer to its own resources.
Attachment #8793431 - Flags: review?(snorp)
BitmapUtils.getLauncherIcon is only called by GeckoApp, and it has some
dependencies on Fennec resources. It makes sense to move it to GeckoApp
entirely.
Attachment #8793432 - Flags: review?(snorp)
Attachment #8793431 - Flags: review?(snorp) → review+
Attachment #8793432 - Flags: review?(snorp) → review+
Remove the gecko-view dependency on gecko-R, now that we no longer refer
to Fennec resources in geckoview sources.
Attachment #8793943 - Flags: review?(nalexander)
(In reply to Jim Chen [:jchen] [:darchons] from comment #5)
> Created attachment 8793943 [details] [diff] [review]
> 3. Remove gecko-R dependency for gecko-view (v1)
> 
> Remove the gecko-view dependency on gecko-R, now that we no longer refer
> to Fennec resources in geckoview sources.

WOOOOOOOOOOOOOO
Comment on attachment 8793943 [details] [diff] [review]
3. Remove gecko-R dependency for gecko-view (v1)

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

BINGO!
Attachment #8793943 - Flags: review?(nalexander) → review+
At some point we had a dynamic scrollbar reference, but I'm pretty sure it went with Bug 1291373.  Perhaps worth checking again.
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/360005433fde
1. Remove dependency on Fennec resources class in BitmapUtils; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd85d47c1654
2. Move BitmapUtils.getLauncherIcon to GeckoApp; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/14c0697b2dfb
3. Remove gecko-R dependency for gecko-view; r=nalexander
https://hg.mozilla.org/mozilla-central/rev/360005433fde
https://hg.mozilla.org/mozilla-central/rev/fd85d47c1654
https://hg.mozilla.org/mozilla-central/rev/14c0697b2dfb
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 52 → mozilla52
You need to log in before you can comment on or make changes to this bug.