Closed Bug 1004556 Opened 10 years ago Closed 10 years ago

Expose list of shipped locales to Fennec Java code

Categories

(Firefox Build System :: Android Studio and Gradle Integration, defect)

Product:
Firefox Build System
Component:
Android Studio and Gradle Integration
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla32

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

Attachments

(2 files, 1 obsolete file)

Expose list of shipped locales to Fennec Java code. r=rnewman
9.56 KB, patch
Details | Diff | Splinter Review
Expose list of shipped locales to Fennec Java code. r=glandium
1.83 KB, patch
glandium
: review+
Details | Diff | Splinter Review 
For the language chooser being developed in Bug 917480, we need to expose the list of languages shipped with Fennec to Java code.  I don't want to block landing that ticket on getting this perfect, so I suggest we land an interface to this as part of LocaleManager, like

public Collection<String> shippedLocales(Context context);

and figure out how to populate that in this ticket.
Argh!  Partial comment.  The most important thing to remember is that at build time, none of the (eventually) shipped locales (save for en-US, which is hard-coded in) are known.  At package time, all are known.

There are appropriate points to do all-locale things:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/installer/Makefile.in?rev=9ac7ed427cd2#51

and per-locale things

http://mxr.mozilla.org/mozilla-central/source/mobile/android/locales/Makefile.in?rev=942d149a7c0c#49
At run time, it appears that Android will provide the list of locale codes exposing a resource to the APK; see http://stackoverflow.com/a/11674720.  However, the list includes resources exposed by the Android platform itself.  We could filter this list, however, by adding a well-known AB_CD string to strings.xml.in, and rejecting any locale code that falls back to the default locale.
> There are appropriate points to do all-locale things:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/installer/
> Makefile.in?rev=9ac7ed427cd2#51

We could certainly write the list of locales (MOZ_CHROME_MULTILOCALE) into the APK somewhere.  One issue is that this list might not mirror the actual list in the APK, for a variety of reasons.  It's not fatal to set a language that your APK doesn't include at all, since everything falls back to en-US by default, but it's not great.

> and per-locale things
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/locales/
> Makefile.in?rev=942d149a7c0c#49

