Last Comment Bug 329729 - (majorupdate) software updater should handle minor and major updates
(majorupdate)
: software updater should handle minor and major updates
Status: RESOLVED FIXED
: fixed1.8.0.8, fixed1.8.1, late-l10n
Product: Toolkit
Classification: Components
Component: Application Update (show other bugs)
: 1.8.0 Branch
: All All
: -- enhancement with 1 vote (vote)
: mozilla1.8.1beta2
Assigned To: (not reading, please use seth@sspitzer.org instead)
:
:
Mentors:
Depends on: 453452
Blocks: 348363 348389 349579 349581 349590 349594 349597 349600 350636 351821 352400 352675 356220 356221 356223 357143 357145
  Show dependency treegraph
 
Reported: 2006-03-07 23:21 PST by Darin Fisher
Modified: 2011-09-16 13:33 PDT (History)
38 users (show)
mconnor: blocking‑firefox2+
dveditz: blocking1.8.0.7-
dveditz: blocking1.8.0.8+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
screen shot of poor man's major update (no string changes, but not ideal) (26.72 KB, image/png)
2006-08-03 18:42 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
patch, rough draft, will list open issue... (7.27 KB, patch)
2006-08-03 18:57 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details | Diff | Splinter Review
screen shot of poor man's minor update (should look the same) (15.69 KB, image/png)
2006-08-03 18:58 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
need to find a new string for the "404 error" case (17.43 KB, image/png)
2006-08-03 19:31 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
need to to find a new string for the download... case (15.08 KB, image/png)
2006-08-03 19:32 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
moving xul around to support major updates an incompatibilty (a little tight on space) (27.09 KB, image/png)
2006-08-07 17:36 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
using new string for "404 error" case (15.92 KB, image/png)
2006-08-07 17:37 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
ew details download string (14.26 KB, image/png)
2006-08-07 23:31 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
screen shot per beltzner's ascii art (for the 2.0 branch) (26.56 KB, image/png)
2006-08-08 02:59 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
updated patch, need to decide on strings (27.96 KB, patch)
2006-08-08 03:03 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details | Diff | Splinter Review
screen shot of a taller wizard, per beltzner (31.88 KB, image/png)
2006-08-09 17:42 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
major update with incompat (29.78 KB, image/png)
2006-08-09 17:43 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
minor update with incompat (19.23 KB, image/png)
2006-08-09 17:44 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
update download (14.69 KB, image/png)
2006-08-09 17:46 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
update failure (16.96 KB, image/png)
2006-08-09 17:49 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
updated patch (32.59 KB, patch)
2006-08-09 18:03 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details | Diff | Splinter Review
new text, per reed - beltzner (27.56 KB, image/png)
2006-08-09 23:18 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
screen shot of license downloading formatted string (16.99 KB, image/png)
2006-08-10 17:28 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
license not found screen shot (17.77 KB, image/png)
2006-08-10 17:36 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
the string changes only (2.84 KB, patch)
2006-08-10 18:16 PDT, (not reading, please use seth@sspitzer.org instead)
mbeltzner: ui‑review+
Details | Diff | Splinter Review
updated patch, still some more functionality to implement. (32.35 KB, patch)
2006-08-10 18:23 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details | Diff | Splinter Review
same set of new strings, but not removing any strings (yet) (2.87 KB, patch)
2006-08-11 10:50 PDT, (not reading, please use seth@sspitzer.org instead)
moco: ui‑review+
mbeltzner: approval1.8.1+
Details | Diff | Splinter Review
adding support for licenseURL attribute, the new strings and the UI are pending review in bug #329729 (40.92 KB, patch)
2006-08-15 15:42 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details | Diff | Splinter Review
improvements for licenseURL support, only enable Next button from EULA page when the EULA is downloaded and the user accepts it (39.01 KB, patch)
2006-08-16 07:45 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details | Diff | Splinter Review
updated patch, includes support for "never" ask again, more work to support isSecurityUpgrade attribute and licenseURL attribute (47.53 KB, patch)
2006-08-16 11:47 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details | Diff | Splinter Review
screen shot minor update where isSecurityUpdate is true in updates.xml (17.97 KB, image/png)
2006-08-16 11:59 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
screen shot minor update where isSecurityUpdate is "false" (or not set) in updates.xml (15.26 KB, image/png)
2006-08-16 12:05 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
screen shot of minor update (isSecurityUpdate="true" or not, they are the same) (15.34 KB, image/png)
2006-08-16 14:24 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
just the new strings (with dead strings removed, as this will be going on trunk and branch) (4.99 KB, patch)
2006-08-16 16:15 PDT, (not reading, please use seth@sspitzer.org instead)
mbeltzner: ui‑review+
Details | Diff | Splinter Review
patch (51.98 KB, patch)
2006-08-16 17:28 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details | Diff | Splinter Review
updated patch per eula behavior specified by beltzner (51.18 KB, patch)
2006-08-16 18:47 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details | Diff | Splinter Review
same patch, but no strings changes, as they have landed already (43.37 KB, patch)
2006-08-16 20:36 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details | Diff | Splinter Review
mac screen shot #1 (eula) (93.50 KB, image/tiff)
2006-08-16 21:10 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
mac screen shot #2 (downloading eula) (49.35 KB, image/tiff)
2006-08-16 21:12 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
mac screen shot #3 (eula not found) (53.35 KB, image/tiff)
2006-08-16 21:13 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
mac screen shot #4 (major update with incompatibilities) (59.61 KB, image/tiff)
2006-08-16 21:15 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
mac screen shot #5 (major version avail) (61.99 KB, image/tiff)
2006-08-16 21:17 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
mac screen shot #6 (minor version avail) (41.74 KB, image/tiff)
2006-08-16 21:29 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
mac screen shot #3a (eula not found) but with an icon from winstripe (55.22 KB, image/tiff)
2006-08-16 21:39 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
supplimental patch to fix the "not found" icon (uses notfound icon from winstripe) (1003 bytes, patch)
2006-08-16 21:41 PDT, (not reading, please use seth@sspitzer.org instead)
mbeltzner: ui‑review+
Details | Diff | Splinter Review
trunk version of the same patch (updates.js had some changes that needed to be backported) (43.32 KB, patch)
2006-08-17 10:03 PDT, (not reading, please use seth@sspitzer.org instead)
bugs: review+
Details | Diff | Splinter Review
branch patch, also includes mac jar.mn change for notfound.png (49.48 KB, patch)
2006-08-17 14:58 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details | Diff | Splinter Review
branch patch, also includes mac jar.mn change for notfound.png (47.67 KB, patch)
2006-08-17 14:59 PDT, (not reading, please use seth@sspitzer.org instead)
bugs: review+
Details | Diff | Splinter Review
trunk version of the latest branch patch (44.17 KB, patch)
2006-08-17 15:01 PDT, (not reading, please use seth@sspitzer.org instead)
bugs: review+
Details | Diff | Splinter Review
branch patch, with type="content" on the xul:brower, and using a different method for loading the content (and detecting errors) (50.48 KB, patch)
2006-08-17 21:18 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details | Diff | Splinter Review
trunk patch, with type="content" on the xul:brower, and using a different method for loading the content (and detecting errors) (47.24 KB, patch)
2006-08-17 21:19 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details | Diff | Splinter Review
branch patch, with type="content" on the xul:brower, and using a different method for loading the content (and detecting errors) (50.46 KB, patch)
2006-08-17 21:36 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details | Diff | Splinter Review
branch patch, with type="content" on the xul:brower, and using a different method for loading the content (and detecting errors) (47.22 KB, patch)
2006-08-17 21:37 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details | Diff | Splinter Review
branch patch, with type="content" on the xul:browser, and using a different method for loading the content (and detecting errors) (50.51 KB, patch)
2006-08-17 21:53 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details | Diff | Splinter Review
trunk patch, with type="content" on the xul:browser, and using a different method for loading the content (and detecting errors) (47.28 KB, patch)
2006-08-17 21:54 PDT, (not reading, please use seth@sspitzer.org instead)
cbiesinger: review-
Details | Diff | Splinter Review
branch patch, with biesi's suggestions, and using a field for the listener so that there is a reference to it, and so that I can call removeListener() in the dtor (50.98 KB, patch)
2006-08-18 08:31 PDT, (not reading, please use seth@sspitzer.org instead)
cbiesinger: review+
dveditz: review+
mtschrep: approval1.8.1+
Details | Diff | Splinter Review
trunk patch, with biesi's suggestions, and using a field for the listener so that there is a reference to it, and so that I can call removeListener() in the dtor (47.73 KB, patch)
2006-08-18 08:32 PDT, (not reading, please use seth@sspitzer.org instead)
darin.moz: review+
Details | Diff | Splinter Review
screen shot, details content to www.mozilla.com (not injected as innerHTML, but loaded as src on the xul:browser) (28.44 KB, image/png)
2006-08-18 08:42 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
screen shot of a .txt eula (http://www.mozilla.com/legal/eula/firefox-en.txt) (not injected as innerHTML, but loaded as src on the xul:browser) (20.66 KB, image/png)
2006-08-18 08:43 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
smoke test successful, installed 8-19 trunk (with my fix) update screen shot (64.29 KB, image/png)
2006-08-20 16:22 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
smoke test successful, sw update upgraded to 8-20 trunk (with my fix) update screen shot (60.06 KB, image/png)
2006-08-20 16:23 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
screen shot of trunk (with my fix) fed an updates.xml (using app.update.url.override pref) to a major update (72.76 KB, image/png)
2006-08-20 16:25 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
backport patch (draft, v1) (63.62 KB, patch)
2006-10-04 16:54 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details | Diff | Splinter Review
how the EULA page will look in 1.5.0.x (63.71 KB, image/jpeg)
2006-10-06 11:22 PDT, (not reading, please use seth@sspitzer.org instead)
mbeltzner: ui‑review+
Details
how the EULA page will look in 2.0.x (65.13 KB, image/jpeg)
2006-10-06 11:25 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
backport patch (draft, v2) (61.97 KB, patch)
2006-10-06 11:32 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details | Diff | Splinter Review
backport patch, v3 (65.88 KB, patch)
2006-10-06 13:14 PDT, (not reading, please use seth@sspitzer.org instead)
dveditz: review+
dveditz: approval1.8.0.8+
Details | Diff | Splinter Review
screen shot of build (with patch v3) where all the new strings are missing. (52.45 KB, image/jpeg)
2006-10-06 13:16 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
screen shot of eula page on windows xp from the 1.5.0.x branch with this patch (25.51 KB, image/png)
2006-10-09 10:02 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
screen shot with no label and no separator (for cbeard) (25.93 KB, image/png)
2006-10-09 10:03 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details
backported strings for locales, draft 1 (74.80 KB, patch)
2006-10-10 10:27 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details | Diff | Splinter Review
patch (using native2ascii in my script) (109.18 KB, patch)
2006-10-10 11:01 PDT, (not reading, please use seth@sspitzer.org instead)
no flags Details | Diff | Splinter Review
patch (covers 37/40 locales, 3 do not have strings to backport from the 1.8 branch) (131.25 KB, patch)
2006-10-10 11:26 PDT, (not reading, please use seth@sspitzer.org instead)
l10n: review-
Details | Diff | Splinter Review
patch (covers 37/40 locales, 3 do not have strings for are getting the en-US version, per axel) (139.28 KB, patch)
2006-10-10 15:40 PDT, (not reading, please use seth@sspitzer.org instead)
l10n: review-
Details | Diff | Splinter Review
patch, danish is fixed but still missing 3 locales, so using english (I have "he" in another bug) (92.90 KB, patch)
2006-10-11 17:42 PDT, (not reading, please use seth@sspitzer.org instead)
l10n: review+
dveditz: approval1.8.0.8+
Details | Diff | Splinter Review
supplimental patch for he locale (1.14 KB, patch)
2006-10-12 20:47 PDT, (not reading, please use seth@sspitzer.org instead)
l10n: review+
Details | Diff | Splinter Review
supplimental patch for he locale, v2 (based on new patch in bug #356221) (47.05 KB, patch)
2006-10-16 13:23 PDT, (not reading, please use seth@sspitzer.org instead)
l10n: review+
dveditz: approval1.8.0.8+
Details | Diff | Splinter Review
complete l10n patch (92.98 KB, patch)
2006-10-18 00:20 PDT, (not reading, please use seth@sspitzer.org instead)
moco: review+
dveditz: approval1.8.0.8+
Details | Diff | Splinter Review

Description Darin Fisher 2006-03-07 23:21:15 PST
Offer FF1.5 users security updates or FF2

Right now, if I offer FF1.5 an update XML file that contains a minor and a major update, FF1.5 will prompt the user to install the major update.  It will not give the user the choice to stick with the minor update (i.e., to stick with the FF1.5 product).  This seems like a problem to me as we will undoubtably keep updating FF1.5 with stability and security bug fixes well after FF2 ships.  Morever, we don't want to force FF2 on people as it might mean that they have to lose out on some extension that they really care about.  At some point, we will end-of-life FF1.5, but until that time, we need to have a good way to offer FF1.5 users both update paths.

I think the update XML file format is expressive enough for this situation (with its minor and major update types), so we just need to develop some UI for this "fork in the road" situation.
Comment 1 Axel Hecht [:Pike] 2006-03-08 01:10:00 PST
UI for 1.5 means localizable strings, sadly.
I wonder if we want to have that UI in l10n, and how to communicate that. I'm
not sure that shipping an english UI for this decision is the best way, we may
want to try to fix that for as many locales as possible.
Comment 2 Mike Beltzner [:beltzner, not reading bugmail] 2006-03-08 08:06:43 PST
Are we sure that we want the migration from 1.5 -> 2.0 to come through AUS? We didn't do that with 1.0.x -> 1.5, opting instead to ask users to make an explicit choice to upgrade.

We don't really have a policy on this, it occurs to me. Maybe the ability to upgrade from major-release to major-release should be a new feature for Firefox 2.0?
Comment 3 Benjamin Smedberg [:bsmedberg] 2006-03-08 18:35:05 PST
I am sure we *want* to (practicality aside): we've shown that users are much more likely to upgrade if we make it easy, and AUS provides more information about the upgrade (such as which existing extensions will become incompatible).

This feature was in the original plan for FF1.5 and it apparently got dropped (although I think Darin and I both thought that it had been included). Let's get a good estimation of the costs (l10n, QA) before making a branch decision.
Comment 4 Daniel Veditz [:dveditz] 2006-04-04 11:29:11 PDT
It's highly unlikely this would ever get accepted for a 1.5.0.x release given 
 1) the unavoidable l10n impact
 2) it's new functionality not a stability fix
[3) assigned to "nobody" isn't a good sign]

Marking blocking FF2 though -- we don't want to have this conversation again for FF2.0.1, and FF3 will be coming soon enough after that.
Comment 5 Mike Connor [:mconnor] 2006-04-05 05:18:41 PDT
(In reply to comment #4)
> It's highly unlikely this would ever get accepted for a 1.5.0.x release given 
>  1) the unavoidable l10n impact
>  2) it's new functionality not a stability fix
> [3) assigned to "nobody" isn't a good sign]
> 
> Marking blocking FF2 though -- we don't want to have this conversation again
> for FF2.0.1, and FF3 will be coming soon enough after that.

