653141 - allow language choice on first-run

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Madhava Enros [:madhava]]

Given that we will be shipping a uni-lingual (English) build, part of the first-run experience must be to let a user choose the language in which he or she would like to use Fennec.

Design work proceeding on the feature page: https://wiki.mozilla.org/Fennec/Features/langchoice

Comment 1

[dynamis (Tomoya ASAI)]

I saw the Brian's mocks but I think it doesn't work well:
https://wiki.mozilla.org/images/b/b6/LanguagePacks_Ideal1stRunUX.pdf

Please note that many of non-English user cannot understand any English label.
So "Continue in English" or "Choose Different language" menu doesn't work.

Firstrun page itself should be localized by default depending on the system locale. If it's difficult to load langpack before we see the firstrun, only the page should be multi-language even in the English build.

Comment 2

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

(In reply to comment #1)

> Firstrun page itself should be localized by default depending on the system
> locale. If it's difficult to load langpack before we see the firstrun, only the
> page should be multi-language even in the English build.

I was thinking that we would put localized strings in the en-US locale for as many languages as we could. We would only fallback to en-US strings if we didn't have a localized version.

We could also try pulling the string from the langpack XPI.

Comment 3

[Madhava Enros [:madhava]]

I believe the intended approach is to include those basic strings ("continue in [language name]", etc.) in the set of languages available for android. That way, we can detect at least the OS language and match that.  Continuing will get the user the full langpack. For users in locales where Firefox has a translation that is not available for the OS, at least the user will see the first message in a language they're likely to understand, given that it's what the phone is set to.

Comment 4

[u60234]

Is this a one time offer for the user? What happens if Fennec don't yet have a localization that matches the OS locale, and the user is forced to choose another language. Will the user get notified if a more suitable Fennec lang pack becomes available?

Comment 5

[dynamis (Tomoya ASAI)]

(In reply to comment #2)
> I was thinking that we would put localized strings in the en-US locale for
> as many languages as we could. We would only fallback to en-US strings if we
> didn't have a localized version.

I think language (locale) list like Mac OS X installer's first dialog ("Use English for the main language" label in every locales) is better for the fallback, not en-US UI.

(In reply to comment #4)
> Is this a one time offer for the user? What happens if Fennec don't yet have
> a localization that matches the OS locale, and the user is forced to choose
> another language. Will the user get notified if a more suitable Fennec lang
> pack becomes available?

Uses can switch the locale in the config page. But I agree it's better to offer once again when the OS locale is switched or when new locale become available.
# it may be another bug?

Comment 6

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

Attachment: simple add-on with a locale helper

Comment 7

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

Comment on attachment 531484 [details]
simple add-on with a locale helper

Some code that fetches add-ons (locales) and builds a simple display list

Comment 8

[Wesley Johnston (:wesj)]

Attachment: WIP

This needs love, but sorta works. Need to get a repo that has some working language addons to really test with.

We will eventually also need to ship strings in the en-US locale from all of our supported locales. I started to work on a script to do this (Probably buried somewhere in the WIP), and with some work on the build script side we can go that way (they will have download all of the language repos for us just to build the en-US one).

Or we can start looking into some alternatives like just keeping these strings in the en-US repo or doing this through some sort of "hidden" addon.

Comment 9

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

Comment on attachment 532099 [details] [diff] [review]
WIP

so I rememeber

Comment 10

[Wesley Johnston (:wesj)]

Attachment: WIP

Not sure what you wanted on the last one. I haven't attempted to clean this patch up at all, but overall it seems to work fairly well. I have a (slowly growing) list of little problems to clean up with this before I put it up for review or feedback.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Patch Version

I've pulled out most of the build system changes required here and put them in a separate patch (coming in a second). Beyond that, this works fairly well. I'll post some screenshots up in flickr in a minute.

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Build changes

These are much more WIP quality, but I'd like feedback on the approach at least. We'll also have to make changes to mozharness for this. Does this look like an ok approach mfinkle, or would you rather I look for a different route?

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Patch v1.01

Whoops. Found a few css glitches while putting together screenshots. This should fix them.

Comment 14

[Wesley Johnston (:wesj)]

Flickr set:

http://www.flickr.com/photos/digdug2k/5727250593/in/set-72157626736328448/

Also I should note two things:

This should update the UI language whenever you select a language. If there is no localized string for a particular string, we should show the en-US string. Since this only has en-US strings right now, it only ever shows en-US strings.

Also, the pref is downloading the list from my dropbox. I did that because the amo links I had either gave me nothing compatible, or a single compatible localization which doesn't actually exist. Its a pref now. Easy to change.

Comment 15

[Wesley Johnston (:wesj)]

Build to play with:

http://dl.dropbox.com/u/72157/fennec-localePicker.apk

This will only show when a new profile is created, so you may have to clear your data every time you want to play with it. You can try going to about:locales, but I haven't tested there at all.

Comment 16

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

Comment on attachment 532708 [details] [diff] [review]
Patch Version

I assume this patch is obsolete?

Comment 17

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

Comment on attachment 532714 [details] [diff] [review]
Patch v1.01


>diff --git a/mobile/app/mobile.js b/mobile/app/mobile.js

> pref("extensions.blocklist.detailsURL", "https://www.mozilla.com/%LOCALE%/blocklist/");
>+pref("extensions.locale.url", "http://dl.dropbox.com/u/72157/get_language_packs.xml");

* line break to separate from blocklist section
* "extensions.getLocales.get.url" might be better name, to make it more like the addon pref we are sorta emulating

>+// True if this is the first time we are showing about:firstrun
>+pref("browser.firstrun", true);

"browser.firstrun.show.uidiscovery"

>+// Show the locale picker when a new profile is created
>+pref("browser.firstrun.useLocalePicker", true);

I'm hoping we can remove this pref and just use the OS locale != current locale

>diff --git a/mobile/chrome/content/LocaleRepository.js b/mobile/chrome/content/LocaleRepository.js

Perhaps this should be a module, like AddonsRespository.jsm ?

>+/* For now I am assuming these are declared somewhere else
>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
>+const Cu = Components.utils;
>+*/

Not if this is a module

>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+Cu.import("resource://gre/modules/NetUtil.jsm");

Might not need these two

>+// A map between XML keys to AddonSearchResult keys for string values

LocaleSearchResult

>+// A map between XML keys to AddonSearchResult keys for string values

LocaleSearchResult

>+// A map between XML keys to AddonSearchResult keys for integer values

LocaleSearchResult

>+LocaleSearchResult.prototype = {
< long list of properties >
>+};

Someday we should really look to see which of these are needed

>+function convertHTMLToPlainText(html) {

We can remove this method if we have no properties that can old HTML

>diff --git a/mobile/chrome/content/aboutLocales.js b/mobile/chrome/content/aboutLocales.js

>+Cu.import("resource://gre/modules/AddonManager.jsm");
>+Cu.import("resource://gre/modules/Geometry.jsm");

These might not be used in this file

>+let stringPrefs = [
>+  {selector: "#continueInButton", pref: "locale.continueIn", data: ["CURRENT_LANGUAGE"] },
>+  {selector: "#changeLanguage", pref: "locale.choose", data: null },
>+  {selector: "#pickerTitle", pref: "locale.chooseLanguage", data: null },
>+  {selector: "#continueButton", pref: "locale.continue", data: null },
>+  {selector: "#cancelButton", pref: "locale.cancel", data: null },
>+  {selector: "#intallingMessage", pref: "locale.installing", data: ["CURRENT_LANGUAGE"]  },
>+  {selector: "#cancelInstallButton", pref: "locale.cancel", data: null },
>+  {selector: "#installingError", pref: "locale.installerror", data: null },
>+  {selector: "#installContinue", pref: "locale.continue", data: null },
>+  {selector: "#loadingMsg", pref: "locale.list.loading", data: null },
>+  {selector: "#loadingErrorMsg", pref: "locale.list.downloadError", data: null },
>+  {selector: "#loadedNoneMsg", pref: "locale.list.noneAvailable", data: null }
>+];

So these are the strings we would need to have localized?

>+let LocaleRepository = null;
>+let defaultLanguage  = "en-US";
>+let currentLanguage  = "en-US";
>+let currentAddon     = null;

* No pretty alignment!
* Using a module would make LocaleRepository unneeded
* Why no "g" prefixes?
* Should these be data members of LocaleUI ?

>+let XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";

XUL_NS is unused.

>+[["localepicker",    "chrome://browser/locale/localepicker.properties"],
>+ ["brand",      "chrome://branding/locale/brand.properties"]].forEach(function (aStringBundle) {

No pretty alignment! 

>+  get LocaleRepository() {
>+    delete this.LocaleRepository;
>+    let scriptLoader = Cc["@mozilla.org/moz/jssubscript-loader;1"].getService(Ci.mozIJSSubScriptLoader);
>+    scriptLoader.loadSubScript("chrome://browser/content/LocaleRepository.js", this);
>+    return this.LocaleRepository;
>+  },

Remove if you switch to a module

>+  addToList: function(aItems, aClear) {
>+  },

Not used?

>+
>+  loadLocales: function() {
>+    showLoading();
>+    this.LocaleRepository.getLocales(this._onDownloadedLocales.bind(this));
>+  },
>+
>+  _onDownloadedLocales: function(aLocales) {

>+    while(this.list.firstChild) {
>+      this.list.removeChild(this.list.firstChild);
>+    }

while (...)
No {} needed for a one-liner

>+function getString(aBundle, aLocale, aSetting, aDataSet, aAddon) {

>+    if (databundle.some(function(aItem) aItem === null))
>+      throw("String not found");

Do we want to fallback to en-US ?

>+function closeWindow() {
>+  // Trying to close this window and open a new one results in a horked UI.
>+  // Force a restart even if we don't need one.

Why?

>+  if (cancelQuit.data == false) {
>+    let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup);
>+    appStartup.quit(Ci.nsIAppStartup.eRestart | Ci.nsIAppStartup.eForceQuit);
>+    return;

No need for the 'return'

>+function start(aRoot) {

>+  updateStrings();
>+  gMainPage = gRoot.querySelector("#mainPage");
>+  gPicker   = gRoot.querySelector("#picker");
>+  gInstaller = gRoot.querySelector("#installer");

No pretty alignment

I wonder why you didn't put more of the global code into LocaleUI?

>diff --git a/mobile/chrome/content/aboutLocales.xul b/mobile/chrome/content/aboutLocales.xul

Given how much trouble we have with about:config in XUL, this frightens me

>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js

>     // Should we restore the previous session (crash or some other event)
>     let ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);
>-    if (ss.shouldRestore()) {
>+    if (ss.shouldRestore() && ss.canRestoreLastSession) {

ss.canRestoreLastSession does not exist in mobile

>diff --git a/mobile/chrome/content/firstrun/firstrun.xhtml b/mobile/chrome/content/firstrun/firstrun.xhtml

>+          if (prefs.getBoolPref("browser.firstrun")) {
>+            startDiscovery();
>+            prefs.setBoolPref("browser.firstrun", false);
>+          } else
>+            endDiscovery();

"browser.firstrun" -> "browser.firstrun.show.uidiscovery"
{ } around the else block since the if block has them

>diff --git a/mobile/components/BrowserCLH.js b/mobile/components/BrowserCLH.js

>+        // Override the default if we have a new profile
>+        if (needHomepageOverride() == "new profile") {
>+          let useLocalePicker = false;
>+          try { useLocalePicker = Services.prefs.getBoolPref("browser.firstrun.useLocalePicker"); }
>+          catch (e) { }
>+
>+          if (useLocalePicker) {
>+            win = openWindow(null, "about:locales", "_blank", "chrome,dialog=no,all", null);
>+            aCmdLine.preventDefault = true;
>+            return;
>+          }
>+        }

We should avoid the preference and check to see if the OS locale is the current Mozilla locale. If not, show the picker.
See:
http://mxr.mozilla.org/mozilla-central/source/intl/locale/idl/nsILocaleService.idl#62 (or line #73)

>-        // Override the default if we have a new profile
>-        if (needHomepageOverride() == "new profile")
>-            defaultURL = "about:firstrun";
>+        if (Services.prefs.getBoolPref("browser.firstrun"))
>+          defaultURL = "about:firstrun";

I don't think we want to use this. Why not use the old code (with the indent fixed) ?


>diff --git a/mobile/locales/en-US/chrome/localepicker.properties b/mobile/locales/en-US/chrome/localepicker.properties

>+en-US.locale.title=Select a language
>+en-US.locale.continueIn=Continue in %S
>+en-US.locale.name=English
>+en-US.locale.choose=Choose Different Language
>+en-US.locale.chooseLanguage=Choose Language
>+en-US.locale.cancel=Cancel
>+en-US.locale.continue=Continue
>+en-US.locale.installing=Installing language %S
>+en-US.locale.installerror=Error installing language
>+en-US.locale.list.loading=Loading...
>+en-US.locale.list.downloadError=Unable to download locales
>+en-US.locale.list.noneAvailable=No languages available

"locale" in the entity name is redundant. You can remove it

OK for now, but we might need to move the localized strings to AMO, and keep these as a fallback.

>diff --git a/mobile/themes/core/aboutLocales.css b/mobile/themes/core/aboutLocales.css

>+/* These are probably not the best thing for us to use, but we dont have a lot to choose from right now */
>+#loadingError {
>+  list-style-image: url("chrome://global/skin/icons/Error.png");
>+}
>+
>+#loadedNone {
>+  list-style-image: url("chrome://global/skin/icons/notfound.png");
>+}

Then let's not use them at all. Text will be enough for now

>+.image-button.continue {
>+  background-image: url("chrome://browser/skin/images/arrowrightdark-16.png"),
>+                    -moz-linear-gradient(top, rgb(255,217,88), orange, rgb(255,217,88));
>+}
>+
>+.image-button.cancel {
>+  background-image: url("chrome://browser/skin/images/search-clear-30.png"),
>+                    -moz-linear-gradient(top, rgb(255,217,88), orange, rgb(255,217,88));
>+}

Lets keep the shading the same as the current styling

Comment 18

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Thanks for the quick review! V2 fixes most of what you asked for. To recap and answer a few questions along the way:

* Only show if the fennecLocale != systemLocale. Hope this isn't to contentious?

* Moved LocaleRepository to a .jsm, and cleaned a bunch of useless stuff out. Probably more can go.

* +Cu.import("resource://gre/modules/NetUtil.jsm"); in LocaleRepository.jsm
NetUtils is needed (unless we want to unneed it)

* Cu.import("resource://gre/modules/Geometry.jsm"); moved to input.js
This is needed by input.js (which we need for scrolling). 

* So these are the strings we would need to have localized?
Yep. A few are less necessary than others though.

* Moved global variables, as well as a bunch of functions into LocaleUI. (Artifacts from an older version)

* Do we want to fallback to en-US ?
In updateString I call getString with the requested locale. If that throws, I call back in with the system locale. If that fails, I try one last time with the current fennec locale. Fixed a few bugs here. I've left some es-US (Espanol) strings that were translated online for me here for testing as well. I'll pull them out before pushing.

> Why do we need a restart?
Some of our browser startup code must rely on being called early. If I just close one window and open the other, the sidebars are positioned incorrectly and there is no browser window. If I then bring up the awesome screen and then switch back things are fine. We can probably make this work. Just need to dig into where the problems are (likely near the resize handler).

> Given how much trouble we have with about:config in XUL, this frightens me
Trying to keep the scrolling code simple. input.js works well with nsIScrollBoxObjects, so I stuck with XUL. This is more like the addons pane than about:config though. browser.xul isn't even loaded here.

> I don't think we want to use this. Why not use the old code (with the indent fixed)? (r.e. BrowserCLH changes)
We will already have a profile the second time around, but still need to show about:firstrun. I don't know of a better way to force this.

Comment 19

[Wesley Johnston (:wesj)]

Updated the build with this guy:

http://dl.dropbox.com/u/72157/fennec-localePicker.apk

NOTE: You won't see the localePicker if you are using en-US android. Change to something else, clear your profile, and start again to see it.

Comment 20

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

(In reply to comment #18)

Some replies before the next review:

> * Only show if the fennecLocale != systemLocale. Hope this isn't to
> contentious?

Seems like we want to always show the language selector window on first run

> * Cu.import("resource://gre/modules/Geometry.jsm"); moved to input.js
> This is needed by input.js (which we need for scrolling). 

Moving is good

> > Why do we need a restart?
> Some of our browser startup code must rely on being called early. If I just
> close one window and open the other, the sidebars are positioned incorrectly
> and there is no browser window. If I then bring up the awesome screen and
> then switch back things are fine. We can probably make this work. Just need
> to dig into where the problems are (likely near the resize handler).

OK. I agree with doing the restart now and looking into how we can stop doing it later.

> > Given how much trouble we have with about:config in XUL, this frightens me
> Trying to keep the scrolling code simple. input.js works well with
> nsIScrollBoxObjects, so I stuck with XUL. This is more like the addons pane
> than about:config though. browser.xul isn't even loaded here.

I completely missed the fact that this window loaded before browser.xul. I was assuming it was an about:locales page, loaded in the browser. Do we need about: page support? Maybe for later?

> > I don't think we want to use this. Why not use the old code (with the indent fixed)? (r.e. BrowserCLH changes)
> We will already have a profile the second time around, but still need to
> show about:firstrun. I don't know of a better way to force this.

We no longer want to use about:firstrun at all. We will be moving straight to about:home (or whatever the home page is set to be)

That will be filed as a separate bug, but we can worry less about firstrun in this bug.

on to the review...

Comment 21

[Wesley Johnston (:wesj)]

Attachment: Patch v3

Incorporates changes from a talk with mfinle, bdils, and madhava. Added a background logo image behind the main section (the current splash we use has a black background so I can't use it). Now shows installed locales in the list as well as the downloaded ones. Removed button icons. Always shows when a new profile is made. A few other little tweaks here and there (resizing should be better).

I also made the matching algorithm for locales a bit more complex. On my phone the system has an en-US locale and an es-US locale (US Spanish). We ship en-ES (spanish Spanish). I think, unless there is an es-US locale in Fennec, those should probably match? Added a function to check that and to bring up the issue.

Haven't tested this on my actual phone yet, but will as soon as I get to work. Wanted to put it up anyway to make sure things kept rolling.

Comment 22

[Wesley Johnston (:wesj)]

Attachment: Patch v4

Found some bugs while adding the preferences UI.

Comment 23

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

Comment on attachment 533866 [details] [diff] [review]
Patch v4

>diff --git a/mobile/chrome/content/aboutLocales.js b/mobile/chrome/content/aboutLocales.js

>+  _showPanel: function() { },

Do we need this?

>+      while(!string && localeIndex < potentialLocales.length) {

Add a space between the while and (

Your use of "elt" and elts" seems to make me sad. Please use full name, it makes it easier to read:

"element" (as in aPref.element) and "elements"

>+}
>\ No newline at end of file

Add newline

Does this really need to be an about: page? Can't we just use a chrome:// URI?

File followup bugs for:
* Why does the UI get horked if we close and open a window?
* Passing the commandline URL through to the locale window and use it during the restart

r+ with nits fixed and bugs filed

If we want to turn this off later, on Aurora, removing (or commenting out) the code from BrowserCLH.js would seem to do the job, right?

Comment 24

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

Comment on attachment 533866 [details] [diff] [review]
Patch v4

>diff --git a/mobile/themes/core/aboutLocales.css b/mobile/themes/core/aboutLocales.css

>+window {

>+  font-family: sans-serif;
>+  font-size: @font_normal@;

platform.css should given you good values for these:
http://mxr.mozilla.org/mozilla-central/source/mobile/themes/core/platform.css#46

Comment 25

[Wesley Johnston (:wesj)]

Attachment: Patch v5

Sorry to add another round on this. This undoes all of my fancy language picking stuff. Instead we are shipping the locale strings normally and refreshing the chrome when you select one. Its a bit tricky maintaining state across refreshes, so I added a pref "localepicker.skipMainPage" to tell us whether not to show the locale list or the main page. i.e. if you tap on Italian we refresh the page, but jump straight to the list.

For downloaded locales, hopefully AMO will send us strings in the future. I haven't addressed that issue here beyond changing the locale strings to read "Continue in Greek" after you install the Greek package.

Also adds a pref "browser.firstrun.show.localepicker" that can be used to hide this during browser-chrome tests.

I'm still testing this on my phone, but it works fine on desktop.

Comment 26

[Axel Hecht [:Pike]]

I'd like to do a review pass on this, too, but I didn't get to it this week. I assume that you want this in by Tuesday's aurora lift?

Comment 27

[Wesley Johnston (:wesj)]

Trying very hard to get it in by then. I'm a bit busy (moving) this weekend, but I'll try to check in often and get to any changes as quickly as I can. We've set this up to be easy to pref off if we have to.

Comment 28

[Axel Hecht [:Pike]]

We may be able to use common pool for downloading localized strings in addition to those we ship. Gandalf?

More comments to come.

Comment 29

[Axel Hecht [:Pike]]

Comment on attachment 532709 [details] [diff] [review]
Build changes

The build stuff here will need more work to support l10n-merge.

i.e., the localized file can be in three different locations, in the LOCALE_MERGEDIR, in the localized srcdir, or in en-US src.

Also, I think you want to move the 
+	@$(MAKE) localepicker

into the chrome: target so that it's also run for en-US, and not just repacks?

There are parts of this patch that I didn't try to understand, but r- based on the lack of l10n-merge support here.

Comment 30

[Axel Hecht [:Pike]]

Comment on attachment 534143 [details] [diff] [review]
Patch v5

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

::: mobile/chrome/content/localePicker.js
@@ +294,5 @@
> +  let short2 = aLocale2;
> +  if (i > -1)
> +    short2 = aLocale2.slice(0, i);
> +  return (short1 == short2) ? GOOD_MATCH : NO_MATCH;
> +}

The actual locale matching code should be BCP47, see bug 525494. For now, it's good enough to make a hack, and this hack looks OK. But in general, locale codes are more complex than ab-CD. Make sure there's a follow-up bug to use the functionality in bug 525494 once we get it?

I'd at least make the code easier by using

short1 = aLocale1.split("-")[0] // just compare language tags.

Comment 31

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to comment #28)
> We may be able to use common pool for downloading localized strings in
> addition to those we ship. Gandalf?

Yeah, once we have CP on production server we should be able to deliver data to Firefox as well :)

Comment 32

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

Comment on attachment 534143 [details] [diff] [review]
Patch v5


>+// True if this is the first time we are showing about:firstrun
>+pref("browser.firstrun.show.uidiscovery", true);
>+pref("browser.firstrun.show.localepicker", true);
>+
>+// Used when reloading the locale picker to skip the main page and go directly to the locales list
>+pref("localepicker.skipMainPage", 0);
>\ No newline at end of file

Add a new line

>diff --git a/mobile/chrome/content/firstrun/firstrun.xhtml b/mobile/chrome/content/firstrun/firstrun.xhtml

>         function init() {
>+          let prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService)
>+                                                              .QueryInterface(Ci.nsIPrefBranch2);

Services.prefs ?

if you keep the raw XPCOM, no wrapping please

>diff --git a/mobile/chrome/content/localePicker.js b/mobile/chrome/content/localePicker.js

>+  cancelPicker: function() {
>+    // restore the last known "good" locale
>+    this.language = Services.prefs.getCharPref("localepicker.lastGoodLocale");

Does this need to be a preference?

>+  cancelInstall: function () {

>+      Services.prefs.setIntPref("localepicker.skipMainPage", 1);

Tell me more about this pref. Why do we need it? I'd like to remove these temporary prefs if possible.

>+    // since we keep reloading the page when the user clicks on locales, we save
>+    // the last "good" locale as a pref so that we can restore it if the user cancels
>+    // out of the dialog
>+    Services.prefs.setCharPref("localepicker.lastGoodLocale", LocaleUI.language);

Why not just keep a calss variable? And why do we need to keep reloading the page?

>\ No newline at end of file

Add one


>diff --git a/mobile/components/BrowserCLH.js b/mobile/components/BrowserCLH.js

>+        // Show the locale selector if we have a new profile
>+        // XXX - if a url was passed in, we need to save it and load it after the restart
>+        if (needHomepageOverride() == "new profile" && Services.prefs.getBoolPref("browser.firstrun.show.localepicker")) {
>+          win = openWindow(null, "chrome://browser/content/localePicker.xul", "_blank", "chrome,dialog=no,all", "newProfile");

Do you need to pass "newProfile" ? Remove if you can.

>diff --git a/mobile/themes/core/localePicker.css b/mobile/themes/core/localePicker.css

>+richlistitem .checkbox {
>+  width: 30px;
>+  height: 30px;
>+  list-style-image: url("chrome://browser/skin/images/check-unselected-30.png");
>+}
>+
>+richlistitem[selected] .checkbox {
>+  list-style-image: url("chrome://browser/skin/images/check-selected-30.png");
>+}

Hmm, the gingerbread patch removes the 30px checkboxes and uses 46px checkboxes. I guess we would need to make a gingerbread specific CSS file.

regarding the temporary prefs, let's talk. We could add something to the localrepository.jsm to hold that data.

Comment 33

[Wesley Johnston (:wesj)]

Attachment: Alternative, no reloads required patch

This just comments out a few lines so we can do some/most of this without having to refresh the page all the time. The commented out code was added in bug 597296, purely to keep the UI from getting into strange states just because the locale pref was changed.

Wanted to put this up for my own bookkeeping. Also pushed to try to see what fails.

Comment 34

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch - revert changes from bug 597296

This patch revert the changes from bug 597296 but keep the UpdateSelectedLocale() method since it fires a message with the observer service.

I've tried running xpcshell tests and they are still working with this change.

Comment 35

[Wesley Johnston (:wesj)]

Attachment: Patch v6

This works with no restarts for already installed locales. For downloaded ones, we may require a restart (unless they can be made restartless xpis?).

Comment 36

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

Comment on attachment 535414 [details] [diff] [review]
Patch v6

>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js

>       // Restore the previous scroll position
>-      let restorePosition = Browser.controlsPosition;
>+      let restorePosition = Browser.controlsPosition || {hideSidebars: true};

{ hideSidebars: true }

>+
>+    if (window.opener)
>+      resizeHandler({ target: window });

Comment as to why we could have an opener and we are doing this

>diff --git a/mobile/chrome/content/firstrun/firstrun.xhtml b/mobile/chrome/content/firstrun/firstrun.xhtml

>+          let prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService)
>+                                                              .QueryInterface(Ci.nsIPrefBranch2);

no wrap

>diff --git a/mobile/chrome/content/localePicker.js b/mobile/chrome/content/localePicker.js

>+let stringPrefs = [
>+  {selector: "#continueInButton", pref: "continueIn", data: ["CURRENT_LANGUAGE"] },
>+  {selector: "#changeLanguage", pref: "choose", data: null },
>+  {selector: "#pickerTitle", pref: "chooseLanguage", data: null },
>+  {selector: "#continueButton", pref: "continue", data: null },
>+  {selector: "#cancelButton", pref: "cancel", data: null },
>+  {selector: "#intallingMessage", pref: "installing", data: ["CURRENT_LANGUAGE"]  },
>+  {selector: "#cancelInstallButton", pref: "cancel", data: null },
>+  {selector: "#installingError", pref: "installerror", data: null },
>+  {selector: "#installContinue", pref: "continue", data: null }
>+];

    { selector:

Add a space

>+let LocaleUI = {

>+  closeWindow : function() {
>+    // Trying to close this window and open a new one results in a corrupt UI.
>+    if (false && LocaleUI._currentInstall) {
>+      let cancelQuit = Cc["@mozilla.org/supports-PRBool;1"].createInstance(Ci.nsISupportsPRBool);
>+      Services.obs.notifyObservers(cancelQuit, "quit-application-requested", "restart");
>+    
>+      if (cancelQuit.data == false) {
>+        let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup);
>+        appStartup.quit(Ci.nsIAppStartup.eRestart | Ci.nsIAppStartup.eForceQuit);
>+        Services.prefs.setBoolPref("browser.sessionstore.resume_session_once", false);
>+      }

I thought we didn't need this anymore?

>diff --git a/mobile/chrome/content/localePicker.xul b/mobile/chrome/content/localePicker.xul

>+<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>

Don't need this. platform.css pulls it in

>+<?xml-stylesheet href="chrome://browser/content/browser.css" type="text/css"?>

Move this to be the first CSS imported (sometime we should do the same in browser.xul too)

>+
>+<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+        onload="start();"
>+        id="aboutLangauges-window"

Do we need the ID? if so use "languages-window" (I try to avoid camelcase in IDs but I see you don't!)

>diff --git a/mobile/locales/en-US/chrome/localepicker.properties b/mobile/locales/en-US/chrome/localepicker.properties

>+choose=Choose Different Language

"Choose a different language" ? or is this supposed to look like a button? If so, "Choose a Different Language"

>+chooseLanguage=Choose Language

"Choose a Language" seems better to me

>diff --git a/mobile/themes/core/localePicker.css b/mobile/themes/core/localePicker.css

Needs license boilerplate

I think you'll need a file for gingerbread too

>+window {
>+  margin: 0px;
>+  background-color: white;

white in gingerbread?

color will likely need to be changed in gingerbread too

>+#installerPage {
>+  background-color: black;
>+  color: white;

really? this seems more like gingerbread to me

>+richlistitem .checkbox {
>+  width: 30px;
>+  height: 30px;

46px in gingerbread

>+  list-style-image: url("chrome://browser/skin/images/check-unselected-30.png");

check-unselected-hdpi.png in gingerbread

>+richlistitem[selected] .checkbox {
>+  list-style-image: url("chrome://browser/skin/images/check-selected-30.png");

check-selected-hdpi.png in gingerbread

r- for the gingerbread changes

Comment 37

[Wesley Johnston (:wesj)]

Attachment: Patch v7

Addressed nits. Had to add a background color for :root. Having a black background maent I also had to play around a bit with link colors.

Comment 38

[Wesley Johnston (:wesj)]

Whoops. There's a debugging dump left in LocaleRepository.jsm that I forgot to remove. Its gone locally, but I don't want to upload a new patch just for that.

Comment 39

[Wesley Johnston (:wesj)]

(In reply to comment #36)
> >+let LocaleUI = {
> 
> >+  closeWindow : function() {
> >+    // Trying to close this window and open a new one results in a corrupt UI.
> >+    if (false && LocaleUI._currentInstall) {
> >+      let cancelQuit = Cc["@mozilla.org/supports-PRBool;1"].createInstance(Ci.nsISupportsPRBool);
> >+      Services.obs.notifyObservers(cancelQuit, "quit-application-requested", "restart");
> >+    
> >+      if (cancelQuit.data == false) {
> >+        let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup);
> >+        appStartup.quit(Ci.nsIAppStartup.eRestart | Ci.nsIAppStartup.eForceQuit);
> >+        Services.prefs.setBoolPref("browser.sessionstore.resume_session_once", false);
> >+      }
> 
> I thought we didn't need this anymore?

Also meant to mention this. I left this in because I don't believe that downloaded language packs xpis are "restartless". At least, in a little limited testing they don't show up in our extensions view unless you restart the browser.

Comment 40

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

Comment on attachment 537622 [details] [diff] [review]
Patch v7

>diff --git a/mobile/chrome/content/localePicker.js b/mobile/chrome/content/localePicker.js

>+function resizeHandler() {
>+  let elements = document.getElementsByClassName("window-width");
>+  for (let i = 0; i < elements.length; i++) {
>+    elements[i].setAttribute("width", Math.min(800,window.innerWidth));
>+  }

nits:
No {} needed for the "for"

Math.min(800,window.innerWidth) -> Math.min(800 ,window.innerWidth)

>diff --git a/mobile/chrome/content/localePicker.xul b/mobile/chrome/content/localePicker.xul

I'll grumble about the camel cased element IDs in this file. 

>diff --git a/mobile/locales/en-US/chrome/localepicker.properties b/mobile/locales/en-US/chrome/localepicker.properties

>+chooseLanguage=Choose a language

This seems to be used as a title, so "Choose a Language" is better 

>diff --git a/mobile/themes/core/gingerbread/localePicker.css b/mobile/themes/core/gingerbread/localePicker.css

>+window {
>+  background-color: black;
>+}

Is this needed? ::root should handle this case

>+.link {
>+  color: @color_text_link@;
>+  text-decoration: underline;
>+}

"link" seems a bit too generic. Can we change to "text-as-link" ?

>+#installerPage {
>+  background-color: black;
>+  color: white;
>+}

Can you add /* force */ to this in both files? We do this in other places where we are not using the defines

  background-color: black; /* force */
  color: white; /* force */

Remember to make the changes in both files (core and core/gingerbread) as needed

r+ with nits fixed and we need to wait for the platform patch to land as well

Comment 41

[Wesley Johnston (:wesj)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/b42bd885a3d5 - instant switching
http://hg.mozilla.org/mozilla-central/rev/fc090d032d45 - localePicker

Comment 42

[Wesley Johnston (:wesj)]

This is failing some talos tests because we are not flipping the pref there. Patch coming to set the pref to false until the talos profiles can be updated.

Comment 43

[Wesley Johnston (:wesj)]

Attachment: Disable pref

Not sure if I need r+ or not for this.

Comment 44

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: disable firstrun for talos via prefs (1.0)

simple patch to modify the mobile profile used for all talos tests (n900 and android).

Comment 45

[Francesco Lodolo [:flod]]

> list.loading=Loading...

The default is to use the single unicode character "…" instead "..." (I know you're going to hate this, but if you fix it you need to update the key name).

Another question: I tried to read all comments but I couldn't find that information. Is it possible to test this feature in desktop builds? If that's not possible, an updated screenshot of the page would be much appreciated.

Comment 46

[Axel Hecht [:Pike]]

I agree on using hellip, I disagree on the entity change, the choice of the ellipsis is up to the localizer anyway.

For desktop, try installing some language packs, and then reset the firstrun prefs from http://hg.mozilla.org/mozilla-central/diff/fc090d032d45/mobile/app/mobile.js#l1.33. I could picture that working.

Comment 47

[Francesco Lodolo [:flod]]

(In reply to comment #46)
> I agree on using hellip, I disagree on the entity change, the choice of the
> ellipsis is up to the localizer anyway.

Ok, good to know
 
> For desktop, try installing some language packs, and then reset the firstrun
> prefs from
> http://hg.mozilla.org/mozilla-central/diff/fc090d032d45/mobile/app/mobile.
> js#l1.33. I could picture that working.

Thanks, I'll try with the next batch of builds (Gecko/20110607 7.0a1 still hasn't those keys).

Comment 48

[alice nodelman [:alice] [:anode]]

Comment on attachment 537870 [details] [diff] [review]
disable firstrun for talos via prefs (1.0)

This looks good to me, too.  I would like to see a note added to the prefs section of mobile.config indicating that this pref is set so that it is easier to track.  Also, if you ever want to run the dirty profile tests we'll need to find a new solution for this.

Comment 49

[Wesley Johnston (:wesj)]

(In reply to comment #45)
> > list.loading=Loading...
> 
> The default is to use the single unicode character "…" instead "..." (I know
> you're going to hate this, but if you fix it you need to update the key
> name).
> 
> Another question: I tried to read all comments but I couldn't find that
> information. Is it possible to test this feature in desktop builds? If
> that's not possible, an updated screenshot of the page would be much
> appreciated.

That's actually not (currently) used in the UI. I'll file a bug to remove it for now.

Comment 50

[Joel Maher ( :jmaher ) (UTC -8)]

talos patch landed:
http://hg.mozilla.org/build/talos/rev/1128691728d8

we need to wait for this to deploy.  Since this will affect Maemo, we need to get this in the official talos.zip which can take a while.

Comment 51

[Wesley Johnston (:wesj)]

Attachment: Turn on

Turn this back on. Try run had some weird purple and one Talos-orange (bug 650570):

http://tbpl.mozilla.org/?tree=Try&rev=ad732da3814a

Comment 52

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

Comment on attachment 538305 [details] [diff] [review]
Turn on

Now you need to work on a simple testing strategy

Comment 53

[Joel Maher ( :jmaher ) (UTC -8)]

I looked at the purple and oranges and there is nothing there that isn't an already known issue unrelated to this bug/patch.

Comment 54

[Mounir Lamouri (:mounir)]

http://hg.mozilla.org/mozilla-central/rev/1b0c76985611

Comment 55

[Aaron Train [:aaronmt]]

A couple of manual Litmus regression tests to start with

https://litmus.mozilla.org/show_test.cgi?&id=22913
https://litmus.mozilla.org/show_test.cgi?&id=22914
https://litmus.mozilla.org/show_test.cgi?&id=22915

Comment 56

[Aaron Train [:aaronmt]]

Verified fixed
Mozilla/5.0 (Android; Linux armv7l; rv:7.0a1) Gecko/20110614 Firefox/7.0a1 Fennec/7.0a1

Comment 57

[Jesper Kristensen]

In mobile/locales/en-US/chrome/localepicker.properties, what is the context of "name"? Can you add a localization note about that? Will the word be used on its own, in a list, as part of a sentence, as the first word in a sentence?