Closed Bug 675252 Opened 12 years ago Closed 12 years ago

Check for locale updates before updating Fennec


(Firefox for Android Graveyard :: General, defect, P2)



(firefox9 affected, firefox10 fixed)

Firefox 10
Tracking Status
firefox9 --- affected
firefox10 --- fixed


(Reporter: wesj, Assigned: wesj)


(Whiteboard: [followup patch needs to land on Aurora])


(3 files, 2 obsolete files)

In the future, with single-locale builds of Fennec, extra locales will be installed using extensions. If a nightly user (or a normal user for that matter?) upgrades from day to day, their installed locale extension may no longer have the strings they need.

We should check if a new version of the locale is available before updating Fennec. Note this will likely have problems with updates coming through the Android market. I'm going to file a separate bug with a separate plan for handling those.
Priority: -- → P2
I have a list of available locales (for nightly builds, built via a little extension) here:

I'm still not exactly sure how nightly builds are going to know from day to day to update these. I think I may just update your locale regardless, whenever we update the app.
You'll need the full matrix of build IDs for both the app and the language packs, I think.
Attached patch Patch v1 (obsolete) — Splinter Review
This marks all locales as incompatible when an update occurs, and then uses the LocaleRepository to download versions that are compatible with the new buildid.
Assignee: nobody → wjohnston
Attachment #552190 - Flags: review?(mark.finkle)
Comment on attachment 552190 [details] [diff] [review]
Patch v1

Need to write the buildids correctly here so they match the ones at Will update urlformatter to support that (via a %BUILDID_EXPANDED% string). Removing review for now.
Attachment #552190 - Flags: review?(mark.finkle)
Updated to use smarter buildids.
Attachment #552190 - Attachment is obsolete: true
Attachment #552531 - Flags: review?(mark.finkle)
This stores the buildid in a pref. My worry here is that a nightly user updates by downloading and installing a new build on their own. In that case, the old locales will still be marked compatible and will likely break Fennec. This forces them through the locale picker once. If we have their language, it will likely just be redownloaded and installed. If we don't, they could potentially choose to continue in an incompatible locale.

Ideally we'd just mark everything incompatible when this happens, but those APIs are async and don't want to play nicely with me. There could also be situations where the user has 6 locales installed, and we would only update the active one. This  just catches the general case.
Attachment #552533 - Flags: review?(mark.finkle)
Comment on attachment 552531 [details] [diff] [review]
Patch v2 - Part 1/2

>+  _showDownloadedNotification: function UP_showDlNotification(aUpdate) {

>-    this._showNotification(aUpdate, title, text, imageUrl, "downloaded");
>+    this._showNotification(aUpdate, title, text, imageUrl, "installed");

What's the affect of this change? 

>+  _reinstallLocales: function UP_reinstallLocales(aUpdate, aListener, aPending) {
>+    LocaleRepository.getLocales((function(aLocales) {
>+      aLocales.forEach(function(aLocale, aIndex, aArray) {
>+        let index = aPending.indexOf(;
>+        if (index > -1) {
>+          aLocale.addon.install.install();
>+          aListener._installing.push(;

Should you put the addon in aListener first?

>diff --git a/mobile/modules/LocaleRepository.jsm b/mobile/modules/LocaleRepository.jsm

>+    if (aFilters) {
>+      if (aFilters.buildID)
>+        url = url.substring(0,4) +
>+              "-" +
>+              url.substring(4).replace(/\d{2}(?=\d)/g, "$&-");
>+    }

Let's just do the %BUILDID_EXPANDED% search and replace here. No need to update URLFormatter. Other code has custom replacements too. Not a big worry, especially since BUILDID_EXPANDED is a custom one-off

Make sure BUILID_EXPANDED can be replaced anywhere in the string

>diff --git a/toolkit/components/urlformatter/nsURLFormatter.js b/toolkit/components/urlformatter/nsURLFormatter.js

Skip this. Just do it in LocalesRepo

This is a sizable change and we need to make sure QA can test this on Nightly and Aurora. Can you put together a "how to test this" tutorial soon?

r+ with nits fixed
Attachment #552531 - Flags: review?(mark.finkle) → review+
Comment on attachment 552533 [details] [diff] [review]
Patch v2 - Part 2/2

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

>   closeWindow : function(aForceRestart) {
>+    var buildID =  Cc[";1"].getService(Ci.nsIXULAppInfo).platformBuildID;
>+    Services.prefs.setCharPref("extensions.compatability.locales.buildid", buildID);
>     // Trying to close this window and open a new one results in a corrupt UI.

* var buildID = Cc  (remove the extra space before Cc
* Add a blank line before comment

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

> function checkCurrentLocale() {
>   if (Services.prefs.prefHasUserValue("general.useragent.locale")) {
>+    // if the user has a compatable locale from a different buildid, we need to update

(spelling error) -> compatible

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

>       }, this);
>+      // store the buildid of these locales so that we can disable locales when the
>+      // user updates through a non-updater channel

Add a blank line before the comment
Attachment #552533 - Flags: review?(mark.finkle) → review+
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Attached patch Use build id - tests and fix (obsolete) — Splinter Review
This tests that the buildid filter and %EXPANDED_BUILDID% in the url are handled correctly. Applies on top of my other tests in bug 689702.
Comment on attachment 563211 [details] [diff] [review]
Use build id - tests and fix

Sorry if I'm innundating you mark. After the first one, these seem pretty simple to review though.
Attachment #563211 - Flags: review?(mark.finkle)
Crap forgot to hg add a file. Also should mention I found a typo while writing. So yay for tests catching bugs! Boo for me making a typo.
Attachment #563211 - Attachment is obsolete: true
Attachment #563211 - Flags: review?(mark.finkle)
Attachment #563215 - Flags: review?(mark.finkle)
Comment on attachment 563215 [details] [diff] [review]
Use build id - tests and fix

This looks like we need it on Aurora too
Attachment #563215 - Flags: review?(mark.finkle) → review+
Comment on attachment 563215 [details] [diff] [review]
Use build id - tests and fix

Nominating for aurora.

This fixes a bug for a feature that is currently preffed off, and adds a test for it. We'd like to get testers on this, and if bug 678111 is fixed we can start pointing them at a more official repository. At that point, they'll run into this problem.
Attachment #563215 - Flags: approval-mozilla-aurora?
Comment on attachment 563215 [details] [diff] [review]
Use build id - tests and fix

Approved for mozilla-aurora
Attachment #563215 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Follow-up landed on trunk for Firefox 10:

The follow-up still needs to land on Aurora for Firefox 9.
Whiteboard: [inbound] → [followup patch needs to land on Aurora]
Target Milestone: Firefox 9 → Firefox 10
Pushed to aurora without the tests:
You need to log in before you can comment on or make changes to this bug.