We need to discuss this more from a 1.5.0.x perspective.  Yes, we have frozen l10n right now, but this seems important enough to push into the 1.5.0.x stream before Fx2 hits, especially if we adopt a more aggressive timeline for retiring releases.  If we plan for it, we should be able to get l10n strings from enough locales in time for 1.5.0.4, if not 1.5.0.3.
Comment 6 Axel Hecht [:Pike] 2006-04-05 05:31:54 PDT
Taking l10n changes on the 1.8.0 branch requires not only l10n resources, but
also build and QA. We then need a commitment to keep the 1.8.0 tinderboxens
running and pushing builds to ftp for testing.
I'd say we should get the fix onto the branch on the first week (2nd latest)
including a test szenario for both l10n and QA, suitable for the pushed 
nightlies, likely a litmus test.

We need to agree on a approval scheme for the l10n repository before that, too.
I'm tempted to open the tree to fixes for that change without individually 
approving the patches.

If we end up with locales that didn't check in a fix, we could add the en-US
strings shortly before bringing up the release bits.
Comment 7 Mike Beltzner [:beltzner, not reading bugmail] 2006-04-05 19:27:37 PDT
Currently when a user gets their product updated they get bounced off to a URL which redirects them to a page of our choosing (see bug 324499 and bug 329945). We could plausibly create a separate update channel for Fx2, and those users who are on Fx1.5 who have not manually upgraded to the new product could be bounced to a page of our design urging them to do so.

It's a workaround, but it's a server-side workaround which eases dveditz's fears of implementation woes, and allows us to broadcast in a pretty aggressive way that users should upgrade.

(Axel: we'd want to l10n these pages, and can redirect people based on their user agent string, if I understand things correctly)
Comment 8 Darin Fisher 2006-04-05 20:38:21 PDT
The first-run homepage sounds like a pretty good fall-back solution to me!
Comment 9 Chris Beard 2006-06-05 18:32:18 PDT
We're re-opening the debate here as I think we really need to have the ability to advertise and strongly encourage users to upgrade to Firefox 2 once it is released, through the 1.5 UI.  

Doing so will provide the best possible user experience and ensure the highest possible take rate on the upgrade. 

Ideally I would like to have a window open with hosted Web content as part of the AUS client code that can be styled (with a consumer friendly and compelling design to advertise the value propositions of the new release) and with options like "Upgrade to Firefox 2", "Remind me Later" and "Don't Ask me Again". (I'm sure that beltzner has ideas for making this an even better user experience, but this is how I'm seeing things now in my ideal world.)

I know that this is against our posted policy about dot releases not introducing UI and l10n changes.  But given our product lifecycle and also our ability to keep people as safe as possible, being aggressive to convince people to switch to the Fx2 update channel should be a high priority.
Comment 10 Darin Fisher 2006-06-05 20:19:18 PDT
If you make the page hosted, then the l10n problem gets nicely punted to the website where you can maybe phase in localized campaigns.  The challenge from a technical POV is  empowering web content to essentially decide whether or not to upgrade the user to FF2.  Doing that in a safe way that cannot be exploited may be tricky.
Comment 11 Mike Beltzner [:beltzner, not reading bugmail] 2006-06-07 00:07:28 PDT
(Missed 1.5.0.5, but we should have something settled for 1.5.0.6.)

How about:
 - adding code to execute different paths for major/minor updates
 - adding a path for major updates that launches a chrome window with different buttons (small l10n hit unless we can find reusable labels) and which pulls content from a specific domain (security implications?)

That way we get the benefit of it being chrome, can have it do subsequent nag warnings if we want, and still decouple most of the UI/l10n hit from the code change to 1.5.x
Comment 12 Daniel Veditz [:dveditz] 2006-06-07 06:18:31 PDT
> If you make the page hosted, then the l10n problem gets nicely punted to the
> website where you can maybe phase in localized campaigns.

No, you can't really phase it in. When we flip the switch on the release people are largely updated within a few days. Any work we do after that wouldn't benefit many.

IMO there's no urgent need for this to be done before late 2.0 betas or even final -- we do promise to support 1.5 for 6-months after 2.0 ships. Get the feature done and localized in 2.0, and then we can steal the localization for a later 1.5.0.x release.

That might be a better plan even apart from localization: let the eager-beavers upgrade right away and don't push it on the general public until most extensions are working with the new release (some extension authors are slow to upgrade). Maybe even wait for 2.0.1 so these folks aren't hit with potential regressions.
Comment 13 Mike Beltzner [:beltzner, not reading bugmail] 2006-06-11 22:27:18 PDT
Here's what I propose. For products on the 1.8.0.x branch, we create a patch that presents the following dialog when a major update is found, even if the "Automatically Install Updates" button is checked (ie: just ignore that pref):

   .-----------------------------------------------------------.
   | Software Update                                           |
   |-----------------------------------------------------------|
   |                                                           |
   | Update Available                                          |
   |                                                           |
   | A new version of %prodName; is available!                 |
   | .-------------------------------------------------------. |
   | | this area is an iFrame                                | |
   | |                                                       | |
   | |                                                       | |
   | |                                                       | |
   | |                                                       | |
   | |                                                       | |
   | |                                                       | |
   | |                                                       | |
   | |                                                       | |
   | |                                                       | |
   | |                                                       | |
   | |                                                       | |
   | |                                                       | |
   | '-------------------------------------------------------' |
   |                                                           |
   | (Later)                       (Download & Install Now >>) |
   '-----------------------------------------------------------'

We can generate the URL (https seems useful) for the iFrame content based on the update name in the XML file and the locale of the browser. A separate bug can be used to create localized content that can populate this iFrame, and that content can be interactive, AJAX-y, have DHTML back/fwd buttons, etc. However, the control for downloading and installing is strictly in the purview of chrome.

If the user clicks "Later", and if a minor update was also available, then they would see the existing dialog for when a minor update is found, or no dialog at all if their preference is checked for an automatic download and install of minor updates (note: I'm not sure about this last bit - that might be disorienting, would appreciate some second-thought support!)

(For the 2.0 branch, since we're not restricted by the old UI, we could change the strings in the chrome to differentiate this new dialog a bit more from the standard "Update Available" dialog, perhaps even accepting strings from the XML file itself to render in the chrome dialog. I'm thinking that it would be more like:

   .-----------------------------------------------------------.
   | Software Update                                           |
   |-----------------------------------------------------------|
   |                                                           |
   | Upgrade to &prodName; &newVersionName; now!               |
   |                                                           |
   | .-------------------------------------------------------. |
   | | this area is an iFrame                                | |
   | |                                                       | |
   | |                                                       | |
   | |                                                       | |
   | |                                                       | |
   | |                                                       | |
   | |                                                       | |
   | |                                                       | |
   | |                                                       | |
   | |                                                       | |
   | |                                                       | |
   | |                                                       | |
   | |                                                       | |
   | '-------------------------------------------------------' |
   |                                                           |
   | (Later) (Never)                      (Get the upgrade >>) |
   '-----------------------------------------------------------'

This would like require a new bug targeted at the 2.0 branch)
Comment 14 Mike Beltzner [:beltzner, not reading bugmail] 2006-06-24 21:34:53 PDT
--> Firefox2 beta2, just to be realistic, but this will need heavy QA support.
Comment 15 Mike Connor [:mconnor] 2006-07-17 23:41:07 PDT
We might have to add this in a point release.
Comment 16 Benjamin Smedberg [:bsmedberg] 2006-07-18 04:58:37 PDT
Can we at least add the necessary strings now?
Comment 17 Axel Hecht [:Pike] 2006-07-19 03:11:01 PDT
'Strings now' is a high risk procedure. We have lots of unused strings in our codebase, and localizers do with that "whatever".
If we go down that route, that at least requires that we settle for the mockups, and that those get posted to .l10n together with a plan. Once we use the strings, we need to reach out for QA, and have a bustage fix cycle, too.
Comment 18 (not reading, please use seth@sspitzer.org instead) 2006-07-24 18:37:44 PDT
Is the ascii art in https://bugzilla.mozilla.org/show_bug.cgi?id=329729#c13 (for FF 2.0, not 1.5.0.x) final?

I have a couple questions:

1)  When do we present this nag dialog?  If they do "later", how long until we nag again?

2)  Should we check extensions so that the user knows that if they upgrade, things will break before they install?  (Darin and I aren't sure if this functionality exists yet, as currently the extension checking code uses the current app version.)

Darin and I met to discuss this bug last week, and we enumerated over half the problem (the easy half), which is:  "what do you do when there is both a minor and a major update?"

We convinced ourselves that in that scenario, the right solution (and the easy one) is to defer to the minor upgrade, and act like their isn't a major one.  Then, on next upgrade, we'd only have the major upgrade.  Handling a major upgrade is the "hard half" of the problem.

Schrep was asking me about how "at risk" this bug is for 2.0, and given the current unknowns, I'm going to say that it is currently very "at risk" for b2.
Comment 19 (not reading, please use seth@sspitzer.org instead) 2006-07-25 11:27:53 PDT
darin points out the 1.8.0 branch already has some strings:

http://lxr.mozilla.org/mozilla1.8.0/source/toolkit/locales/en-US/chrome/mozapps/update/updates.properties#2


  3 updateType_major=New Version
  4 updateType_minor=Security Update
  5 introType_minor=An important Security Update for %S is available:
  6 introType_major=A new version of %S is available:
Comment 20 (not reading, please use seth@sspitzer.org instead) 2006-07-31 17:08:55 PDT
about this comment:

>  5 introType_minor=An important Security Update for %S is available:
>  6 introType_major=A new version of %S is available:

Those strings are already used.  if the isSecurityUpdate attribute on an update is "true", the user will see the introType_minor string, instead of the introType_major.

from http://lxr.mozilla.org/mozilla1.8/source/toolkit/mozapps/update/content/updates.js#620

620     var severity = update.isSecurityUpdate ? "minor" : "major";
621     var displayType = gUpdates.strings.getString("updateType_" + severity);

when presented with a major and a minor, the code in selectUpdate() currently defers to the major:

1302     // If there's a major update, always try and fetch that one first, 
1303     // otherwise fall back to the newest minor update.
1304     return majorUpdate || minorUpdate;

(see http://lxr.mozilla.org/mozilla1.8/source/toolkit/mozapps/update/src/nsUpdateService.js.in#1304)

switching to:

+ // If there's a minor update, always try and fetch that one first, 
+ // otherwise use the newest major update.
+ return minorUpdate || majorUpdate;

would be a client side fix to achieve part of what has been discussed, which was: "We convinced ourselves that in that scenario, the right solution (and the easy one) is to defer to the minor upgrade, and act like their isn't a major one. Then, on next upgrade, we'd only have the major upgrade."

note, rob strong points out that for 1.5.0.x, the same effect could be achieved on the aus / server / build config side, by generating the update .xml files that only presented the major or the minor update (but not both) to 1.5.0.x users.  But, as he also points out, this would make the aus / server / build config / QA side more complicated.
Comment 21 (not reading, please use seth@sspitzer.org instead) 2006-08-02 18:43:23 PDT
after a recent bon echo meeting (and discussion with schrep) I have a better understand of what we're looking for.
Comment 22 (not reading, please use seth@sspitzer.org instead) 2006-08-02 18:47:43 PDT
after a recent bon echo meeting (and discussion with schrep) I have a better understand of what we're looking for.

in the wizard, we the "updatesfound" wizard page currently shows something similar for minor and major update.

http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/update/content/updates.xul#88

I'm going to work on making that page, when we're doing a major update look like what beltzner described in comment #13 and figure out what strings I need (so that we can get them in for b2.)

we already have the "updateMoreInfoURL" url, and when it's a minor update, we show a link.  for major, I'll just have a <xul:browser> and we'll load the url in it.

I'll report back with some screen shots for review.

Comment 23 (not reading, please use seth@sspitzer.org instead) 2006-08-03 18:42:40 PDT
Created attachment 232054 [details]
screen shot of poor man's major update (no string changes, but not ideal)
Comment 24 (not reading, please use seth@sspitzer.org instead) 2006-08-03 18:57:16 PDT
Created attachment 232057 [details] [diff] [review]
patch, rough draft, will list open issue...
Comment 25 (not reading, please use seth@sspitzer.org instead) 2006-08-03 18:58:12 PDT
Created attachment 232058 [details]
screen shot of poor man's minor update (should look the same)
Comment 26 (not reading, please use seth@sspitzer.org instead) 2006-08-03 19:10:59 PDT
here's some comments / known issues:

1) not showing any sort of incompatibility warnings (like we would for a minor release)

2) using the <license> element means I'm using the following strings:

 13 license404Error=The license file could not be found. Please contact the distributor.
 14 downloadingLicense=Downloading license text...

I'll bet there are suitable "404 error" and "downloading..." strings I could use instead, or I may have to add two new strings here.

3) for major vs minor, I'm using the severity, which maps to the isSecurityUpdate attribute on the update for the existing strings:

  3 updateType_major=New Version
  4 updateType_minor=Security Update
  5 introType_minor=An important Security Update for %S is available:
  6 introType_major=A new version of %S is available:

note, update already has a type="major" or type="minor" which I could use.

updateType_major and updateType_minor appear to be unused, except here where we get the string (but do nothing)
 
var displayType = gUpdates.strings.getString("updateType_" + severity);

4)  would be good to rename the <license> binding, given how I'm using it
Comment 27 (not reading, please use seth@sspitzer.org instead) 2006-08-03 19:31:29 PDT
Created attachment 232062 [details]
need to find a new string for the "404 error" case
Comment 28 (not reading, please use seth@sspitzer.org instead) 2006-08-03 19:32:13 PDT
Created attachment 232063 [details]
need to to find a new string for the download... case
Comment 29 (not reading, please use seth@sspitzer.org instead) 2006-08-03 19:34:52 PDT
From beltzner's ascii art, he asked for:

A new version of %prodName; is available!

I'm using:

"introType_major=A new version of %S is available:"

