Closed Bug 675254 Opened 9 years ago Closed 8 years ago

Check in BrowserCLH for locale problems

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 9

People

(Reporter: wesj, Assigned: wesj)

Details

(Whiteboard: [inbound][QA?])

Attachments

(1 file, 3 obsolete files)

For Fennec, we're initially worried about this as we move to single locale builds with other locales sent through AMO/(something else for nightly?). If, an updated locale can't be found before the update, when we restart Fennec will be set to either an incompatible (marked disabled?) locale, or potentially for nightly users, a just broken locale.

Our current potential solution to this:

1.) Check if the current fennec locale is actually our compatible locales list.
  1a.) If not, check AMO for an updated locale.
         If found, install it and restart in the new locale.
         If not, show the user the locale picker again? or just restart in en-US
  1b.)If it is compatible, try to open browser.xul.
      Check if the browser.xul window has "undefined entity" errors.
        If we find them, reset the locale to en-US. Restart.
Attached patch WIP Patch (obsolete) — Splinter Review
Starting out stuff here. Just comparing the currentLocale to the available ones. If we can't find one, we have a problem and I start the locale picker in order to:

1.) Update an app-disabled locale pack
2.) Reset the locale pref for user-disabled locales

Next step is to try opening browser.xul and attempt to determine if there are xml errors.
Assignee: nobody → wjohnston
Axel mentioned some potential issues with our approach here. This only checks browser.xul for issues, but we might have other files (dialogs) and properties files that are not caught by this type of check.
FYI: When we do an update, the add-on manager should check add-ons and disable any that are no longer compatible:

http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/browser-ui.js#558

I'm not sure if this happens before or after a commandline handler is run.
As Mark said, I don't see any promise in this attempt. It doesn't buy us safety, and is a bunch of work.

Also, as mentioned in the email reply I just sent, I don't see a list of compatible locales. We're dealing with a full matrix of language packs and builds, each by build ID, I guess.
For those cases when the addon should be marked incompatable, the addonManager (Service not the .jsm) does that in:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/addonManager.js#84

which is called by

http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#730

which I have been assured will fire before BrowserCLH. I am going to try and talk to Mossop today about using buildId to determine compatability (seems doable quickly? and removes a lot of these headaches. Nightly Locales would just mark themselves incompatible when you update, and then we can try to update them on startup.
Attached patch Add buildid to install.rdf (obsolete) — Splinter Review
This just adds the buildid to generated locale xpis.
Attached patch Patch v1 (obsolete) — Splinter Review
We've decided not to try the "open the window and see if its ok" approach anymore. This just implements the check during BrowserCLH to see if the user selected language is still compatible. If its not, we attempt to download it again. If that fails, we give them the locale picker (so they can choose a different language).
Attachment #550263 - Attachment is obsolete: true
Attachment #550558 - Attachment is obsolete: true
Attached patch Patch v2Splinter Review
Updated patch. I don't think a lot has changed, but just want to be sure.
Attachment #552250 - Attachment is obsolete: true
Attachment #552534 - Flags: review?(mark.finkle)
Comment on attachment 552534 [details] [diff] [review]
Patch v2

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


> function haveSystemLocale() {
>+  return false;

leftover debugging? or comment needed?

>         // Show the locale selector if we have a new profile
>-        if (needHomepageOverride() == "new profile" && Services.prefs.getBoolPref("browser.firstrun.show.localepicker") && !haveSystemLocale()) {
>+        if ((needHomepageOverride() == "new profile" &&
>+             Services.prefs.getBoolPref("browser.firstrun.show.localepicker") &&
>+             !haveSystemLocale()) ||
>+            !checkCurrentLocale()) {

This makes me cry. Can you put the Services.prefs.getBoolPref("browser.firstrun.show.localepicker") in a local variable and put this on one line?


r+ with those changes
Attachment #552534 - Flags: review?(mark.finkle) → review+
This relies pretty heavily on the patch in bug 668838. I'm going to wait for it before pushing this.
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/b47c8ec93ccb
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Whiteboard: [inbound] → [inbound][QA?]
You need to log in before you can comment on or make changes to this bug.