Last Comment Bug 675252 - Check for locale updates before updating Fennec
: Check for locale updates before updating Fennec
Status: RESOLVED FIXED
[followup patch needs to land on Aurora]
:
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: Firefox 10
Assigned To: Wesley Johnston (:wesj)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-29 10:36 PDT by Wesley Johnston (:wesj)
Modified: 2011-11-07 14:56 PST (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 (4.30 KB, patch)
2011-08-10 12:59 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Review
Patch v2 - Part 1/2 (5.38 KB, patch)
2011-08-11 15:53 PDT, Wesley Johnston (:wesj)
mark.finkle: review+
Details | Diff | Review
Patch v2 - Part 2/2 (2.77 KB, patch)
2011-08-11 15:57 PDT, Wesley Johnston (:wesj)
mark.finkle: review+
Details | Diff | Review
Use build id - tests and fix (2.97 KB, patch)
2011-09-28 15:56 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Review
Use build id - tests and fix (4.64 KB, patch)
2011-09-28 16:01 PDT, Wesley Johnston (:wesj)
mark.finkle: review+
christian: approval‑mozilla‑aurora+
Details | Diff | Review

Description Wesley Johnston (:wesj) 2011-07-29 10:36:04 PDT
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) 2011-08-01 18:19:59 PDT
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] 2011-08-03 04:23:05 PDT
You'll need the full matrix of build IDs for both the app and the language packs, I think.
Comment 3 Wesley Johnston (:wesj) 2011-08-10 12:59:36 PDT
Created attachment 552190 [details] [diff] [review]
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) 2011-08-11 08:44:05 PDT
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) 2011-08-11 15:53:26 PDT
Created attachment 552531 [details] [diff] [review]
Patch v2 - Part 1/2

Updated to use smarter buildids.
Comment 6 Wesley Johnston (:wesj) 2011-08-11 15:57:32 PDT
Created attachment 552533 [details] [diff] [review]
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?) 2011-08-12 12:18:51 PDT
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) 2011-08-15 10:51:10 PDT
Part 1 inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/083551a45a83
Comment 9 :Ms2ger 2011-08-16 03:16:55 PDT
Part 1 for Fennec 8: http://hg.mozilla.org/mozilla-central/rev/083551a45a83
Comment 10 Mark Finkle (:mfinkle) (use needinfo?) 2011-08-27 15:02:53 PDT
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 12 Marco Bonardo [::mak] 2011-09-03 02:58:43 PDT
http://hg.mozilla.org/mozilla-central/rev/4dad5acf5a1e
Comment 13 Wesley Johnston (:wesj) 2011-09-28 15:56:07 PDT
Created attachment 563211 [details] [diff] [review]
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) 2011-09-28 15:56:50 PDT
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) 2011-09-28 16:01:20 PDT
Created attachment 563215 [details] [diff] [review]
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?) 2011-09-28 17:55:04 PDT
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) 2011-09-29 12:15:20 PDT
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) 2011-09-29 12:19:52 PDT
tests - http://hg.mozilla.org/integration/mozilla-inbound/rev/5d06ccaa88f0
Comment 19 christian 2011-09-29 14:29:39 PDT
Comment on attachment 563215 [details] [diff] [review]
Use build id - tests and fix

Approved for mozilla-aurora
Comment 20 Matt Brubeck (:mbrubeck) 2011-09-29 20:46:58 PDT
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) 2011-11-07 14:56:26 PST
Pushed to aurora without the tests:
http://hg.mozilla.org/releases/mozilla-aurora/rev/d348ad4a966c

Note You need to log in before you can comment on or make changes to this bug.