792077 - Fennec multi-locale apk should contain only java, browser, and select toolkit strings

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Axel Hecht [:Pike]]

Attachment: breaking the build, proof of concept, though

To include more localizations in our fennec multi-locale builds, we want to get rid of a bunch of toolkit cruft.

The idea is to just package the java files, browser chrome, and add a select list of localized files from toolkit/dom/etc as overlays.

We'd keep the full en-US toolkit strings as fallback in case we didn't cover all our bases.

I'm attaching a patch to start.

Comment 1

[Axel Hecht [:Pike]]

Comment on attachment 662187 [details] [diff] [review]
breaking the build, proof of concept, though

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

Here a few notes on the p-o-c patch that I did, and why I don't think we should use it as is. Oh, lovely, r- myself.

::: mobile/android/installer/Makefile.in
@@ +45,4 @@
>  DEFINES += -DMOZ_CHILD_PROCESS_NAME=$(MOZ_CHILD_PROCESS_NAME)
>  
>  ifdef MOZ_PKG_MANIFEST_P
> +#MOZ_PKG_MANIFEST = package-manifest

no idea why I needed this

::: mobile/android/locales/Makefile.in
@@ +48,2 @@
>  ifeq ($(OS_TARGET),Android)
> +	#@$(MAKE) -C $(DEPTH)/mobile/android/base/locales AB_CD=$* XPI_NAME=locale-$* BOTH_MANIFESTS=1

This breaks single-locale builds, we probably don't want this to start.

It's one way to generate toolkit-only langpacks, though, but we could also copy the generic langpack stuff into toolkit/locales, and actually create langpacks for teh toolkit app id there, which might be interesting for people doing gecko apps, too. We'd need input on that from a build peer, though.

::: mobile/locales/Makefile.in
@@ +169,5 @@
>  	@$(MAKE) -C $(DEPTH)/intl/locales AB_CD=$* XPI_NAME=locale-$* BOTH_MANIFESTS=1
> +	#@$(MAKE) -B $(bookmarks) AB_CD=$*
> +	#@$(MAKE) -B searchplugins AB_CD=$* XPI_NAME=locale-$*
> +	#@$(MAKE) libs AB_CD=$* XPI_NAME=locale-$* PREF_DIR=defaults/pref BOTH_MANIFESTS=1
> +	#@$(MAKE) -C $(DEPTH)/$(MOZ_BRANDING_DIRECTORY)/locales AB_CD=$* XPI_NAME=locale-$* BOTH_MANIFESTS=1

this is also the libs-% target, we probably don't want this, as it breaks single-locale builds. just hacked that to get the langpack working.

@@ +175,5 @@
>  # Tailored target to just add the chrome processing for multi-locale builds
>  chrome-%:
>  	$(display-deps)
> +	#@$(MAKE) -C $(DEPTH)/toolkit/locales chrome-$*
> +	#@$(MAKE) -C $(DEPTH)/services/sync/locales chrome AB_CD=$*

This is where the actual magic happens to not include the toolkit strings in the multi-locale pack.

This would also be the spot where we hook up our list of overrides, and we'd need at least intl.properties to not break the web.

We'll need input from a build peer on how to pick up files and create the manifest override lines, with files possibly coming from dom, toolkit, netwerk. Those would need to fake l10n-merge logic with different relative_srcdir paths (picking up from merge, l10n, en-US in that order).

Comment 2

[Axel Hecht [:Pike]]

List of possible override candidates:

chrome://global/locale/intl.properties for sure, we need that for the web.

chrome://global-platform/locale/intl.properties is also referenced from greprefs.js
mobile.js only references stuff in the browser package, so we're good.

Other locale files I found explicitly referenced from within mobile/android are:

chrome://global/locale/aboutRights.dtd
chrome://global/locale/charsetTitles.properties
chrome://global/locale/commonDialogs.properties
chrome://global/locale/global.dtd
chrome://global/locale/netError.dtd (already overloaded)
chrome://global/locale/search/search.properties
chrome://mozapps/locale/handling/handling.properties
chrome://mozapps/locale/update/updates.properties
chrome://passwordmgr/locale/passwordmgr.properties

Comment 3

