675252 - Check for locale updates before updating Fennec

Status: RESOLVED FIXED

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

Description

[Wesley Johnston (:wesj)]

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.

Comment 1

[Wesley Johnston (:wesj)]

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.

Comment 2

[Axel Hecht [:Pike]]

You'll need the full matrix of build IDs for both the app and the language packs, I think.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch v1

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.

Comment 4

[Wesley Johnston (:wesj)]

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.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch v2 - Part 1/2

Updated to use smarter buildids.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch v2 - Part 2/2

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.

Comment 7

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

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

Comment 8

[Wesley Johnston (:wesj)]

Part 1 inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/083551a45a83

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Part 1 for Fennec 8: http://hg.mozilla.org/mozilla-central/rev/083551a45a83

Comment 10

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

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

Comment 11

[Wesley Johnston (:wesj)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/4dad5acf5a1e

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/4dad5acf5a1e

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Use build id - tests and fix

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 14

[Wesley Johnston (:wesj)]

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.

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Use build id - tests and fix

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.

Comment 16

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

Comment on attachment 563215 [details] [diff] [review]
Use build id - tests and fix

This looks like we need it on Aurora too

Comment 17

[Wesley Johnston (:wesj)]

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.

Comment 18

[Wesley Johnston (:wesj)]

tests - http://hg.mozilla.org/integration/mozilla-inbound/rev/5d06ccaa88f0

Comment 19

[LegNeato]

Comment on attachment 563215 [details] [diff] [review]
Use build id - tests and fix

Approved for mozilla-aurora

Comment 20

[Matt Brubeck (:mbrubeck)]

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.

Comment 21

[Wesley Johnston (:wesj)]

Pushed to aurora without the tests:
http://hg.mozilla.org/releases/mozilla-aurora/rev/d348ad4a966c