We could build the list of locales up using py_action buildlist in chrome-%.  This avoids the list mis-match of the previous scheme.  But this is delicate, and it's not immediately obvious to me when the list gets reset.  At package time if MOZ_CHROME_MULTILOCALES is undefined?
(In reply to Nick Alexander :nalexander from comment #3)
> At run time, it appears that Android will provide the list of locale codes
> exposing a resource to the APK; see http://stackoverflow.com/a/11674720.

Those are Java locale codes ("en_US"), not standard ones ("en-US"), so they'd need processing before they were useful. Not a big deal, but worth noting.
Comment on attachment 8416117 [details] [diff] [review]
Expose list of shipped locales to Fennec Java code. r=rnewman

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

How does this look?  I like doing this at runtime 'cuz it's flexible and easy to understand.

I haven't benchmarked this; we could cache it (and invalidate on package upgrade or similar) if it's a perf hit.  But since it's lazy, we could also just compute it on delayed startup, or at GeckoPreferences open time.
Attachment #8416117 - Flags: review?(rnewman)
Comment on attachment 8416117 [details] [diff] [review]
Expose list of shipped locales to Fennec Java code. r=rnewman

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

Smart hack. Questions inline.

::: mobile/android/base/BrowserLocaleManager.java
@@ +311,5 @@
> +     * @return collection of shipped locales.
> +     */
> +    private Set<String> getShippedLocales(Context context, boolean shouldLog) {
> +        final AssetManager assets = context.getResources().getAssets();
> +        final String[] locales = assets.getLocales();

What happens here when we ship a locale that Android doesn't itself support? Does it get included correctly?

@@ +324,5 @@
> +
> +        for (String locale : locales) {
> +            configuration.locale = new Locale(locale);
> +            final Resources localeResources = new Resources(assets, metrics, configuration);
> +            final String localeTag = localeResources.getString(R.string.AB_CD);

Making sure I understand this right: we ask Android for an array of `locales`:

  {"de_AT", "en_US", "en_GB", ...}

then we walk through these, asking Android to load the strings for that locale and extract a nugget that we put into each.

Android will happily run through the fallback case and return "en_US", so we check for that.

Right?

If so, fun.

I have some misgivings about the perf characteristics here, though. Spidey sense says Android might keep those resources open (especially if it uses our classloader to do it!) which is bad from a memory standpoint, and of course we're reading 38+ resources to figure this out. Even if we only do that when opening preferences, I could see that taking some time.

Did you do any informal measurement of how long this takes to run, and what kind of allocation profile it has?

@@ +329,5 @@
> +            if (shouldLog) {
> +                Log.d(LOG_TAG, "Found Android locale " + locale + " corresponding to Gecko locale " + localeTag);
> +            }
> +            if (!fallbackLocale.equals(localeTag)) {
> +                shippedLocales.add(localeTag);

Should be 

  shippedLocales.add(getLanguageTag(localeTag));

@@ +341,5 @@
> +     * @see #getShippedLocales(Context, boolean).
> +     */
> +    @Override
> +    public synchronized Collection<String> getShippedLocales(Context context) {
> +        if (this.shippedLocales == null) {

I don't think the overhead of synchronizing here is better than the risk of doing the work twice -- it should be very rare for this to be called by multiple threads at once. Probably enough to just make shippedLocales volatile, no?
(In reply to Richard Newman [:rnewman] from comment #8)
> Comment on attachment 8416117 [details] [diff] [review]
> Expose list of shipped locales to Fennec Java code. r=rnewman
> 
> Review of attachment 8416117 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Smart hack. Questions inline.
> 
> ::: mobile/android/base/BrowserLocaleManager.java
> @@ +311,5 @@
> > +     * @return collection of shipped locales.
> > +     */
> > +    private Set<String> getShippedLocales(Context context, boolean shouldLog) {
> > +        final AssetManager assets = context.getResources().getAssets();
> > +        final String[] locales = assets.getLocales();
> 
> What happens here when we ship a locale that Android doesn't itself support?
> Does it get included correctly?

Interesting question.  My guess is yes, since this method is exposing all the locale codes present in the assets (basically res/values-*), and I see no reason that would be filtered by Android's choices.  The list of Android locale tags the call gives me is roughly

[lt_LT, eu_ES, zh_CN, ur_PK, ro_RO, el_GR, pt_PT, en_NZ, en_GB, uz_UZ, ca_ES, sk_SK, nb_NO, de_AT, is_IS, sr_RS, it_IT, fi_FI, hu_HU, vi_VN, tr_TR, sl_SI, kk_KZ, hy_AM, pt_BR, pl_PL, en_US, ja_JP, ga_IE, bg_BG, ru_RU, uk_UA, fr_CH, en_IE, cs_CZ, zh_TW, in_ID, es_ES, fa_IR, nl_BE, en_AU, lv_LV, ko_KR, hr_HR, de_DE, az_AZ, fr_FR, es_US, ka_GE, ar_AE, de_CH, mk_MK, gl_ES, da_DK, sv_SE, et_EE, nl_NL, en_ZA]

We could try adding something totally disjoint, like xx_XX or something.

> @@ +324,5 @@
> > +
> > +        for (String locale : locales) {
> > +            configuration.locale = new Locale(locale);
> > +            final Resources localeResources = new Resources(assets, metrics, configuration);
> > +            final String localeTag = localeResources.getString(R.string.AB_CD);
> 
> Making sure I understand this right: we ask Android for an array of
> `locales`:
> 
>   {"de_AT", "en_US", "en_GB", ...}
> 
> then we walk through these, asking Android to load the strings for that
> locale and extract a nugget that we put into each.
> 
> Android will happily run through the fallback case and return "en_US", so we
> check for that.
> 
> Right?
> 
> If so, fun.

Yep.

> I have some misgivings about the perf characteristics here, though. Spidey
> sense says Android might keep those resources open (especially if it uses
> our classloader to do it!) which is bad from a memory standpoint, and of
> course we're reading 38+ resources to figure this out. Even if we only do
> that when opening preferences, I could see that taking some time.

Each call to new Resource() is a memory hit, but the underlying assets is all the same.  I think the Resource() creation is just the interface to Android's matcher.  Wish it wasn't a new object, but it's ... whatever.  So I'm not worried about "keeping resources open" -- the assets are all shared.  But yeah, this iteration is the suck.

> Did you do any informal measurement of how long this takes to run, and what
> kind of allocation profile it has?

We could dig into it.

> @@ +329,5 @@
> > +            if (shouldLog) {
> > +                Log.d(LOG_TAG, "Found Android locale " + locale + " corresponding to Gecko locale " + localeTag);
> > +            }
> > +            if (!fallbackLocale.equals(localeTag)) {
> > +                shippedLocales.add(localeTag);
> 
> Should be 
> 
>   shippedLocales.add(getLanguageTag(localeTag));
> 
> @@ +341,5 @@
> > +     * @see #getShippedLocales(Context, boolean).
> > +     */
> > +    @Override
> > +    public synchronized Collection<String> getShippedLocales(Context context) {
> > +        if (this.shippedLocales == null) {
> 
> I don't think the overhead of synchronizing here is better than the risk of
> doing the work twice -- it should be very rare for this to be called by
> multiple threads at once. Probably enough to just make shippedLocales
> volatile, no?

wfm.

Having made this work, I'm leaning towards including a derivative of $(MOZ_CHROME_MULTILOCALE) into our (default-only?) strings.  It's easy and it's available at package time for multi-l10n repacks.
Something like MOZ_CHROME_MULTILOCALE is closest to the patch I've been testing with, which would make this bug smaller and less risky, for sure.
(In reply to Richard Newman [:rnewman] from comment #10)
> Something like MOZ_CHROME_MULTILOCALE is closest to the patch I've been
> testing with, which would make this bug smaller and less risky, for sure.

Meh, I'm not worried about the size of this ticket at runtime.

I tried including MOZ_CHROME_MULTILOCALE and its awkward: it's not defined at build time (values/strings.xml), but it is defined at re-pack time (values-*/strings.xml).  It should have the same value for each re-pack, but there's no *guarantee*.  And to cap it off, we don't regenerate values/strings.xml after build time (IIRC, because we don't have the correct dependencies for en-US at re-pack or package time).  So including it as a string is tricky.

Let me see what it feels like to include it at package time.
> Let me see what it feels like to include it at package time.

... include it as a file in omnijar at package time.
Also, I should query others to see if we're doing unnecessary work and this already exists.  axel, glandium: do you know of a place that the list of locales shipped with Fennec is stored, accessible without requiring Gecko?
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(l10n)
> I have some misgivings about the perf characteristics here, though. Spidey
> sense says Android might keep those resources open (especially if it uses
> our classloader to do it!) which is bad from a memory standpoint, and of
> course we're reading 38+ resources to figure this out. Even if we only do
> that when opening preferences, I could see that taking some time.
> 
> Did you do any informal measurement of how long this takes to run, and what
> kind of allocation profile it has?

I did one.  For three locales (en-US, de, ar), I'm seeing runs like the following.  Times are in milliseconds; this is on my very fast Samsung Galaxy S4.  Most runs are after a package re-install, so cold disk cache.  The allocation profile isn't that bad; it's a handful of objects each iteration.  I took a heap dump afterward and there were 6 Resource instances around; 2 were clearly Android system and Gecko resources; 4 others looked system.  Nowhere near the 30 or 40 of the total locale set.

W XXX(3606)                   time: 15
W XXX(3606)                   time: 10
W XXX(3606)                   time: 10
W XXX(3606)                   time: 9
W XXX(3606)                   time: 11
W XXX(3606)                   time: 11
W XXX(3606)                   time: 11
W XXX(3606)                   time: 11
W XXX(3606)                   time: 9
W XXX(3606)                   time: 9
W XXX(5502)                   Shipped locales: [de, en-US, ar]

W XXX(5502)                   time: 14
W XXX(5502)                   time: 10
W XXX(5502)                   time: 10
W XXX(5502)                   time: 12
W XXX(5502)                   time: 11
W XXX(5502)                   time: 12
W XXX(5502)                   time: 10
W XXX(5502)                   time: 11
W XXX(5502)                   time: 10
W XXX(5502)                   time: 9
W XXX(5502)                   Shipped locales: [de, en-US, ar]

W XXX(9709)                   time: 33
W XXX(9709)                   time: 16
W XXX(9709)                   time: 13
W XXX(9709)                   time: 14
W XXX(9709)                   time: 12
W XXX(9709)                   time: 13
W XXX(9709)                   time: 15
W XXX(9709)                   time: 14
W XXX(9709)                   time: 16
W XXX(9709)                   time: 16
W XXX(9709)                   Shipped locales: [de, en-US, ar]


> 
> @@ +329,5 @@
> > +            if (shouldLog) {
> > +                Log.d(LOG_TAG, "Found Android locale " + locale + " corresponding to Gecko locale " + localeTag);
> > +            }
> > +            if (!fallbackLocale.equals(localeTag)) {
> > +                shippedLocales.add(localeTag);
> 
> Should be 
> 
>   shippedLocales.add(getLanguageTag(localeTag));

I don't think we need this, since the localeTag is AB_CD from the build system, which is straight out of all-locales.

The more I think about this, the more I think we need LocaleManager.getShippedAndroidResources and getShippedGeckoResources.  And I'm pretty happy with this approach for the former.  I move to land a version of this for the first cut of the language chooser.
(In reply to Nick Alexander :nalexander from comment #14)

> I did one.  For three locales (en-US, de, ar), I'm seeing runs like the
> following.  Times are in milliseconds; this is on my very fast Samsung
> Galaxy S4.

Do you have a patch I can try out with the full locale set?

> > Should be 
> > 
> >   shippedLocales.add(getLanguageTag(localeTag));
> 
> I don't think we need this, since the localeTag is AB_CD from the build
> system, which is straight out of all-locales.

Correct. That comment was if you were using the Java locales that Android gives you.

> The more I think about this, the more I think we need
> LocaleManager.getShippedAndroidResources and getShippedGeckoResources.

Any reason for splitting those?
> > The more I think about this, the more I think we need
> > LocaleManager.getShippedAndroidResources and getShippedGeckoResources.
> 
> Any reason for splitting those?

Seems likely that at some point, these will diverge: perhaps we will ship additional omni.ja languages before AndroidManifest.xml updates.
So, I just tested with additional locales (xx and yy-rYY) and ... no love.  They're listed in the APK (use |aapt dump badging| to see them) but they're not exposed by the method I use.  Which just sucks.
I don't the list of shipped locales exposed even to Gecko; if either
of you know how to fish it, that would be valuable as a comparison.
This is simple enough that I'd like to get it onto Nightly to make
sure it's working before we try to land any code that depends on it.
Attachment #8417469 - Flags: review?(mh+mozilla)
Attachment #8417469 - Flags: feedback?(l10n)
Comment on attachment 8417469 [details] [diff] [review]
Expose list of shipped locales to Fennec Java code. r=glandium

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

::: mobile/android/installer/Makefile.in
@@ +63,5 @@
>  	  printf '$(BINPATH)/chrome/'"$$LOCALE"'$(JAREXT)\n' >> $@; \
>  	  printf '$(BINPATH)/chrome/'"$$LOCALE"'.manifest\n' >> $@; \
>  	done
> +	$(PYTHON) -c 'import json; import sys; locales = sys.argv[1+sys.argv.index("--"):]; print json.dumps({"locales":locales})' \
> +	  -- en-US $(MOZ_CHROME_MULTILOCALE) \

You don't need python to output this. The {"locales": ...} part seems useless too. Can't java read a json that's just a list?

@@ +64,5 @@
>  	  printf '$(BINPATH)/chrome/'"$$LOCALE"'.manifest\n' >> $@; \
>  	done
> +	$(PYTHON) -c 'import json; import sys; locales = sys.argv[1+sys.argv.index("--"):]; print json.dumps({"locales":locales})' \
> +	  -- en-US $(MOZ_CHROME_MULTILOCALE) \
> +	  > $(FINAL_TARGET)/res/locales.json

Might as well add this file to $@ like it's done for the manifests above, instead of putting it in package-manifest.in.
Attachment #8417469 - Flags: review?(mh+mozilla) → review-
(In reply to Mike Hommey [:glandium] from comment #19)
> Comment on attachment 8417469 [details] [diff] [review]
> Expose list of shipped locales to Fennec Java code. r=glandium
> 
> Review of attachment 8417469 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/installer/Makefile.in
> @@ +63,5 @@
> >  	  printf '$(BINPATH)/chrome/'"$$LOCALE"'$(JAREXT)\n' >> $@; \
> >  	  printf '$(BINPATH)/chrome/'"$$LOCALE"'.manifest\n' >> $@; \
> >  	done
> > +	$(PYTHON) -c 'import json; import sys; locales = sys.argv[1+sys.argv.index("--"):]; print json.dumps({"locales":locales})' \
> > +	  -- en-US $(MOZ_CHROME_MULTILOCALE) \
> 
> You don't need python to output this. The {"locales": ...} part seems
> useless too. Can't java read a json that's just a list?

No, it doesn't require Python, and yes, Java can read just a list.  We've found in other places that it's useful to wrap lists in objects; for one thing, if you ever want two things, it's easy to add another.

> @@ +64,5 @@
> >  	  printf '$(BINPATH)/chrome/'"$$LOCALE"'.manifest\n' >> $@; \
> >  	done
> > +	$(PYTHON) -c 'import json; import sys; locales = sys.argv[1+sys.argv.index("--"):]; print json.dumps({"locales":locales})' \
> > +	  -- en-US $(MOZ_CHROME_MULTILOCALE) \
> > +	  > $(FINAL_TARGET)/res/locales.json
> 
> Might as well add this file to $@ like it's done for the manifests above,
> instead of putting it in package-manifest.in.

Will do.
(In reply to Nick Alexander :nalexander from comment #20)

> No, it doesn't require Python, and yes, Java can read just a list.  We've
> found in other places that it's useful to wrap lists in objects; for one
> thing, if you ever want two things, it's easy to add another.

+1 from me (the consumer) on using an object. This might well accrue locale fallback trees, multiple locale sets, or more attributes in the future.

(I stop short of versioning it, though: it's coupled to the code in the tree, so versioning should be unnecessary.)
Sorry, got nothing new or enlightening to add.
Flags: needinfo?(l10n)
Attachment #8417469 - Flags: feedback?(l10n)
> for one thing, if you ever want two things, it's easy to add another.

Well, in a *locales*.json file, i do hope you won't have something that's not locales ;)
Flags: needinfo?(mh+mozilla)
Updated to write to multilocale.json (a more generic name than
locales.json ;) and to printf to package-manifest.in.

I'd like to make the package-manifest rule a FORCE, to avoid needing
to locally require the hijinks that the buildbots do when multilocale
building.  You seemed okay with this in
https://bugzilla.mozilla.org/show_bug.cgi?id=933290#c23; can I land
this with that FORCE added?
Attachment #8417469 - Attachment is obsolete: true
Attachment #8419113 - Flags: review?(mh+mozilla)
Comment on attachment 8419113 [details] [diff] [review]
Expose list of shipped locales to Fennec Java code. r=glandium

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

::: mobile/android/installer/Makefile.in
@@ +61,5 @@
>  	  printf '$(BINPATH)/chrome/'"$$LOCALE"'$(JAREXT)\n' >> $@; \
>  	  printf '$(BINPATH)/chrome/'"$$LOCALE"'.manifest\n' >> $@; \
>  	done
> +	$(PYTHON) -c 'import json; import sys; locales = sys.argv[1+sys.argv.index("--"):]; print json.dumps({"locales":locales})' \
> +	  -- en-US $(MOZ_CHROME_MULTILOCALE) \

COMMA=,
echo '{"locales": ["en-US"$(foreach l,$(MOZ_CHROME_MULTILOCALE),$(COMMA) "$(l)")]}'
Attachment #8419113 - Flags: review?(mh+mozilla) → review+
Pushed with cosmetic changes:

https://hg.mozilla.org/integration/fx-team/rev/3c38de342576
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/3c38de342576
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Attachment #8416117 - Flags: review?(rnewman)
chocho:gecko nalexander$ hg contains -c cf89b5d018f8 3c38de342576
true

and sure enough, the corresponding TBPL Nightly from

http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/2014/05/2014-05-09-03-02-27-mozilla-central-android/fennec-32.0a1.multi.android-arm.apk

contains multilocale.json,

  -rw-rw-r--       167   1-Jan-2010  00:00:00  res/multilocale.json

with content

{"locales": ["en-US", "cs", "da", "de", "es-ES", "fi", "fr", "ja", "ko", "it", "nb-NO", "nl", "pl", "pt-BR", "pt-PT", "ru", "sk", "sv-SE", "zh-CN", "zh-TW", "en-US"]}
Status: RESOLVED → VERIFIED
(In reply to Nick Alexander :nalexander from comment #26)
> Pushed with cosmetic changes:
> 
> https://hg.mozilla.org/integration/fx-team/rev/3c38de342576

I'm surprised this actually works the way you landed it: COMMA needs to be a make variable, you just made it a shell variable. And now I understand: there's a COMMA definition in config/config.mk.
(In reply to Mike Hommey [:glandium] from comment #29)
> (In reply to Nick Alexander :nalexander from comment #26)
> > Pushed with cosmetic changes:
> > 
> > https://hg.mozilla.org/integration/fx-team/rev/3c38de342576
> 
> I'm surprised this actually works the way you landed it: COMMA needs to be a
> make variable, you just made it a shell variable. And now I understand:
> there's a COMMA definition in config/config.mk.

You're correct; I expected this to fail but tested it and found it worked, was surprised that it worked, and figured I just didn't have Make's scoping rules correct.  I'll fix it some time when I'm in the area.
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 32 → mozilla32
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: