When a user applies a software update, the version numbers are checked against the installed extensions and themes and the user is warned that any incompatabilities will result in the extension or theme being disabled. It currently lists extensions and themes which have compatibility updates available (at the extension's update location), resulting in a pessimistic list. This abundance of caution will likely result in many users cancelling the update to protect their download, or simply causing undue worry/stress. Instead, the updater should check for available extension updates, and if compatability updates are available for extension A, then extension A should not be included in the list and instead should be automatically after the application update. (note: I'm pretty sure this is a dupe against something that came up pre 1.5, but I can't remember/find that bug after looking for hours)

This screenshot shows the warning message; the first two entries in that list actually have updates available which make them compatible with 1.5.0.* (by bumping the maxVer), but the user doesn't know that and might be worried about losing the extension's function so might cancel the update.

Fixing unintended change to Target Milestone, sorry for bugspam.

As described in comment #0 and comment #1 this seems like an edgecase. We should already be checking for / applying updated compatibility info every 24 hours so for this to provide value for a user they would have to have an extension installed that has had its compatibility info updated within that 24 hours period when they upgrade which is unlikely with normal usage though would be pretty easy to reproduce artificially.

I don't think it's that unlikely: we're seeing people experience this already with the 1.5.0.1 beta channel, and they aren't going out of their way to trigger it. I also suspect strongly that we will get a rash of last-minute extension version updates as authors see that there's a new push on the horizon -- or possibly after the update goes out and the mis-versioning is reported to them. So the "last 24 hours" case may not be especially rare, even ignoring the fact that we might not be connected when the timer fires (do we queue it up for next connection, or just wait on the timer again?), etc. We should be very aggressive about making sure that we have all version (and in the future blacklist) info up to date when we tell the user about the compatibility effects of their update.

Not sure how that applies. Going from 1.5.0.0 to 1.5.0.1 should not cause an extension to be made incompatible if it is using 1.5.0.* for the maxVersion (e.g. the recommended value that is also used on AMO). What I do know is seen on the beta channel are people that have forced compatibility for an extension (e.g. with Nightly Tester Tools or Local Install which uses the code from Nightly Tester Tools) and the targetApplication maxVersion is being reset during the first run on upgrade which is due to bug 324084.

bah - it is due to Bug 303718.

I think metaphors are being mixed. :P A compatibility update is where only the targetApplication maxVersion has changed (technically minVersion as well). It appears that the items mentioned in comment #1 were not made compatible during the compatibility update and required an updated extension to be installed for them to be compatible. Perhaps what you are looking for is the software update wizard to check if there is an updated version of the extension that is compatible with the upgraded version of the application?

Attaching at rob strong's request.

I checked Tabbrowser Prefs and found the following: The installed version is 1.2.8.7 and it has a targetApplication maxVersion of 1.5 The update rdf has no data for this version so a compatibility update will never be applied to the installed version. The update rdf has a newer version that has a targetApplication maxVersion of 1.5.0.* which would make it compatible with 1.5.0.1 So, it appears that updating compatibility info at least for this one extension would not solve this bug whereas checking / installing the updated version would.

I talked with rob in the office today, and we worked the terminology thing out (and I got some more information about how all this stuff works - very illuminating!) Here's where we are: when an application update is applied, a check is made against install.rdf to compare the new appVer with the list of maxVers for the installed extensions and themes. If any maxVer < appVer, then the user is presented with a warning message saying that "some themes and extensions may not work with this [mandatory] update" and they're offered to see a list of precisely which valuable functionality they'll lose by applying the security update. This forces users into a choice between a) invisible security update and b) the extension they wanted enough to brave our extension installation system for. That's a no-brainer for the user, and they'll always choose b) We, however, want them to choose a). It's the best thing for them. Part of our way of doing this is not breaking extensions. Part of it is ensuring that if we do, we get the word to those extension authors out there and have them make changes and release a new version with a maxVer >= appVer. The last key to the puzzle is to change what happens when an appUpdate is available. At that time, we should - check against install.rdf to see if addons' maxVer < appVer - if so use the updateUrl to see if a new version of the addon exists where maxVer >= appVer - if no such update can be found, show the warning; otherwise, don't show the warning After the update is applied, compatability updates will be applied, and then we already show the list of extensions which need updates in order to be compatible, and offer an "Update Now" button which gives the user the choice of doing just that. The net effect change is that we only show them a dissuading warning when we absolutely know that there's no way for that addon to ever work with the about-to-be-installed appUpdate.

s/install.rdf/extensions.rdf/ for comment 10 w/apologies :(

It looks like work started on this but it wasn't finished http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/update/src/nsUpdateService.js.in#2759

So, when we add back extension manager update notification will this bug still need to be fixed? I personally would prefer not to keep track of the extra metadata required to accomplish this or have the code check the update rdf's for the items that fall into this category. In other words this would only affect users that a) don't update their extensions b) users that upgrade within the sliding 24 hour window of an extension they have installed that is incompatible with the new application version that has an updated version they could install that is compatible. Better still, when we have extension manager update notification the app update service could trigger an update check, the user could then choose to install any updates that are available, and then app update could do its thing.

We absolutely need to have this, as users will hold off on upgrades if their extensions will break. Barriers to upgrading have to be eliminated, in those users' best interests. Consider the following common scenario (especially when we upgrade major releases): An extension with versions X and Y, which are only compatible with app versions A and B, respectively. The user has version X of the extension installed with app version A, and the update to version B is available. We can't just upgrade their extension first, because it isn't compatible. We can't just tell them version X isn't compatible and will be disabled, because they won't upgrade. We have to know that the new appversion will have a compatible update, and just install it on upgrade. We really also need to get a fix into the last 1.5.0.x release before Firefox 2, so that users are not blocked on upgrading nased on false information.

What I am planning on doing for this is providing a list of the add-ons that are installed / enabled / incompatible with the app version that will be installed and according to the info in the add-on's update rdf don't have a compatible version available. On upgrade the upgrade compatibility wizard for add-ons will display all add-ons that are incompatible as it does today and this will include add-ons that aren't in the app upgrade incompatible list. Also, if the user clicks cancel in the wizard which can be done before the list of incompatible add-ons is displayed then all incompatible add-ons will just be disabled. Not the best user experience IMO but it is better than where we are now. Perhaps we should display a warning when the user cancels instead? I already fixed most if not all of the issues with installing an add-on when the app is upgraded and it should be possible to stage the updated versions of the add-ons before the app restarts for an upgrade. Perhaps this would be something we want for Firefox 3.0?

Rob - any chance we g

Minusing for the 2.0 release, but we should consider this for a security and stable release (2.0.0.1?) for going from 2.0 -> 3.0.

(In reply to comment #14; mconnor) > We absolutely need to have this, as users will hold off on upgrades if their > extensions will break. Barriers to upgrading have to be eliminated, in those > users' best interests. I agree; I wish I'd seen the [at risk] addition a week ago, or really known what it meant, so that I could have tried to find a way to make this work, or discussed it when I was in MtV last week. If it had made b2, we might well have been able to look at bringing it into a 1.5.0.6 to make that update easier, but without that capability in a 1.8.1 beta there's really no credible way to push for that. Remind me to watch this bug like a hawk in the 2.0.1 cycle! :( Mike

I'm going to try to finish up the patch for this in the next day or two... a couple of other bugs were pushed to be included into 2.0 distracted me from finishing this. :(

Rob, please nominate for approval1.8.1 if you can get it done. Are we still uiwanted on this, or combined with the fixes for the app update compatibility wizard, are we cool? I can't really remember ...

I haven't done a review of the existing software update compatibility wizard but when you and I discussed this there were a couple of things you wanted to change. At that time I asked you to have someone else take on the ui changes you wanted done (mostly not related to this) and that I would handle the backend changes to the Extension Manager so it would return a list of extensions as described in this bug. IMO ui changes aren't needed to fix this bug but they may be desired for other reasons.

Duplicate of bug 302059?

Should this make Firefox 3? Not sure if it is still relevant.

This does not block the final release of Firefox 3.

Mossop: if you're bored, can you take a look at this? It helps with the major upgrade experience a lot. Not sure (as per comment 21) that new UI is needed, as opposed to the ability to check for compatibility updates before showing the list of incompatible addons.

Though the UI is part of App Update this needs to be provided by an interface of the Add-ons Manager code so moving over there where the actual work needs to be done.

removing uiwanted keyword... we already have ui and any significant changes to the ui should be done in a separate bug.

Dave, I suspect there are additional tests that should be done... suggestions welcome

This is for the most part an update of Clare's original patch... still need to do some cleanup and testing.

Comment on attachment 336999 [details] [diff] [review] patch rev1 - still needs work note: the ui on this needs work... beltzner said he though it might be a good idea to put the incompatible add-on list on another wizard page and this would make the ui a lot easier to fix.

Comment on attachment 336999 [details] [diff] [review] patch rev1 - still needs work Forgot to include the changes to nsUpdateService.js.in but it needs more work too so I am going to hold off a bit.

Adding dependency on bug 453870... I want bug 453870 prior to making the changes to nsUpdateService.js.in for this bug in order to simplify the changes.

This should be pretty good to go for the EM work that needs to be done. I'm going to patch the update service in a separate patch but it should prevent getting this reviewed and landed.

Comment on attachment 337134 [details] [diff] [review] EM patch rev2 w/ tests > checkForUpdates: function ExtensionItemUpdater_checkForUpdates(aItems, > aItemCount, > aUpdateCheckType, >+ aUpdateNewAvailable, >+ aAppVersion, >+ aPlatformVersion, > aListener) { aUpdateNewAvailable is a terrible name... suggestions welcome

(In reply to comment #36) > This should be pretty good to go for the EM work that needs to be done. I'm > going to patch the update service in a separate patch but it should prevent > getting this reviewed and landed. bah... that should be "but it *shouldn't* prevent getting this reviewed and landed."

Comment on attachment 337134 [details] [diff] [review] EM patch rev2 w/ tests I'm not too keen on the API for this method. There are I think two alternatives that we could use. The simplest would be to add the versions as optional parameters to the existing update method, and add a new updateCheckType for it. I think maybe a better but slightly more involved option, though I'd appreciate your thoughts, would be to add a new update method as you have but include a behaviour option, and then the versions on the end as optional. The behaviour would act a little like the updateCheckType but be more descriptive about what the caller wants. There are a few different aspects it could control: Updating compatibility info in the datasource, the caller may never want this, always want it, or only want it if there is new information. Remembering update info, the caller may or may not want this. Notifying the listener, the caller may only want to hear about new versions, or the existing version, or the new version in preference to existing (might even want an option to see both current and new versions in the future). That could be passed as 3 different arguments, or just one in a bitfield I guess. All of the existing updateCheckTypes are covered in those options so the old update method could still be kept there for compatibility easily. There are a good bunch of uses I can think of app and extension developers where they don't want the datasource touched, we have them too such as the install time compatibility check. What do you think to this? A couple of comments on the existing patch. It looks like this will still add new version info into the datasource as it is since that gets added through the onAddonUpdateEnded on the datasource. I wonder if we should be ditching the AddonUpdateCheckListener which passes out notification to both the caller's listener and the datasource and instead just update the datasource within the RDFItemUpdater? I think we need to include the app version in the update url since the remote site may return different rdfs depending on that. However I suspect we probably want to be including existing app version as well. Maybe %APP_VERSION% should be the version of the app we are checking for and add a new APP_CUR_VERSION or so. The tests look good. It might be useful, as this is a new API, to include a check that an add-on with a remote compatibility update does what we expect.

requesting wanted1.9.1 flag. Drivers, Please approve.

also requesting blocking1.9.1 flag. Mossop tells me patches are in, tests are in. Just need reviews, but should be good to go for beta 1.

This won't update the extensions datasource with updated compatibility info when update is called with an update type of UPDATE_NOTIFY_NEWVERSION. We do that on first run after an upgrade anyways so I'm not worried about that.

Comment on attachment 340293 [details] [diff] [review] EM patch rev3 w/ tests I haven't convinced myself that changing the UPDATE_ values is worth it... new patch coming up with that change

Comment on attachment 340302 [details] [diff] [review] EM patch rev4 w/ tests Looks good, just a few things: > checkForUpdates: function ExtensionItemUpdater_checkForUpdates(aItems, > aItemCount, > aUpdateCheckType, >- aListener) { >- this._listener = new AddonUpdateCheckListener(aListener, this._emDS); >+ aListener, >+ aAppVersion, >+ aPlatformVersion) { >+ if (aUpdateCheckType == Ci.nsIExtensionManager.UPDATE_NOTIFY_NEWVERSION) >+ this._listener = new AddonUpdateCheckListener(aListener, null); Can't you just say |this._listener = aListener| here? We seem to null check this._listener everywhere and all it does at this point is redirect calls to aListener. You don't need the null checks for this._ds in there in that case either. >+ else >+ this._listener = new AddonUpdateCheckListener(aListener, this._emDS); >+ > if (this._listener) > this._listener.onUpdateStarted(); > this._updateCheckType = aUpdateCheckType; > this._items = aItems; > this._responseCount = aItemCount; >+ this._appVersion = aAppVersion ? aAppVersion : gApp.version; >+ this._platformVersion = aPlatformVersion ? aPlatformVersion >+ : gApp.platformVersion; Can you add a test that we are only using a different version if aUpdateCheckType is CHECK_NOTIFY_NEWVERSION. If an app/extension called update with SYNC_COMPATIBILITY and whatever version we'd end up in a bad state. We still need to add the extra version parameter to the updateURL I think.

I've started discussions of the change to extensions.update.url with morgamic. He is fine with using currentAppVersion but I am going to discuss this with the people doing metrics before landing. As is, this should be good to go though the order of the updateURL may change.

Comment on attachment 340460 [details] [diff] [review] EM patch rev5 w/ tests >diff --git a/toolkit/mozapps/extensions/public/nsIExtensionManager.idl b/toolkit/mozapps/extensions/public/nsIExtensionManager.idl >--- a/toolkit/mozapps/extensions/public/nsIExtensionManager.idl >+++ b/toolkit/mozapps/extensions/public/nsIExtensionManager.idl >... >+ * @param appVersion (optional) >+ * The version of the application to check for compatible updates. >+ * Defaults to the current version of the application. >+ * @param platformVersion (optional) >+ * The version of the toolkit to check for compatible updates. >+ * Defaults to the current version of the toolkit. I'll add a comment about these two values only being honored for the UPDATE_NOTIFY_NEWVERSION case

This fixes this bug for the background check case. Still need to do the foreground check case. So far the change to the default add-ons update url is a go.

I'd like to change the downloading wizard page to make it so the download counters are less likely to wrap, to make this more like the download manager, and to increase the space for the download stats for Bug 414326. The screenshot doesn't have the padding, etc. set correctly or the final images yet... I just want to find out if this approach is acceptable.

Look good Rob - I like the incompatible add-ons shown on a separate page. Only question is the "Not Now" button, which seems to imply that the user will be asked again or that the installation is being paused rather than stopped. Perhaps "Cancel" would be better to relate that the termination of the install is complete?

The Details link is used in a couple of other places so I kept it in the downloading page. I'm somewhat leaning towards the removal of the throbber since it is a tad redundant since there is already a progress meter. If it is kept should it remain in the original position to the left of the update download stats or to the left of the update download name?

> The Details link is used in a couple of other places so I kept it in the > downloading page. I'm somewhat leaning towards the removal of the throbber > since it is a tad redundant since there is already a progress meter. If it is > kept should it remain in the original position to the left of the update > download stats or to the left of the update download name? A better alternative to the throbber may be to give the loading bar just a small bit of animation. The only purpose of the throbber is so that the user knows that the process has not stopped when they have little feedback for it - so if the throbber is shifting slightly, such as pulsing a subtle gradient left to right, this would eliminate the need for the separate throbber. If it's impossible to move the throbber, I'd vote for having it next to "Downloading" (first pic in screenshot) rather than to the left of the MB done. This is because the upper left placement is 1. easier to scan for at the top/first corner 2. makes more sense as an indication of "downloading" (a continual process) than "5.2 MB" (a static amount that jumps at intervals)

(In reply to comment #53) > A better alternative to the throbber may be to give the loading bar just a > small bit of animation. The only purpose of the throbber is so that the user > knows that the process has not stopped when they have little feedback for it - > so if the throbber is shifting slightly, such as pulsing a subtle gradient left > to right, this would eliminate the need for the separate throbber. The progress meter doesn't have support for that beyond the incrementing of the progress as the download completes. The download manager also uses it in this way without a throbber and provides additional feedback via the status line below the progress meter as does the app update downloading page. I suspect this is enough feedback though I am not adverse to leaving the throbber except from the standpoint of consistency with the downloading ui. > If it's impossible to move the throbber, I'd vote for having it next to > "Downloading" (first pic in screenshot) rather than to the left of the MB done. > This is because the upper left placement is 1. easier to scan for at the > top/first corner 2. makes more sense as an indication of "downloading" (a > continual process) than "5.2 MB" (a static amount that jumps at intervals) Agreed with the caveat above that having the throbber is inconsistent with the download manager as seen in this screenshot.

Borris, besides a general review I'm not really happy with having a "Not Now" custom wizard button string and location for what is typically the Cancel button that has a Standard location.

(In reply to comment #55) If you're referring to the attachment, are you looking at the final "Not Now" after the update download, or for the incompatible add-ons being found, or both?

Any "Not Now" buttons since it is equivalent to Cancel. Wizards have standard locations for the Back, Next / Finish, and Cancel buttons and by having a "Not Now" button the Cancel button must be hidden which then shifts Back, Next / Finish to the right unless the "Not Now" button is using the Cancel button in which case the difference in the length between "Not Now" and Cancel will cause a shift. I've done a bit of rework on the wizard and think I can come up with a decent solution on it at which time I'll repost screenshots.

Hey Borris, I'm pretty happy with this. I went with only having the "Not Now" button on the last page with the Restart button which make the ui more consistent by having a Cancel button on the other pages. I also left the "Not Now" button in the same position as Cancel since it is essentially synonymous with the Cancel button.

include tests

attachment #340460 [details] [diff] [review] includes unit tests for the em work and I'll be adding other tests where possible and time permits.

ah, overlooked comment 46. thanks rob.

Just want to confirm that Rob Strong & I talked about this offline and are happy with the current solution.

I'm going to check in the EM part of the patch. I'll check in the pref change with the AUS part of the patch after add-on metrics has been updated.

Comment on attachment 344170 [details] [diff] [review] EM portion (checked in) Checked in the EM part of the patch http://hg.mozilla.org/mozilla-central/rev/4f23875d24e2c85688b229a44ee64dd92984cc8f

per comment 64, marking resolved fixed.

Tony, this still requires a fix for the updater code before it is complete.

Hey Dave, this is a fairly large patch that includes fixes for the bugs that I am going to add dependencies for after I submit this.

boriss has already given me a verbal r+ on these

Moving this over to app update since the remaing work is all in app update.

Comment on attachment 345397 [details] screenshots For minor updates or cases where the user has explicitly started a check for updates, giving the user about task flows is appropriate as there is no question about whether or not they wish to actually update their browser. For major updates that come unbidden, though, we must always explicitly ask the user if they want to update before asking them to answer a bunch of questions. I'll make comments about the screens in the attachment, using row.column as a way of identifying a screen. I've taken efforts here to also hide the fact that this is a wizard, as "back" isn't really needed in a lot of cases: Screen 1.1 - change text to: "Looking for newer versions of Minefield..." - remove the back and next buttons Screen 2.1 - change wizard page title to "Checking Add-on Compatibility" - change text to: "Looking for newer versions of your add-ons..." - remove the back and next buttons Screen 3.1, 3.2 - this should always look the same (ie: 3.1=3.2) - change text to: "Do you want to upgrade to Minefield 4.0 now?" - it would be awesome if the <browser> panel could be a fixed minimum size in pixels - buttons should be: ( Ask Later ) ( No Thanks ) (( Get the new version )) where (( Get the new version )) = next in the wizard flow and ( Ask Later ) dismisses the dialog until the next update check and ( No Thanks ) declines the major update offer Screen 3.3, 3.4 - this should always look the same (ie: 3.3=3.4) - change text to "A security and stability update for Minefield is available:" - change text to "It is strongly recommended that you apply this update for Minefield as soon as possible." - buttons should be: ( Ask Later ) (( Update Minefield )) where (( Update Minefield )) = next in the wizard flow and ( Ask Later ) dismisses the dialog until the next update check Screen 4.1, 4.2 - since we'll be using about:rights (see bug 456439) to show these sorts of terms, we can remove this page from the flow Screen 5.1 - change wizard page title to "Incompatible Add-ons Found" for major updates ... - change text to: "Some of your add-ons won't work with Minefield 4.0, and will be disabled. As soon as they are made compatible, Minefield will update and re-enable these add-ons:" for minor updates, or if you can't do different text for different update types ... - change text to: "Some of your add-ons won't work with this update, and must be disabled. As soon as they are made compatible, Minefield will update and re-enable these add-ons:" - change buttons to: ( Back ) (( OK )) where ( Back ) goes back to the previous page (3.1) and (( OK )) moves to the next page in the flow Screen 6.1, 6.2 - get rid of back/next/cancel buttons Screen 7.1 - change wizard page title to "Update Ready to Install" - change text to "The update will be installed the next time Minefield starts. You can restart Minefield now, or continue working and restart later." - change "Not Now" to " Restart Later" Screen 8.1 - change wizard page title to "Update Ready to Install" - change text to "A security and stability update for Minefield has been downloaded and is ready to be installed." - change text to "The update will be installed the next time Minefield starts. You can restart Minefield now, or continue working and restart later." - change "Not Now" to "Restart Later" (you can assume ui-r+ with those changes)

(In reply to comment #70) Mike, with all due respect... the attempts to make it not look like a wizard make it look strange especially for the vast majority of users which are using or have used wizards. Specifically: changing standard button labels, hiding back and next buttons, changing the position of the button that moves the wizard to the next page, wizard size, wizard banner height, and optionally having a button label that denotes all the info has been gathered / given with the next step starting the install vs. having it on the first page before the info has been gathered / given. I will defer to you and implement the changes below unless I have convinced you otherwise. Since the buttons are completely non-standard I would like to remove the back button completely (it has never worked before anyways) so none of the buttons will resemble a wizard. > (From update of attachment 345397 [details]) > For minor updates or cases where the user has explicitly started a check for > updates, giving the user about task flows is appropriate as there is no > question about whether or not they wish to actually update their browser. For > major updates that come unbidden, though, we must always explicitly ask the > user if they want to update before asking them to answer a bunch of questions. Is this in reference to the first screen having a "Get the new version" or "Update Minefield" button? If not, could you reword this especially the "giving the user about task flows is appropriate" part? > Screen 3.1, 3.2 > - this should always look the same (ie: 3.1=3.2) > - change text to: "Do you want to upgrade to Minefield 4.0 now?" > - it would be awesome if the <browser> panel could be a fixed minimum size in > pixels Wizards are not sized based on the locale but this UI is so doing this isn't so simple. I've been thinking of a couple of ways to accomplish this... specifically, verifying that all locales look good with a fixed size ui which would make it simpler to have a fixed size for the remote content without it looking strange. > - buttons should be: > ( Ask Later ) ( No Thanks ) (( Get the new version )) > where (( Get the new version )) = next in the wizard flow > and ( Ask Later ) dismisses the dialog until the next update check > and ( No Thanks ) declines the major update offer > > Screen 3.3, 3.4 > - this should always look the same (ie: 3.3=3.4) > - change text to "A security and stability update for Minefield is available:" > - change text to "It is strongly recommended that you apply this update for > Minefield as soon as possible." > - buttons should be: > ( Ask Later ) (( Update Minefield )) > where (( Update Minefield )) = next in the wizard flow > and ( Ask Later ) dismisses the dialog until the next update check > > Screen 4.1, 4.2 > - since we'll be using about:rights (see bug 456439) to show these sorts of > terms, we can remove this page from the flow Any problem with leaving it in so third parties can use this? All it takes to not show this page is to not have a license url in the update snippet.

(In reply to comment #71) > (In reply to comment #70) > Mike, with all due respect... the attempts to make it not look like a wizard > make it look strange especially for the vast majority of users which are using > or have used wizards. Specifically: changing standard button labels, hiding Sure. I see the fact that this is implemented as a wizard as a kludge; if we had the time to redo the entire thing so that it was a single, interactive page, boy howdy would I suggest we do that. In the interest of not boiling the ocean, though, I wanted to fake things so that in the best, most common cases it sorta looks like it's just one page (where the best case flow is 3 -> 7 for background major update, and just -> 8 for background minor update) and even in the worst cases it doesn't look like a heavy wizard. > back and next buttons, changing the position of the button that moves the > wizard to the next page, wizard size, wizard banner height, and optionally > having a button label that denotes all the info has been gathered / given with > the next step starting the install vs. having it on the first page before the > info has been gathered / given. The thing is that we're only ever really asking users *one* question: do you want to install this update/upgrade now? While we might need to confirm that they want to continue installing the update in the cases where they don't agree with license terms or where add-ons aren't compatible, those are points where they'd cancel out of the install process, not really multiple steps in a wizard. > I will defer to you and implement the changes below unless I have convinced you > otherwise. Since the buttons are completely non-standard I would like to remove > the back button completely (it has never worked before anyways) so none of the > buttons will resemble a wizard. That's fine with me, although I'm curious to know what you'd put in the place of the EULA and incompatible add-on screens should a user decide that they no longer want to continue. "Cancel" is inappropriate as it's unclear what the outcome would be: if I say I want to install an update, and then I cancel, is that the same as "Ask Later" or "No Thanks"? The former, I guess, but it's a little ambiguous. > > Screen 4.1, 4.2 > > - since we'll be using about:rights (see bug 456439) to show these sorts of > > terms, we can remove this page from the flow > Any problem with leaving it in so third parties can use this? All it takes to > not show this page is to not have a license url in the update snippet. Sure. The controls should be "Cancel" (or "Back" as discussed above) and "Accept Terms".

(In reply to comment #72) > (In reply to comment #71) > > (In reply to comment #70) > > Mike, with all due respect... the attempts to make it not look like a wizard > > make it look strange especially for the vast majority of users which are using > > or have used wizards. Specifically: changing standard button labels, hiding > > Sure. I see the fact that this is implemented as a wizard as a kludge; if we > had the time to redo the entire thing so that it was a single, interactive > page, boy howdy would I suggest we do that. Well, this is the first I heard of it being implemented as a wizard as being a kludge. Typical installers on Windows (90+ percent of the user base) use wizards so I don't see it as a kludge except for the previous non-standard UI and the even more non-standard UI that has been suggested. As long as there is a major update billboard I highly doubt there will ever be a satisfactory single interactive page beyond what the wizard already provides though the changing of button labels make it seem even less like a single interactive page IMO. > In the interest of not boiling the ocean, though, I wanted to fake things so > that in the best, most common cases it sorta looks like it's just one page > (where the best case flow is 3 -> 7 for background major update, and just -> 8 > for background minor update) and even in the worst cases it doesn't look like a > heavy wizard. The best minor update scenario is -> 8 The best major update scenario is -> 3 -> 6 -> 7 With the downloading page being required for the must display UI cases there just isn't a way to make it look like one page and trying to do so seems counterproductive / kludgey until such a time that a single page UI is designed and implemented. As is, the single page case where only 8 is displayed we don't display the back / next / cancel buttons and though I would like to have those for consistency in 7 it just isn't a good experience with the custom button labels (e.g. Not Now and Restart brandShortName). The changing of buttons labels makes it look even more like there are multiple pages which defeats your stated goal. The full scenarios are: 1. Minor background update w/ no incompatible add-ons as I outlined to you last week -> 8 2. Major and Minor manual update -> 1 -> the remainder is the same as 3 below. 3. Major background (always show UI) and Minor background update w/ incompatible add-ons -> 2 is always shown if the user has incompatible add-ons prior to the compatibility check so there is an accurate list of add-ons (e.g. install, uninstall, disable, and compatibility update may occur between the alert notification and the user seeing the update UI 12 hours later). -> 3 is always shown -> 4 is only shown when there is a licenseURL in the update snippet -> 5 is only shown if there are incompatible add-ons as I outlined to you last week -> 6 and 7 are always shown > > back and next buttons, changing the position of the button that moves the > > wizard to the next page, wizard size, wizard banner height, and optionally > > having a button label that denotes all the info has been gathered / given with > > the next step starting the install vs. having it on the first page before the > > info has been gathered / given. > > The thing is that we're only ever really asking users *one* question: do you > want to install this update/upgrade now? While we might need to confirm that > they want to continue installing the update in the cases where they don't agree > with license terms or where add-ons aren't compatible, those are points where > they'd cancel out of the install process, not really multiple steps in a > wizard. I disagree, the steps involve more than just asking questions in that it must also inform. The step prior to starting the install can inform the user that continuing will start the install / upgrade but stating it at the beginning via the button when the next step is going to gather or provide more info is not standard... though there may be other UI out there that does so I don't know of any. Also, in the originally proposed UI we change the button label at the point where the next step starts the actual install so the best case scenario without a license and with no incompatible add-ons we would show an "Install Now" button on the first page. > > I will defer to you and implement the changes below unless I have convinced you > > otherwise. Since the buttons are completely non-standard I would like to remove > > the back button completely (it has never worked before anyways) so none of the > > buttons will resemble a wizard. > > That's fine with me, although I'm curious to know what you'd put in the place > of the EULA and incompatible add-on screens should a user decide that they no > longer want to continue. I was going back to the original behavior where if there was a license and the user clicked next they ended up in the same position as you outlined except they would have a cancel button (Ask Later?). > "Cancel" is inappropriate as it's unclear what the outcome would be: if I say I > want to install an update, and then I cancel, is that the same as "Ask Later" > or "No Thanks"? The former, I guess, but it's a little ambiguous. Having a page where the user can't obviously exit the UI except via the window close button is just bad IMO unless the current operation cannot or should not be canceled which is why I tried to include it in every page. I would have also liked to include it on the last page and the downloading page with it prompting to cancel or download the update in the background as it does elsewhere. As for outcome cancel should just return to the state prior to opening the wizard. I see your point about it not being clear that the update system is still going to prompt to update in the future and typically this type of information would be provided in the text in the page. > > > Screen 4.1, 4.2 > > > - since we'll be using about:rights (see bug 456439) to show these sorts of > > > terms, we can remove this page from the flow > > Any problem with leaving it in so third parties can use this? All it takes to > > not show this page is to not have a license url in the update snippet. > > Sure. The controls should be "Cancel" (or "Back" as discussed above) and > "Accept Terms". So, get rid of the current radio buttons and instead relabel the rightmost button?

(In reply to comment #73) > Well, this is the first I heard of it being implemented as a wizard as being a > kludge. Typical installers on Windows (90+ percent of the user base) use This isn't an installer. It's an updater. Very few in-application software updaters are wizards, and those that are, shouldn't be. Also, while I understand that 90% of Windows installers are wizard based, 0% of them should be. Newer installers like the Google Pack installer are far better user experiences. But this is getting rapidly off-topic :) > single interactive page beyond what the wizard already provides though the > changing of button labels make it seem even less like a single interactive At the end of the day, "Next >" is a bad way to answer the question "Do you want to upgrade Firefox." That's what I'm saying. As long as we have to ask that question (and we always have to ask) then we're going to have this conflict. I should have surfaced it sooner, but since this UI never looked like a wizard before (the buttons were always action-based verbs) I didn't realize that it was going to come up. My bad. > With the downloading page being required for the must display UI cases there > just isn't a way to make it look like one page and trying to do so seems > counterproductive / kludgey until such a time that a single page UI is Rob, you know what a wizard is, and know what to expect it to look like, and find it disturbing that back/next are gone. I get that. I'm asserting (quite strongly now) that users will not have that cognitive dissidence to overcome, and will just see this as a dialog that has a couple of follow-up questions and a progress meter that appears when they are downloading. If it would make you feel better to always have a (Cancel) button available so users can bail at any point and so that it looks like a continuous dialog, I guess we can do that, but that felt harder than just relabeling the buttons. > labels (e.g. Not Now and Restart brandShortName). The changing of buttons > labels makes it look even more like there are multiple pages which defeats > your stated goal. Maybe, but it makes it look less like a wizard, which is an inappropriate metaphor for software updating, period. > I disagree, the steps involve more than just asking questions in that it must > also inform. The step prior to starting the install can inform the user that > continuing will start the install / upgrade but stating it at the beginning How is that information conveyed? If I've never seen your flow diagram, or never done an update, how will I know that "Next" wouldn't just start the install (it's certainly what I would expect, as the question is "Do you want to install this?"). While I understand that's wizard convention, I don't think it's very well understood convention, and it's not as powerful a UI convention as ensuring that buttons are labeled in the ways that they will behave. "Do you want to upgrade to Firefox 4" --> "Get the new version" "You OK with the license?" --> "OK" "You OK with some of your add-ons not working?" --> "OK" I do get what you're saying, I'm just saying that I don't want this to be a wizard, as I think that's the greater evil of the two that I'm being forced to choose. > I was going back to the original behavior where if there was a license and the > user clicked next they ended up in the same position as you outlined except > they would have a cancel button (Ask Later?). Yeah, I guess that's fine. It does mean that they can get into a loop, though, where they say "yes, I want the upgrade", then say "no, I don't want the upgrade if it means that I can't get my add-ons" and the result is that in 12 hours we ask them again if they want the upgrade. Not a huge deal, but the reason I included the "< Back" button then was it allowed them - with their newfound context - to actually make a more informed decision (note: this is why we used to show the little yellow dialog inline before - that way users could make the decision all at once) > Having a page where the user can't obviously exit the UI except via the window > close button is just bad IMO unless the current operation cannot or should not > be canceled which is why I tried to include it in every page. I would have As mentioned above, I guess that's OK. I don't think there's a huge usability cost to making it "Back" instead of "Cancel", but I would rather only have one than both, and understand your argument here. > > Sure. The controls should be "Cancel" (or "Back" as discussed above) and > > "Accept Terms". > So, get rid of the current radio buttons and instead relabel the rightmost > button? No, keep the radio buttons, but make it: ( Cancel ) (( Accept Terms ))

Though I disagree with much of this especially regarding the difference between updating vs. installing I am going to implement it as you originally requested. (In reply to comment #74) > (In reply to comment #73) > > I disagree, the steps involve more than just asking questions in that it must > > also inform. The step prior to starting the install can inform the user that > > continuing will start the install / upgrade but stating it at the beginning > > How is that information conveyed? In the text in the page itself with the option of having the next button labeled Install on the page prior to the actual install starting with all the information gathered / given.

Comment on attachment 345663 [details] screenshots per comment #70 I've fixed the these add-ons:"> text and the padding on the Accept Terms button locally.

I'll request review after the ui review.

Updated a couple of comments... requesting review with the ui-r+ from addressing comment #70 though I still want the screenshots ui reviewed.

Comment on attachment 345663 [details] screenshots per comment #70 uir=beltzner, thanks, rob!

Comment on attachment 345764 [details] [diff] [review] AUS Patch rev3 Dave, I've also changed the following in updates.js - gDownloadingPage onProgress function to remove a strict warning - var progress = Math.round(100 * (progress/maxProgress)); + var currentProgress = Math.round(100 * (progress/maxProgress)); ... - p.setProperty("progress", progress); + p.setProperty("progress", currentProgress); ... - this._downloadProgress.value = progress; + this._downloadProgress.value = currentProgress;

Comment on attachment 345985 [details] [diff] [review] patch - EM test fix Is AMO correctly giving results with the new request verison?

Comment on attachment 345764 [details] [diff] [review] AUS Patch rev3 This is taking me a bit longer to get through than I had hoped. I just need to satisfy myself that I'm seeing how it all fits together in the bigger picture, but you might as well get starting on the bits I've found so far. I suspect there won't be much more to come, I hope to be done either tonight or at the latest in my morning tomorrow. If you have a test update url or something I can use so I can run through the UI then that would be helpful. I did test it once and found that the Ok button on the no updates found page doesn't close the window, testing on OSX. This is pretty all little bits, I'm afraid a couple are in code you haven't even changed. I haven't looked too hard at the changes to the LOG messages and function naming, I'm presuming they are all ok. >diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js >+// app.update.interval is in branding section >+ >+// The time interval in seconds before showing an alert service notification >+// reminding the user to restart to complete the update. default = 12 hours >+pref("app.update.nagTimer.waitTime", 43200); >+ >+// The time interval in seconds before showing an alert service notification >+// reminding the user to restart to complete the update. default = 1 hour >+pref("app.update.nagTimer.interval", 3600); These two prefs have identical comments, can you differentiate them. >diff --git a/toolkit/mozapps/update/content/updates.js b/toolkit/mozapps/update/content/updates.js >+ * Sets the attributes needed for this Wizard's control buttons (labels, >+ * disabled, hidden, etc.) >+ * @param extra1ButtonString >+ * The property in the stringbundle containing the label to put on >+ * the first Extra button, or null to hide the first extra button. >+ * @param extra2ButtonString >+ * The property in the stringbundle containing the label to put on >+ * the second Extra button, or null to hide the first extra button. "or null to hide the second extra button" Extra doesn't need capitalisation either I think. >+ var bnf = this.wiz.getButton((this.wiz.onLastPage ? "finish" : "next")); Does this always correctly choose the finish button even if we are finishing in the middle of the wizard? > never: function () { >- // if the user hits "Never", we should not prompt them about this >- // major version again, unless they manually do "Check for Updates..." >- // which, will clear the "never" pref for the version presented >- // so that if they do "Later", we will remind them later. >+ // If the user clicks "Never", we should not prompt them about updating to >+ // this major update version again, unless they manually do >+ // "Check for Updates..." which will clear the "never" pref for the version >+ // presented and remind them later about this available update. > // >- // fix for bug #359093 >- // version might one day come back from AUS as an >- // arbitrary (and possibly non ascii) string, so we need to encode it >- var neverPrefName = PREF_UPDATE_NEVER_BRANCH + encodeURIComponent(gUpdates.update.version); >+ // Encode version since it could be a string (bug 359093) Include the point that it could be a non-ascii string in the comment > onLoad: function() { > // Advance to the Start page. >- gUpdates.wiz.currentPage = this.startPage; >+ startPage = this.startPage; Need a var there I think. > /** > * Return the <wizardpage> object that should be displayed first. > * > * This is determined by how we were called by the update prompt: > * >- * U'Prompt Method: Arg0: Update State: Src Event: p'Failed: Result: >- * showUpdateAvailable nsIUpdate obj -- background -- updatesfound >+ * Prompt Method: Arg0: Update State: Src Event: Failed: Result: >+ * showUpdateAvailable nsIUpdate obj -- background -- incompatibleCheck > * showUpdateDownloaded nsIUpdate obj pending background -- finishedBackground > * showUpdateInstalled nsIUpdate obj succeeded either -- installed This case seems to have been replaced with passing "installed" as Arg0, update the comment to reflect. > * showUpdateError nsIUpdate obj failed either partial errorpatching > * showUpdateError nsIUpdate obj failed either complete errors > * checkForUpdates null -- foreground -- checking > * checkForUpdates null downloading foreground -- downloading The cases for DOWNLOAD_FAILED and DOWNLOADING with Arg0 as nsIUpdate don't seem to be mentioned. Can you add them for completeness. > onWizardCancel: function() { >- if (this._checker) { >- const nsIUpdateChecker = Components.interfaces.nsIUpdateChecker; >- this._checker.stopChecking(nsIUpdateChecker.CURRENT_CHECK); >- } >+ if (this._checker) >+ this._checker.stopChecking(Ci.nsIUpdateChecker.CURRENT_CHECK); I don't see this._checker getting cleared anywhere so I think the test is unnecessary. >+ onPageShow: function() { >+ var ai = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULAppInfo); >+ if (!gUpdates.update.extensionVersion || >+ gUpdates.update.extensionVersion == ai.version) { It might be worth using the version comparator to compare that the versions are the same, to catch "3.1.0" == "3.1" f.e. The same comes up later in the patch. >- this._updateMoreInfoContent = >- document.getElementById("updateMoreInfoContent"); >- >+ var moreInfoContent = document.getElementById("moreInfoContent"); >+ if (updateTypeElement.getAttribute("severity") == "major") { Just use |severity| here. > while (updateTypeElement.hasChildNodes()) > updateTypeElement.removeChild(updateTypeElement.firstChild); > updateTypeElement.appendChild(document.createTextNode(intro)); You can just replace this with |updateTypeElement.textContent = intro| I think. >- /** >- * User clicked the "More Details..." button >- */ >- onShowMoreDetails: function() { > var updateTypeElement = document.getElementById("updateType"); updateTypeElement is already declared previously now. > var moreInfoURL = document.getElementById("moreInfoURL"); >- var moreInfoContent = document.getElementById("moreInfoContent"); > > if (updateTypeElement.getAttribute("severity") == "major") { Again you can just use |severity| >- // fix for bug #359093 >- // version might one day come back from AUS as an >- // arbitrary (and possibly non ascii) string, so we need to encode it >- var neverPrefName = PREF_UPDATE_NEVER_BRANCH + encodeURIComponent(gUpdates.update.version); >+ // Encode version since it could be a string (bug 359093) Mention non-ascii again. >+var gIncompatibleListPage = { >+ /** >+ * Initialize >+ */ >+ onPageShow: function() { >+ var incompatibleListDesc = document.getElementById("incompatibleListDesc"); >+ while (incompatibleListDesc.hasChildNodes()) >+ incompatibleListDesc.removeChild(incompatibleListDesc.firstChild); >+ incompatibleListDesc.appendChild(document.createTextNode(intro)); Again textContent should suffice. >+ var addons = gIncompatibleCheckPage.addons; >+ for (var i = 0; i < addons.length; ++i) { >+ var listitem = document.createElement("listitem"); >+ listitem.setAttribute("label", addons[i].name + " " + addons[i].version); I wonder if this needs to be localised at all. But then I guess we have the same in the extension manager display. > var gDownloadingPage = { >+ onHide: function() { >- var rv = ps.confirmEx(window, title, message, flags, null, null, null, null, { }); >+ var rv = ps.confirmEx(window, title, message, flags, null, null, null, >+ null, { }); > if (rv == 1) { > downloadInBackground = false; > } Drop the braces and can you compare against nsIPromptService.BUTTON_POS_0. > onWizardFinish: function() { > // Notify all windows that an application quit has been requested. >- var os = Components.classes["@mozilla.org/observer-service;1"] >- .getService(Components.interfaces.nsIObserverService); >- var cancelQuit = >- Components.classes["@mozilla.org/supports-PRBool;1"]. >- createInstance(Components.interfaces.nsISupportsPRBool); >+ var os = Cc["@mozilla.org/observer-service;1"]. >+ getService(Ci.nsIObserverService); >+ var cancelQuit = Cc["@mozilla.org/supports-PRBool;1"]. >+ createInstance(Ci.nsISupportsPRBool); > os.notifyObservers(cancelQuit, "quit-application-requested", "restart"); I can't remember if the FUEL APIs are just available here, but if so just replace all this with |Application.restart()|. Maybe even if it isn't just get the FUEL service and call that since it does the right thing. >diff --git a/toolkit/mozapps/update/content/updates.xml b/toolkit/mozapps/update/content/updates.xml > <destructor><![CDATA[ >- // clean up the listener >- // but you may not have one if you never showed the page with >- // a <license> element >- if (this._licenseProgressListener) >+ // clean up the listener but you may not have one if you never showed >+ // the page with a <remotecontent> element >+ if (this._remoteProgressListener) Correct the intentation > <method name="onLoad"> > <body><![CDATA[ >- this.setAttribute("selectedIndex", "1"); >+ this.setAttribute("selectedIndex", "1"); >+ this.setAttribute("state", "loaded"); And again > <property name="url"> > <getter><![CDATA[ > return this.getAttribute("url"); > ]]></getter> > <setter><![CDATA[ > var self = this; > >- this._licenseProgressListener = { >+ this._remoteProgressListener = { I think there ought to be a check for an existing _remoteProgressListener and do something sensible. I don't think we can hit that currently though so you can spin it off into another bug if it's hassle. >diff --git a/toolkit/mozapps/update/content/updates.xul b/toolkit/mozapps/update/content/updates.xul >- <wizardpage id="license" pageid="license" next="downloading" >+ <wizardpage id="license" pageid="license" next="incompatibleList" > object="gLicensePage" label="&license.titleText;" >- onpageshow="gLicensePage.onPageShow();"> >+ onpageshow="gLicensePage.onPageShow();" >+ onextra1="gLicensePage.onExtra1();"> Fix the indentation >+ <wizardpage id="incompatibleList" pageid="incompatibleList" >+ next="downloading" label="&incompatibleList.title;" >+ object="gIncompatibleListPage" >+ onpageshow="gIncompatibleListPage.onPageShow();" >+ onextra1="gIncompatibleListPage.onExtra1();"> And again > <wizardpage id="finished" pageid="finished" >- label="&finished.title;" object="gFinishedPage" >- onpageshow="gFinishedPage.onPageShow();"> >- <label>&finished.text;</label> >+ label="&finishedPage.title;" object="gFinishedPage" >+ onpageshow="gFinishedPage.onPageShow();" >+ onextra1="gFinishedPage.onExtra1()"> Ditto > <wizardpage id="finishedBackground" pageid="finishedBackground" >- label="&finishedBackground.title;" object="gFinishedPage" >- onpageshow="gFinishedPage.onPageShowBackground();"> >- <label>&finishedBackground.text;</label> >+ label="&finishedPage.title;" object="gFinishedPage" >+ onpageshow="gFinishedPage.onPageShowBackground();" >+ onextra1="gFinishedPage.onExtra1()"> ... >diff --git a/toolkit/mozapps/update/src/nsUpdateService.js.in b/toolkit/mozapps/update/src/nsUpdateService.js.in >- getNext: function() { >+ getNext: function hasMoreElements_getNext() { Maybe a better name would be ArrayEnumerator_getNext >+ _selectAndInstallUpdate: function AUS__selectAndInstallUpdate(updates) { >+ var shouldShowPrompt = false; >+ if (update.type == "major") { >+ LOG("Checker", "_selectAndInstallUpdate - prompting because it is a " + >+ "major update"); >+ shouldShowPrompt = true; >+ } >+ else if (!getPref("getBoolPref", PREF_APP_UPDATE_AUTO, true)) { >+ LOG("Checker", "_selectAndInstallUpdate - prompting because silent " + >+ "install is disabled"); >+ shouldShowPrompt = true; >+ } I think this will read better if you just call this._showPrompt and return in both these cases and remove the shouldShowPrompt variable. >+ /** >+# From this point on there are two possible outcomes: >+# 1. download and install the update automatically >+# 2. notify the user about the availability of an update >+# >+# Notes: >+# a) if automatic download and install is disabled then the user will >+# be notified. >+# b) Mode is determined by the value of the app.update.mode preference. >+# >+# The outcome is determined as follows: >+# >+# Update Type Mode Incompatible Add-ons Outcome >+# Major all N/A Notify >+# Minor 0 N/A Auto Install >+# Minor 1 or 2 Yes Notify >+# Minor 1 or 2 No Auto Install >+ */ This comment should probably move higher and include a note about the effect of app.update.auto. >+ _showPrompt: function AUS__showPrompt() { >+ var prompter = Cc["@mozilla.org/updates/update-prompt;1"]. >+ createInstance(Ci.nsIUpdatePrompt); >+ prompter.showUpdateAvailable(this._update); >+ this._update = null; >+ }, I think this works better if you pass in the update to prompt about. Still need to keep it in this._update for the add-on compatibility check case though. >+ onAddonUpdateEnded: function AUS_onAddonUpdateEnded(addon, status) { >+ if (status != Ci.nsIAddonUpdateCheckListener.STATUS_UPDATE && >+ status != Ci.nsIAddonUpdateCheckListener.STATUS_VERSIONINFO) >+ return; >+ >+ for (var i = 0; i < this._addons.length; ++i) { >+ if (this._addons[i].id == addon.id) { >+ LOG("UpdateService", "onAddonUpdateEnded - found update for add-on " + >+ "ID: " + addon.id); >+ this._addons.splice(i, 1); >+ break; >+ } > } > }, As it stands removing the items from the array seems unnecessary, just counting up the number with updates would be sufficient to determine that they all do, unless we don't trust the EM update checker. >+ initRestartNagTimer: function UP_initRestartNagTimer(update) { >+ if (!this._enabled) >+ return; Might as well have that at the top to save the variable instantiation. >+ // Give the user x seconds to react before showing the big UI >+ var waitTime = getPref("getIntPref", PREF_APP_UPDATE_NAGTIMER_WAITTIME, 43200); >+ LOG("UpdatePrompt", "initRestartNagTimer - nag timer wait time: " + waitTime); >+ if (observer.timer) { >+ observer.timer.cancel(); >+ } When is observer.timer ever going to be already set? >+ var openFeatures = "chrome,centerscreen,dialog=no,resizable=no,titlebar,toolbar=no"; >+ var ww = Cc["@mozilla.org/embedcomp/window-watcher;1"]. >+ getService(Ci.nsIWindowWatcher); >+ var param = Cc["@mozilla.org/supports-array;1"]. >+ createInstance(Ci.nsISupportsArray); >+ var arg = Cc["@mozilla.org/supports-string;1"]. >+ createInstance(Ci.nsISupportsString); >+ arg.data = page; >+ param.AppendElement(arg); >+ ww.openWindow(null, URI_UPDATE_PROMPT_DIALOG, null, openFeatures, param); There is no need to create the nsISupportsArray here, just pass arg in place of param and the window watcher will do the rest. >+ _showUnobtrusiveUI: function UP__showUnobtrusiveUI(parent, uri, features, name, page, update, > title, text, imageUrl) { >+ if (observer.timer) { >+ observer.timer.cancel(); >+ } Same again. >diff --git a/toolkit/themes/gnomestripe/mozapps/update/updates.css b/toolkit/themes/gnomestripe/mozapps/update/updates.css >+#pauseButton { >+ -moz-appearance: none; >+ list-style-image: url(chrome://mozapps/skin/downloads/downloadButtons.png); >+ -moz-image-region: rect(0px, 48px, 16px, 32px); >+ background-color: transparent; >+ border: none; >+ padding: 0; >+ margin: 0; >+ min-width: 0; >+ min-height: 0; >+} I'm a little nervous about using icons from the download manager. Last I heard Seamonkey didn't package it which would cause problems. Maybe see what faaborg thinks but it might be better to just copy these.

(In reply to comment #83) > (From update of attachment 345985 [details] [diff] [review]) > Is AMO correctly giving results with the new request verison? It does return the same results and morgamic was cc'd in the discussion where this was requested.

(In reply to comment #84) > (From update of attachment 345764 [details] [diff] [review]) > This is taking me a bit longer to get through than I had hoped. I just need to > satisfy myself that I'm seeing how it all fits together in the bigger picture, > but you might as well get starting on the bits I've found so far. I suspect > there won't be much more to come, I hope to be done either tonight or at the > latest in my morning tomorrow. Thanks! > If you have a test update url or something I can use so I can run through the > UI then that would be helpful. I did test it once and found that the Ok button > on the no updates found page doesn't close the window, testing on OSX. I've been using the following with the app.update.url.override pref. Note that the big mar url doesn't pass the hash verification intentionally. http://exchangecode.com/robert/work/noupdates.xml http://exchangecode.com/robert/work/empty.xml http://exchangecode.com/robert/work/major.xml http://exchangecode.com/robert/work/major_big_mar.xml http://exchangecode.com/robert/work/minor.xml http://exchangecode.com/robert/work/major_nolicense.xml http://exchangecode.com/robert/work/major_big_license.xml http://exchangecode.com/robert/work/major_bad_license.xml

A couple of UI oddities on OSX. The license agreement page gets a scrollbar that scrolls the description, license and radio boxes. Also on both this and the previous page the remotecontent could do with a border I think.

Comment on attachment 345764 [details] [diff] [review] AUS Patch rev3 >diff --git a/toolkit/locales/en-US/chrome/mozapps/update/updates.properties b/toolkit/locales/en-US/chrome/mozapps/update/updates.properties >+okButton=OK >+okButton.accesskey=O >+askLaterButton=Ask Later >+askLaterButton.accesskey=A >+noThanksButton=No Thanks >+noThanksButton.accesskey=N >+updateButton_minor=Update %S >+updateButton_minor.accesskey=U >+updateButton_major=Get the new version Much as I don't want to reopen the UI debate, I think this button text should be capitalised as the other buttons are. >diff --git a/toolkit/mozapps/update/content/updates.js b/toolkit/mozapps/update/content/updates.js >-const nsIUpdateItem = Components.interfaces.nsIUpdateItem; >-const nsIIncrementalDownload = Components.interfaces.nsIIncrementalDownload; >+const Cc = Components.classes; >+const Ci = Components.interfaces; >+const Cr = Components.results; This is, sadly, a problem on OSX. There macBrowserOverlay.xul is overlayed onto updates.xul, and contains scripts that already declare these constants, leaving you with broken menus and things. Possibly what was causing the Ok button to not work that I saw but not sure. Oh and just for fun that only happens in Firefox OSX, not generally so you can't even just use a platform ifdef. I'm not sure of a particularly good way around this, we have the same issue for the extension manager.

Ok I think I'm fairly happy otherwise with the patch now, would just like to spin over the updated version when it's done.

(In reply to comment #84) >... > >+ var bnf = this.wiz.getButton((this.wiz.onLastPage ? "finish" : "next")); > > Does this always correctly choose the finish button even if we are finishing in > the middle of the wizard? We never technically finish in the middle of the wizard since none of the pages we finish on have next defined on the wizardpage... so, it does always select the correct button. > > onWizardFinish: function() { >... > >+ var os = Cc["@mozilla.org/observer-service;1"]. > >+ getService(Ci.nsIObserverService); > >+ var cancelQuit = Cc["@mozilla.org/supports-PRBool;1"]. > >+ createInstance(Ci.nsISupportsPRBool); > > os.notifyObservers(cancelQuit, "quit-application-requested", "restart"); > > I can't remember if the FUEL APIs are just available here, but if so just > replace all this with |Application.restart()|. Maybe even if it isn't just get > the FUEL service and call that since it does the right thing. I'd prefer to go with what the EM and other code currently does and this doesn't appear to be available to other apps > >diff --git a/toolkit/mozapps/update/content/updates.xml b/toolkit/mozapps/update/content/updates.xml >... > >- this._licenseProgressListener = { > >+ this._remoteProgressListener = { > > I think there ought to be a check for an existing _remoteProgressListener and > do something sensible. I don't think we can hit that currently though so you > can spin it off into another bug if it's hassle. Filed bug 462940 > >+ // Give the user x seconds to react before showing the big UI > >+ var waitTime = getPref("getIntPref", PREF_APP_UPDATE_NAGTIMER_WAITTIME, 43200); > >+ LOG("UpdatePrompt", "initRestartNagTimer - nag timer wait time: " + waitTime); > >+ if (observer.timer) { > >+ observer.timer.cancel(); > >+ } > > When is observer.timer ever going to be already set? If the UI is displayed and the user clicks Restart Later we setup the timer. If the user manually opens the UI again and clicks restart later there will already be a timer. I've also added a comment to this affect. > >+ _showUnobtrusiveUI: function UP__showUnobtrusiveUI(parent, uri, features, name, page, update, > > title, text, imageUrl) { > > >+ if (observer.timer) { > >+ observer.timer.cancel(); > >+ } > > Same again. One instance is if there is a background update notification created, the user manually checks for updates, and during the download clicks hide. I've also added a comment to this affect.

(In reply to comment #88) > (From update of attachment 345764 [details] [diff] [review]) > >diff --git a/toolkit/locales/en-US/chrome/mozapps/update/updates.properties b/toolkit/locales/en-US/chrome/mozapps/update/updates.properties >... > Much as I don't want to reopen the UI debate, I think this button text should > be capitalised as the other buttons are. I'll ask beltzner what he thinks when I see him

(In reply to comment #88) > (From update of attachment 345764 [details] [diff] [review]) >... > >diff --git a/toolkit/mozapps/update/content/updates.js b/toolkit/mozapps/update/content/updates.js > >-const nsIUpdateItem = Components.interfaces.nsIUpdateItem; > >-const nsIIncrementalDownload = Components.interfaces.nsIIncrementalDownload; > >+const Cc = Components.classes; > >+const Ci = Components.interfaces; > >+const Cr = Components.results; > > This is, sadly, a problem on OSX. There macBrowserOverlay.xul is overlayed onto > updates.xul, and contains scripts that already declare these constants, leaving > you with broken menus and things. Possibly what was causing the Ok button to > not work that I saw but not sure. Oh and just for fun that only happens in > Firefox OSX, not generally so you can't even just use a platform ifdef. I'm not > sure of a particularly good way around this, we have the same issue for the > extension manager. Is there a bug on this?

Hey Dave, I believe this should cover the changes you asked for besides the string which I'll ask beltzner about.

Comment on attachment 346235 [details] [diff] [review] AUS Patch rev3 btw: I noticed that the pinstripe buttons image is significantly different from winstrip. Could you verify it looks decent on Mac using http://exchangecode.com/robert/work/major_big_mar.xml for the app.update.url.override pref? Thanks

(In reply to comment #94) > (From update of attachment 346235 [details] [diff] [review]) > btw: I noticed that the pinstripe buttons image is significantly different from > winstrip. Could you verify it looks decent on Mac using > http://exchangecode.com/robert/work/major_big_mar.xml for the > app.update.url.override pref? Thanks The buttons look ok. The lack of border around remotecontent I noted is still a problem. Also the Ok button on the no updates found page still doesn't work.

Thanks Dave, I forgot to fix the styling and to set canAdvance on the no updates page as well as hiding the cancel button for the error page.

Comment on attachment 346237 [details] [diff] [review] AUS Patch rev5 Sorry, have to r- this since I still think the observer bits are wrong, see below. The UI looks better on Mac however now both the license and major update pages get the odd scrollbars. I think there might be a layout issue because I can't see anything wrong with the XUL so I won't block the landing of this on it, but we should get it fixed before 3.1 final. >diff --git a/toolkit/mozapps/update/content/updates.js b/toolkit/mozapps/update/content/updates.js > onProgress: function(request, context, progress, maxProgress) { >- request.QueryInterface(nsIIncrementalDownload); >- LOG("UI:DownloadingPage.onProgress", " " + request.URI.spec + ", " + progress + >- "/" + maxProgress); >+ LOG("gDownloadingPage", "onProgress - progress: " + progress + "/" + >+ maxProgress); > >- var name = gUpdates.strings.getFormattedString("downloadingPrefix", [gUpdates.update.name]); >+ var name = gUpdates.getAUSString("downloadingPrefix", [gUpdates.update.name]); > let status = this._updateDownloadStatus(progress, maxProgress); >- var progress = Math.round(100 * (progress/maxProgress)); >+ var currentProgress = Math.round(100 * (progress/maxProgress)); Nit, spaces around the "/" >diff --git a/toolkit/mozapps/update/src/nsUpdateService.js.in b/toolkit/mozapps/update/src/nsUpdateService.js.in >+ initRestartNagTimer: function UP_initRestartNagTimer(update) { >+ if (!this._enabled) >+ return; >+ >+ var observer = { >+ updatePrompt: this, >+ service: null, >+ timer: null, >+ notify: function UP_IRNT_notify() { >+ // the user hasn't restarted yet => prompt when idle >+ this.service.removeObserver(this, "quit-application"); >+ this.service = null; >+ this.timer.cancel(); >+ this.timer = null; >+ this.updatePrompt._setupRestartNagTimer(update); >+ }, >+ observe: function UP_IRNT_observe(aSubject, aTopic, aData) { >+ if (aTopic == "quit-application") { >+ this.service.removeObserver(this, "quit-application"); >+ this.service = null; >+ this.timer.cancel(); >+ this.timer = null; >+ } >+ } >+ }; >+ >+ if (!observer.service) { >+ observer.service = Cc["@mozilla.org/observer-service;1"]. >+ getService(Ci.nsIObserverService); >+ observer.service.addObserver(observer, "quit-application", false); >+ } >+ >+ // Give the user x seconds to react before displaying the notification >+ var waitTime = getPref("getIntPref", PREF_APP_UPDATE_NAGTIMER_WAITTIME, 43200); >+ // The update UI can call initRestartNagTimer multiple times so cancel the >+ // timer if it has been initialized. >+ if (observer.timer) >+ observer.timer.cancel(); >+ else >+ observer.timer = Cc["@mozilla.org/timer;1"]. >+ createInstance(Ci.nsITimer); >+ >+ observer.timer.initWithCallback(observer, waitTime * 1000, >+ observer.timer.TYPE_ONE_SHOT); >+ }, If initRestartNagTimer is being called repeatedly, which I can see it can be, then all you are doing here is creating a new observer for each call, and a new timer for it. You'll need to stash a reference to the observer on the UpdatePrompt object to avoid recreating it each time, and make sure to unreference that when the timer fires or the app shuts down. Same probably goes for _setupRestartNagTimer.

Dave, I'm going to leave the current _showUnobtrusiveUI alone for now including the pre-existing bug and address it and other problems with the UpdatePrompt implementation in bug 462568. I'm calling showUpdateDownloaded from the extra1 button and if you prefer I will remove it for now as well.

Comment on attachment 346318 [details] [diff] [review] AUS Patch rev6 Ok yeah let's get this landed

I forgot to remove the winstripe warning.gif and removed it in this patch.

Pushed to mozilla-central http://hg.mozilla.org/mozilla-central/rev/a6e0b26a65e89e43dfbe844d34b7bbef387ded71

note: the vertical scrollbars on Mac is bug 374820

Fixed before 1.9.1 branching: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a6e0b26a65e8 Appears to still be in source as of today, so I think I can verify this for both 1.9.1 and trunk.

Rob, do the unit tests cover each part of this problem? Or do we need a Litmus test as the flag is asking for?

The unit tests cover what the majority of this bug and there are other unit tests that cover parts of the codebase though this bug also entailed a significant rewrite of the code so as long as there are also general litmus tests for app update that reflect the ui changes this should be covered.

This potentially blocks major updates from 3.0 to 3.1 (3.5). Reopening.

You could have requested blocking-1.9.0.8/9 and avoided spamming people for all the dependent bugs for this.

Restoring verified -> fixed

This got reopened and the closed without capturing the reason it was reopened in comment 107 or the alternate attention-getting method proposed in comment 108 :-( Don't we want to fix this on the 1.9.0 branch before we push major updates to people? Probably too late for 1.9.0.12 but 1.9.0.13 should still be out before we push updates (as opposed to making them available to people who "Check for updates" which will probably be earlier).

It would be great to fix this for 1.9.0.x but the patch will be rather large and invasive which typically drivers don't approve for a pre-existing bug in the code. We've been living with this bug since the updater code has displayed incompatible add-ons and the work to backport this is very non-trivial. Can I get your assurance that if the patch is backported you or another driver will approve it for 1.9.0.x?

Nevermind... based on the title I thought the "AUS patch" was for the AUS server and only looked at the 39K client patch which looked reasonable. We couldn't take a patch of that size or with those localization changes. The rules for "preexisting bugs" can get bent for update fixes because fixing the *next* version only helps us two versions from now.

litmus testcases were added to its own testgroup for major software updates for 3.0 to 3.5. Those testcases take care of the fix for this bug. Marking as in-litmus+

(In reply to comment #113) > litmus testcases were added to its own testgroup for major software updates for > 3.0 to 3.5. Those testcases take care of the fix for this bug. Marking as > in-litmus+ Can you please list those tests?

Please use this litmus link to get to the testgroup, Henrik. It's listed there for you to see. https://litmus.mozilla.org/show_test.cgi?id=6724 https://litmus.mozilla.org/show_test.cgi?id=6725 https://litmus.mozilla.org/show_test.cgi?id=6738 https://litmus.mozilla.org/show_test.cgi?id=6739 https://litmus.mozilla.org/show_test.cgi?id=6740 https://litmus.mozilla.org/show_test.cgi?id=6741 https://litmus.mozilla.org/show_test.cgi?id=6742 https://litmus.mozilla.org/show_test.cgi?id=6743

(In reply to comment #115) > Please use this litmus link to get to the testgroup, Henrik. It's listed there > for you to see. It's not for me. It's for a general reference which Litmus tests exists for a particular bug.