Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Check for locale updates before updating Fennec

RESOLVED FIXED in Firefox 10

Status

Fennec Graveyard
General
P2
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

Trunk
Firefox 10

Details

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

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

6 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

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

Comment 2

6 years ago
You'll need the full matrix of build IDs for both the app and the language packs, I think.
(Assignee)

Comment 3

6 years ago
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.
Assignee: nobody → wjohnston
Attachment #552190 - Flags: review?(mark.finkle)
(Assignee)

Comment 4

6 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

6 years ago
Created attachment 552531 [details] [diff] [review]
Patch v2 - Part 1/2

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

Comment 6

6 years ago
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.
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+
(Assignee)

Comment 8

6 years ago
Part 1 inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/083551a45a83
Whiteboard: [inbound]
Part 1 for Fennec 8: http://hg.mozilla.org/mozilla-central/rev/083551a45a83
Whiteboard: [inbound]
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+
(Assignee)

Comment 11

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/4dad5acf5a1e
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/4dad5acf5a1e
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
(Assignee)

Comment 13

6 years ago
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.
(Assignee)

Comment 14

6 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

6 years ago
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.
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

6 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?
(Assignee)

Comment 18

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

Comment 19

6 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.
status-firefox10: --- → fixed
status-firefox9: --- → affected
Whiteboard: [inbound] → [followup patch needs to land on Aurora]
Target Milestone: Firefox 9 → Firefox 10
(Assignee)

Comment 21

6 years ago
Pushed to aurora without the tests:
http://hg.mozilla.org/releases/mozilla-aurora/rev/d348ad4a966c
You need to log in before you can comment on or make changes to this bug.