This bug covers persisting and loading the selected locale from within the Java application. This involves: * Refreshing the pref-based storage code that's already in the tree. (Very bitrotted.) * Ripping out the Gecko-driven locale switching code that still runs. * Providing hooks for UI code to invoke a switch. * TODO: watching for, and optionally reflecting, system locale choices -- the Java equivalent of matchOS. * TODO: appropriately restarting activities when necessary. * TODO: correctly reflecting locale changes into Gecko and restarting it if necessary. (Apparently this is "if it ain't running... and it has to be running to have set the pref, so that's always". Sadface.)

WIP, partly tested.

Turns out that the historical estimate of expense here was incorrect -- we used to use a custom pref file for this, which took time to load. Using GeckoApp's SharedPreferences file instead gets us here: I/GeckoApp(15598): Locale set took: 0ms; app locale is en_US Yeah.

Here's the current state of this. A selected locale is persisted to and loaded from shared preferences. It's correctly recorded in FHR. It's correctly propagated to Gecko, which will start in the correct locale on its next restart. The Java UI is pretty good about showing the right thing; we only need to redisplay the activity. (Closing then opening about:home will switch languages, for example.) With final UI, rather than with my Tools-menu add-on, your locale selection will presumably be happening in a different activity anyway. If we hard-relaunch (just as we do for entering Guest Mode), Gecko switches, too. This experience isn't bad. Subsequent launches of the browser are displayed in the correct locale (both within Gecko and Java). I'm not 100% sure if the Java part of this is timing-related, but see below. There is some UI that doesn't ever reflect the locale setting; for example, "Enter Search or Address". I haven't yet investigated why -- e.g., whether that's due to being hard-coded as a resource reference versus being programmatically set, but that should be fixable. Fixing this might involve an onLocaleReady callback, which would ensure no timing issues. In theory, there is no need to restart the browser. We'd have to restart *Gecko*, and redisplay our current activity. I'm happy leaving that for a follow-up, but it might be a pretty small follow-up (at least, the Java part would be small). That would be really great, though, because quitting and relaunching the browser is *usually* fast, but is sometimes janky as hell, with these fun log snippets just a small part of the four-second startup time: Parsing INI: 11-11 16:42:56.576 D/StrictMode(12011): StrictMode policy violation; ~duration=159 ms: HardwareUtils.isLowMemoryPlatform: 11-11 16:42:56.846 D/StrictMode(12011): StrictMode policy violation; ~duration=53 ms: 11-11 16:42:56.916 E/GeckoLibLoad(12011): Loaded libs in 192ms total, 100ms(230ms) user, 30ms(70ms) system, 0(1) faults Downloads: APK: https://dl.dropboxusercontent.com/u/3911373/fennec/936756/fennec-28.0a1.multi.android-arm.apk Tools menu add-on: https://dl.dropboxusercontent.com/u/3911373/fennec/936756/about-pages.xpi

Question here for Axel. I've read all of Bug 768939, and I see hints that Gecko locale switching doesn't require a restart (Bug 768939 Comment 15 and 17, for example). Is this the case?

Oh, and another thing to fix: configuration changes (e.g., rotation) need to be handled.

Awesomesauce ! There shouldn't be a requirement to restart gecko, and setting the pref should hit http://mxr.mozilla.org/mozilla-central/source/chrome/src/nsChromeRegistryChrome.cpp#340 and flush the cashes. That should take care of things, together with the reload that comes from redoing the activities that hold on to gecko content, I'd expect. I think you want to set matchOS to false, too. And also, if the explicit locale setting is unset, we want to revert both prefs to default. Re the Enter Search UI, can you check if other android:hint UI is affected, too?

Note to self for later: I anticipate pain when switching between LTR and RTL languages.

Note to self for later: make sure that SysInfo.getLocale is re-documented and re-jigged to grab the OS locale prior to any possible change.

Progress report: I appear to have this entirely and completely working, without restarts, with the only issue being that my menu regeneration code is somehow spawning additional Reload buttons on the toolbar. I'll hunt down that bug today and then start tidying up my patch queue.