So, we'd need a new string for that (for ! vs : )  
Comment 30 (not reading, please use seth@sspitzer.org instead) 2006-08-07 17:36:29 PDT
Created attachment 232639 [details]
moving xul around to support major updates an incompatibilty (a little tight on space)
Comment 31 (not reading, please use seth@sspitzer.org instead) 2006-08-07 17:37:50 PDT
Created attachment 232640 [details]
using new string for "404 error" case
Comment 32 (not reading, please use seth@sspitzer.org instead) 2006-08-07 23:31:29 PDT
Created attachment 232667 [details]
ew details download string
Comment 33 (not reading, please use seth@sspitzer.org instead) 2006-08-08 02:59:05 PDT
Created attachment 232684 [details]
screen shot per beltzner's ascii art (for the 2.0 branch)
Comment 34 (not reading, please use seth@sspitzer.org instead) 2006-08-08 03:03:14 PDT
Created attachment 232687 [details] [diff] [review]
updated patch, need to decide on strings
Comment 35 Mike Beltzner [:beltzner, not reading bugmail] 2006-08-09 08:13:50 PDT
(In reply to comment #34)
> Created an attachment (id=232687) [edit]
> updated patch, need to decide on strings

OK, string review, with my head hung low since I'm pretty sure I wrote most of these in my crappy ASCII art :)

> +introType_major_app_and_version=Upgrade to %S %S now!

"You can download and install %S %S now!"

In addition to this, we should change the wizard title to be "A New Version is Available" (it's currently "Update Available"). If that's not too much trouble.

+details404Error=The details file could not be found. Please contact the distributor.

I'm guessing we're saying "the distributor" to allow for other products to use the update software and not lock us in to mozilla.com or Firefox-specific stuff. I'd like to be able to give the user a bit more of a hint, like:

"Additional details about this version could not be found. Please visit the %S homepage for more information."

+downloadingDetails=Downloading details text...

"Getting more details about %S %S..."

I think we're using an ellipses entity; check with rob strong or with other patches.

+downloadButton_major=Get the upgrade »

"Get the new version »"

+downloadButton_major.accesskey=G

Still works!

+neverButton=Never
+neverButton.accesskey=N

The only change I'm tempted to propose here is to use "Ask Later" and "Never Ask", but I'm not sure it's needed. Let's leave things as "Never" for now.

I think we also want the flow of the dialog to be:

Software Update
-------------------------------------------
A New Version is Available
-------------------------------------------

You can download and install Firefox 2.0 now!
.---------------------------------------.
|  blah blah blah blah                  |
|                                       |
|                                       |
|                                       |
|                                       |
|                                       |
|                                       |
|                                       |
'---------------------------------------'
   .
  /!\ This version may not be compatible
  ''' with some of your add-ons.

(Later) (Never)  (Get the new version >>)

so, reverse the order of the blurb and the warning?
Comment 36 Mike Beltzner [:beltzner, not reading bugmail] 2006-08-09 14:36:08 PDT
1.8.0.7 drivers: this is an essential fix as without it we won't be able to comfortably ship a major update to users on the 1.5.* product line. We will be producing a patch for 1.8.0.7 which implements the new functionality with as few strings as possible (hopefully none!). Please let us know how willing you'd be to take string changes.
Comment 37 Daniel Veditz [:dveditz] 2006-08-09 15:55:39 PDT
I was hoping we could get Firefox 2 to ship with this first so we can steal the localized strings. Conceptually this is a feature change we want, but I don't want to hold 1.5.0.7 security fixes hostage until a volunteer localization team gets around to doing these strings.

It seems that this can land any time before 1.5.0.x end-of-life, we don't have to push 2.0 right away. In fact, there would be advantages in waiting until 2.0.0.1 -- plenty of eager beavers will jump on the 2.0 release as it is, and those who don't are probably the more cautious types who need to have all their extensions working and the major regressions ironed out.

It sounds like the current patch might not warn people about incompatible extensions. That's troubling, it seems FAR more important to warn on the 1.5->2.0 transition than the 1.5.0.x -> 1.5.0.x+1 transition since there shouldn't even be any breakage on the latter.
Comment 38 (not reading, please use seth@sspitzer.org instead) 2006-08-09 17:35:52 PDT
thanks for the strings, mike.

> "You can download and install %S %S now!"

done, screen shot coming.

> change the wizard title to be "A New Version is Available" (it's currently "Update Available")

done, screen shot coming.

> +details404Error=The details file could not be found. Please contact the distributor.
>
> I'm guessing we're saying "the distributor" to allow for other products to use the update software and not lock us in to mozilla.com or Firefox-specific stuff. I'd like to be able to give the user a bit more of a hint, like:
>"Additional details about this version could not be found. Please visit the %S homepage for more information."

done, screen shot coming.

and fixed the license error to be:

license404Error=The license file for this version could not be found. Please visit the %S homepage for more information.

> +downloadingDetails=Downloading details text...
>"Getting more details about %S %S..."
> and fixed the license text to be:
> downloadingLicense=Getting license for %S %S...

done, screen shot coming.
 
> I think we're using an ellipses entity; check with rob strong or with other patches.

I checked with rob and with lxr and we are still doing "..." instead of the ellipses entity

> +downloadButton_major=Get the upgrade »
> "Get the new version »"

done, screen shot coming.

> The only change I'm tempted to propose here is to use "Ask Later" and "Never Ask", but I'm not sure it's needed. 
> Let's leave things as "Never" for now.

OK, will leave them be for now.

>> 6. Can we heighten the dialog? Or push the warning to a second page (ew!)?
>>
> Let me heighten that dialog and get a new screen shot.

done.  sample screen shot coming.

but I'm not sure what we should be doing here.  (changing the height and width of all wizards, to be the standard on windows, or just changing this one?)  Perphaps the size issue should be spun off.

> so, reverse the order of the blurb and the warning?

done, screen shot coming.

note, I left it as Later and Never, per your comments above.
Comment 39 (not reading, please use seth@sspitzer.org instead) 2006-08-09 17:42:32 PDT
Created attachment 233007 [details]
screen shot of a taller wizard, per beltzner
Comment 40 (not reading, please use seth@sspitzer.org instead) 2006-08-09 17:43:21 PDT
Created attachment 233008 [details]
major update with incompat
Comment 41 (not reading, please use seth@sspitzer.org instead) 2006-08-09 17:44:03 PDT
Created attachment 233009 [details]
minor update with incompat
Comment 42 (not reading, please use seth@sspitzer.org instead) 2006-08-09 17:46:39 PDT
Created attachment 233010 [details]
update download
Comment 43 (not reading, please use seth@sspitzer.org instead) 2006-08-09 17:49:21 PDT
Created attachment 233011 [details]
update failure
Comment 44 (not reading, please use seth@sspitzer.org instead) 2006-08-09 18:03:36 PDT
Created attachment 233015 [details] [diff] [review]
updated patch
Comment 45 Reed Loden [:reed] (use needinfo?) 2006-08-09 20:37:55 PDT
Comment on attachment 233011 [details]
update failure

Umm, so... how does the user know what the Bon Echo homepage is?
Comment 46 Reed Loden [:reed] (use needinfo?) 2006-08-09 20:42:43 PDT
Comment on attachment 233015 [details] [diff] [review]
updated patch

>+license404Error=The license file for this version could not be found. Please visit the %S homepage for more information.
>+details404Error=Additional details about this version could not be found. Please visit the %S homepage for more information.

Same thought as my previous comment. How does the user know what the product's homepage is?

>+updatesfound_major.title=A New Version is Available

I really hate the case usage here. When used in the screenshot, this text looks very ugly. How about just "A new version is available"?

>+downloadButton_minor=Download & Install Now »
>+downloadButton_major=Get the new version »

I see some weird 'A' symbol before the >> thing. Is that supposed to be there?
Comment 47 Reed Loden [:reed] (use needinfo?) 2006-08-09 21:30:19 PDT
(In reply to comment #46)
> >+updatesfound_major.title=A New Version is Available

Per discussion with beltzner, please change this entity to be "New Version Available".
Comment 48 (not reading, please use seth@sspitzer.org instead) 2006-08-09 23:18:30 PDT
Created attachment 233040 [details]
new text, per reed - beltzner
Comment 49 Mike Beltzner [:beltzner, not reading bugmail] 2006-08-09 23:31:32 PDT
This looks great, Seth. Answers to your questions:

(In reply to comment #38)
> but I'm not sure what we should be doing here.  (changing the height and width
> of all wizards, to be the standard on windows, or just changing this one?) 
> Perphaps the size issue should be spun off.

Yeah, let's make that a separate issue for now, and we can discuss it offline. XUL changes are easier to make after the fact and can be handled post beta2.

> note, I left it as Later and Never, per your comments above.

Yup, I think that less text down there is better.

Let's get this into the branch and trunk for some testing and baking?
Comment 50 (not reading, please use seth@sspitzer.org instead) 2006-08-09 23:36:03 PDT
>> Perphaps the size issue should be spun off.
>
>Yeah, let's make that a separate issue for now, and we can discuss it offline.
>XUL changes are easier to make after the fact and can be handled post beta2.

ok, good.  I know rob strong has some suggestions about the wizard size (on win32) to add.  I'll log a new bug about the size issue of major upgrade with incompatible extensions (or themes) to be solved after this lands.

> Let's get this into the branch and trunk for some testing and baking?

I have some functionality to finish before I'm ready for reviews.  for b2, I can attach a string only patch.
Comment 51 (not reading, please use seth@sspitzer.org instead) 2006-08-09 23:40:06 PDT
> I see some weird 'A' symbol before the >> thing. Is that supposed to be there?

reed, yes.  try viewing  http://lxr.mozilla.org/mozilla1.8/source/toolkit/locales/en-US/chrome/mozapps/update/updates.properties as UTF8 (and not latin-1 or us-ascii) to see that it is really the desired ">>" string.
Comment 52 Mike Schroepfer 2006-08-10 07:57:11 PDT
Looks like we are closing in on a solution Seth - once we get this landed can you sitdown with Davel or Robcc and work through a good exhaustive testplan for all the edge cases?   
Comment 53 (not reading, please use seth@sspitzer.org instead) 2006-08-10 10:32:37 PDT
> Looks like we are closing in on a solution Seth - once we get this landed can
> you sitdown with Davel or Robcc and work through a good exhaustive testplan for
> all the edge cases?   

yes, definitely.
Comment 54 (not reading, please use seth@sspitzer.org instead) 2006-08-10 17:28:43 PDT
Created attachment 233184 [details]
screen shot of license downloading formatted string
Comment 55 (not reading, please use seth@sspitzer.org instead) 2006-08-10 17:36:55 PDT
Created attachment 233186 [details]
license not found screen shot
Comment 56 (not reading, please use seth@sspitzer.org instead) 2006-08-10 18:16:46 PDT
Created attachment 233191 [details] [diff] [review]
the string changes only

the three removed strings are not used.  there are some old strings still in this .properties file that I will be removing once the rest of this fix lands, but until then, we need both old and new.
Comment 57 (not reading, please use seth@sspitzer.org instead) 2006-08-10 18:23:35 PDT
Created attachment 233192 [details] [diff] [review]
updated patch, still some more functionality to implement.
Comment 58 (not reading, please use seth@sspitzer.org instead) 2006-08-11 10:50:03 PDT
Created attachment 233255 [details] [diff] [review]
same set of new strings, but not removing any strings (yet)

I'm going to wait to remove any strings until after the complete patch has landed. 

so here's the new strings again, carrying over ui-r from beltzner.

I'm going to land this on the trunk, and then seek a= for the branch, and then resume working on the rest of the functionality.
Comment 59 (not reading, please use seth@sspitzer.org instead) 2006-08-11 11:32:19 PDT
Comment on attachment 233255 [details] [diff] [review]
same set of new strings, but not removing any strings (yet)

strings landed on trunk, seeking a= for the branch.
Comment 60 (not reading, please use seth@sspitzer.org instead) 2006-08-11 11:47:52 PDT
> I was hoping we could get Firefox 2 to ship with this first 
> so we can steal the localized strings.

right, that is the plan.  I'm seeking a= for the strings changes for ff2b2.

additionally, I'm going to finish this feature, so that we can test it with ff2 and find any issues, so that once we are sure everything is good, I could back port the fix (and the strings) to 1.5.0.something to allow for 1.5.0.something users to upgrade to 2.0.something,

> It sounds like the current patch might not warn people about incompatible
> extensions.

previous versions did not warn, but the current one does, see https://bugzilla.mozilla.org/attachment.cgi?id=233008
Comment 61 Mike Beltzner [:beltzner, not reading bugmail] 2006-08-11 13:54:20 PDT
Comment on attachment 233255 [details] [diff] [review]
same set of new strings, but not removing any strings (yet)

a=drivers, please land on MOZILLA_1_8_BRANCH
Comment 62 (not reading, please use seth@sspitzer.org instead) 2006-08-11 15:49:16 PDT
I've landed the strings on both trunk and branch, so they can be localized while I finish up this bug.

from comments #54 and #55:
> screen shot of license downloading formatted string
> license not found screen shot

to be clear, my patch does not add that license UI is not part of the flow.  that code is already in the product, but it is not (currently) exercised.

I forced it (in my local tree) into the flow to make sure my patch (with affects the <license> binding) did not break anything.

see bug #348389 for a new issue / requirement about adding the license to the upgrade flow. 

Comment 63 Daniel Veditz [:dveditz] 2006-08-14 11:55:49 PDT
If ff1.5.0.7 ships in advance of the slipping FF2 then this will be a ff1.5.0.8 bug given the need to steal back the translated strings from localized FF2.
Comment 64 (not reading, please use seth@sspitzer.org instead) 2006-08-14 16:44:07 PDT
note, this bug (and bug #348389) would involve using (for the first time, I believe) the <license> binding in http://lxr.mozilla.org/mozilla1.8/source/toolkit/mozapps/update/content/updates.xml#180

notice, it uses a xul:browser.  

for security reasons, should I be disabling js, auth, plugins, meta redirects, subframes on the docshell?

cc dveditz for his opinion.

thanks to robert strong for pointing this out.


Comment 65 Daniel Veditz [:dveditz] 2006-08-14 23:33:26 PDT
> notice, it uses a xul:browser.  
> for security reasons, should I be disabling js, auth, plugins, meta redirects,
> subframes on the docshell?

Disable anything and everything you possibly can!
Comment 66 Chris Beard 2006-08-15 09:16:27 PDT
Is there no way that we can use the existing strings from the Win32 installer?  That is, the product/legal requirement for the presentation and acceptance of the license agreement are the same as the installer (i.e. you need to explicit check or select the "I accept" and not have the default action allow you to click through) and it doesn't make a whole lot of sense (to me) to have different strings here. 
Comment 67 Chris Beard 2006-08-15 09:19:48 PDT
(In reply to comment #66)
> Is there no way that we can use the existing strings from the Win32 installer? 

Sorry, this should be on the related bug 348389, re: properly supporting licenseURL attribute.
Comment 68 Daniel Veditz [:dveditz] 2006-08-15 14:31:20 PDT
We're trying to get 1.5.0.7 out in advance of FF2, before the release candidate churn especially, so we won't have any translated strings to steal.

1.5.0.8 will be released on the heels of FF2 and can then pick up the translated strings, we can just wait to offer 2.0 until then.
Comment 69 (not reading, please use seth@sspitzer.org instead) 2006-08-15 15:42:06 PDT
Created attachment 233871 [details] [diff] [review]
adding support for licenseURL attribute, the new strings and the UI are pending review in bug #329729
Comment 70 (not reading, please use seth@sspitzer.org instead) 2006-08-16 07:45:38 PDT
Created attachment 234005 [details] [diff] [review]
improvements for licenseURL support, only enable Next button from EULA page when the EULA is downloaded and the user accepts it
Comment 71 (not reading, please use seth@sspitzer.org instead) 2006-08-16 11:47:40 PDT
Created attachment 234038 [details] [diff] [review]
updated patch, includes support for "never" ask again, more work to support isSecurityUpgrade attribute and licenseURL attribute
Comment 72 (not reading, please use seth@sspitzer.org instead) 2006-08-16 11:57:29 PDT
there a couple new strings.  after chatting with morgamic recently, I learned that aus has not been setting isSecurityUpdate (see http://wiki.mozilla.org/Software_Update:updates.xml_Format)

I've added support, for minor updates only, to show "An important Security Update for <app name> is available:" if isSecurityUpdate is show.

otherwise, for a minor update, we continue to show: "A new version of <app name> is available:" like we have done until now.

so the new strings are:

+introType_security_app=An important Security Update for %S is available:
+introType_minor_app=A new version of %S is available:

all the strings in there are:

introType_minor=An important Security Update for %S is available:
introType_major=A new version of %S is available:
introType_security_app=An important Security Update for %S is available:
introType_minor_app=A new version of %S is available:
introType_major_app_and_version=You can download and install %S %S now!

introType_minor and introType_major will no longer be used, and I've adde them to bug #348363 (for code cleanup)

I'll attach some screen shot for ui-r from beltzner.

note, the change to support introType_security_app isn't strictly necessary, as we may not want to change the wording if isSecurityUpdate is set.

but adding introType_minor_app is necessary so that the UI for minor updates stays as it has for non-major updates.
Comment 73 (not reading, please use seth@sspitzer.org instead) 2006-08-16 11:59:07 PDT
Created attachment 234040 [details]
screen shot minor update where isSecurityUpdate is true in updates.xml
Comment 74 (not reading, please use seth@sspitzer.org instead) 2006-08-16 12:05:08 PDT
Created attachment 234044 [details]
screen shot minor update where isSecurityUpdate is "false" (or not set) in updates.xml
Comment 75 Mike Beltzner [:beltzner, not reading bugmail] 2006-08-16 14:07:34 PDT
(In reply to comment #72)
> there a couple new strings.  after chatting with morgamic recently, I learned
> that aus has not been setting isSecurityUpdate (see
> http://wiki.mozilla.org/Software_Update:updates.xml_Format)

Interesting. All of our updates on the product channel have been security updates, and that flag should probably be set for those. However, at this moment, I don't see any reason for us to react differently to security vs. non security updates.

> +introType_security_app=An important Security Update for %S is available:
> +introType_minor_app=A new version of %S is available:

Suggest using:

introType_minor_app=An update for %S is available:
introType_security_app=An update for %S is available:

Until we decide to treat these things differently.
Comment 76 (not reading, please use seth@sspitzer.org instead) 2006-08-16 14:17:00 PDT
>introType_minor_app=An update for %S is available:
>introType_security_app=An update for %S is available:

thanks mike.  I'll switch to your new version of introType_minor_app.

introType_security_app is new a new strings, and is not necessary if we don't change behavior based on isSecurityUpdate.

my next version of the patch will not have the introType_security_app string, and will not have the code to change the intro text based on isSecurityUpdate.
Comment 77 (not reading, please use seth@sspitzer.org instead) 2006-08-16 14:24:19 PDT
Created attachment 234074 [details]
screen shot of minor update (isSecurityUpdate="true" or not, they are the same)
Comment 78 (not reading, please use seth@sspitzer.org instead) 2006-08-16 15:51:49 PDT
1) as discussed, we always show the prompt for major updates.  for minor updates, we follow the existing logic we have
always followed, unless there is a change to the EULA.  I store the last accepted license URL in a pref, and if that doesn't match, the user would see the prompt for a minor release.

note, if we are prompting, if the current licenseURL matches the last one, we don't show the license page to the user.

2) if the user hits "Never" on a major, they can always do a manual "Check for Updates..." to get the major upgrade

3) minor updates trump major updates, as discussed.  this is also has the benefit that after a user does "never" to a major upgrade, 
the will continue to get minor upgrades.    

4)  if the user hits "Never", but then does "Check for Updates...", we clear the "never" pref for that version.
This so if they do "Never", then "Check for Updates...", and then hit "Later", we will prompt again when it is "later".

5)  if the user hits "Later" (for major or minor), it just acts like a cancel.

There was code in onInstallLater() to not ask them again (for minor or major) during the current session
and to set up another nag timer.  I've moved that code into my "Later()" function, but it is commented out.
To reduce risk, I'll just keep on using the existing remind / nag / later behavior.

one known issue:

1) the back button is disabled in the EULA page, as back isn't working.  (the back wizard button appears to be disabled in all pages of the software update dialog).  I'll log a spin off bug about this.

Comment 79 (not reading, please use seth@sspitzer.org instead) 2006-08-16 16:15:59 PDT
Created attachment 234105 [details] [diff] [review]
just the new strings (with dead strings removed, as this will be going on trunk and branch)

seeking beltzner's ui-r on the strings changes.
Comment 80 (not reading, please use seth@sspitzer.org instead) 2006-08-16 17:13:16 PDT
for defensiveness, I've done the following on the xul:browser that is used by the license binding (which we use to show the EULA and the details of a major upgrade)

+        // just to be safe, turn off auth, plugins and subframes
+        var docShell = this._content.docShell;
+        //allow javascript
+        //docShell.allowJavascript = false;
+        docShell.allowAuth = false;
+        docShell.allowPlugins = false;
+        //allow redirects
+        //docShell.allowMetaRedirects = false;
+        docShell.allowSubframes = false;
+        //allow images
+        //docShell.allowImages = false;

I figured that for update content (and possibly the eula) we may want images, js, and redirects.

dveditz, cbeard:  am I too lenient?  or am I too restrictive?
Comment 81 (not reading, please use seth@sspitzer.org instead) 2006-08-16 17:28:01 PDT
Created attachment 234117 [details] [diff] [review]
patch

I think darin (or ben goodger) would be the right person to review this patch.
Comment 82 (not reading, please use seth@sspitzer.org instead) 2006-08-16 18:27:42 PDT
Comment on attachment 234117 [details] [diff] [review]
patch

I've gotten more feedback from beltnzer about the EULA behavior, so re-working that part of this patch.
Comment 83 (not reading, please use seth@sspitzer.org instead) 2006-08-16 18:47:41 PDT
Created attachment 234133 [details] [diff] [review]
updated patch per eula behavior specified by beltzner
Comment 84 (not reading, please use seth@sspitzer.org instead) 2006-08-16 18:52:32 PDT
to clarify, the new behavior from beltzner (from bug #348389) is a simplification.

> for minor updates, we follow the existing logic we have
> always followed, unless there is a change to the EULA.  

correction, the existing logic is always followed as EULAs (licenseURL on a minor release) is ignored.

and, even if the user does get prompted (because they have their prefs set for that, or if they have incompatible extensions, etc), they won't see or be asked to agree to a eula if the type of the update is minor.

>I store the last accepted license URL in a pref, and if that doesn't match, >the user would see the prompt for a minor release.
>note, if we are prompting, if the current licenseURL matches the last one, we
>don't show the license page to the user.

not doing this, either.  if the update is a major, and we have a licenseURL, we show the eula, even if the user has already accepted that same url from a previous major upgrade.

seeking review from darin on the latest patch.
Comment 85 (not reading, please use seth@sspitzer.org instead) 2006-08-16 20:10:53 PDT
I've landed the new strings to the branch, so they can be localized.
Comment 86 (not reading, please use seth@sspitzer.org instead) 2006-08-16 20:18:52 PDT
> I've landed the new strings to the branch, so they can be localized.

trunk as well.

I'll attach a new patch (that will not have the string changes)
Comment 87 (not reading, please use seth@sspitzer.org instead) 2006-08-16 20:36:22 PDT
Created attachment 234144 [details] [diff] [review]
same patch, but no strings changes, as they have landed already
Comment 88 (not reading, please use seth@sspitzer.org instead) 2006-08-16 21:10:52 PDT
Created attachment 234145 [details]
mac screen shot #1 (eula)
Comment 89 (not reading, please use seth@sspitzer.org instead) 2006-08-16 21:12:16 PDT
Created attachment 234146 [details]
mac screen shot #2 (downloading eula)
Comment 90 (not reading, please use seth@sspitzer.org instead) 2006-08-16 21:13:07 PDT
Created attachment 234147 [details]
mac screen shot #3 (eula not found)
Comment 91 (not reading, please use seth@sspitzer.org instead) 2006-08-16 21:15:47 PDT
Created attachment 234148 [details]
mac screen shot #4 (major update with incompatibilities)
Comment 92 (not reading, please use seth@sspitzer.org instead) 2006-08-16 21:17:49 PDT
Created attachment 234149 [details]
mac screen shot #5 (major version avail)
Comment 93 (not reading, please use seth@sspitzer.org instead) 2006-08-16 21:18:46 PDT
Comment on attachment 234147 [details]
mac screen shot #3 (eula not found)

notice, the "not found" icon is not there.
Comment 94 (not reading, please use seth@sspitzer.org instead) 2006-08-16 21:22:33 PDT
for the mac, the reason the "not found icon" is not there is because there is no chrome://global/skin/icons/notfound.png in the pinstripe theme.

the css (http://lxr.mozilla.org/mozilla1.8/source/toolkit/themes/pinstripe/mozapps/update/updates.css#58) points to it, see:

 57 .licenseLoadingThrobber[state="error"] {
 58   list-style-image: url("chrome://global/skin/icons/notfound.png");
 59 }

what about using the same 16x16 px notfound.png from winstripe, which is pretty pinstripey?

http://lxr.mozilla.org/mozilla1.8/source/toolkit/themes/winstripe/global/icons/notfound.png
Comment 95 (not reading, please use seth@sspitzer.org instead) 2006-08-16 21:29:00 PDT
Created attachment 234150 [details]
mac screen shot #6 (minor version avail)
Comment 96 Jesse Ruderman 2006-08-16 21:39:04 PDT
Should "I accept" be disabled until the EULA loads?

The screenshot of the EULA looks like line breaks have been ignored.

Please use PNG screenshots in the future instead of tiff.
Comment 97 (not reading, please use seth@sspitzer.org instead) 2006-08-16 21:39:57 PDT
Created attachment 234151 [details]
mac screen shot #3a (eula not found) but with an icon from winstripe
Comment 98 (not reading, please use seth@sspitzer.org instead) 2006-08-16 21:41:35 PDT
Created attachment 234152 [details] [diff] [review]
supplimental patch to fix the "not found" icon (uses notfound icon from winstripe)
Comment 99 (not reading, please use seth@sspitzer.org instead) 2006-08-16 21:54:15 PDT
> Should "I accept" be disabled until the EULA loads?

the Next button is not enabled unless the EULA has loaded and the user chooses "I accept".

do you think I should also disable the radio group as well?

> The screenshot of the EULA looks like line breaks have been ignored.

you are right.  

I'm loading a simple .txt (http://www.mozilla.com/legal/eula/firefox-en.txt) into a <xul:browser>, and it is being treated as HTML  

windows looks like same (see https://bugzilla.mozilla.org/attachment.cgi?id=234004)

the code that loads the url is here: http://lxr.mozilla.org/mozilla1.8/source/toolkit/mozapps/update/content/updates.xml#253

we are issuing a xmlhttprequest and then setting the innerHTML of the document element to be the response text.

that would explain why my simple .txt file looks like that.
Comment 100 Jesse Ruderman 2006-08-16 22:11:21 PDT
The EULA is only shown if it's different from the old EULA, right?  (If Firefox 3 doesn't have a EULA change, users upgrading from Firefox 2 shouldn't see a EULA dialog.)

> the Next button is not enabled unless the EULA has loaded and the user chooses
> "I accept".
> do you think I should also disable the radio group as well?

Not disabling the radio group is fine with me, then.  IANAL :)
Comment 101 (not reading, please use seth@sspitzer.org instead) 2006-08-16 22:20:46 PDT
> The EULA is only shown if it's different from the old EULA, right?  (If Firefox
> 3 doesn't have a EULA change, users upgrading from Firefox 2 shouldn't see a
> EULA dialog.)

no, you'd see the EULA pane of the upgrade wizard.  

(see comment #84 and the equally verbose bug #348389).

I had a solution for this, which was to remember the last "accepted" licenseURL, but I've removed that behavior, after discussing this with beltzner. 

> Not disabling the radio group is fine with me, then.  IANAL :)

good point, I'm not a lawyer either.  I'll email beard/sherman to make sure they (or the lawyers) are ok with that behavior.
Comment 102 Steffen Wilberg 2006-08-17 00:30:11 PDT
(In reply to comment #75)
> Suggest using:
> 
> introType_minor_app=An update for %S is available:
> introType_security_app=An update for %S is available:
> 
> Until we decide to treat these things differently.

How about adding the string for security updates, even if we don't use it yet, but so we can start using it in e.g. Firefox 2.0.0.1? Note that we didn't add or modify any strings for the Firefox 1.5.0.x releases because it would've been too much effort.
Comment 103 (not reading, please use seth@sspitzer.org instead) 2006-08-17 09:10:29 PDT
I have landed the notfound.png and jar.mn fix to the trunk for pinstripe, and I have tagged the image with the MOZILLA_1_8_BRANCH branch tag.

the jar.mn fix for pinstripe branch will be part of the complete fix for branch.

I've also backported the fix for this bug to the trunk, and I'll attach that.
Comment 104 Mike Beltzner [:beltzner, not reading bugmail] 2006-08-17 09:18:22 PDT
Seth, have all the beta2 parts of this landed? If so, can you change the TM to Firefox2, assuming the rest is making it into an RC and not the beta?
Comment 105 (not reading, please use seth@sspitzer.org instead) 2006-08-17 09:28:14 PDT
> Seth, have all the beta2 parts of this landed? If so, can you change the TM to
> Firefox2, assuming the rest is making it into an RC and not the beta?

yes, I have added the new strings (and left the old ones in there, for now, otherwise thing would break) and checked in the the new icon for pinstripe (but not the jar.mn change) to the 1.8 branch.  

the rest of the patch is still pending review, so changing TM to FF2.
Comment 106 Mike Schroepfer 2006-08-17 09:42:23 PDT
Resetting Milestone back to b2.  We really did want this landed before then if possible to get wide-scale b2->rc1 SU testing (even if it is just the minor update path).

(In reply to comment #105)
> > Seth, have all the beta2 parts of this landed? If so, can you change the TM to
> > Firefox2, assuming the rest is making it into an RC and not the beta?
> 
> yes, I have added the new strings (and left the old ones in there, for now,
> otherwise thing would break) and checked in the the new icon for pinstripe (but
> not the jar.mn change) to the 1.8 branch.  
> 
> the rest of the patch is still pending review, so changing TM to FF2.
> 

Comment 107 (not reading, please use seth@sspitzer.org instead) 2006-08-17 10:03:11 PDT
Created attachment 234241 [details] [diff] [review]
trunk version of the same patch (updates.js had some changes that needed to be backported)
Comment 108 Ben Goodger (use ben at mozilla dot org for email) 2006-08-17 10:26:49 PDT
Comment on attachment 234241 [details] [diff] [review]
trunk version of the same patch (updates.js had some changes that needed to be backported)

>+  later: function () {
>+    // The user said "Later", so stop all update checks for this session
>+    // so that we don't bother them again.
>+    try {
>+/*    XXX sspitzer
>+
>+      this code comes from the old onInstallLater(), 
>+      which was not being called

Does this code not work? Did you add it here? What is it doing? Can you explain why you're inserting it here commented it out?

>+    else {
>+      // for minor updates, do not include the version
>+      // just use the brandName for the intro
>+      intro = gUpdates.strings.getFormattedString(
>+        "introType_" + severity + "_app", [gUpdates.brandName]);
>     
>     var updateMoreInfoURL = document.getElementById("updateMoreInfoURL");
>     updateMoreInfoURL.href = gUpdates.update.detailsURL;
>+    }

Is this a -w diff? Seems like this block should be indented two spaces, if not. 

>+/*   XXX sspitzer
>+     this code comes from the existing onWizardCancel() for gLicense,
>+     which was not being called.

Ditto comment on why this is here. 

>Index: mozapps/update/content/updates.xul
>+    <radiogroup id="acceptDeclineLicense" 
>+       onselect="gLicensePage.onAcceptDeclineRadio();">
>+      <radio id="accept" label="&license.accept;" 
>+             accesskey="&license.accept.accesskey;"/>
>+      <radio id="decline" label="&license.decline;" 
>+             accesskey="&license.decline.accesskey;"
>+             selected="true"/>
>+    </radiogroup>

Please check to make sure focus rects on these don't extend across the width of the dialog. If they do, you need an align="start" on the containing radiogroup. 

r=ben@mozilla.org with these things checked out/answered/addressed.
Comment 109 (not reading, please use seth@sspitzer.org instead) 2006-08-17 11:33:29 PDT
working on answering ben's questions, as well as switching from onload to an onreadystate change listener, like we do in http://lxr.mozilla.org/mozilla1.8/source/browser/components/search/nsSearchSuggestions.js#487, and disabling the radiogroup until the load is complete (to fix an edge case that ben and I discussed over aim.)
Comment 110 (not reading, please use seth@sspitzer.org instead) 2006-08-17 14:30:26 PDT
> Does this code not work? Did you add it here? What is it doing? Can you explain
> why you're inserting it here commented it out?
...
> Ditto comment on why this is here. 

back on 2005-08-03, bug https://bugzilla.mozilla.org/show_bug.cgi?id=302269 was fixed, to 
"Make "Updates Available" page use standard wizard buttons".  

when we switched from custom buttons to
the wizard buttons, the custom next button (which called onInstallNow()) turned into a standard wizard next button
so onInstallNow() was no longer being called (but it remained in updates.js)

the custom later button (which called onInstallLater()), turned into a extra1 button, and the onextra1 hander just
did wiz.cancel().  onInstallLater() was no longer being called (but it remained in updates.js)

I'll log a spin off bug, where we can debate adding back the code that makes it so on later (or when the user cancels the license page)
we do not nag the user again in the current session, using this code:

     const nsIUpdateChecker = Components.interfaces.nsIUpdateChecker;
     var aus =
       Components.classes["@mozilla.org/updates/update-service;1"].
       getService(Components.interfaces.nsIApplicationUpdateService);
     aus.backgroundChecker.stopChecking(nsIUpdateChecker.CURRENT_SESSION);

but for the trunk, and especiall for the branch, I do not want to change the behavior that we've had since 8/5

> Is this a -w diff? Seems like this block should be indented two spaces, if not. 

just an artifact of the diff.  the indentation is correct in my local tree.

> Please check to make sure focus rects on these don't extend across the width of
> the dialog. If they do, you need an align="start" on the containing radiogroup. 

they do not extend the width of the dialog.

> as well as switching from onload to an onreadystate change listener, like we do in
> http://lxr.mozilla.org/mozilla1.8/source/browser/components/search/nsSearchSuggestions.js#487,
>and disabling the radiogroup until the load is complete (to fix an edge case
> that ben and I discussed over aim.)

I proposed switching to that, based on some issues I was seeing on the trunk, which was onLoad() was getting called
multiple times.  it turns out, on the trunk, the onreadystatechange listener was also getting called multiple times
(with state == 4, or completed).  So, I've left it alone.  The branch doesn't seem to have this problem, and I'll 
log bug to investigate.

since my patch, I have made two other minor changes:

1)  if in onLoad() (for the license binding, which is used for the "about this upgrade" content and the eula)
the _request.status is a 404, I will switch back in order to show the error, instead of showing a 404 page from the server.
(I've got a test case that exhibits this.)  this can happen if my eula is http://www.sspitzer.org/xxxx.xxx.xxx.html, for example.
I don't want to see the 404 page there.

2)  by default, as suggested last night by jesse and again today by ben, I'm disabling the entire radiogroup by default,
and only enabling it once the eula is succesfully loaded.  this prevents an edge case where an anxious user clicks "I agree" before the eula loads.
if they did that, the next button would not be enabled, as I was only changing the disabled state of the next button when the
user clicked on the radiogroup, and I would not enable it because the eula was not loaded.    this is no longer an issue, since you can't click on "I accept" until the eula is loaded.

3)  in onLicenseLoaded(), I check the state of the licesnseContent, and if "error", I disasble the radiogroup,
otherwise, I enable it.  the reason for this, on the trunk at least, it is possible to get multiple calls to onLoad().
first where the state is "loading" and then when the state is "error".  this prevents the radiogroup from being
enabled if the eula failed to load.

I'll attach a new patch for both trunk and branch.
Comment 111 (not reading, please use seth@sspitzer.org instead) 2006-08-17 14:58:06 PDT
Created attachment 234302 [details] [diff] [review]
branch patch, also includes mac jar.mn change for notfound.png
Comment 112 (not reading, please use seth@sspitzer.org instead) 2006-08-17 14:59:28 PDT
Created attachment 234304 [details] [diff] [review]
branch patch, also includes mac jar.mn change for notfound.png
Comment 113 (not reading, please use seth@sspitzer.org instead) 2006-08-17 15:01:39 PDT
Created attachment 234305 [details] [diff] [review]
trunk version of the latest branch patch
Comment 114 Ben Goodger (use ben at mozilla dot org for email) 2006-08-17 15:06:10 PDT
Comment on attachment 234304 [details] [diff] [review]
branch patch, also includes mac jar.mn change for notfound.png

r=ben@mozilla.org
Comment 115 (not reading, please use seth@sspitzer.org instead) 2006-08-17 15:45:53 PDT
update:  

ben and I are chatting over aim about if we should be doing:

xul:browser type="content" 

(for security reasons)

and using the src attribute, instead of using innerHTML
Comment 116 Daniel Veditz [:dveditz] 2006-08-17 18:54:30 PDT
> ben and I are chatting over aim about if we should be doing:
> xul:browser type="content" (for security reasons)

Given that you're allowing JavaScript it *must* be content. JavaScript ought to be disallowed, though, there's no reason for it. Even then, remotely loading pages in a non-content browser is needlessly increasing our risk.

Where will the EULA live? If it's anywhere other than aus2--and so far the plan seems to be www.mozilla.org--then it's another attack vector. It's another server that could be hacked, but more likely it's not SSL so the victim could be redirected and get who knows what. If it's just inert text the worst case is defacement. Make it a chrome docshell with JavaScript and we're in trouble.

Even if JS is off I'd worry there might be some unknown hole that allows running JS anyway. We've found one or two of those in Firefox and Thunderbird over the past couple of years so the non-chrome <browser> is a necessary safety-net.
Comment 117 (not reading, please use seth@sspitzer.org instead) 2006-08-17 21:18:31 PDT
Created attachment 234355 [details] [diff] [review]
branch patch, with type="content" on the xul:brower, and using a different method for loading the content (and detecting errors)
Comment 118 (not reading, please use seth@sspitzer.org instead) 2006-08-17 21:19:28 PDT
Created attachment 234356 [details] [diff] [review]
trunk patch, with type="content" on the xul:brower, and using a different method for loading the content (and detecting errors)
Comment 119 (not reading, please use seth@sspitzer.org instead) 2006-08-17 21:36:30 PDT
Created attachment 234358 [details] [diff] [review]
branch patch, with type="content" on the xul:brower, and using a different method for loading the content (and detecting errors)
Comment 120 (not reading, please use seth@sspitzer.org instead) 2006-08-17 21:37:19 PDT
Created attachment 234359 [details] [diff] [review]
branch patch, with type="content" on the xul:brower, and using a different method for loading the content (and detecting errors)
Comment 121 (not reading, please use seth@sspitzer.org instead) 2006-08-17 21:53:34 PDT
Created attachment 234360 [details] [diff] [review]
branch patch, with type="content" on the xul:browser, and using a different method for loading the content (and detecting errors)
Comment 122 (not reading, please use seth@sspitzer.org instead) 2006-08-17 21:54:25 PDT
Created attachment 234361 [details] [diff] [review]
trunk patch, with type="content" on the xul:browser, and using a different method for loading the content (and detecting errors)
Comment 123 (not reading, please use seth@sspitzer.org instead) 2006-08-17 22:05:27 PDT
all the changes since ben's last review are in mozapps/update/content/updates.xml

here's what's new:

1) I'm now doing <xul:browser type="content">

2) on the docshell for the xul:browser, js, auth, plugins, and subframes.  
   meta redirects and images are still allowed

3) the big change is instead of issuing an XMLHttpRequest, and setting the innerHTML of the document of the xul browser with the responseText from the XMLHttpRequest, I'm now setting the src attribute of the xul:browser to load the "details about the major update" URL or the EULA URL and setting up my own web progress listener.

in order to determine if the load failed (due to unknown host name, or a 4xx error, etc), or if it succeeded, I need a nsIWebProgressListener.

in the listener, On STATE_START, I call the old code (now in an onStart() method on the <license> binding) to display the "loading..." message and the busy animated image

On STATE_STOP, I call either onError(), which displas the "unable to load...." message and the not found image, or I call onLoad() which switches the selected index of the deck to the box with the xul:browser and fires the "load" event, as we did before.  (in the case of the EULA, this event allows me to enable the radio group)
  
finally, stopDownloading() just calls the stop() method of the xul:browser.
Comment 124 Christian :Biesinger (don't email me, ping me on IRC) 2006-08-17 22:18:45 PDT
Comment on attachment 234361 [details] [diff] [review]
trunk patch, with type="content" on the xul:browser, and using a different method for loading the content (and detecting errors)

+                    if (httpResponse >= 200 && httpResponse < 300) {

requestSucceeded

+                    // the QI to nsIHttpChannel can fail if you try to hit
+                    // www.mozillaxxxx.org or something that does not exist
+                    // so we treat that as an error

so, that shouldn't be the case. it is kind of strange that it is. what is aRequest.name in this case?
Comment 125 (not reading, please use seth@sspitzer.org instead) 2006-08-18 08:31:16 PDT
Created attachment 234411 [details] [diff] [review]
branch patch, with biesi's suggestions, and using a field for the listener so that there is a reference to it, and so that I can call removeListener() in the dtor
Comment 126 (not reading, please use seth@sspitzer.org instead) 2006-08-18 08:32:34 PDT
Created attachment 234412 [details] [diff] [review]
trunk patch, with biesi's suggestions, and using a field for the listener so that there is a reference to it, and so that I can call removeListener() in the dtor
Comment 127 (not reading, please use seth@sspitzer.org instead) 2006-08-18 08:42:50 PDT
Created attachment 234413 [details]
screen shot, details content to www.mozilla.com (not injected as innerHTML, but loaded as src on the xul:browser)
Comment 128 (not reading, please use seth@sspitzer.org instead) 2006-08-18 08:43:58 PDT
Created attachment 234414 [details]
screen shot of a .txt eula (http://www.mozilla.com/legal/eula/firefox-en.txt) (not injected as innerHTML, but loaded as src on the xul:browser)
Comment 129 (not reading, please use seth@sspitzer.org instead) 2006-08-18 09:04:28 PDT
in response to biesi comment #124:

1)

>+                    if (httpResponse >= 200 && httpResponse < 300) {
>
>requestSucceeded

fixed, thanks

2)

>+                    // the QI to nsIHttpChannel can fail if you try to hit
>+                    // www.mozillaxxxx.org or something that does not exist
>+                    // so we treat that as an error
>
>so, that shouldn't be the case. it is kind of strange that it is. what is
>aRequest.name in this case?

my mistake, the QI wasn't failing, it was the call to get the httpResponse
(and now requestSucceed)

aRequest.name is:  http://www.sspitzerx.org/

and the exception I get is:

[Exception... "Component returned failure code: 0x80040111
 (NS_ERROR_NOT_AVAILABLE) [nsIHttpChannel.requestSucceeded]"  nsresult:
"0x80040
111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame ::
chrome://mozapps/content/u
pdate/updates.xml :: anonymous :: line 302"  data: no]

I've fixed my comment.
Comment 130 Christian :Biesinger (don't email me, ping me on IRC) 2006-08-18 09:05:06 PDT
oh... one more thing:
+                    var channel = aRequest.QueryInterface(nsIHttpChannel);
+                    if (channel && channel.requestSucceeded) 

Since QueryInterface throws when the component does not implement that interface, channel will never be null on the second line here.
Comment 131 (not reading, please use seth@sspitzer.org instead) 2006-08-18 09:09:26 PDT
>oh... one more thing:
>+                    var channel = aRequest.QueryInterface(nsIHttpChannel);
>+                    if (channel && channel.requestSucceeded) 
>
>Since QueryInterface throws when the component does not implement that
>interface, channel will never be null on the second line here.

thanks, I've changed it to be:

>+                    var channel = aRequest.QueryInterface(nsIHttpChannel);
>+                    if (channel.requestSucceeded) 

in my local branch and trunk trees.
Comment 132 Jesse Ruderman 2006-08-18 14:44:03 PDT
> https://bugzilla.mozilla.org/attachment.cgi?id=234414&action=view

> screen shot of a .txt eula
> (http://www.mozilla.com/legal/eula/firefox-en.txt)
> (not injected as innerHTML, but loaded as src on the xul:browser)

Eww, horizontal scrolling.  Can you make the dialog wider, or use an HTML version that wraps appropriately?
Comment 133 (not reading, please use seth@sspitzer.org instead) 2006-08-18 14:54:08 PDT
> Eww, horizontal scrolling.  Can you make the dialog wider, or use an HTML
> version that wraps appropriately?

jesse:

that .txt eula isn't the one we're going to be using, it was just one I was using for test purposes, and to show that we could load a .txt file.

I think the eventual eula would be HTML, so it would wrap (and not result in horizontal scroll bars.)
Comment 134 Christian :Biesinger (don't email me, ping me on IRC) 2006-08-18 17:44:50 PDT
Comment on attachment 234411 [details] [diff] [review]
branch patch, with biesi's suggestions, and using a field for the listener so that there is a reference to it, and so that I can call removeListener() in the dtor

r=biesi on the webprogresslistener parts

(you could use Components.isSuccessCode(aStatus) before checking isRequestSucceeded if you wanted to avoid the exception. but, either way is fine with me)
Comment 135 Christian :Biesinger (don't email me, ping me on IRC) 2006-08-18 17:46:18 PDT
(this code will also fail for non-HTTP/HTTPS URLs, but that's fine, right?)
Comment 136 (not reading, please use seth@sspitzer.org instead) 2006-08-18 19:35:11 PDT
thanks for the review, biesi.

> (this code will also fail for non-HTTP/HTTPS URLs, but that's fine, right?)

yes, that is acceptable.

I've just gotten r=rstrong on the stuff that's changed in updates.xml since ben's review.  He had two comments:

1)

+        if (this._licenseProgressListener) 
+        {
+          this._content.webProgress
+              .removeProgressListener(this._licenseProgressListener);
+        }

remove the extra { }, which I have done in my local trees.

2)       linkify the "Please visit the XXX home page for more information" on errors.  (I'll log a spin off bug on this UI request.)

I'm going to land on the trunk, and then seek approval for the branch.
Comment 137 (not reading, please use seth@sspitzer.org instead) 2006-08-18 22:36:56 PDT
fix landed on the trunk.  will seek approval for the branch.
Comment 138 (not reading, please use seth@sspitzer.org instead) 2006-08-20 16:22:28 PDT
Created attachment 234713 [details]
smoke test successful, installed 8-19 trunk (with my fix) update screen shot
Comment 139 (not reading, please use seth@sspitzer.org instead) 2006-08-20 16:23:31 PDT
Created attachment 234714 [details]
smoke test successful, sw update upgraded to 8-20 trunk (with my fix) update screen shot
Comment 140 (not reading, please use seth@sspitzer.org instead) 2006-08-20 16:25:33 PDT
Created attachment 234715 [details]
screen shot of trunk (with my fix) fed an updates.xml (using app.update.url.override pref) to a major update
Comment 141 (not reading, please use seth@sspitzer.org instead) 2006-08-20 16:30:37 PDT
just did a little smoke testing of the trunk to help convince drivers to take this on the branch.

I checked my fix into the trunk on 8-18.  I downloaded the 8-19 build, and got presented with the software update dialog to take me to the 8-20 build. (see the first screen shot, https://bugzilla.mozilla.org/attachment.cgi?id=234713)

the 8-19 has my fix, because (as the screen shot shows), it has the new string for a minor update:

+introType_minor_app=An update for %S is available:

I was successfully able to use Software Update to take this minor update to 8-20 (see the second screen shot, https://bugzilla.mozilla.org/attachment.cgi?id=234714.).

after running the 8-20, I fed the trunk a major update by setting the app.update.url.override hidden pref to my test updates.xml file that provides a major update.  (see the third screen shot https://bugzilla.mozilla.org/attachment.cgi?id=234715)
Comment 142 (not reading, please use seth@sspitzer.org instead) 2006-08-20 19:15:34 PDT
fix landed on the 1.8 branch so that it makes ff2b2
Comment 143 Daniel Veditz [:dveditz] 2006-08-21 10:38:03 PDT
Comment on attachment 234411 [details] [diff] [review]
branch patch, with biesi's suggestions, and using a field for the listener so that there is a reference to it, and so that I can call removeListener() in the dtor

r=dveditz on the security precautions.
Comment 144 (not reading, please use seth@sspitzer.org instead) 2006-08-21 15:15:53 PDT
something I wanted to clarify, after chatting with dveditz this morning.

when aus starts sending update.xml with both minor and major releases, here's what would happen:

in firefox, as described earlier in this bug, minor updates trump major updates.  So, if you were running 2.0.1 and both 2.0.2 and 3.0 were available, and both were in the update.xml for 2.0.1 users, the user would only see that 2.0.2 was available.

In order to get 3.0, they would have to upgrade twice, but this requires that the update.xml presented to 2.0.2 users *only* list the major update, because if a minor is listed, we would defer to it.

Currently, when the most update-to-date point release (for example 2.0.2 in my example) build requests and update, aus feeds the client an update.xml with an empty updates tag, like this:

<updates></updates>

In my example, it would present the user with <update> tags for major updates (3.0).  (If presented with both major and minor, the minor would trump the major.)

When presented with only the major <update> tags, the 2.0.2 user would get the new "major update" UI and choose between later, never, or accept the upgrade.

originally, there was discussion about changing the software update UI to present the user with a choice to do either the major or the minor.  That UI was not implemented, but I'll log a spin off bug, in case that is something that we are interested in for a future release.

I hope this additional information helps clarify that is now checked into the trunk and the 1.8 branch, and what is planned to be backported to the 1.8.0 at some point.
Comment 145 (not reading, please use seth@sspitzer.org instead) 2006-08-21 17:50:51 PDT
here are the 5 spin off bugs I promised to log:

> originally, there was discussion about changing the software update UI to
> present the user with a choice to do either the major or the minor.  That UI
> was not implemented, but I'll log a spin off bug, in case that is something
> that we are interested in for a future release.

see bug #349579

> I'll log a new bug about the size issue of major upgrade with
> incompatible extensions (or themes) to be solved after this lands.

see bug #349590

> 1) the back button is disabled in the EULA page, as back isn't working.  (the
> back wizard button appears to be disabled in all pages of the software update
> dialog).

see bug #349594

> I'll log a spin off bug, where we can debate adding back the code that makes it
> so on later (or when the user cancels the license page)
> we do not nag the user again in the current session, using this code:

see bug #349597

>  linkify the "Please visit the XXX home page for more information" on
> errors.  (I'll log a spin off bug on this UI request.)

see bug #349600
Comment 146 Darin Fisher 2006-08-21 19:57:33 PDT
Comment on attachment 234412 [details] [diff] [review]
trunk patch, with biesi's suggestions, and using a field for the listener so that there is a reference to it, and so that I can call removeListener() in the dtor

This patch looks good to me, but I don't know the UI code as well as Ben.  You should still get his eyes on this.
Comment 147 (not reading, please use seth@sspitzer.org instead) 2006-09-04 13:10:12 PDT
what's coming next for this bug:

1)  real world testing in the nightly channel, but perhaps also in the beta channel, when go from 2.0b2 -> 2.0rc1, but this depends on the version attribute 2.0rc1 might have, as a "never" decision will persist (in prefs) for major updates.  (See bug #350636).

2)  back porting the patch (and the localization of the new strings) to the 1.8.0.x branch, including the supplimental fix for bug #350636

I'll follow up with preed / morgamic about #1.
Comment 148 Axel Hecht [:Pike] 2006-09-12 07:52:27 PDT
Should we start thinking about the timeline for backporting, and when to offer which update to whom? Comment 37 is mentioning doing that with 2.0.0.1.

Might be worth a separate bug, maybe.
Comment 149 Daniel Veditz [:dveditz] 2006-09-19 15:36:09 PDT
Restoring lost blocking flag
Comment 150 Daniel Veditz [:dveditz] 2006-09-26 15:01:50 PDT
Please ask for approval.8.0.8? when you have a 1.8.0 branch roll-up patch.
Comment 151 (not reading, please use seth@sspitzer.org instead) 2006-09-27 00:47:52 PDT
working on the backport for 1.5.0.8.  I have to do it mostly by hand, due to the difference between the 1.8.0 branch and the 1.8 (1.5.0.x vs 2.0)

The bugs I'm backporting are:

bug #329729 [software updater should handle minor and major updates]
bug #348389 [properly support licenseURL attribute in updates.xml on the client side]
bug #352400 [Update page flashes a large textbox (really, a blank xul:browser) when showing the details for a minor update]
bug #349590 [software update wizard is crowded (too short) for major updates with incompatible extensions]
bug #352675 [add busy throbber to "Please wait..." page in software update wizard, fix scrollbar issue from bug #351821]
bug #351821 [A redundant button  (Download and install now >) appears on 'Software Update' window]
bug #350636 ["Never" decision by the user for a major update should not impact a minor update to the same version]
part of bug #350299 [Software Update wizard no longer loses focus after clicking "Restart Now", if Firefox was not focused when the wizard appeared]

The changes will be to mozilla/tookit/:

themes/pinstripe/global/jar.mn
content/widgets/wizard.xml
locales/en-US/chrome/mozapps/update/updates.dtd
locales/en-US/chrome/mozapps/update/updates.properties
themes/pinstripe/mozapps/update/updates.css
themes/winstripe/mozapps/update/updates.css
mozapps/update/content/updates.js
mozapps/update/content/updates.xml
mozapps/update/content/updates.xul
mozapps/update/src/nsUpdateService.js.in
Comment 152 Benjamin Smedberg [:bsmedberg] 2006-09-27 05:58:11 PDT
Seth, we need to make sure that this code is "missing-string safe" on the 1.8.0 branch: if the new strings needed for this code are not present, I'm not sure what we should do (probably avoid offering the major update entirely). This can be the case when a user has installed a language pack.
Comment 153 (not reading, please use seth@sspitzer.org instead) 2006-10-04 16:36:34 PDT
I've back ported all the patches for this bug, and I'll attach v1 of the patch.

I still need to do a lot of testing as the patch was made by hand, because the files in question have had other changes since we made the MOZILLA_1_8_BRANCH.

The files in question are (from mozilla/toolkit/)

themes/pinstripe/global/jar.mn
content/widgets/wizard.xml
locales/en-US/chrome/mozapps/update/updates.dtd
locales/en-US/chrome/mozapps/update/updates.properties
themes/pinstripe/mozapps/update/updates.css
themes/winstripe/mozapps/update/updates.css
mozapps/update/content/updates.js
mozapps/update/content/updates.xul
mozapps/update/content/updates.xml
mozapps/update/src/nsUpdateService.js.in

Benjamin, sorry for the delay in responding to your comment. 

> we need to make sure that this code is "missing-string safe" on the 1.8.0
> branch: if the new strings needed for this code are not present, I'm not sure
> what we should do (probably avoid offering the major update entirely). This can
> be the case when a user has installed a language pack.

I'n hoping that instead of doing that, I can use existing strings.  I've audited the strings I'm adding, and while it won't be pretty, it looks possible to use strings that exist on the 1.8.0 branch already.
Comment 154 (not reading, please use seth@sspitzer.org instead) 2006-10-04 16:40:21 PDT
here's the result of my audit of the strings I've added to the 1.8 branch and how I do without them on the 1.8.0 branch.

instead of :

+<!ENTITY  license.titleText               "Software License Agreement">
+<!ENTITY  license.introText               "Terms and conditions for using this software.">
+<!ENTITY  license.instructionText         "Please read the following license agreement. Use the scroll bar to view the rest of this agreeme
nt.">

I could use the existing:

 <!ENTITY  license.title                   "License Agreement">
 <!ENTITY  license.intro                   "To install this update you must agree to this license agreement. 
                                            Please read it carefully:">
 <!ENTITY  license.instructions            "If you agree to the terms of this agreement, click I Agree 
                                            below to continue installing this update.">

Instead of:

+<!ENTITY  license.accept                  "I accept the terms of the License Agreement">
+<!ENTITY  license.decline                 "I do NOT accept the terms of the License Agreement">

I could use:

IAgreeLabel=I Agree
IDoNotAgreeLabel=I Disagree

from updates.properties.  

The 1.5.0.x branch doesn't need accesskeys, since that is a 1.8 branch / trunk improvement, so I don't need these:

+<!ENTITY  license.accept.accesskey        "c">
+<!ENTITY  license.decline.accesskey       "T">

Instead of :

+introType_minor_app=An update for %S is available:
+introType_major_app_and_version=You can download and install %S %S now!

I could use:

 introType_minor=An important Security Update for %S is available:
 introType_major=A new version of %S is available:

Instead of:

+licenseContentNotFound=The license file for this version could not be found. Please visit the %S homepage for more information.
+licenseContentDownloading=Getting license for %S %S...

I could use:

 license404Error=The license file could not be found. Please contact the distributor.
 downloadingLicense=Downloading license text...

Instead of:

+updateMoreInfoContentNotFound=Additional details about this version could not be found. Please visit the %S homepage for more information.
+updateMoreInfoContentDownloading=Getting more details about %S %S...

I could use:

failed=Failed
downloading=Downloading

mozilla/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties

Instead of:
 
+updatesfound_minor.title=Update Available
+updatesfound_major.title=New Version Available

I could use:

updateType_minor=Security Update
updateType_major=New Version

and/or

<!ENTITY  updatesfound.title              "Update Available">
 
Instead of:

+downloadButton_minor=Download & Install Now <C2><BB>
+downloadButton_major=Get the new version <C2><BB>

I could use:

<!ENTITY  download.label                  "Download &amp; Install Now &#0187;">

Again, since the 1.5.0.x branch doesn't need accesskeys, since that is a 1.8 branch / trunk improvement, so I don't need these:

+downloadButton_minor.accesskey=D
+downloadButton_major.accesskey=G

Instead of:

+neverButton=Never

I could use mozilla/toolkit/locales/en-US/chrome/global/nsProgressDialog.properties, line 41 -- dontAskAgain=Don't ask me again.

Again, since the 1.5.0.x branch doesn't need accesskeys, since that is a 1.8 branch / trunk improvement, so I don't need these:

+neverButton.accesskey=N
Comment 155 (not reading, please use seth@sspitzer.org instead) 2006-10-04 16:54:44 PDT
Created attachment 241258 [details] [diff] [review]
backport patch (draft, v1)
Comment 156 (not reading, please use seth@sspitzer.org instead) 2006-10-04 17:21:22 PDT
as far as benjamin's suggestion ("if the new strings needed for this code are not present, I'm not sure
what we should do (probably avoid offering the major update entirely)."), I'm worried about what will happen in the entities are missing from the .dtd file because the user has installed a language pack.

That would cause an XML Parsing Error ("undefined entity") and prevent the software update wizard from working at all.

So, perhaps the right solution for the 1.8.0 branch is to make the fix use all properties in bundles (and no entities), and if I detect a property is missing, I ignore the major update.

by ignoring the major update, I should be able to just re-using the existing 1.8.0 strings.

Here are the new entities:

+<!ENTITY  license.titleText               "Software License Agreement">
+<!ENTITY  license.introText               "Terms and conditions for using this software.">
+<!ENTITY  license.instructionText      "Please read the following license agreement. Use the scroll bar to view the rest of this agree
ment.">
+<!ENTITY  license.accept                  "I accept the terms of the License Agreement">
+<!ENTITY  license.decline                 "I do NOT accept the terms of the License Agreement">
Comment 157 Benjamin Smedberg [:bsmedberg] 2006-10-04 18:18:37 PDT
Hrm, when I read the original patch I thought only .properties files had changed. Yes, I think we should try to use only properties files for the added strings.
Comment 158 (not reading, please use seth@sspitzer.org instead) 2006-10-06 11:17:59 PDT
my failure to think ahead about the language pack problem has a price:  the EULA page will not have the nice, new wording from beltzner.  I have to rely on the existing strings already in updates.properties on the MOZILLA_1_8_0_BRANCH.  Luckily, they aren't too different.  (I'll attach screen shots).

Now I've got a patch that doesn't require and changes to updates.dtd.  The only changes necessary are to updates.properties.  The string changes that we need to backport from the MOZILLA_1_8_BRANCH to the MOZILLA_1_8_0_BRANCH (for the various locales) are:

> introType_minor_app=An update for %S is available:
> introType_major_app_and_version=You can download and install %S %S now!
> licenseContentNotFound=The license file for this version could not be found. Please visit the %S homepage for more information.
> updateMoreInfoContentNotFound=Additional details about this version could not be found. Please visit the %S homepage for more information.
> licenseContentDownloading=Getting license for %S %S...
> updateMoreInfoContentDownloading=Getting more details about %S %S...
> updatesfound_minor.title=Update Available
> updatesfound_major.title=New Version Available
> downloadButton_minor=Download & Install Now »
> downloadButton_major=Get the new version »
> neverButton=Never

I'll attach draft 2 of the backport.
Comment 159 (not reading, please use seth@sspitzer.org instead) 2006-10-06 11:22:20 PDT
Created attachment 241471 [details]
how the EULA page will look in 1.5.0.x
Comment 160 (not reading, please use seth@sspitzer.org instead) 2006-10-06 11:25:17 PDT
Created attachment 241472 [details]
how the EULA page will look in 2.0.x
Comment 161 (not reading, please use seth@sspitzer.org instead) 2006-10-06 11:27:44 PDT
In addition to the four text elements being different (title, instructions, agree radio text, decline radio text), the 1.5.0.x version will not have access keys for the radio buttons, as those were defined in updates.dtd (and existing versions did not exist in updates.properties on the MOZILLA_1_8_0_BRANCH.

as a consolation, there are other elements in the update UI (buttons) that don't have access keys on the MOZILLA_1_8_0_BRANCH.  This was fixed for 2.0.
Comment 162 (not reading, please use seth@sspitzer.org instead) 2006-10-06 11:32:47 PDT
Created attachment 241473 [details] [diff] [review]
backport patch (draft, v2)
Comment 163 (not reading, please use seth@sspitzer.org instead) 2006-10-06 13:14:47 PDT
Created attachment 241485 [details] [diff] [review]
backport patch, v3

this patch will gracefully handle missing strings.  in nsUpdateService.js.in, I check the updates.properties bundle for the all the new strings that are required.  If I fail to find any of them, we will act like there is no major update.

For minor updates, i've fixed the software update code to fail gracefully and use the existing strings if it fails to find the new ones.  Note, some of which are in updates.properties and some of which are in updates.dtd.

I'll attach a screen shot of a minor update (which is all that is possible if a string is missing) from this patch.

I'll also enumerate all the test cases that I've run through before seeing a review.
Comment 164 (not reading, please use seth@sspitzer.org instead) 2006-10-06 13:16:23 PDT
Created attachment 241486 [details]
screen shot of build (with patch v3) where all the new strings are missing.
Comment 165 Mike Beltzner [:beltzner, not reading bugmail] 2006-10-06 13:45:43 PDT
Comment on attachment 241471 [details]
how the EULA page will look in 1.5.0.x

I'm OK with this if it's what we have to do, but I thought we'd been talking about being OK adding strings from the 2.0 branch? Or are you saying that they're now packaged in different files, so we can't access them without other changes being backported?

Anyway, this WFM.

Asking cbeard for sr since there might be legal requirements in the phrasing that I'm not aware of.
Comment 166 (not reading, please use seth@sspitzer.org instead) 2006-10-06 13:55:23 PDT
> I'm OK with this if it's what we have to do, but I thought we'd been talking
> about being OK adding strings from the 2.0 branch? Or are you saying that
> they're now packaged in different files, so we can't access them without other
> changes being backported?

We are going to be backporting strings from the MOZILLA_1_8_BRANCH.  But the problem is that some of the new strings I added to the MOZILLA_1_8_BRANCH were to updates.dtd.  Unlike strings that I added to updates.properties, if an entity is missing from updates.dtd, we will get an XML error and the user won't be able to use the software update dialog, even for a minor update.  For strings missing from updates.properties, I handle that in JS.

Note:  missing a string can happen to a 1.5.0.x user when they have installed a language pack, see comment #152 from bsmedberg.

Since it's too late for the MOZILLA_1_8_BRANCH to move the new strings from updates.dtd to updates.properties, the MOZILLA_1_8_0_BRANCH has to make use of what it has.  Luckily, we already had these EULA strings on the MOZILLA_1_8_0_BRANCH!
Comment 167 (not reading, please use seth@sspitzer.org instead) 2006-10-06 16:55:02 PDT
note, that patch (on Mac OS X MOZILLA_1_8_0_BRANCH BUILD) passed all of my test cases.

See http://litmus.mozilla.org/show_test.cgi?type=by_category&product=1&testgroup=38&subgroup=623

now to test on windows.
Comment 168 (not reading, please use seth@sspitzer.org instead) 2006-10-09 10:02:15 PDT
Created attachment 241724 [details]
screen shot of eula page on windows xp from the 1.5.0.x branch with this patch
Comment 169 (not reading, please use seth@sspitzer.org instead) 2006-10-09 10:03:13 PDT
Created attachment 241725 [details]
screen shot with no label and no separator (for cbeard)
Comment 170 (not reading, please use seth@sspitzer.org instead) 2006-10-10 10:27:38 PDT
Created attachment 241840 [details] [diff] [review]
backported strings for locales, draft 1
Comment 171 (not reading, please use seth@sspitzer.org instead) 2006-10-10 10:30:17 PDT
Comment on attachment 241840 [details] [diff] [review]
backported strings for locales, draft 1

well, my script is not going to work (as is), as it is not uft8 (?) friendly
Comment 172 (not reading, please use seth@sspitzer.org instead) 2006-10-10 11:01:54 PDT
Created attachment 241846 [details] [diff] [review]
patch (using native2ascii in my script)
Comment 173 (not reading, please use seth@sspitzer.org instead) 2006-10-10 11:09:38 PDT
first, here is my script:

#!/bin/tcsh
rm $1
cvs up $1
cat ~/Desktop/branch/l10n/$1 | grep "^introType_minor_app="  | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^introType_major_app_and_version="  | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^licenseContentNotFound="  | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^updateMoreInfoContentNotFound="  | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^licenseContentDownloading="  | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^updateMoreInfoContentDownloading="  | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^updatesfound_minor.title="  | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^updatesfound_major.title="  | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^downloadButton_minor="  | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^downloadButton_major="  | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^neverButton="  | native2ascii >> $1
cvs diff $1
cvs diff $1 | wc -l

second, in the 150x tree, we have 40 localizations (with updates.properties files) checked in under the l10n directory:

./ar/toolkit/chrome/mozapps/update/updates.properties
./bg/toolkit/chrome/mozapps/update/updates.properties
./ca/toolkit/chrome/mozapps/update/updates.properties
./cs/toolkit/chrome/mozapps/update/updates.properties
./da/toolkit/chrome/mozapps/update/updates.properties
./de/toolkit/chrome/mozapps/update/updates.properties
./el/toolkit/chrome/mozapps/update/updates.properties
./en-GB/toolkit/chrome/mozapps/update/updates.properties
./es-AR/toolkit/chrome/mozapps/update/updates.properties
./es-ES/toolkit/chrome/mozapps/update/updates.properties
./eu/toolkit/chrome/mozapps/update/updates.properties
./fi/toolkit/chrome/mozapps/update/updates.properties
./fr/toolkit/chrome/mozapps/update/updates.properties
./fy-NL/toolkit/chrome/mozapps/update/updates.properties
./ga-IE/toolkit/chrome/mozapps/update/updates.properties
./gu-IN/toolkit/chrome/mozapps/update/updates.properties
./he/toolkit/chrome/mozapps/update/updates.properties
./hu/toolkit/chrome/mozapps/update/updates.properties
./hy-AM/toolkit/chrome/mozapps/update/updates.properties
./it/toolkit/chrome/mozapps/update/updates.properties
./ja/toolkit/chrome/mozapps/update/updates.properties
./ja-JP-mac/toolkit/chrome/mozapps/update/updates.properties
./ko/toolkit/chrome/mozapps/update/updates.properties
./lt/toolkit/chrome/mozapps/update/updates.properties
./mk/toolkit/chrome/mozapps/update/updates.properties
./mn/toolkit/chrome/mozapps/update/updates.properties
./nb-NO/toolkit/chrome/mozapps/update/updates.properties
./nl/toolkit/chrome/mozapps/update/updates.properties
./nn-NO/toolkit/chrome/mozapps/update/updates.properties
./pa-IN/toolkit/chrome/mozapps/update/updates.properties
./pl/toolkit/chrome/mozapps/update/updates.properties
./pt-BR/toolkit/chrome/mozapps/update/updates.properties
./ro/toolkit/chrome/mozapps/update/updates.properties
./ru/toolkit/chrome/mozapps/update/updates.properties
./sk/toolkit/chrome/mozapps/update/updates.properties
./sl/toolkit/chrome/mozapps/update/updates.properties
./sv-SE/toolkit/chrome/mozapps/update/updates.properties
./tr/toolkit/chrome/mozapps/update/updates.properties
./zh-CN/toolkit/chrome/mozapps/update/updates.properties
./zh-TW/toolkit/chrome/mozapps/update/updates.properties

of these 40, my script failed to backport the new 11 strings (from the l10n directory on the MOZILLA_1_8_BRANCH) for 9 locales:

./es-ES/toolkit/chrome/mozapps/update/updates.properties
./he/toolkit/chrome/mozapps/update/updates.properties ./hy-AM/toolkit/chrome/mozapps/update/updates.properties ./it/toolkit/chrome/mozapps/update/updates.properties    ./ja/toolkit/chrome/mozapps/update/updates.properties ./ja-JP-mac/toolkit/chrome/mozapps/update/updates.properties ./ro/toolkit/chrome/mozapps/update/updates.properties
./ru/toolkit/chrome/mozapps/update/updates.properties
./zh-CN/toolkit/chrome/mozapps/update/updates.properties

I'm looking into why...
Comment 174 (not reading, please use seth@sspitzer.org instead) 2006-10-10 11:11:54 PDT
> I'm looking into why...

ah, it's because some locales have:

neverButton = Nunca

instead of 

neverButton= Nunca

new patch coming soon...
Comment 175 (not reading, please use seth@sspitzer.org instead) 2006-10-10 11:17:56 PDT
a new version of the script:

#!/bin/tcsh
rm $1
cvs up $1
cat ~/Desktop/branch/l10n/$1 | grep "^introType_minor_app"  | grep -v .accesskey | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^introType_major_app_and_version"  | grep -v .accesskey | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^licenseContentNotFound"  | grep -v .accesskey | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^updateMoreInfoContentNotFound"  | grep -v .accesskey | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^licenseContentDownloading"  | grep -v .accesskey | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^updateMoreInfoContentDownloading"  | grep -v .accesskey | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^updatesfound_minor.title"  | grep -v .accesskey | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^updatesfound_major.title"  | grep -v .accesskey | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^downloadButton_minor"  | grep -v .accesskey | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^downloadButton_major"  | grep -v .accesskey | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^neverButton"  | grep -v .accesskey | native2ascii >> $1
cvs diff $1
cvs diff $1 | wc -l
Comment 176 (not reading, please use seth@sspitzer.org instead) 2006-10-10 11:26:01 PDT
Created attachment 241850 [details] [diff] [review]
patch (covers 37/40 locales, 3 do not have strings to backport from the 1.8 branch)
Comment 177 (not reading, please use seth@sspitzer.org instead) 2006-10-10 11:31:11 PDT
three locales were not backported:

./he/toolkit/chrome/mozapps/update/updates.properties
./hy-AM/toolkit/chrome/mozapps/update/updates.properties
./ro/toolkit/chrome/mozapps/update/updates.properties

These locales do not have any of the new 11 strings on the MOZILLA_1_8_BRANCH for me to backport. 

Note, this means that those 3 locales are going have problems with FF 2.0 and software update, even a minor update. as there are strings missing.

For example, the "ro" locale is missing license.instructionText from updates.dtd (and other entities, as well as the 11 new strings from updates.properties) so that locale will get an undefined xml identity in the software update dialog.
Comment 178 Axel Hecht [:Pike] 2006-10-10 11:58:29 PDT
Comment on attachment 241850 [details] [diff] [review]
patch (covers 37/40 locales, 3 do not have strings to backport from the 1.8 branch)

Locales without trailing newline are broken, I found that in fr.

Regarding he, hy-AM, and ro: Those won't ship with 2.0, but I'd like to know what happens if we ship them in 2.0.0.1, and don't get the strings for 1.8.0.x branch in the right timeframe. Or rather, what's the right timeframe there?
Comment 179 Axel Hecht [:Pike] 2006-10-10 12:02:07 PDT
Tomer, could you take your current work towards 2.0.0.x and create a patch that backports that to the 1.8.0.x branch, like attachement 241850 does for other locales? Thanks.
Comment 180 (not reading, please use seth@sspitzer.org instead) 2006-10-10 13:17:00 PDT
> Locales without trailing newline are broken, I found that in fr.

axel, can you elaborate?  In my patch, there are three .properties files that I now have the newline (where we didn't before).

(for example, cs/toolkit/chrome/mozapps/update/updates.properties)

> Regarding he, hy-AM, and ro: Those won't ship with 2.0

thanks for the info.

> but I'd like to know
> what happens if we ship them in 2.0.0.1, and don't get the strings for 1.8.0.x
> branch in the right timeframe. Or rather, what's the right timeframe there?

If the strings are not part of 1.5.0.8, it prevents us from offering a major update to 1.5.0.8 -> 2.0.0.1

But, if we had them later, we could add the strings for 1.5.0.9, to do do 1.5.0.9 -> 2.0.0.1

Does that answer your questions?
Comment 181 Axel Hecht [:Pike] 2006-10-10 14:43:33 PDT
(In reply to comment #180)
> > Locales without trailing newline are broken, I found that in fr.
> 
> axel, can you elaborate?  In my patch, there are three .properties files that I
> now have the newline (where we didn't before).
> (for example, cs/toolkit/chrome/mozapps/update/updates.properties)

-updaterIOErrorText=Un ou plusieurs fichiers n'ont pas pu \u00eatre mis \u00e0 jour. Veuillez v\u00e9rifier qu'aucune autre application n'est lanc\u00e9e et que vous avez bien la permission de modifier les fichiers. Ensuite, relancez %S pour essayer \u00e0 nouveau.
\ No newline at end of file
+updaterIOErrorText=Un ou plusieurs fichiers n'ont pas pu \u00eatre mis \u00e0 jour. Veuillez v\u00e9rifier qu'aucune autre application n'est lanc\u00e9e et que vous avez bien la permission de modifier les fichiers. Ensuite, relancez %S pour essayer \u00e0 nouveau.introType_minor_app=Une mise \u00e0 jour pour %S est disponible\u00a0:


As you can see, the first added line is concated to the old previous line, breaking both.

> > Regarding he, hy-AM, and ro: Those won't ship with 2.0
> 
> thanks for the info.
> 
> > but I'd like to know
> > what happens if we ship them in 2.0.0.1, and don't get the strings for 1.8.0.x
> > branch in the right timeframe. Or rather, what's the right timeframe there?
> 
> If the strings are not part of 1.5.0.8, it prevents us from offering a major
> update to 1.5.0.8 -> 2.0.0.1
> 
> But, if we had them later, we could add the strings for 1.5.0.9, to do do
> 1.5.0.9 -> 2.0.0.1
> 
> Does that answer your questions?

I think we should add English strings for those locales. I have good hopes to get Hebrew strings, which is what I CCed Tomer for.
If we don't add English strings, it sounds like we'd have to not push updates for those locales until we have successfully backported the strings. That's a nice-to-have, and I'd welcome to have bugs for each locale on that, but I doubt that we'll manage that right, with all required testing and such, and want to pay the price for it. We should just release the outstanding locales as soon as they're ready, and not have to hold for some backporting patch.
Comment 182 (not reading, please use seth@sspitzer.org instead) 2006-10-10 15:08:11 PDT
> As you can see, the first added line is concated to the old previous line,
breaking both.

whoops!  Thanks for catching that.  Fixing my script to deal with that, and I'll attach a new patch.

> I think we should add English strings for those locales.

ok, I will do that.

> If we don't add English strings, it sounds like we'd have to not push updates
> for those locales until we have successfully backported the strings.

Actually, that's not the case.  If the AUS server side advertizes a major update to a 1.5.0.8 client with this patch, but the the client doesn't have the strings, we will fail gracefully, and ignore the major update.

I'll attach a patch with the english versions of the strings for those three locales for the MOZILLA_1_8_0_BRANCH, and then log three bug on fixing the localizations in on both the MOZILLA_1_8_BRANCH and the MOZILLA_1_8_0_BRANCH.

question for axel:  should I also add the english version of the missing strings to the MOZILLA_1_8_BRANCH and the trunk?
Comment 183 (not reading, please use seth@sspitzer.org instead) 2006-10-10 15:35:38 PDT
here's a new version of the script:

#!/bin/tcsh
rm $1
cvs up $1
echo "" >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^introType_minor_app"  | grep -v .accesskey | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^introType_major_app_and_version"  | grep -v .accesskey | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^licenseContentNotFound"  | grep -v .accesskey | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^updateMoreInfoContentNotFound"  | grep -v .accesskey | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^licenseContentDownloading"  | grep -v .accesskey | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^updateMoreInfoContentDownloading"  | grep -v .accesskey | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^updatesfound_minor.title"  | grep -v .accesskey | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^updatesfound_major.title"  | grep -v .accesskey | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^downloadButton_minor"  | grep -v .accesskey | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^downloadButton_major"  | grep -v .accesskey | native2ascii >> $1
cat ~/Desktop/branch/l10n/$1 | grep "^neverButton"  | grep -v .accesskey | native2ascii >> $1
cvs diff $1
cvs diff $1 | wc -l

the way I use it is I have two trees:  ~/Desktop/150x and ~/Desktop/branch

from ~/Desktop/150x/l10n, I do:

$ find . -name "updates.properties" -print | xargs -n1 ~/my-script.tcsh

for these files, I added the english version of the strings by hand:

./ro/toolkit/chrome/mozapps/update/updates.properties ./he/toolkit/chrome/mozapps/update/updates.properties ./hy-AM/toolkit/chrome/mozapps/update/updates.properties

Comment 184 (not reading, please use seth@sspitzer.org instead) 2006-10-10 15:40:26 PDT
Created attachment 241886 [details] [diff] [review]
patch (covers 37/40 locales, 3 do not have strings for are getting the en-US version, per axel)
Comment 185 (not reading, please use seth@sspitzer.org instead) 2006-10-10 15:42:15 PDT
> and then log three bug

see bug #356220, #356221 #356223.
Comment 186 Axel Hecht [:Pike] 2006-10-10 16:24:00 PDT
Comment on attachment 241886 [details] [diff] [review]
patch (covers 37/40 locales, 3 do not have strings for are getting the en-US version, per axel)

Sorry, found the next problem. Apparently, native2ascii does the wrong thing.

I think we should just use utf-8 strings as in the original source, there shouldn't be any problem with that.

What happens is that each non-ascii character seems to be converted to two unicode escapes. In particular, for German (which is the only language I can really check) it says "f\u221a\u00bar" for "für", which looks like it's converting the two bytes of the utf-8 encoding separately. Bad mojo. (And it looks like foobar for a good reason.)
Comment 187 Axel Hecht [:Pike] 2006-10-10 16:36:21 PDT
A question for the danish team, I think I found a bug in http://lxr.mozilla.org/l10n-mozilla1.8/source/da/toolkit/chrome/mozapps/update/updates.properties#60, where it says "AldrigNever", I think that should just be "Aldrig". Is that right?

Additional comment on the patch, seems like cs, de, and fr have their trailing newline problems resolved.
Comment 188 (not reading, please use seth@sspitzer.org instead) 2006-10-10 16:50:33 PDT
> A question for the danish team, I think I found a bug in
> http://lxr.mozilla.org/l10n-mozilla1.8/source/da/toolkit/chrome/mozapps/update/updates.properties#60,
> where it says "AldrigNever", I think that should just be "Aldrig". Is that
>right?

see bug #356232
Comment 189 (not reading, please use seth@sspitzer.org instead) 2006-10-11 17:42:45 PDT
Created attachment 242009 [details] [diff] [review]
patch, danish is fixed but still missing 3 locales, so using english (I have "he" in another bug)
Comment 190 (not reading, please use seth@sspitzer.org instead) 2006-10-11 17:46:35 PDT
Comment on attachment 242009 [details] [diff] [review]
patch, danish is fixed but still missing 3 locales, so using english (I have "he" in another bug)

axel, can you review one more time?  note, compare this version with the previous one and you will see differences.

this one I'm using a perl script I wrote (instead of cat and grep), something dveditz suggested.
Comment 191 Axel Hecht [:Pike] 2006-10-12 08:56:03 PDT
Comment on attachment 242009 [details] [diff] [review]
patch, danish is fixed but still missing 3 locales, so using english (I have "he" in another bug)

Yep, this looks good.

Nevertheless, we should do some spot check testing of this, if anyhow possible.

Seth mentioned on irc that he would like to include the strings from attachement 241998 for hebrew, that's OK with me.
Comment 192 (not reading, please use seth@sspitzer.org instead) 2006-10-12 20:47:56 PDT
Created attachment 242142 [details] [diff] [review]
supplimental patch for he locale
Comment 193 Lil' Timmy Spitzer 2006-10-12 21:59:23 PDT
Dad, is this where you keep your blog?
Comment 194 (not reading, please use seth@sspitzer.org instead) 2006-10-16 13:23:54 PDT
Created attachment 242435 [details] [diff] [review]
supplimental patch for he locale, v2 (based on new patch in bug #356221)
Comment 195 Daniel Veditz [:dveditz] 2006-10-17 16:57:43 PDT
Comment on attachment 241485 [details] [diff] [review]
backport patch, v3

> updateName=%S %S
> updateFullName=%S (%S)
>+introType_major_app_and_version=You can download and install %S %S now!
>+licenseContentDownloading=Getting license for %S %S...
>+updateMoreInfoContentDownloading=Getting more details about %S %S...

Shouldn't these double-replacements use the %1$S %2$S syntax, plus
a comment explaining to the translators what each part is?

> introType_minor=An important Security Update for %S is available:
>+introType_minor_app=An update for %S is available:

We're no longer telling users it's an important security update?

>+    if (severity == "major") {
     ...
>+        "introType_" + severity + "_app_and_version", 

This seems silly, there is only one app_and_version string and we know
severity == "major". It'd be much clearer just to use the full
literal "introType_major_app_and_version"

>+    else {
      ...
>+          "introType_" + severity + "_app", [gUpdates.brandName]);

ditto "introType_minor_app"

>+      catch (ex) {
>+        intro = gUpdates.strings.getFormattedString(
>+          "introType_major", [gUpdates.brandName]);
>+      }

Why not the existing introType_minor?

>Index: mozapps/update/content/updates.xul
>===================================================================
> <!ENTITY % updateDTD SYSTEM "chrome://mozapps/locale/update/updates.dtd">
> <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd">
>+<!ENTITY % pippkiDTD SYSTEM "chrome://pippki/locale/pippki.dtd">

You may want to comment that this is to re-use strings and limited
to the 1.8.0 branch

>+++ themes/pinstripe/global/jar.mn	6 Oct 2006 20:12:25 -0000
>@@ -97,16 +97,17 @@ classic.jar:
>++  skin/classic/global/icons/notfound.png                             (icons/notfound.png)

I couldn't find this referenced anywhere in the patch. If it is used,
should it go in the winstripe theme too?

r=dveditz
Comment 196 Daniel Veditz [:dveditz] 2006-10-17 17:05:04 PDT
Comment on attachment 242009 [details] [diff] [review]
patch, danish is fixed but still missing 3 locales, so using english (I have "he" in another bug)

approved for 1.8.0 branch, a=dveditz
Comment 197 Daniel Veditz [:dveditz] 2006-10-17 17:06:01 PDT
Comment on attachment 242435 [details] [diff] [review]
supplimental patch for he locale, v2 (based on new patch in bug #356221)

approved for 1.8.0 branch, a=dveditz
Comment 198 Daniel Veditz [:dveditz] 2006-10-17 17:47:30 PDT
Comment on attachment 241485 [details] [diff] [review]
backport patch, v3

approved for 1.8.0 branch, a=dveditz
Comment 199 (not reading, please use seth@sspitzer.org instead) 2006-10-17 23:31:07 PDT
dan, thanks for the review.  

here are the answers to your comments:

1)

> updateName=%S %S
> updateFullName=%S (%S)
>+introType_major_app_and_version=You can download and install %S %S now!
>+licenseContentDownloading=Getting license for %S %S...
>+updateMoreInfoContentDownloading=Getting more details about %S %S...

> Shouldn't these double-replacements use the %1$S %2$S syntax, plus
> a comment explaining to the translators what each part is?

Yes, you are right.  I'm not sure what I was thinking, other than I was following the existing updateName properties.  I'll log a bug to fix this on the trunk. 

2)

> introType_minor=An important Security Update for %S is available:
>+introType_minor_app=An update for %S is available:

> We're no longer telling users it's an important security update?

We never did.  The old code did this:

var severity = gUpdates.update.isSecurityUpdate ? "minor" : "major";
var intro = gUpdates.strings.getFormattedString(
   "introType_" + severity, [gUpdates.brandName]);

We've never had isSecurityUpdate be true, so it's always been:

introType_major=A new version of %S is available:

The new version is "introType_minor_app=An update for %S is available:", per beltzner.

3)

>+    if (severity == "major") {
     ...
>+        "introType_" + severity + "_app_and_version", 

This seems silly, there is only one app_and_version string and we know
severity == "major". It'd be much clearer just to use the full
literal "introType_major_app_and_version"

>+    else {
      ...
>+          "introType_" + severity + "_app", [gUpdates.brandName]);

ditto "introType_minor_app"

You are right, it is silly.  I'll log a bug and clean that up on the trunk.

4)

>+      catch (ex) {
>+        intro = gUpdates.strings.getFormattedString(
>+          "introType_major", [gUpdates.brandName]);
>+      }

Why not the existing introType_minor?

The new version is "introType_minor_app=An update for %S is available:", per beltzner.  I have the new string, so I'm using it.

5)

>Index: mozapps/update/content/updates.xul
>===================================================================
> <!ENTITY % updateDTD SYSTEM "chrome://mozapps/locale/update/updates.dtd">
> <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd">
>+<!ENTITY % pippkiDTD SYSTEM "chrome://pippki/locale/pippki.dtd">

You may want to comment that this is to re-use strings and limited
to the 1.8.0 branch

The same trick was necessary on the 1.8 branch as well, see:

http://lxr.mozilla.org/mozilla1.8/source/toolkit/mozapps/update/content/updates.xul#47
http://lxr.mozilla.org/mozilla1.8/source/toolkit/mozapps/update/content/updates.xul#110

In both branches, I've got this message in updates.xul:

+            <!-- since we are after the string freeze, re-use the "Please Wait..."
+                 string from pippki.dtd -->

The trunk didn't require this trick.  Do you still feel another comment in necessary?

6)

>+++ themes/pinstripe/global/jar.mn	6 Oct 2006 20:12:25 -0000
>@@ -97,16 +97,17 @@ classic.jar:
>++  skin/classic/global/icons/notfound.png                             (icons/notfound.png)

I couldn't find this referenced anywhere in the patch. If it is used,
should it go in the winstripe theme too?

Sorry for the confusion.  On the branch, the css for this already exists in updates.css for both themes, but the icon was not there.
See http://lxr.mozilla.org/mozilla1.8.0/source/toolkit/themes/pinstripe/mozapps/update/updates.css
Comment 200 (not reading, please use seth@sspitzer.org instead) 2006-10-18 00:20:47 PDT
Created attachment 242610 [details] [diff] [review]
complete l10n patch

carrying over axel's r= and dan's a=
Comment 201 (not reading, please use seth@sspitzer.org instead) 2006-10-18 01:28:41 PDT
fix landed on the MOZILLA_1_8_0_BRANCH.

axel will be landing the complete l10n patch today (thanks axel!)
Comment 203 Daniel Veditz [:dveditz] 2006-10-18 10:22:31 PDT
Comment on attachment 242610 [details] [diff] [review]
complete l10n patch

approved for 1.8.0 branch, a=dveditz
Comment 204 (not reading, please use seth@sspitzer.org instead) 2006-10-18 14:41:44 PDT
to follow up:

1)

>> Shouldn't these double-replacements use the %1$S %2$S syntax, plus
>> a comment explaining to the translators what each part is?
>
>Yes, you are right.  I'm not sure what I was thinking, other than I was
>following the existing updateName properties.

on the MOZILLA_1_8_0_BRANCH, I added these three comments before checking in:

# Note to localizers:  %S %S will become something like "Firefox 2.0"
introType_major_app_and_version=You can download and install %S %S now!
# Note to localizers:  %S %S will become something like "Firefox 2.0"
licenseContentDownloading=Getting license for %S %S...
# Note to localizers:  %S %S will become something like "Firefox 2.0"
updateMoreInfoContentDownloading=Getting more details about %S %S...

logged bug #357145 on this issue

2)

>>This seems silly, there is only one app_and_version string and we know
>>severity == "major". It'd be much clearer just to use the full
>>literal "introType_major_app_and_version"
>>...
>>ditto "introType_minor_app"
>
>You are right, it is silly.  I'll log a bug and clean that up on the trunk.

logged bug #357143 on this issue

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