Closed Bug 653141 Opened 13 years ago Closed 13 years ago

allow language choice on first-run

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(firefox7 fixed, fennec7+)

VERIFIED FIXED
Firefox 7
Tracking Status
firefox7 --- fixed
fennec 7+ ---

People

(Reporter: madhava, Assigned: wesj)

References

Details

(Keywords: uiwanted)

Attachments

(5 files, 12 obsolete files)

11.20 KB, application/zip
Details
3.53 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
44.34 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
619 bytes, patch
mfinkle
: review+
Details | Diff | Splinter Review
546 bytes, patch
mfinkle
: review+
Details | Diff | Splinter Review
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
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.
(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.
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.
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?
OS: Mac OS X → All
Hardware: x86 → All
(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 on attachment 531484 [details]
simple add-on with a locale helper

Some code that fetches add-ons (locales) and builds a simple display list
Attachment #531484 - Attachment description: simpl → simple add-on with a locale helper
Attached patch WIP (obsolete) — Splinter Review
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.
Assignee: nobody → wjohnston
Attachment #532099 - Flags: review?
Comment on attachment 532099 [details] [diff] [review]
WIP

so I rememeber
Attachment #532099 - Flags: review? → review?(mark.finkle)
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #532099 - Attachment is obsolete: true
Attachment #532099 - Flags: review?(mark.finkle)
Attached patch Patch Version (obsolete) — Splinter Review
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.
Attachment #532395 - Attachment is obsolete: true
Attachment #532708 - Flags: review?(mark.finkle)
Attached patch Build changes (obsolete) — Splinter Review
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?
Attachment #532709 - Flags: feedback?(mark.finkle)
Attached patch Patch v1.01 (obsolete) — Splinter Review
Whoops. Found a few css glitches while putting together screenshots. This should fix them.
Attachment #532714 - Flags: review?(mark.finkle)
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.
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 on attachment 532708 [details] [diff] [review]
Patch Version

I assume this patch is obsolete?
Attachment #532708 - Attachment is obsolete: true
Attachment #532708 - Flags: review?(mark.finkle)
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
Attachment #532714 - Flags: review?(mark.finkle) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #532714 - Attachment is obsolete: true
Attachment #533076 - Flags: review?(mark.finkle)
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.
(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...
Attached patch Patch v3 (obsolete) — Splinter Review
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.
Attachment #533076 - Attachment is obsolete: true
Attachment #533076 - Flags: review?(mark.finkle)
Attachment #533636 - Flags: review?(mark.finkle)
Attached patch Patch v4 (obsolete) — Splinter Review
Found some bugs while adding the preferences UI.
Attachment #533636 - Attachment is obsolete: true
Attachment #533636 - Flags: review?(mark.finkle)
Attachment #533866 - Flags: review?(mark.finkle)
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?
Attachment #533866 - Flags: review?(mark.finkle) → review+
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
Attached patch Patch v5 (obsolete) — Splinter Review
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.
Attachment #533866 - Attachment is obsolete: true
Attachment #534143 - Flags: review?(mark.finkle)
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?
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.
We may be able to use common pool for downloading localized strings in addition to those we ship. Gandalf?

More comments to come.
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.
Attachment #532709 - Flags: review-
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.
tracking-fennec: --- → 6+
(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 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.
Attachment #534143 - Flags: review?(mark.finkle) → review+
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.
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.
Attachment #534810 - Flags: review?(benjamin)
Attached patch Patch v6 (obsolete) — Splinter Review
This works with no restarts for already installed locales. For downloaded ones, we may require a restart (unless they can be made restartless xpis?).
Attachment #532709 - Attachment is obsolete: true
Attachment #534143 - Attachment is obsolete: true
Attachment #534609 - Attachment is obsolete: true
Attachment #532709 - Flags: feedback?(mark.finkle)
Attachment #535414 - Flags: review?(mark.finkle)
tracking-fennec: 6+ → 7+
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
Attachment #535414 - Flags: review?(mark.finkle) → review-
Attached patch Patch v7Splinter Review
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.
Attachment #535414 - Attachment is obsolete: true
Attachment #537622 - Flags: review?(mark.finkle)
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.
(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 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
Attachment #537622 - Flags: review?(mark.finkle) → review+
Attachment #534810 - Flags: review?(benjamin) → review+
Pushed:
http://hg.mozilla.org/mozilla-central/rev/b42bd885a3d5 - instant switching
http://hg.mozilla.org/mozilla-central/rev/fc090d032d45 - localePicker
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: in-litmus?
Depends on: 662623
Target Milestone: --- → Firefox 7
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.
Attached patch Disable prefSplinter Review
Not sure if I need r+ or not for this.
Attachment #537865 - Flags: review?(mark.finkle)
Attachment #537865 - Flags: review?(mark.finkle) → review+
simple patch to modify the mobile profile used for all talos tests (n900 and android).
Attachment #537870 - Flags: review?(wjohnston)
Attachment #537870 - Flags: feedback?(anodelman)
Attachment #537870 - Flags: review?(wjohnston) → review+
> 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.
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.
(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).
Flags: in-litmus? → in-litmus?(aaron.train)
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.
Attachment #537870 - Flags: feedback?(anodelman) → feedback+
(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.
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.
Depends on: 662905
Attached patch Turn onSplinter Review
Turn this back on. Try run had some weird purple and one Talos-orange (bug 650570):

http://tbpl.mozilla.org/?tree=Try&rev=ad732da3814a
Attachment #537870 - Attachment is obsolete: true
Attachment #538305 - Flags: review?(mark.finkle)
Comment on attachment 538305 [details] [diff] [review]
Turn on

Now you need to work on a simple testing strategy
Attachment #538305 - Flags: review?(mark.finkle) → review+
I looked at the purple and oranges and there is nothing there that isn't an already known issue unrelated to this bug/patch.
Whiteboard: [inbound]
Whiteboard: [inbound]
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
Flags: in-litmus?(aaron.train) → in-litmus+
Verified fixed
Mozilla/5.0 (Android; Linux armv7l; rv:7.0a1) Gecko/20110614 Firefox/7.0a1 Fennec/7.0a1
Status: RESOLVED → VERIFIED
Depends on: 665523
Depends on: 665526
Depends on: 666357
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?
You need to log in before you can comment on or make changes to this bug.