User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; fr; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10 Build Identifier: If the pref in Option dialog "Ask me what I want to do" is set in Option/Advanced/Update, a step with a button should be added : [check for updates ] -> *[download update] xxx Mb* -> [apply the update] This is needed for people that pay they internet accès depending on the transmeted data (that the case in some countries, and some developping countries) and not by time or by avaible access to the web. It seems to me important for firefox market penetration and development. As the check in the About window (when auto is off) by clicking [check for update] do not depend on the pref set in Option/Advanced/Update, then the option "Ask me what I want to do" shouldn't depend on the check pref. Or at least "Ask me what I want to do" should be hidden/disable by default and Show/enabled if check for Firefox updates is unchecked. Reproducible: Always

beltzner, this is what we discussed on irc a while back in that when a user has selected "Ask me what I want to do" in preferences we don't automatically check for updates when opening the about window but we do automatically download if the user checks for updates and an update is found. I suggested having an interim step where a "Download" button is displayed if an update is found. I do see your point that if a user clicks the button to check for an update that it isn't unreasonable to assume that by checking that they would want to download but I do think that people that change the pref likely want to control the initiating of the download as well so I'd prefer you to decide whether this should be fixed or not. If it is to be fixed we would need to add the string for the button.

So, why not making it clear for the user by changing the caption button [Check and download updates] ? Or [Download and install updates] (assuming that users that want to change the auto-check pref want to control over download and not checking, so the checking could be implicit). This is an alternative solution ;-)

I would think that the about window can check for updates automatically if the preference "Automatically check for updates to: Minefield" is set. But then when there is an update available and the user selected "Ask me what I want to do", the user needs to be able to decide whether he wants to download it or not, because the other option there says "Automatically download and...". Furthermore, these two options are captioned "When updates to Minefield are found:" so they should not influence the check for updates itself.