[Axel Hecht [:Pike]]

Attachment: use relativesrcdir directive in jar.mn to package toolkit files

Mark, Brad, this is the patch I'd leave you with, on top of the JarMaker queue in bug 797745.

I pushed this to try https://tbpl.mozilla.org/?tree=Try&rev=6789c3cde60e, which passed mochitests, so the overrides in en-US don't seem to break anything. Didn't really test beyond that.

If you land a variation of this, mind keeping note of perf improvements that we get? I'd love to pretend that we can regress some of those wins if we add a bunch of locales.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Status: Wes is going to make a build so we can check APK size, test Talos on Try server and we can start doing some tests to look for missing/English strings.

Comment 5

[Wesley Johnston (:wesj)]

I've been looking at this today. I'm not convinced our multi-locale packs were ever actually shipping multi-locale Native java strings. I don't see any strings.xml inside the apk at least, nor do they appear in objdir/dist/fennec/res during the build. They are in obj-dir/mobile/android/base/res, so we just need to copy them over.

I'm fixing the build files for that right now.

Comment 6

[Axel Hecht [:Pike]]

That's how apks work. They're built into the resources.arsc file, in a totally weird and encumbered way. They end up in 16bit strings, too, so just using 'strings resources.arsc' didn't work out for me, either. I used less and patience to verify what's in there, in the cases where I cared.
Similar to the way that the xml files aren't the xml files we have, but "binary xml" that nobody but android can actually read.

Comment 7

[Wesley Johnston (:wesj)]

Ahh. Sorry, you're right! I have es-US on my phone which is the problem. Korean works though. Now that I know things are working, I'll try to get some numbers today.

Comment 8

[Wesley Johnston (:wesj)]

This saves us about 2megs on the build.

Without patch: 20.8MB
With path: 18.8 MB

looking into broken strings, but at a quick glance, about:apps/downloads/addons won't load

Comment 9

[Wesley Johnston (:wesj)]

Ahh. The strings are broken because I'm using korean, which apparently doesn't ship an updated aboutAddons.dtd:

http://hg.mozilla.org/mozilla-central/file/182185d15e6b/mobile/android/locales/en-US/chrome/aboutAddons.dtd
https://hg.mozilla.org/l10n-central/ko/file/5c1396e9e66a/mobile/android/chrome/aboutAddons.dtd

Comment 10

[Axel Hecht [:Pike]]

Maybe try their aurora repo instead? That might be more up-to-date, even compared to central.

Also, are the numbers for one locale, or how many?

Comment 11

[Wesley Johnston (:wesj)]

17 locales. Build is at https://dl.dropbox.com/u/72157/fennecLocales.apk

Comment 12

[chris hofmann]

wes,  sounds like the next step here is to get some performance numbers on that build you created to figure out how much of a gain we see at start up.  does that sound correct?

the basic solution proposed here might also be useful to gaia so figuring this out soon may help on that project as well.

Comment 13

[Wesley Johnston (:wesj)]

Looking at this now. Sorry for the delay. Here's a build with 42 locales (jp, sq, and sr had build problems):
http://dl.dropbox.com/u/72157/fennecMultiTwo.apk

With a single locale (pointing the multi-locale build to an empty list of locales to build) the results are 18MB. With all 42 locales, the builds are ~26MB. Our current nightlies on ftp.mozilla.org ship 19 locales and also weigh in at 26MB.

I ran warm startup tests with this (set to English and Chinese) and a single locale build (set to English) on my S3. Basically i launched fennec over and over 5 times and recorded the zerdatime output into logcat. I see about an 8-10% slowdown. I took the time between the first and last "Throbber stop" messages (i.e. the first and last I see during startup):

Fennec Single locale: avg startup time 1594ms
Fennec Multi locale English: avg startup time 1694ms
Fennec Multi locale Chinese: avg startup time 1732ms

Comment 14

[Wesley Johnston (:wesj)]

I should say the stddev of those timings was pretty small too (not big enough to account for the differences):

Fennec Single locale: avg startup time 1594ms +- 28ms
Fennec Multi locale English: avg startup time 1694ms +- 82ms
Fennec Multi locale Chinese: avg startup time 1732ms +- 55ms