Let's just say I'm QAing this thoroughly. If you install with one locale, switch the OS locale, manually set the app locale to match the OS locale, and then switch the OS locale to something else (or permutations like that), we get into an NPE crash loop on next activity display: W/System.err(23114): java.lang.NullPointerException W/System.err(23114): at org.mozilla.gecko.gfx.NinePatchTileLayer.drawPatch(NinePatchTileLayer.java:104) W/System.err(23114): at org.mozilla.gecko.gfx.NinePatchTileLayer.drawPatches(NinePatchTileLayer.java:56) W/System.err(23114): at org.mozilla.gecko.gfx.NinePatchTileLayer.draw(NinePatchTileLayer.java:37) W/System.err(23114): at org.mozilla.gecko.gfx.LayerRenderer$Frame.drawBackground(LayerRenderer.java:603) W/System.err(23114): at dalvik.system.NativeStart.run(Native Method) W/System.err(23114): at dalvik.system.NativeStart.run(Native Method) W/System.err(23114): java.lang.NullPointerException W/System.err(23114): at org.mozilla.gecko.gfx.ScrollbarLayer.draw(ScrollbarLayer.java:203) W/System.err(23114): at org.mozilla.gecko.gfx.LayerRenderer$Frame.drawForeground(LayerRenderer.java:630) W/System.err(23114): at dalvik.system.NativeStart.run(Native Method) W/System.err(23114): at dalvik.system.NativeStart.run(Native Method) W/System.err(23114): java.lang.NullPointerException W/System.err(23114): at org.mozilla.gecko.gfx.NinePatchTileLayer.drawPatch(NinePatchTileLayer.java:104) W/System.err(23114): at org.mozilla.gecko.gfx.NinePatchTileLayer.drawPatches(NinePatchTileLayer.java:56) W/System.err(23114): at org.mozilla.gecko.gfx.NinePatchTileLayer.draw(NinePatchTileLayer.java:37) W/System.err(23114): at org.mozilla.gecko.gfx.LayerRenderer$Frame.drawBackground(LayerRenderer.java:603) W/System.err(23114): at dalvik.system.NativeStart.run(Native Method) W/System.err(23114): at dalvik.system.NativeStart.run(Native Method) W/System.err(23114): java.lang.NullPointerException W/System.err(23114): at org.mozilla.gecko.gfx.ScrollbarLayer.draw(ScrollbarLayer.java:203) W/System.err(23114): at org.mozilla.gecko.gfx.LayerRenderer$Frame.drawForeground(LayerRenderer.java:630) W/System.err(23114): at dalvik.system.NativeStart.run(Native Method) W/System.err(23114): at dalvik.system.NativeStart.run(Native Method) E/StrictMode(23114): class org.mozilla.fennec_rnewman.App; instances=6; limit=1 E/StrictMode(23114): android.os.StrictMode$InstanceCountViolation: class org.mozilla.fennec_rnewman.App; instances=6; limit=1 E/StrictMode(23114): at android.os.StrictMode.setClassInstanceLimit(StrictMode.java:1) (Note the six instances of .App!) My bet is that somewhere a resource is getting half-way discarded, but we'll see what my next drink finds. Other than that, I'm pretty happy with this patch queue.

My conclusion re that issue: modifying the Configuration instance in GeckoApplication.onConfigurationChanged -- something we *must* do in order for rotation and activity changes to persist locale choices correctly -- is causing the activity to relaunch in a loop when: * The app is open in the background * The locale is switched twice * The activity is reopened and the current system locale is not the same as ours, and thus we attempt to modify the config. This does not affect installs that don't have a locale pref set. The issue does not arise if we do nothing in GeckoApplication.onConfigurationChanged, or if we only call Locale.setDefault. But that's not enough to preserve the behavior we need. I'm not above calling this an Android bug -- I notice that often a visit to Settings in reproducing this will screw up the orientation of the device, then re-refresh, in a way that feels very much like a re-applied config. An adequate workaround, IMO, is to end the activity when the system locale changes. I will test that next.

