675254 - Check in BrowserCLH for locale problems

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

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.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

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.

Comment 2

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

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.

Comment 3

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

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.

Comment 4

[Axel Hecht [:Pike]]

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.

Comment 5

[Wesley Johnston (:wesj)]

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.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Add buildid to install.rdf

This just adds the buildid to generated locale xpis.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch v1

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).

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Updated patch. I don't think a lot has changed, but just want to be sure.

Comment 9

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

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

Comment 10

[Wesley Johnston (:wesj)]

This relies pretty heavily on the patch in bug 668838. I'm going to wait for it before pushing this.

Comment 11

[Wesley Johnston (:wesj)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/b47c8ec93ccb

Comment 12

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/b47c8ec93ccb