Comment 15

[Wesley Johnston (:wesj)]

Hmm... Not sure what happened, but my numbers (at least for size) are looking different now (around 20MB instead of 26), but we still come out a bit ahead with more locales. I'm also making a build with just locales that Android supports from this list:

http://us.dinodirect.com/Forum/Latest-Posts-5/Android-Versions-and-their-Locales-1-86587/

Reduced down to match with our locales list thats 39 locales:
ar
bg
ca
cs
da
de
el
en-GB
en-ZA
es-ES
fi
fr
he
hi-IN
hr
hu
id
it
ja
ko
lt
lv
nb-NO
nl
pl
pt-BR
pt-PT
ro
ru
sk
sl
sr
sv-SE
th
tr
uk
vi
zh-CN
zh-TW

which is actually shorter than what all-locales contains (56), and has a lot of duplicated locales I don't think we'd actually want to support (I don't think we need to ship en-US, en-GB, and en-ZA). Building that now. I'll also build something with all 100 some of our locales from https://mxr.mozilla.org/l10n-mozilla-aurora/source/

Comment 16

[Besnik Bleta]

(In reply to Wesley Johnston (:wesj) from comment #13)
> Looking at this now. Sorry for the delay. Here's a build with 42 locales
> (jp, sq, and sr had build problems):

Could you tell me what kind of problems sq had? I'd like to help fixing them.

Comment 17

[Axel Hecht [:Pike]]

Comment on attachment 670792 [details] [diff] [review]
use relativesrcdir directive in jar.mn to package toolkit files

Actually putting the notes I had elsewhere here in the bug:

The current patch breaks `chromereg.getSelectedLocale(toolkitpackage)`, because that'd be still returning 'en-US' instead of the selected locale of the overwriting package, 'browser' in this case.

We need that code to work for url formatting of pings, but more importantly to get RTL right, or at least not break it further.

Thus I'm giving myself an r- on this patch.

Next step is to figure out if there's an acceptable way to make this work still. I had three ways in my head:
- make the chrome urls/channels fall back to en-US and really have sparse toolkit localizations. This sounds really intrusive, and probably hard.
- make all call-sites of getSelectedLocale work around that, which I don't think is feasible.
- add a prefbranch that gives "package overrides" or so, so that the chrome reg would return the selected locale for the override package instead of the one it's getting asked for, something like
pref("chrome.locale.override.global", "browser");

That's probably a platform thread involving at least :bs.

Comment 18

[Axel Hecht [:Pike]]

Pushed this again to try, including the chrome registry stuff, https://tbpl.mozilla.org/?tree=Try&rev=cd9a6eb14d45

Comment 19

[Axel Hecht [:Pike]]

Attachment: chrome registry changes and mobile packaging in one

Mark, this is a patch that includes the modified patch for mobile (mostly mobile.js and merge conflicts), plus the patch to tweak the chrome registry.

I'm wondering if you could get some perf/size measurements organized for this patch?

I'd rather know that this is getting us where we want to be in terms of perf and size before going through the review cycle for the chrome registry changes. Makes sense?

Comment 20

[Axel Hecht [:Pike]]

PS: try is looking good.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 721878 [details] [diff] [review]
chrome registry changes and mobile packaging in one

The approach looks good to me. I'll get someone to look at size/speed.

Comment 22

[Wesley Johnston (:wesj)]

Pike, this still doesn't seem to work right. Builds at:

http://people.mozilla.com/~wjohnston/withoutPatch.apk
http://people.mozilla.com/~wjohnston/withPatch.apk

the withPatch builds are 6MB smaller, but have dtd errors all over :(

Comment 23

[Axel Hecht [:Pike]]

Attachment: chrome registry changes and mobile packaging in one, without bustage

Sorry Wes, I thought sending to try would help, but apparently it didn't.

The culprit was that I broke global.dtd, which we include to do RTL. I had also broken charsetTitles.properties, but you probably didn't recognize.

This patch has the updates patch from bug 848297, and the fix to jar.mn.

For folks reading along in this bug, bug 848297 has a unhappy r+. I'd really like to see that we're good on the mobile front before getting back to :bs about how unhappy he really is.

I tested this code with a local i386 android build on the emulator and l10n-merged en-US, de, fr, nl, it build. I tested locale switching, java UI language, gecko language on both browser and toolkit stuff. I didn't try to test a RTL locale, just because we don't really have one right now. I also tested content language negotiation by going to http://mozilla.org/firefox, and that directs to the correct language version for de and nl.

I also didn't find a way to confirm that stuff that uses the selected locale sends the right data. Not sure where I'd do that.

If it's of any use, I uploaded the i386 android build to http://people.mozilla.org/~axel/fennec-23.0a1.multi.android-i386.apk.

Comment 24

[Axel Hecht [:Pike]]

PS: Wes, in your build there are actually a good deal more locales than the 39 in comment 15, https://wiki.mozilla.org/Mobile/Projects/Localization#Specifications lists 21.

Comment 25

[Wesley Johnston (:wesj)]

Builds with and without the patches are here: 

http://people.mozilla.com/~wjohnston/multilocaleBuilds/

Pretty huge space savings, but it would be good to get some testing to know we aren't missing strings we need (the slight differences in the single with/without builds are probably a result of pulling mozilla-aurora between builds, which is just part of the build process):

Size     Build
24472328 allLocalesWithPatch.apk
31313066 allWithoutPatch.apk
24695319 maemoWithoutPatch.apk
22588429 maemoWithPatch.apk
21699721 singleLocaleWithPatch.apk
21687954 singleWithoutPatch.apk

Comment 26

[Wesley Johnston (:wesj)]

I ran some startup timings on these today, but I can't really detect a strong change in startup time with any of these. I measured the time (using logcat) from the first GeckoApp(19887): Enabling Android StrictMode (http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1749) to a few different places (using logcat timestamps... are those accurate enough for this?)

Starting Gecko:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoThread.java#96
                       With patch          Without Patch
All Locales         1.859 +- .235          1.772 +- .044
Maemo               1.812 +- .101          1.825 +- .072
Single              1.755 +- .031          1.732 +- .066


browser chrome startup finished:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#219
                       With patch         Without Patch
All Locales         2.503 +- .274         2.666 +- .424
Maemo               2.575 +- .413         2.620 +- .389
Single              2.415 +- .173         2.386 +- .197

Throbber stop (2nd one since two fire during every startup):
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserToolbar.java#1039
                       With patch         Without Patch
All Locales         2.977 +- .272         3.157 +- .495
Maemo               3.010 +- .435         3.076 +- .419
Single              2.873 +- .236         2.887 +- .225

There's three runs averaged for each measurement, but there's enough noise there that I'd be hard pressed to say these mean anything...

Comment 27

[Wesley Johnston (:wesj)]

Comment on attachment 738601 [details] [diff] [review]
chrome registry changes and mobile packaging in one, without bustage

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

I talked to QA and they may do a rough spot check of their own. But I think we should land this on nightly/aurora. The number of users on both builds isn't huge, but we'll need wider testing to hope to catch anything. Its mostly a question of when? Do we want till the next branch or do it now?

Comment 28

[Wesley Johnston (:wesj)]

Comment on attachment 738601 [details] [diff] [review]
chrome registry changes and mobile packaging in one, without bustage

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

I talked to QA and they may do a rough spot check of their own. But I think we should land this on nightly/aurora. The number of users on both builds isn't huge, but we'll need wider testing to hope to catch anything. Its mostly a question of when? Do we want till the next branch or do it now?

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 738601 [details] [diff] [review]
chrome registry changes and mobile packaging in one, without bustage

r+ on the /mobile code. It looks like the other code was landed as part of bug 848297.

Let's land on Nightly and see what the Nightly user base finds for us.

Comment 30

[Axel Hecht [:Pike]]

Landed on inbound, https://hg.mozilla.org/integration/mozilla-inbound/rev/2aed3055baf6

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2aed3055baf6
https://hg.mozilla.org/mozilla-central/rev/d4b0ad30ba0d

Comment 32

[Kevin Brosnan [Ex-Mozilla]]

Verified by running our l10n testing on Aurora where our localization is done. https://moztrap.mozilla.org/results/cases/?filter-run=1336