It's nice to know that the time-honored technique of butting one's head against a problem for hours and hours still sometimes work. Please bang on these, folks. APK: https://www.dropbox.com/s/sgte8h28p06yll5/fennec-28.0a1.multi.android-arm.apk Tools menu add-on: https://dl.dropboxusercontent.com/u/3911373/fennec/936756/about-pages.xpi I have nine patches that cover subparts and a refactoring of the initial implementation (ripping it out of GeckoApp*). I'm tempted to post them as-is, rather than squelching and having to explain why things are the way they are in bug comments. (Would flatten for landing.) Thoughts?

Comment on attachment 832797 [details] [diff] [review] Part 0: refactor prefs access in GeckoApp. v1 Sorted `implements` fields in GeckoApp; added SharedPreferences access to ContextGetter; added license to ContextGetter; used these in GeckoAppShell. r? mfinkle, 'cos GeckoView.

Original parts: Bug 936756 - Part 3: handle configuration changes by reapplying locale. Bug 936756 - Part 5: refactor locale management into a separate class. Bug 936756 - Part 6: cache current saved locale to avoid reconstituting it onConfigurationChanged. Bug 936756 - Part 7: move timing code into LocaleManager. Bug 936756 - Part 8: clarify comment. Bug 936756 - Part 9: correctly handle system locale changes.

Comment on attachment 832800 [details] [diff] [review] Part 2: implement loading of locale from prefs. Note that there is a TODO here, but I'm happy to leave it until we implement UI for this. (For example, there's no way to clear the settings yet.)

Comment on attachment 832801 [details] [diff] [review] Part 3: rebuild menus and action items on locale change. Sriram gets this one. In order to rebuild the menu when the locale changes, I altered the definition of the item cache -- it's no longer just for when the menu isn't ready yet, but it's also a parallel copy of the original add-on items. I split out the adding of a new item from the adding of an item to the menu itself in order to facilitate two code paths. The TODO here is a question about the existing code -- it seems like we could just use our captured item. I don't know the code well enough to answer either way. Other than that, the comments should be pretty thorough.

As you can see, I elected to fold the patches down. Let me know if you want to see the evolution.

A few comments on the questions asked in the patches: If we set the gecko locale explicitly, we need to matchOS to false, but it should be set to true by default. I wonder how much we win by actually setting the gecko locale explicitly, or if we can just tweak the process ENV to be what we want? That might also be helpful for js toLocaleString() where we still hook into libc. If we're setting locales explicitly, we probably need to pay attention to the odd locale names iw and in, see http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/Makefile.in#7 and http://code.google.com/p/android/issues/detail?id=3639

(In reply to Axel Hecht [:Pike] from comment #22) > A few comments on the questions asked in the patches: Thanks for chiming in! > If we set the gecko locale explicitly, we need to matchOS to false, but it > should be set to true by default. I think that's the case. > I wonder how much we win by actually setting the gecko locale explicitly, or > if we can just tweak the process ENV to be what we want? That might also be > helpful for js toLocaleString() where we still hook into libc. It's only something we do on change -- afterwards we allow Gecko to decide from its own prefs -- and it does express the user's intent. But this is the area I've paid least attention to: I don't know if matchOS needs to stick around by default for when we're matching the Android locale, for example. Another downside I could see to the process env is that we'd need to do it every time we started a new process. Prefs are fo' eva. > If we're setting locales explicitly, we probably need to pay attention to > the odd locale names iw and in, see > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/ > Makefile.in#7 and http://code.google.com/p/android/issues/detail?id=3639 Yup, already marked down as a constraint in my notes; viewing that as a follow-up before we ship UI on top of this. (We don't ship those locales yet, IIRC, and I already have a headsup in the Fennec wiki for when `in` gets to the front of the queue.)

Forgot that Android 2.2 doesn't have String.isEmpty. (Ain't gonna import TextUtils just for that...)

