1104203 - Make LocaleAware and Application own the singleton LocaleManager

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Locale switching and selection, defect)

Description

[Nick Alexander :nalexander [he/him]]

This is motivated by trying to minimize the dependencies between android-sync and Fennec proper.

Right now, we're 80% of the way to a separated interface:

1) There's a LocaleManager interface;
2) BrowserLocaleManager provides it for Fennec;
3) A stub BLM provides it for android-sync.

The BLM dependency ties a-s and Fennec because we do BLM.getInstance() to fetch the singleton.  To cut that knot, we need to remove the reference to the concrete type.  The question is, where to store the singleton reference.  I see two options:

1) as part of the Application;
2) in some other class shared between a-s and Fennec.

Storing things directly in the Application is a bad idea because we want (maybe?) for the LocaleManager to be available to GeckoView and don't want GV consumers to have to override Application.

I nominate LocaleAware as the place to store the singleton reference.  We make the Application onCreate set the singleton reference.  (In a-s, we include a StubApplication and a StubLocaleManager for the android-sync.apk only.)  This works as long as everything is in the same process, which is a strong assumption that we already have.

While digging into this I evaluated using a full-blown dependency injection solution, which on Android is pretty much Square's dagger.  In future, I think we should do this but it's over-kill here.  However, the DI approach would be to store the "object graph" (right now, just the LocaleManager) as part of the Application (or, like I suggest here, stored elsewhere and set by the Application).  It's very similar, but much more flexible moving forward.

Comment 1

[Nick Alexander :nalexander [he/him]]

rnewman: I've mostly implemented this, but haven't tested the details.  Can I get your feedback on the approach?  Is Bug 1101527 relevant?

Comment 2

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #0)

> I nominate LocaleAware as the place to store the singleton reference.  We
> make the Application onCreate set the singleton reference.  (In a-s, we
> include a StubApplication and a StubLocaleManager for the android-sync.apk
> only.)  This works as long as everything is in the same process, which is a
> strong assumption that we already have.

I can't fault your logic; we can't impose on the Application, so there has to be some other place to hang this.

Things to consider:

* LocaleAware was intended to be a really simple place to put repetitive code, rather than an extension point. That's just a gut feel thing, though.

* storeAndNotifyOSLocale is called directly from GeckoApp, so LocaleManager is really a fake extension point that only exists to make android-sync happy. You can't feasibly replace BLM in a GeckoView application and have it still work.

I think you can address both of these concerns: make storeAndNotifyOSLocale non-static (reworking it as appropriate -- e.g., don't directly use GeckoAppShell!), and consider renaming LocaleAware ("Locales"?).

I'd also think about a little more splitting: move the static helpers (getLanguageTag, for example) from BLM to the new Locales class.

I'll do the latter now.

Comment 3

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.