(In reply to comment #3) > I would think that the about window can check for updates automatically if the > preference "Automatically check for updates to: Minefield" is set. > But then when there is an update available and the user selected "Ask me what I > want to do", the user needs to be able to decide whether he wants to download > it or not, because the other option there says "Automatically download and...". > Furthermore, these two options are captioned "When updates to Minefield are > found:" so they should not influence the check for updates itself. Agreed. And if auto-check for updates and "Ask me what I want to do" are unchecked, we need to add the step with a button described in comment 1.

"Ask me what I want to do" should really be putting up a dialog indicating that there is an update available, not changing the behaviour such that the user must manually check for updates. I believe, in fact, that's what it does, as per: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#1430 If the user manually checks for updates, and an update is available, then we will fetch that update immediately. The user *asked* us to check for updates. If they don't want to download it, they can opt to not check for them. Simple as that. If you want to be notified of when updates are available, and control when they are downloaded, then select "Ask me what to do" and don't manually check for updates.

(In reply to comment #5) > If the user manually checks for updates, and an update is available, then we > will fetch that update immediately. The user *asked* us to check for updates. > If they don't want to download it, they can opt to not check for them. Simple > as that. The definition of "check" in this context is "To make an examination or investigation;..." When a user "checks" they are not asking for it to be downloaded but only if it is available. I recently wanted to see if the problem with Mac OS X updates had been resolved. I was unpleasantly surprised to find that "check" no longer meant what it had previously meant, that my preference to ask me what I want to do when an update was found - which seems should have applied in this situation - didn't, and that subsequently an update was applied with a bug that rendered Minefield unusable. If you don't want to change the behavior to comport with peoples expectations, at least the wording on the button or in the window should be changed to warn that clicking the button means download update if available. Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20101004 Firefox/4.0b7pre ID:20101004030754

Agreed, wanting to check updates != wanting to install updates.

Same here. It's extremely annoying that the user can no longer decide what he wants but gets forced the update just because he made the "mistake" of checking. The WONTFIX is unjustified by miles, since as I could figure out, this has been annoying more users than just me meanwhile.

Mike: (#5) "If you want to be notified of when updates are available, and control when they are downloaded, then select "Ask me what to do" and don't manually check for updates." That's simply not the same, I will explain you why: If you select "Ask me what to do" you will get update notifications EVERYTIME an update is available. But user-side checks can be made when? Right, completely randomly. 3 November 2010. OK, no interest for updating since the build runs _extraordinarily_ perfectly, and eventually the user decides shortly after Christmas to do another update. 23 December 2010. With "Ask me what to do", you would instead have gotten $DAYS_DIFF times as many popup windows, in this example roughly 50. Thanks a lot. =D

In comment 3 I also said that the current behavior is a bit confusing to me. It's a bigger step, but I think it would be nice to change the preferences ui to something in the fashion of the windows options described in bug 600505 comment 0 (and implement those options) and then change the buttons in the about window in a corresponding way (as described in this bug).

This came up in comments about the Firefox 5 release: If the prefs are set to "automatically check for updates", but "ask me what I want to do if updates are found", we can't trick users into installing the update when he merely wants to check if updates are present. Wanting to check for updates != wanting to install updates. So reopening. Bug 600505 proposes new options, like Windows does: > 1. Install updates automatically (recommended) > 2. Download update but let me choose whether to install them > 3. Check for updates but let me choose whether to download and install them > 4. Never check for updates (not recommended) We probably don't need #2. In case 1: no change needed, download and install on restart or when the Install/Restart button is clicked. In case 3: Check for updates when opening the About dialog, then present a "Download and Install Firefox 7" button. In case 4: Present a "Check for Updates" button. If updates are found, present a "Download and Install Firefox 7" button.

Faaborg gave ui-review+ for these update prefs in bug 600505: (*) Automatically install updates (recommended: improved security) [x] warm me if this will disable any of my add-ons ( ) Check for updates, but let me choose whether to install them ( ) Never check for updates (not recommended: security risk) In line with that, I'd like to suggest the following for the About dialog: Always check for updates, because opening the About dialog is considered manually checking for updates, just like the former Help -> Check for Updates menu item. Currently, a "Check for Updates" button is shown if app.update.enabled or app.update.auto are false (equivalent to cases 2 and 3 above). That step should be skipped. Then, if updates are available: In case 1: Install the update on the next restart, or when the "Apply Update" button is clicked (no change here). In case 2: Present a new "Update to Firefox X" button. Don't install the update unless that button is clicked; currently, we present a "Check for Updates" button, but clicking it really means "Check for Updates, and automatically download install them without further asking". In case 3: Same as case 2.

Having an option available that prevents downloading multiple megabytes is important in today's world, given that data roaming is often expensive - I don't want to use the expensive bytes of my phone connection for a download while traveling when I can wait a few hours and use a free wifi connection, e.g. in a hotel.

Yeah, the Firefox in a helicopter connected with a satellite phone use case :p If the user selects check for updates we can probably hold off on the download until they say ok. So we would want the text to be: ( ) Check for updates, but let me choose whether to download and install them

Implements comment 12. The current string for the new button is "Update to X", where X is the name of the update, i.e. either provided by the <update> element, or the string: "<App Name> <Update App Version>". Example: "Update to Nightly 9.0a1"

Comment on attachment 556145 [details] [diff] [review] patch v1 >- setupUpdateButton: function(aKeyPrefix) { >+ setupUpdateButton: function(aKeyPrefix, updateName) { > this.updateBtn.label = this.bundle.GetStringFromName(aKeyPrefix + ".label"); >+ if (updateName) >+ this.updateBtn.label += " "+updateName; This isn't l10n-friendly.

Thanks, this should address this.

Comment on attachment 556248 [details] [diff] [review] patch v1.1 (In reply to Steffen Wilberg from comment #15) > Created attachment 556145 [details] [diff] [review] > patch v1 > > Implements comment 12. > > The current string for the new button is "Update to X", where X is the name > of the update, i.e. either provided by the <update> element, or the string: > "<App Name> <Update App Version>". > Example: "Update to Nightly 9.0a1" Please get UX approval for the string first

Comment on attachment 556248 [details] [diff] [review] patch v1.1 Requesting ui-review for the button label "Update to X", where X means name and version, e.g. "Update to Firefox 9". See comment 12 for the complete description of the patch.

In the case of point releases or security patches it might be odd for the user to see a button that says "Update to Firefox 13" more than once. Perhaps just "Update Firefox" (and we leave the version number to the full display with date in the subsequent about dialog)? I'm hesitant to provide more detailed version information directly into the button since it starts to get kind of jargony.

Comment on attachment 556248 [details] [diff] [review] patch v1.1 I think we're going to want to stick with a generic phrase like "Apply Update" or "Update &AppName" Rationale being: Saying: "Update to Firefox 6" (6.0) "Update to Firefox 6" (6.0.2) is redundant, and saying: "Update to Firefox 6.0" "Update to Firefox 6.0.2" provides a level of detail that is a bit too jargony and long for a button. Now I would never suggest removing the version number (that would be Limi... kidding!), but in this case I think we can simplify the action.

The question is if someone who has set the "ask me" pref is less or more likely to install the update if he doesn't know if it's a minor update (like 6.0 to 6.0.2) or "major" (like 6.0 to 7.0)... Anyway, I'd suggest the string "Download and Install Update" to match the pref label you suggested in comment 14 ("Check for updates, but let me choose whether to download and install them").

Right, I was doing a large number of ui-reviews in a row and didn't think about this one carefully enough. If the user has already selected "let me choose to download and install them" we now know much more about them, and there is actually a pretty good chance that they will care about the version number they are about to install (and may already have an opinion on if they want to install it, or will use information that we provide to form that opinion). So under that much more specific context, the phrase: "Download and update to 6.0.3" makes more sense for those particular users. sorry about kind of phoning in my earlier review.

No problem. New string is (for Nightly) "Download and update to 9.0a1" (per comment 24).

Comment on attachment 562318 [details] [diff] [review] patch v3 I've had discussions with UX and others about how multi-word buttons are evil and since I don't want to have the same discussion again and I don't own this code I am punting the review over to Gavin.

Comment on attachment 562318 [details] [diff] [review] patch v3 multiword buttons can indeed cause l10n issues as they get longer than the fixed size of the window. localizers are going to need to check that whatever they write is going to fit. But overall the ui-review is supposed to be about the interface itself as opposed to dealing with implementation level concerns (that would be code review), and this is the cleanest way for us to describe to users exactly what is going to happen when they click the button, so ui-review+

What I surmise from Nicolas Mandil's (first post and comment 3) is "Check for downloads" should only do that. It should not initiate the full download. It should rename the buttom "Download Now" with a Y/N option or something like that. I have my setting "Check for updates but let me choose"... I discovered this when I clicked Help/About - Check for updates and when it started I immediately closed the browser. When I restarted the browser the download was complete and automatically updated FF. I had no choice at that point. You guys/ladies are doing a great job!

Fixing some bitrot. This would technically obsolete patch v3, but I can't give up my #1 position in Gavin's queue after 4.5 months...

Good day. I don't want to open-up a can of worms here, but shouldn't Firefox aim to be as flexible for users as possible? When a user clicks on Check for Updates, shouldn't he then be presented with a LIST of all possible update paths, not just a button showing the latest and greatest? Maybe there's a minor update available, and also a major. The user should be given the choice of both. This will never fit in a button. Countless other programs do just that, and it seems to me to be the natural way to handle updates...

Booze, there always is only one supported update path, and that's the latest version for the respective channel.

So long as it presents minors first, and then if you "Check" again it presents the next major, I'm happy with it (I think this is what you mean; it's the way it used to work anyway). Thanks.

Comment on attachment 596383 [details] [diff] [review] patch v3, unbitrotted Sorry that this took so long to get to. It appears that much of this code is untested, so it requires careful review, and it never managed to find its way to the top of my priority list. Now that I've taken the time to read through all of the relevant code, I should be more responsive in the future. The re-use of "updateButtonBox" for multiple purposes is very confusing and leads to very hard-to-verify code (buttonOnCommand is a nightmare, particularly after this patch where there are three different cases, each with a complicated set of conditions). It seems like this code would be much easier to follow if there were separate "panes" for the separate cases: - download/install confirmation prompt (added by this bug) - need to show billboard/licenseURL / add-on confirmation case - "restart to apply update" Now on to comments with the patch itself... >diff --git a/browser/base/content/aboutDialog.js b/browser/base/content/aboutDialog.js >+ setupUpdateButton: function(aKeyPrefix, updateVersion) { >+ this.updateBtn.label = this.bundle.formatStringFromName(aKeyPrefix + ".label", [updateVersion], 1); It looks like "updateVersion" is optional, so you shouldn't use formatStringFromName unconditionally; it could cause all sorts of trouble depending on platform quirks or localizer mistakes. Check to see whether it's specified and use GetStringFromName if not. > buttonOnCommand: function() { >- if (this.update && (this.update.billboardURL || this.update.licenseURL || >- this.addons.length != 0)) { >+ if (this.update && this.addons && (this.update.billboardURL || >+ this.update.licenseURL || this.addons.length != 0)) { This change looks wrong. .addons is only non-null when an addons compatbility check has been performed, but AFAICT it's possible for this pane to be displayed before that happens (if the update has a billboardURL or licenseURL), and we want to open the update prompt in that case. >+ if (!gAppUpdater.update.appVersion || >+ Services.vc.compare(gAppUpdater.update.appVersion, >+ Services.appinfo.version) == 0) { >+ gAppUpdater.startDownload(); >+ } else { >+ gAppUpdater.checkAddonCompatibility(); > } Seems like it would be useful to factor this into a gAppUpdater.doUpdate() helper given that it's duplicated both here and in onCheckComplete. This caller can also use "this" instead of gAppUpdater. >diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties >+# LOCALIZATION NOTE (downloadAndInstallButton.label): %S is replaced by the >+# version of the update: "Download and update to 13.0". >+update.downloadAndInstallButton.label=Download and update to %S >+update.downloadAndInstallButton.accesskey=u I think "Download and update to <version>" is awkward. What was wrong with "Update to <version>"? I know Alex thought it was jargony, but this only shows up for users who've asked to be prompted for updates, and for them getting the precise version number is useful. It also avoids the "long button string" issue.

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #39) > The re-use of "updateButtonBox" for multiple purposes is very confusing and > leads to very hard-to-verify code (buttonOnCommand is a nightmare, > particularly after this patch where there are three different cases, each > with a complicated set of conditions). It seems like this code would be much > easier to follow if there were separate "panes" for the separate cases: > - download/install confirmation prompt (added by this bug) > - need to show billboard/licenseURL / add-on confirmation case > - "restart to apply update" OK, I've split up buttonOnCommand() into doUpdate(), buttonApplyAfterDownload(), and buttonApplyBillboard(). I set "oncommand" in setupUpdateButton(). This let me remove those complicated checks. I haven't adjusted the indent yet to keep the patch readable. > >diff --git a/browser/base/content/aboutDialog.js b/browser/base/content/aboutDialog.js > > >+ setupUpdateButton: function(aKeyPrefix, updateVersion) { > >+ this.updateBtn.label = this.bundle.formatStringFromName(aKeyPrefix + ".label", [updateVersion], 1); > > It looks like "updateVersion" is optional, so you shouldn't use > formatStringFromName unconditionally; it could cause all sorts of trouble > depending on platform quirks or localizer mistakes. Check to see whether > it's specified and use GetStringFromName if not. updateVersion is optional. formatStringFromName works even if updateVersion is undefined, and doesn't even complain in the error console. But I've made the change to use GetStringFromName in that case. > > buttonOnCommand: function() { > > >- if (this.update && (this.update.billboardURL || this.update.licenseURL || > >- this.addons.length != 0)) { > >+ if (this.update && this.addons && (this.update.billboardURL || > >+ this.update.licenseURL || this.addons.length != 0)) { > > This change looks wrong. .addons is only non-null when an addons > compatbility check has been performed, but AFAICT it's possible for this > pane to be displayed before that happens (if the update has a billboardURL > or licenseURL), and we want to open the update prompt in that case. I can't remember why I made that change half a year ago. But I could get rid of the entire check :) > >+ if (!gAppUpdater.update.appVersion || > >+ Services.vc.compare(gAppUpdater.update.appVersion, > >+ Services.appinfo.version) == 0) { > >+ gAppUpdater.startDownload(); > >+ } else { > >+ gAppUpdater.checkAddonCompatibility(); > > } > > Seems like it would be useful to factor this into a gAppUpdater.doUpdate() > helper given that it's duplicated both here and in onCheckComplete. This > caller can also use "this" instead of gAppUpdater. Done. > >diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties > > >+# LOCALIZATION NOTE (downloadAndInstallButton.label): %S is replaced by the > >+# version of the update: "Download and update to 13.0". > >+update.downloadAndInstallButton.label=Download and update to %S > >+update.downloadAndInstallButton.accesskey=u > > I think "Download and update to <version>" is awkward. What was wrong with > "Update to <version>"? I know Alex thought it was jargony, but this only > shows up for users who've asked to be prompted for updates, and for them > getting the precise version number is useful. It also avoids the "long > button string" issue. Sure, we don't refer to downloading before installing in the update preferences UI either. Done. I also got rid of - // The button label value must be set so its height is correct. - this.setupUpdateButton("update.checkInsideButton"); by setting an initial label="" on the button.

I build with export MOZ_BUILD_DATE=20120401000000 and this patch for testing.

Fixed bitrot by bug 661057. (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #39) > Now that I've taken the time to read through all of the relevant code, > I should be more responsive in the future. My review request is one month old already...

Sorry, the number of bugs in my review queue was in the single digits when I wrote that, and has since ballooned up to >20. I'm as tired of listing my excuses as you probably are of hearing them, but it's worth noting that you do have many other options for reviewers: https://wiki.mozilla.org/Modules/Firefox In general, there are some other things you could do to help make reviewing this easier, too: - split your work into smaller pieces - avoid adding unrelated changes to subsequent iterations of the patch (e.g. the last part of comment 40)

Comment on attachment 623760 [details] [diff] [review] patch v4, unbitrotted >diff --git a/browser/base/content/aboutDialog.js b/browser/base/content/aboutDialog.js >@@ -157,22 +154,26 @@ function appUpdater() >+ // after checking, onCheckComplete() is called >+ return; You don't need an explicit return at the end of a function. >+ setupUpdateButton: function(mode, updateVersion) { >+ switch (mode) { I think that putting this logic into this method confuses things more than it helps. I think I would prefer having multiple actual "panes", with separate buttons/labels, and hardcoded actions. That way this code just needs to call selectPanel() rather than dynamically changing all of the button labels/actions. Is there a reason this wouldn't work well?

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #44) > I think I would prefer having multiple actual "panes", with > separate buttons/labels, and hardcoded actions. Heh, I still had my alternate patch where I did exactly that. It's not all that pretty either, I still need to update the button label to insert the version. It also had weird keyboard issues: you could activate a button which was not visible by pressing enter. I thought about disabling all buttons and only enable the selected button. That was the point where I gave up and wrote patch v4. I just hacked around this by unconditionally focussing the button in selectPanelFromButton().

Gavin, which of the patches would you like me to unbitrot? Do you still want to review this, or should I ask Robert Strong again? Robert said in comment 26, 8 months ago: > I've had discussions with UX and others about how multi-word buttons are > evil and since I don't want to have the same discussion again > and I don't own this code I am punting the review over to Gavin. So if you give your OK with the multi-word button... Robert wrote most of this code in bug 596813, and reviewed Ehsan's changes in bug 307181, bitrotting my patches...

Steffen, I'm a tad overloaded and won't have time to review these atm.

btw: this is simply xul and can be reviewed by most if not any of the front end devs.

Comment on attachment 623886 [details] [diff] [review] patch v5, multiple buttons This version looks good, but I think the strings have been simplified now? I don't really understand why we use slightly different strings for the buttons in the billboard case, or why we use different strings depending on whether it's a major update. It would be nice to consolidate where possible and remove the need for so many different hardcoded buttons, but that's somewhat tangential. I don't understand why you need selectPanelFromButton - just use selectPanel, and in the one place where you need to set the button label dynamically, do that separately. You can have selectPanel check for the presence of a button and focus it (perhaps with a focusedElement check similar to the one in setupUpdateButton now).

Still happening with Firefox 16 upgrading to 17. Downloaded without asking confirmation after I clicked on "Check for updates", while option "Check for updates, but let me choose whether to install them" is active.

Sorry for taking 10 months, real life intervened... > Comment on attachment 623886 [details] [diff] [review] > patch v5, multiple buttons > > This version looks good, but I think the strings have been simplified now? I > don't really understand why we use slightly different strings for the > buttons in the billboard case, or why we use different strings depending on > whether it's a major update. It would be nice to consolidate where possible > and remove the need for so many different hardcoded buttons, but that's > somewhat tangential. OK, I've removed the difference whether or not it's a major update. In both cases we show "Restart to Update". But in the billboard/license case, we show "Apply Update…". That's because clicking the button doesn't apply the update right away, but shows the old update dialog: chrome://mozapps/content/update/updates.xul > I don't understand why you need selectPanelFromButton - just use > selectPanel, and in the one place where you need to set the button label > dynamically, do that separately. You can have selectPanel check for the > presence of a button and focus it (perhaps with a focusedElement check > similar to the one in setupUpdateButton now). Done.

For testing purposes, use the patch from comment 41, and build with ./mach build MOZ_BUILD_DATE=20130301000000

Fixed focusing the button.

- Introduced a local variable in isPending() to fix a bogus javascript strict error (this.update being undefined when you click the "apply updates" button, although dump says this.update.state == "pending", and although isPending has been called before without error). I did the same to isApplied(). - Changed to logic in setPanel to return early. - Re-added a focusedElement check before focusing the button to prevent stealing the focus. - Removed the redundant "if" clause in applyBillboard. - Removed the unused "checkingAddonCompat" panel.

- Introduced a local variable in isPending() to fix a bogus javascript strict error (this.update being undefined when you click the "apply updates" button, although dump says this.update.state == "pending", and although isPending has been called before without error). I did the same to isApplied(). - Changed to logic in setPanel to return early. - Re-added a focusedElement check before focusing the button to prevent stealing the focus. - Removed the redundant "if" clause in applyBillboard. - Removed the unused "checkingAddonCompat" panel.

Try builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/steffen.wilberg@web.de-d7ae99b92b6b

Comment on attachment 772860 [details] [diff] [review] patch v6.2, unbitrotted again Brian, you have worked on aboutDialog.js recently (bitrotting my patch in the process), and have a slightly shorter review queue than Gavin. May I ask you to review my patch?

Let's ask Gavin who he would like to review it since there a quite a few changes and there are many people on his team more familiar with xul.

Comment on attachment 772860 [details] [diff] [review] patch v6.2, unbitrotted again Canceling the review per Comment 61

Comment on attachment 772860 [details] [diff] [review] patch v6.2, unbitrotted again I asked Felipe to help out.

See these bugs: TBird: Bug 707489 Bug 902597 (duplicates 707489) Fox: Bug 690964, was reopened 2012-11-06

Comment on attachment 772860 [details] [diff] [review] patch v6.2, unbitrotted again Review of attachment 772860 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay in looking into this, but it required some careful read of the patch and history. I will still give another pass but here's one full feedback pass through it. Also, this patch is making more UI changes than what the bug description implies (e.g. simplification of labels, removal of the isMajor "update/upgrade" distiction), so I think listing them in a single place and getting a new UI review is necessary (the UI changes were only discussed a long time ago so let's make those changes clear and re-verify that they are OK to do) ::: browser/base/content/aboutDialog.js @@ +133,5 @@ > + // in onCheckComplete. > + this.selectPanel("checkingForUpdates"); > + this.isChecking = true; > + this.checker.checkForUpdates(this.updateCheckListener, true); > + // after checking, onCheckComplete() is called This is now a fallback case but I think it'd be better to keep all cases explictly stated with the combination of properties that they are expecting. @@ +145,5 @@ > // true when there is an update already staged / ready to be applied. > get isPending() { > if (this.update) { > + let state = this.update.state; > + return state == "pending" || state == "pending-service"; this change is very weird.. are you sure it was not being caused by some other code change? we can't make this change unless we really understand why it's happening @@ +210,4 @@ > */ > selectPanel: function(aChildID) { > + let panel = document.getElementById(aChildID); > + if (panel.firstChild.localName != "button") { this is very non-descriptive, and special casing this shouldn't be done. just do the proper thing of checking if the firstChild is a button on the part of the code that deals with the button. this change also remind that this.updateBtn is obsolete and can be removed from the constructor. I wonder why the disabled was being set here.. maybe even when a deck is not selected the button is still active. so it's probably something we still need to worry about @@ +215,5 @@ > + return; > + } > + > + let button = panel.firstChild; > + if (button.id == "downloadAndInstallButton") { let's do this based on the aChildID param instead of the button id ::: browser/base/content/aboutDialog.xul @@ +51,5 @@ > #ifdef MOZ_UPDATER > <deck id="updateDeck" orient="vertical"> > + <hbox id="downloadAndInstall" align="center"> > + <button id="downloadAndInstallButton" > + oncommand="gAppUpdater.doUpdate();"/> Add a <!-- comment --> saying that this button label/accesskey will be filled by JS Why was the align="start" dropped? @@ -59,5 @@ > <image class="update-throbber"/><label>&update.checkingForUpdates;</label> > </hbox> > - <hbox id="checkingAddonCompat" align="center"> > - <image class="update-throbber"/><label>&update.checkingAddonCompat;</label> > - </hbox> is this just removing dead code? ::: browser/locales/en-US/chrome/browser/browser.properties @@ -199,5 @@ > -update.restart.updateButton.accesskey=R > -update.openUpdateUI.upgradeButton.label=Upgrade Now… > -update.openUpdateUI.upgradeButton.accesskey=U > -update.restart.upgradeButton.label=Upgrade Now > -update.restart.upgradeButton.accesskey=U dead strings too?

Looks like this is still happening in Firefox 28.

(In reply to Liz Henry :lizzard from comment #68) > Looks like this is still happening in Firefox 28. Until this bug gets marked as fixed, you can assume that every new update will have this bug. No need to comment. :) If you stop seeing the bug, that's worth commenting about. When this bug gets marked as fixed, there will be info regarding which version has the fix.

Thanks Felipe!

(In reply to :Felipe Gomes from comment #66) Thanks for the feedback. I know it's not the nicest patch to review. It got much larger due to requests from Gavin to simplify the existing code... But I think it's worth it, the new code is much easier to read than the existing code, because instead of one button with multiple identities (labels) and a single buttonOnCommand function with several cases, we now have multiple buttons with a single label and oncommand funtion: doUpdate, buttonRestartAfterDownload and buttonApplyBillboard. And I tried my best to add comments throughout. > Also, this patch is making more UI changes than what the bug description > implies (e.g. simplification of labels, removal of the isMajor > "update/upgrade" distiction), so I think listing them in a single place and > getting a new UI review is necessary (the UI changes were only discussed a > long time ago so let's make those changes clear and re-verify that they are > OK to do) So the changes are: Before the patch, if updates are set to manual, we display a "Check for Updates" button. If the user clicks that button, Firefox not only checks, but immediately proceeds to download the update and then displays the "Restart to Update" button. The user feels tricked into the update, because he only wanted to check for updates. With the patch, we always check for updates. If in manual mode, we then let the user decide whether or not he wants to download and apply the update. The button then says "Update to 28.0a1", referring to the version of the update. That's what this bug is about. The other change is dropping the distinction between major and minor updates with regard to the button labels, per comment 49. Right now, when only a minor update is available, it would display "Restart to Update". A major update would show as "Upgrade Now". We now always show "Restart to Update". > ::: browser/base/content/aboutDialog.js > @@ +133,5 @@ > > + // in onCheckComplete. > > + this.selectPanel("checkingForUpdates"); > > + this.isChecking = true; > > + this.checker.checkForUpdates(this.updateCheckListener, true); > > + // after checking, onCheckComplete() is called > > This is now a fallback case but I think it'd be better to keep all cases > explictly stated with the combination of properties that they are expecting. I don't understand what you mean here. This is not the fallback case, it's the main case: If the update is not disabled, there's no update pending, or download already in progress, we show the spinner, and then checkForUpdates. You need to continue reading in onCheckComplete. > > @@ +145,5 @@ > > // true when there is an update already staged / ready to be applied. > > get isPending() { > > if (this.update) { > > + let state = this.update.state; > > + return state == "pending" || state == "pending-service"; > > this change is very weird.. are you sure it was not being caused by some > other code change? we can't make this change unless we really understand why > it's happening The change was only about introducing a local variable. See comment 55: I got an error saying this.update is undefined when clicking the "apply updates" button. I couldn't reproduce that bug, so I reverted that. > @@ +210,4 @@ > > */ > > selectPanel: function(aChildID) { > > + let panel = document.getElementById(aChildID); > > + if (panel.firstChild.localName != "button") { > > this is very non-descriptive, and special casing this shouldn't be done. > just do the proper thing of checking if the firstChild is a button on the > part of the code that deals with the button. OK, I changed the order of the code in selectPanel around. > this change also remind that this.updateBtn is obsolete and can be removed > from the constructor. I removed all instances of that already!? > I wonder why the disabled was being set here.. maybe even when a deck is not > selected the button is still active. so it's probably something we still > need to worry about Precisely. The button in the previously selected panel can still have focus. That means that you could be looking at the "Apply Update" button, after having downloaded the update, but if you press enter, you activate the "Check for Updates" button, which still has focus. That happens if you comment out the button.focus() line in selectPanel. > @@ +215,5 @@ > > + return; > > + } > > + > > + let button = panel.firstChild; > > + if (button.id == "downloadAndInstallButton") { > > let's do this based on the aChildID param instead of the button id Done. > ::: browser/base/content/aboutDialog.xul > @@ +51,5 @@ > > #ifdef MOZ_UPDATER > > <deck id="updateDeck" orient="vertical"> > > + <hbox id="downloadAndInstall" align="center"> > > + <button id="downloadAndInstallButton" > > + oncommand="gAppUpdater.doUpdate();"/> > > Add a <!-- comment --> saying that this button label/accesskey will be > filled by JS Done. > Why was the align="start" dropped? I think it wasn't needed. > @@ -59,5 @@ > > <image class="update-throbber"/><label>&update.checkingForUpdates;</label> > > </hbox> > > - <hbox id="checkingAddonCompat" align="center"> > > - <image class="update-throbber"/><label>&update.checkingAddonCompat;</label> > > - </hbox> > > is this just removing dead code? Yes. > ::: browser/locales/en-US/chrome/browser/browser.properties > @@ -199,5 @@ > > -update.restart.updateButton.accesskey=R > > -update.openUpdateUI.upgradeButton.label=Upgrade Now… > > -update.openUpdateUI.upgradeButton.accesskey=U > > -update.restart.upgradeButton.label=Upgrade Now > > -update.restart.upgradeButton.accesskey=U > > dead strings too? Sure. "Check for Updates" is becoming a dead string, because we don't present a button with such a label anymore. "Resume Downloading %S…" is a dead string already. The restart.updateButton ("Restart to Update") and the restart.upgradeButton ("Upgrade Now") are being merged into the new updateButton in the dtd ("Restart to Update"). This is dropping the distinction between minor and major update per comment 49. Likewise, the openUpdateUI.applyButton ("Apply Update…) and openUpdateUI.upgradeButton ("Upgrade Now…") are being merged into the new applyButtonBillboard in the dtd ("Apply Update…"). This is dropping the distinction between minor and major update as well for the billboard case.

Comment on attachment 833290 [details] [diff] [review] patch v7: address comment 66 Review of attachment 833290 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Steffen! The code is indeed simpler to ready now. I only have two nits, but this is r+! Before landing this change I'd like to get an OK again from UI and rstrong about the changes. In the meantime I've sent your patch to tryserver and it is green: https://tbpl.mozilla.org/?tree=Try&rev=1025ded26067 ::: browser/base/content/aboutDialog.js @@ +211,5 @@ > */ > selectPanel: function(aChildID) { > + let panel = document.getElementById(aChildID); > + > + if (panel.firstChild.localName == "button") { in order to prevent this from breaking if the XUL structure is changed, let's do it like this: let button = panel.querySelector("button"); if (button) { ::: browser/base/content/aboutDialog.xul @@ +50,5 @@ > <vbox id="updateBox"> > #ifdef MOZ_UPDATER > <deck id="updateDeck" orient="vertical"> > + <hbox id="downloadAndInstall" align="center"> > + <button id="downloadAndInstallButton" let's keep the align="start" just because. It might be necessary for some theme other than the one you've developed on (or custom themes)

I've already looked over the changes to the calls to the app update service and I'm fine with them so no need for me to look over the patch.

Comment on attachment 833290 [details] [diff] [review] patch v7: address comment 66 Thanks Robert! Yeah didn't mean to look at the patch, just wanted to bring up the overall change. Stephen, care to check this? It was discussed with Faaborg long ago but I want to double check. The change is: Before the patch, if updates are set to manual, we display a "Check for Updates" button. If the user clicks that button, Firefox not only checks, but immediately proceeds to download the update and then displays the "Restart to Update" button. The user feels tricked into the update, because he only wanted to check for updates. With the patch, we always check for updates. If in manual mode, we then let the user decide whether or not he wants to download and apply the update. The button then says "Update to 28.0a1", referring to the version of the update. Also, it drops the distinction between major and minor updates with regard to the button labels, per comment 49. Right now, when only a minor update is available, it would display "Restart to Update". A major update would show as "Upgrade Now". We now always show "Restart to Update".

(In reply to Steffen Wilberg from comment #71) > So the changes are: > Before the patch, if updates are set to manual, we display a "Check for > Updates" button. If the user clicks that button, Firefox not only checks, > but immediately proceeds to download the update and then displays the > "Restart to Update" button. The user feels tricked into the update, because > he only wanted to check for updates. > > With the patch, we always check for updates. If in manual mode, we then let > the user decide whether or not he wants to download and apply the update. > The button then says "Update to 28.0a1", referring to the version of the > update. > > The other change is dropping the distinction between major and minor updates > with regard to the button labels, per comment 49. > Right now, when only a minor update is available, it would display "Restart > to Update". A major update would show as "Upgrade Now". > We now always show "Restart to Update". In the interest of a clear and concise summary for those recently discovering this now lengthy bug (please review/clarify/correct for accuracy): BEFORE - The pref "Check for updates, but let me choose whether to install them" was actually "Automatically download and install updates if found, but let me choose when to check for them" - Checking was a manual process only available by clicking the [Check for Updates] button in Help > About - User not notified what update was found, and would only discover the new version after the update was installed and the app restarted - No release notes or link thereto - After the update was applied a restart is required; however, the prompt differs: major version updates show the label [Upgrade Now] minor version updates show the label [Restart to Update] AFTER The pref "Check for updates, but let me choose whether to install them" will: - Automatically check for updates. - If update found the Help > About [Check for Updates] button is now [Update to <version>]. (But you have to go to Help > About to discover an update was found) - No release notes or link thereto (yet) - Clicking [Update to <version>] downloads and installs the update. - After installing update, regardless of major or minor version, the button text is now consistent: [Restart to Update]. Please clarify whether the above summary is accurate and when this is anticipated to land. Particularly interested to see this land (at least in Aurora) before v28 makes it to Aurora -- don't mind updating v27 but don't want to upgrade to v28 (non-Holly). If that's not tenable, please mention so users can switch to Holly as necessary before the next release cycle window. Also, it would be handy if there was a link to release notes whenever an update is found, to assist the user in making an informed decision -- after all, they did opt-in to selective updates. Thanks!

Comment 75 is mostly correct, assuming that the user has updates set to either "Never check for updates" or "Check for updates, but let me choose whether to install them". (The difference between those is that the first one doesn't check in the background, outside of the About dialog; the second one does check in the background, and either displays a small notification or a large dialog offering to update or "ask later", also offering a link to the release notes). On the minor/major update part: Even changes from one Firefox release to another is a minor update. See the update.xml file for the update from 24 to 25.0.1, which has type="minor": https://aus3.mozilla.org/update/3/Firefox/24.0/20130910160258/WINNT_x86-msvc/en-US/release/Windows_NT%206.1.1.0%20(x64)/default/default/update.xml So there are no major updates at all. In other words, removing the different strings for major updates in the About Firefox dialog is dead code removal.

(In reply to linux.user.since.2002 from comment #75) > Please clarify whether the above summary is accurate and when this is > anticipated to land. Particularly interested to see this land (at least in > Aurora) before v28 makes it to Aurora -- don't mind updating v27 but don't > want to upgrade to v28 (non-Holly). If that's not tenable, please mention > so users can switch to Holly as necessary before the next release cycle > window. I want this change to have a full cycle of user testing, so I don't want to land it at the end of the 28 Nightly cycle. I intend to push it one or two days after m-c becomes v29, next week. > Also, it would be handy if there was a link to release notes whenever an > update is found, to assist the user in making an informed decision -- after > all, they did opt-in to selective updates. > > Thanks! That is a good suggestion, and it's worth discussion (not sure if it's been discussed in the past or not), but it's out of the scope of this bug, so it's better to file a new bug for this suggestion.

Actually I prefer to push my patches myself. I would've done so already, but you've requested another ui-review from shorlander, which hasn't been granted yet. Of course I'm fine with landing in the 29 cycle. This already took more than 2 years, I don't care about a couple of weeks. The release notes suggestion is bug 685209.

Ah ok, I didn't know if you had commit access

Comment on attachment 833290 [details] [diff] [review] patch v7: address comment 66 Review of attachment 833290 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to :Felipe Gomes from comment #74) > Stephen, care to check this? It was discussed with Faaborg long ago but I > want to double check. The change is: > > Before the patch, if updates are set to manual, we display a "Check for > Updates" button. If the user clicks that button, Firefox not only checks, > but immediately proceeds to download the update and then displays the > "Restart to Update" button. The user feels tricked into the update, because > he only wanted to check for updates. > With the patch, we always check for updates. If in manual mode, we then let > the user decide whether or not he wants to download and apply the update. > The button then says "Update to 28.0a1", referring to the version of the > update. > > > Also, it drops the distinction between major and minor updates with regard > to the button labels, per comment 49. > Right now, when only a minor update is available, it would display "Restart > to Update". A major update would show as "Upgrade Now". > We now always show "Restart to Update". Sounds good, better reflects the purpose and wording of the pref.

I've addressed comment 72 and pushed: https://hg.mozilla.org/integration/fx-team/rev/a656dc7ef423

(In reply to Steffen Wilberg from comment #81) > I've addressed comment 72 and pushed: > https://hg.mozilla.org/integration/fx-team/rev/a656dc7ef423 In the future, when you backout and reland, please do so in one push to save resources.

Sure, will do. Thanks for taking care of that.

How about changing the wording of Tools -> Options -> Advanced -> Update as follows? Instead of -> Never check for updates (not recommended: security risk) ... how about something like this? -> Never check for updates (For security reasons, when Help -> About Holly is clicked, Firefox will automatically check to see if an update is available.) I know it's a bit long, but at least it explains what actually happens. With the new behavior, I'd say the wording "Never check for updates" has been rendered somewhat misleading.

Perhaps "Never check for updates *automatically* (not recommended: security risk)"?

Please excuse me Mr. Wilberg, I know you've been working on this for a long time, and I've really tried to restrain myself, but you - and a lot of people at Mozilla - don't seem to get the point. At issue here is the fact that "never" means "NEVER". In fact, Fx DOES check *automatically*, albeit "in the background" as you've termed it. If I may, sir, in all the time you've been working on this, you don't appear to have asked yourself why people select "Never check for updates", although they're explicitly told it's a security risk and not recommended by Mozilla. Though I've got only one statistic (myself), I'd bet you dollars to doughnuts that the overwhelming majority of the people who do this are people - like me - who also have the Automatic Updates turned off in Windows. In fact, just for the record, not one of the dozens and dozens of programs on my many computers has permission to automatically check for updates. Again if I may, sir, I find your new implementation of this function as domineering and arrogant (ouch!) as the decision to force people to have one tab open all the time and - on a much grander scale - the abomination that is Australis. You guys are really on a roll when it comes to forcing people to use a program that traditionally prided itself on user choice in a specific way. No, sir, "Never check for updates *automatically* (not recommended: security risk)" simply isn't telling the truth. I certainly can't force you to rethink your implementation, but I implore you to explain to users what's actually happening. How you formulate it is up to you. Please excuse my harsh tone, sir, but you caught me on a bad night with this one. Thanks for your understanding.

The concerns about the change in behavior for that option are valid, but let's discuss it in the proper bug, bug 950357

Checking the "Check for updates, but let me choose whether to install them" pref and then opening the Help/About dialog displays the "Update to 29.0a1" button now. Verified fixed FF 29.0a1(2014-01-08), win 7 x64