In my little bit of testing, the only hiccup I managed to cause was when I switch OS locales to a locale that wasn't listed in under Menu -> Tools. Testing on Samsung Galaxy Tab 10.1 Android 4.2 To replicate: 1) Start with Android OS in en-US 2) Open special apk with about:pages add-on installed. 3) Change app-locale in Menu -> Tools. 4) Verify that locale changes in app. 5) Change Android OS locale to pt-BR 6) Return to running Fennec and repeat step 3. Result: Locale options vanish. Expected result: Locale options should still display and be available for locale switching.

(In reply to jbeatty from comment #25) > In my little bit of testing, the only hiccup I managed to cause was when I > switch OS locales to a locale that wasn't listed in under Menu -> Tools. > > Testing on Samsung Galaxy Tab 10.1 Android 4.2 > To replicate: > 1) Start with Android OS in en-US > 2) Open special apk with about:pages add-on installed. > 3) Change app-locale in Menu -> Tools. > 4) Verify that locale changes in app. > 5) Change Android OS locale to pt-BR > 6) Return to running Fennec and repeat step 3. > > Result: Locale options vanish. > Expected result: Locale options should still display and be available for > locale switching. I replicated this bug on a HTC one. 4.1.2 I was able to make the locale options restored but when selected will not return language back to english After step "6" of Jeff's instructions I did the following. 1)Open menu>tools>add-ons. 2)Select about:pages . 3)disable about:pages add-on. 4)exit out of menu. 5)enter menu>tools>add-ons. 6)select about:pages. 7)enable about:pages. 8)exit out of menu. 9)select menu>tools. Locale options restored. 10)selecting different locale will not return fennec to starting locale.

Comment on attachment 832801 [details] [diff] [review] Part 3: rebuild menus and action items on locale change. Review of attachment 832801 [details] [diff] [review]: ----------------------------------------------------------------- Mostly looks good. r+ with few changes. ::: mobile/android/base/BrowserApp.java @@ +125,5 @@ > public boolean checked = false; > public boolean enabled = true; > public boolean visible = true; > public int parent; > + public boolean added = false; // So we can re-add after a locale change. This comment is not needed. @@ +1788,5 @@ > + * the root (mMenu). > + */ > + private void addAddonMenuItemToMenu(final Menu menu, final MenuItemInfo info) { > + info.added = true; > + White space. @@ +1833,5 @@ > BitmapUtils.getDrawable(this, info.icon, new BitmapUtils.BitmapLoader() { > @Override > public void onBitmapFound(Drawable d) { > + // TODO: why do we re-find the item? > + MenuItem item = destination.findItem(id); Because, there was a bug that the menu item is lost when the drawable is ready or something. So, a "final MenuItem" might not be holding to actual menu item / or may be pointing to different item. So, we find it again and set it. @@ +1857,5 @@ > + if (mAddonMenuItemsCache == null) { > + mAddonMenuItemsCache = new Vector<MenuItemInfo>(); > + } > + > + // Mark it as added if the menu was ready. I would re-order it this way. mAddonMenuItemsCache.add(info); if (mMenu != null) { addAddonMenuItemToMenu(mMenu, info); } the second method takes care of setting the "added" property. ::: mobile/android/base/menu/GeckoMenu.java @@ +9,5 @@ > import android.content.ComponentName; > import android.content.Context; > import android.content.Intent; > import android.util.AttributeSet; > +import android.util.Log; Unused. @@ +220,5 @@ > if (menuItem.hasSubMenu()) { > + SubMenu sub = menuItem.getSubMenu(); > + if (sub == null) { > + continue; > + } New line after this. @@ +225,5 @@ > + try { > + sub.clear(); > + } catch (Exception ex) { > + Log.e(LOGTAG, "Couldn't clear submenu.", ex); > + } What exception could happen?

Comment on attachment 833262 [details] [diff] [review] Part 2: implement loading of locale from prefs. v2 Might as well pick an actual reviewer, even if he 302s.

Comment on attachment 833262 [details] [diff] [review] Part 2: implement loading of locale from prefs. v2 >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java >+ final String uiLocale = appLocale; >+ ThreadUtils.postToUiThread(new Runnable() { >+ @Override >+ public void run() { >+ GeckoApp.this.onLocaleReady(uiLocale); >+ } >+ }); Is this the normal startup path? We need to "prime" the system with a call to onLocaleReady? I guess we aren't setting the hint in the toolbar via XML layout anymore, so we need a call to get that to show up. Does the call to onConfigurationChanged in onLocaleReady add an extra unneeded step during startup? > public void onConfigurationChanged(Configuration newConfig) { >+ Log.d(LOGTAG, "onConfigurationChanged: " + newConfig.locale); >+ LocaleManager.correctLocale(getResources(), newConfig); "correctLocale" has me a bit confused about the purpose. The adjective form of "correct" is in conflict with the verb form, and I think the verb is your intention. >diff --git a/mobile/android/base/LocaleManager.java b/mobile/android/base/LocaleManager.java >+ public static void correctLocale(Resources res, Configuration config) { Rename to adjustLocale ? or adjustSystemForLocale ? >+ private static void persistLocale(String localeCode) { >+ final SharedPreferences settings = getSharedPreferences(); >+ settings.edit().putString(getPrefName(), localeCode).commit(); commit() is blocking and will likely show up as a strict mode violation when run on the UI thread. >+ public static String getAndApplyPersistedLocale() { >+ final long t1 = android.os.SystemClock.uptimeMillis(); >+ final long t2 = android.os.SystemClock.uptimeMillis(); >+ Log.i(LOG_TAG, "Locale read and update took: " + (t2 - t1) + "ms."); Do we want to keep the timing code in place? >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js >+ setLocale: function (locale) { >+ sendMessageToJava({type: "Locale:Set", locale: locale}); >+ }, Is this for add-ons or something? I don't see it called anywhere. What do we pass it as the locale arg? nit: { type: "Locale:Set", locale: locale } r- just to get some discussion on the patch

(In reply to Mark Finkle (:mfinkle) from comment #29) > Is this the normal startup path? We need to "prime" the system with a call > to onLocaleReady? I guess we aren't setting the hint in the toolbar via XML > layout anymore, so we need a call to get that to show up. Yes. I also expect this pattern to grow in the future -- e.g., when we support downloadable locales. We should be delaying all string-related operations until the locale is ready, because (a) it might not be when our early-lifecycle activity methods are called, and (b) it might change at runtime. > Does the call to onConfigurationChanged in onLocaleReady add an extra > unneeded step during startup? Not IIRC. (That is, as I remember, things don't work correctly without it.) > >+ LocaleManager.correctLocale(getResources(), newConfig); > > "correctLocale" has me a bit confused about the purpose. The adjective form > of "correct" is in conflict with the verb form, and I think the verb is your > intention. If it were the adjective form, it'd be isCorrectLocale, right? Yes, verb is my intention (and OO convention). This is called "correcting" because Android keeps giving us an erroneous config object, so we have to correct it. "Adjust" loses some of the precision, but if the ambiguity really irks you, we can switch. > >+ private static void persistLocale(String localeCode) { > >+ final SharedPreferences settings = getSharedPreferences(); > >+ settings.edit().putString(getPrefName(), localeCode).commit(); > > commit() is blocking and will likely show up as a strict mode violation when > run on the UI thread. I eagerly await the day we can use the API9+ non-blocking equivalent. > Do we want to keep the timing code in place? For now, yeah. > Is this for add-ons or something? I don't see it called anywhere. What do we > pass it as the locale arg? This is what the testing add-on -- and, eventually, the settings UI -- use to control this subsystem. Think of it as a decoupled one-way method call that works from JS.

(In reply to Mark Finkle (:mfinkle) from comment #29) > commit() is blocking and will likely show up as a strict mode violation when > run on the UI thread. This is only called from the message loop, which, as far as I can tell, is not invoked on the UI thread. > Is this for add-ons or something? I don't see it called anywhere. What do we > pass it as the locale arg? I've added a docstring for this.

TODO: how do I change skia's locale? 11-25 16:39:55.619 D/skia (30947): new locale es-Latn-US Also, WTF kind of locale code is that, Android? At least the Java side gets it right: 11-25 16:39:39.491 D/GeckoApplication(29291): onConfigurationChanged: es_US, background: true

(In reply to Aaron from comment #26) > (In reply to jbeatty from comment #25) > > Result: Locale options vanish. > > Expected result: Locale options should still display and be available for > > locale switching. The problem here is that a system locale change finishes and recreates all activities (which is why switching apps is so clunky for a minute afterwards). What it *doesn't* do is relaunch the Gecko thread, so while we've rebuilt the menu, losing all prior state, we haven't re-run all of our init code (e.g., restarting AddonManager), so the add-on doesn't get to add its menu items again. This is presumably one reason why GeckoAppShell used to relaunch Fennec when the system locale changed. Instead, we can watch for 'locale' in configChanges. This will require a small amount of extra code to handle the case where the user has not made a datatype election -- we'll need to switch to match. Unfortunately, in API 17 and up, 'locale' is joined by 'layoutDirection' (RTL/LTR)... which for no apparently reason doesn't build correctly in my manifest, even if I bump the targetSdkVersion to 17 or 18. Fingers crossed that by targeting API 16 we don't get bitten.

... aaaaand we do get bitten. So we have to detect when the system locale changes and restart appropriately.

This is mostly a restoration of the old GeckoAppShell logic in Part 2, but a little tidier. I've confirmed that the app locale and the menu items are persisted across system locale changes.

Nits addressed. See Comment 30 and Comment 31 for substantive responses.

Comment on attachment 8338256 [details] [diff] [review] Part 4: restore restart-on-locale-change logic. v1 I'm keeping this as a separate patch for clarity, rather than rolling it into Part 2. I'm hopeful that we'll end up with one of two solutions: * An onConfigurationChanged approach that builds, and allows us to use our dynamic locale switching to reflect OS locale changes in the match-OS scenario. * A way to selectively re-init Gecko if that's not the case -- it's pretty heavyweight to relaunch the process just to get add-ons to re-add themselves to the Java UI!

https://hg.mozilla.org/integration/fx-team/rev/9ebed437d78f https://hg.mozilla.org/integration/fx-team/rev/606c4b4ab77f https://hg.mozilla.org/integration/fx-team/rev/686b4b5a5be3 https://hg.mozilla.org/integration/fx-team/rev/c46514ce6886 https://hg.mozilla.org/integration/fx-team/rev/9cbe6533c480

Comments on follow-ups and open issues: (In reply to Richard Newman [:rnewman] from comment #9) > Note to self for later: make sure that SysInfo.getLocale is re-documented > and re-jigged to grab the OS locale prior to any possible change. Note that we restart on system locale change, and we pass the correct app locale to BrowserHealthRecorder, so this *should* be good. (In reply to Axel Hecht [:Pike] from comment #22) > That might also be > helpful for js toLocaleString() where we still hook into libc. I did not address this (yet?). > If we're setting locales explicitly, we probably need to pay attention to > the odd locale names iw and in, see This'll be a follow-up that blocks the actual landing of the feature we'll eventually build. (In reply to Richard Newman [:rnewman] from comment #32) > TODO: how do I change skia's locale? This is still an open question. I've seen no issues with the European languages I've used for testing, but when this feature is live we should test it with, e.g., Arabic. Apparently skia is involved with glyph handling.

https://hg.mozilla.org/mozilla-central/rev/9ebed437d78f https://hg.mozilla.org/mozilla-central/rev/606c4b4ab77f https://hg.mozilla.org/mozilla-central/rev/686b4b5a5be3 https://hg.mozilla.org/mozilla-central/rev/c46514ce6886 https://hg.mozilla.org/mozilla-central/rev/9cbe6533c480

Backed out: https://hg.mozilla.org/mozilla-central/rev/4bf430d990e5

Re-landed by backing out the backout. https://hg.mozilla.org/integration/fx-team/rev/cd918aef9e64