Check for locale updates before updating Fennec

RESOLVED FIXED in Firefox 10

Status

defect
P2
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

Trunk
Firefox 10

Firefox Tracking Flags

(firefox9 affected, firefox10 fixed)

Details

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

Attachments

(3 attachments, 2 obsolete attachments)

Assignee

Description

8 years ago
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
Assignee

Comment 1

8 years ago
I have a list of available locales (for nightly builds, built via a little extension) here:

http://people.mozilla.com/~wjohnston/nightly_locales.xml

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

Comment 3

8 years ago
Posted 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)
Assignee

Comment 4

8 years ago
Comment on attachment 552190 [details] [diff] [review]
Patch v1

Need to write the buildids correctly here so they match the ones at ftp://ftp.mozilla.org/pub/mobile/nightly/. Will update urlformatter to support that (via a %BUILDID_EXPANDED% string). Removing review for now.
Attachment #552190 - Flags: review?(mark.finkle)
Assignee

Comment 5

8 years ago
Updated to use smarter buildids.
Attachment #552190 - Attachment is obsolete: true
Attachment #552531 - Flags: review?(mark.finkle)
Assignee

Comment 6

8 years ago
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(aLocale.addon.id);
>+        if (index > -1) {
>+          aLocale.addon.install.install();
>+          aListener._installing.push(aLocale.addon.id);

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["@mozilla.org/xre/app-info;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+
http://hg.mozilla.org/mozilla-central/rev/4dad5acf5a1e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Assignee

Comment 13

8 years ago
Posted 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.
Assignee

Comment 14

8 years ago
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)
Assignee

Comment 15

8 years ago
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+
Assignee

Comment 17

8 years ago
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 19

8 years ago
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: https://hg.mozilla.org/mozilla-central/rev/5d06ccaa88f0

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
You need to log in before you can comment on or make changes to this bug.