596813 - Check for updates inside the About window without opening a new window

Status: VERIFIED FIXED

(Firefox :: General, enhancement)

Description

[Tyler Downer [He/Him]]

When you click the check for updates in the new about window, it open up a dialog box with the progress icon the checks for updates. It basically works like the old Help>Check for Updates.
From a usability standpoint, this open up another dialog box infront of a second one to simply check for an update. What should probably happen is something like chrome. You click "check for updates" the button dissapears and a spinner replaces it along with the text "checking for updates" somewhat like in the new AOM. If no updates are found, it says so. if an update is found, then it can give the update banner like it currently does.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Well, if we're going to do this, we might as well go the whole way and have it automatically check for updates:

Initial state:

 Minefield
 4.0b7pre (released 09-15-2010)
 @ Checking for updates...

No update available:

 Minefield
 4.0b7pre (released 09-15-2010)
 This is the latest available version

Minor update available:

 Minefield
 4.0b7pre (released 09-15-2010)
 @ Downloading update — 879 KB of 2.1 MB

Minor update downloaded:

 Minefield
 4.0b7pre (released 09-15-2010)
 [ Apply Update ]

--> restarts browser and applies update
Major update avaliable:

 Minefield
 4.0b7pre (released 09-15-2010)
 [ Upgrade Now... ]

--> leads to major update UI.

The @ is meant to be a spinner :)

(Not sure if we want to make "Check for Updates..." in the Application Menu on OS X just open the About Firefox window if we do this - let's leave that as a separate bug, but might be good to not have the duplicated functionality anymore!)

Comment 2

[Tyler Downer [He/Him]]

Agreed, however, maybe a link to release notes?
 Minefield
 4.0b7pre (released 09-15-2010)
 [ Apply Update ] (release notes)
Or should we just leave that out as the firstrun page will have a "what's new in this release"?

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Don't think the releasenotes are really worthwhile, and we rarely used that function to date. The firstrun page has the 4-1-1, as does the build itself.

Comment 4

[Robert Strong (they/them - no direct email)]

(In reply to comment #1)
> Well, if we're going to do this, we might as well go the whole way and have it
> automatically check for updates:
> 
> Initial state:
> 
>  Minefield
>  4.0b7pre (released 09-15-2010)
>  @ Checking for updates...
> 
> No update available:
> 
>  Minefield
>  4.0b7pre (released 09-15-2010)
>  This is the latest available version
> 

additional item - if update found:
Minefield
4.0b7pre (released 09-15-2010)
@ Checking add-on compatibility...
or similar wording

We'll also either need to show the regular update UI 
or possibly a new window to display the add-ons that will be disabled after updating.

> Minor update available:
> 
>  Minefield
>  4.0b7pre (released 09-15-2010)
>  @ Downloading update — 879 KB of 2.1 MB
> 
> Minor update downloaded:
> 
>  Minefield
>  4.0b7pre (released 09-15-2010)
>  [ Apply Update ]
> 
> --> restarts browser and applies update
> Major update avaliable:
> 
>  Minefield
>  4.0b7pre (released 09-15-2010)
>  [ Upgrade Now... ]
> 
> --> leads to major update UI.
We only show the billboard when there is a billboardURL specified in the update snippet.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #4)
> additional item - if update found:
> Minefield
> 4.0b7pre (released 09-15-2010)
> @ Checking add-on compatibility...
> or similar wording

That wording looks good. In cases where there is an add-on compatibility conflict, I think we should show

  [ Apply Update... ]

which would then lead to the appropriate spot in the regular update UI.

> >  Minefield
> >  4.0b7pre (released 09-15-2010)
> >  [ Upgrade Now... ]
> > 
> > --> leads to major update UI.
> We only show the billboard when there is a billboardURL specified in the update
> snippet.

Yup, that's OK.

Comment 6

[Robert Strong (they/them - no direct email)]

Attachment: strings patch

There are more states needed than what is needed by the existing implementation so I went with a new string section. A couple of the strings are duplicated but I think this is cleaner especially if we remove the menuitem.

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 475750 [details] [diff] [review]
strings patch

Looks ok to me but put a bug reference against these so localisers can read up a bit on what sort of context these will appear in.

Comment 8

[Robert Strong (they/them - no direct email)]

Attachment: strings patch rev2 - with additional l10n notes (checked in)

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 475756 [details] [diff] [review]
strings patch rev2 - with additional l10n notes (checked in)

Looks good

Comment 10

[Robert Strong (they/them - no direct email)]

Comment on attachment 475756 [details] [diff] [review]
strings patch rev2 - with additional l10n notes (checked in)

I either need a= on this patch or this bug needs to block for me to land the strings... dag nabbit!

Comment 11

[Robert Strong (they/them - no direct email)]

Comment on attachment 475756 [details] [diff] [review]
strings patch rev2 - with additional l10n notes (checked in)

Strings pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/93b0d22eee9e

Comment 12

[Jesse Ruderman]

> Well, if we're going to do this, we might as well go the whole way and have 
> it automatically check for updates

I agree.  Even if the user has unchecked "automatically check for updates" in the updates prefs?

Comment 13

[Robert Strong (they/them - no direct email)]

(In reply to comment #12)
> > Well, if we're going to do this, we might as well go the whole way and have 
> > it automatically check for updates
> 
> I agree.  Even if the user has unchecked "automatically check for updates" in
> the updates prefs?
Yes, that is in regards to the background update check / download. This will automatically check when the UI is opened and then provide the user with the option to update/upgrade.

Comment 14

[Robert Strong (they/them - no direct email)]

Actually, you are correct in that it just downloads at that stage if everything is compatible but I think that is the best behavior.

Comment 15

[Robert Strong (they/them - no direct email)]

beltzner, to address the case Jesse brought up we could just display the Check for Updates" button instead of automatically checking when the pref to auto check is false. This should cover the vast majority of users that don't disable the auto check.

Comment 16

[Dave Garrett]

I like the idea of having an automatic check and only an "install update" button when available, but I suggest not downloading too much automatically without user approval. Everyone here probably has the bandwidth to spare, but there are plenty of users who still complain about ever last KB because they're stuck on a slow connection.

Comment 17

[IU]

Please don't be automatically downloading updates when auto update is disabled.  Not every Firefox/Minefield or Thunderbird/Shredder update is desirable, and people have good reasons to not update -- especially when the update breaks important functionality or compatibility, security be damned.

I especially hate it when Firefox or Thunderbird downloads updates, while I'm testing regressions for a particular build, or downloads an update I have decided to not use, because it has a regression that seriously impedes my ability to access certain sites/content.  For this reason, I have automatic updates disable.

And if you automatically download an update the user (*by right*) decides to never install, what then, when a future update appears, that the user does wish to install?  Which will get installed?  Is Firefox going to keep needlessly downloading updates every time, regardless?

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

rs alerted me to a couple of cases I hadn't considered, and as IU mentions, we shouldn't automatically check every time a user opens the window, especially if:

 - the user has set a pref to not automatically check
 - the user doesn't have privs to apply the update
 - the user has been restriced from applying updates via mozilla.config

Updated Design Document, superseding the one in comment 1

Well, if we're going to do this, we might as well go the whole way and have it
automatically check for updates:

- if app.update.auto = true and mozilla.config allows users to check for updates:

// S0: Initial State

 Minefield
 4.0b7pre (released 09-15-2010)
 @ Checking for updates...

--> if there is a minor update available, skip to S2
--> if there is a major update available, skip to S7

// S1: No update available

 Minefield
 4.0b7pre (released 09-15-2010)
 This is the latest available version

// S2: Minor update available:

--> if the user doesn't have OS access to apply an update, skip to S10

 Minefield
 4.0b7pre (released 09-15-2010)
 @ Checking Add-on compatibility...

--> if there is no add-on compatibility problem, skip to S4

// S3: Add-on compatibility problems

 Minefield
 4.0b7pre (released 09-15-2010)
 [ Apply Update...]

 --> this brings up the update UI to show which add-ons are incompatible and determine if the user wants to continue. The user has two options in this UI, either to continue with the update or to cancel.

Cancel should bring the user back to S3
Continue should bring the user to S4

// S4: Minor update downloading

 Minefield
 4.0b7pre (released 09-15-2010)
 @ Downloading update — 879 KB of 2.1 MB

--> if the partial update fails, just automatically start downloading the full update, no need to notify the user
--> if there is a problem in the download, skip to S6

// S5: Minor update downloaded

 Minefield
 4.0b7pre (released 09-15-2010)
 [ Apply Update ]

// S6: Problem with download

 Minefield
 4.0b7pre (released 09-15-2010)
 Updates available at www.firefox.com

--> the updater UI should pop up on the error screen
--> the About Window should look as above

// S7: Major update avaliable

--> if the user doesn't have OS access to apply an update, skip to S10

 Minefield
 4.0b7pre (released 09-15-2010)
 [ Upgrade Now... ]

--> open major update UI if a billboardURL is present, otherwise skip to S8

--> if the user cancels/turns down the major update in the updater UI, the about window should return to S7

// S8: Downloading major update

 Minefield
 4.0b7pre (released 09-15-2010)
 @ Downloading update — 879 KB of 2.1 MB

--> if there is a problem with the download, go to S6

// S9: Major update downloaded

 Minefield
 4.0b7pre (released 09-15-2010)
 [ Apply Update ]

// S10: User doesn't has insufficient rights to apply update

--> open minor update UI to the Unable to Update page, and set the About Window to read:

 Minefield
 4.0b7pre (released 09-15-2010)
 An update is available, but you do not have access to apply it
 

That set of states should cover the standard case. Now, some modifications:

- if auto.app.update = false 
--> the initial state should be

 Minefield
 4.0b7pre (released 09-15-2010)
 [ Check for Updates ]

--> clicking the button leads to S0

- if mozilla.config doesn't allow application updates:

--> the initial state should be

 Minefield
 4.0b7pre (released 09-15-2010)
 Updates disabled by administrator

That should about cover it, I think!

Comment 19

[Robert Strong (they/them - no direct email)]

(In reply to comment #18)
>...
>  --> this brings up the update UI to show which add-ons are incompatible and
> determine if the user wants to continue. The user has two options in this UI,
> either to continue with the update or to cancel.
> 
> Cancel should bring the user back to S3
> Continue should bring the user to S4
What do you think about closing the about dialog if the update ui is opened? I'm ok with not doing that but think it might be a good thing to do.

> // S4: Minor update downloading
> 
>  Minefield
>  4.0b7pre (released 09-15-2010)
>  @ Downloading update — 879 KB of 2.1 MB
> 
> --> if the partial update fails, just automatically start downloading the full
> update, no need to notify the user
> --> if there is a problem in the download, skip to S6
> 
> // S5: Minor update downloaded
> 
>  Minefield
>  4.0b7pre (released 09-15-2010)
>  [ Apply Update ]
> 
> // S6: Problem with download
> 
>  Minefield
>  4.0b7pre (released 09-15-2010)
>  Updates available at www.firefox.com

This will be a bit confusing since the flow will be
@ Downloading update — 879 KB of 2.1 MB
then
Updates available at www.firefox.com

> 
> --> the updater UI should pop up on the error screen
> --> the About Window should look as above
> 
> // S7: Major update avaliable
> 
> --> if the user doesn't have OS access to apply an update, skip to S10
> 
>  Minefield
>  4.0b7pre (released 09-15-2010)
>  [ Upgrade Now... ]
> 
> --> open major update UI if a billboardURL is present, otherwise skip to S8
> 
> --> if the user cancels/turns down the major update in the updater UI, the
> about window should return to S7
Same here and elsewhere regarding closing the UI.

> 
> // S8: Downloading major update
> 
>  Minefield
>  4.0b7pre (released 09-15-2010)
>  @ Downloading update — 879 KB of 2.1 MB
> 
> --> if there is a problem with the download, go to S6
> 
> // S9: Major update downloaded
> 
>  Minefield
>  4.0b7pre (released 09-15-2010)
>  [ Apply Update ]
Previously you had "Upgrade Now" for the major update downloaded. Do you want it to change and if so what about S7?

> 
> // S10: User doesn't has insufficient rights to apply update
> 
> --> open minor update UI to the Unable to Update page, and set the About Window
> to read:
> 
>  Minefield
>  4.0b7pre (released 09-15-2010)
>  An update is available, but you do not have access to apply it
We currently show the manual update page with a link to download. How about using the S6 download failed case or some variant here instead?
Updates available at www.firefox.com

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #19)
> > Cancel should bring the user back to S3
> > Continue should bring the user to S4
> What do you think about closing the about dialog if the update ui is opened?
> I'm ok with not doing that but think it might be a good thing to do.

Honestly, whatever's easiest. I think the cleanest UX would be for the updater UI to appear in cases where more interaction/space is needed, and then once those situations are resolved, the user would return to the About Firefox window.

However, if that's tricksy to do, I'm happy with us saying "as soon as it's a non-standard path, we close the About Window and bring up the Updater UI", too.

> This will be a bit confusing since the flow will be
> @ Downloading update — 879 KB of 2.1 MB
> then
> Updates available at www.firefox.com

Yeah, I thought about that, but wanted to keep the string short. Maybe instead:

   Automatic update failed. _Download the latest version_

with the latter part being a link to http://www.firefox.com

> > // S9: Major update downloaded
> > 
> >  Minefield
> >  4.0b7pre (released 09-15-2010)
> >  [ Apply Update ]
> Previously you had "Upgrade Now" for the major update downloaded. Do you want
> it to change and if so what about S7?

Yeah, that should be

     [ Apply Upgrade ]

to keep the consistency between downloading and application. (Of course, if we close this window and show the major update billboard/UI, then this is moot)

> > // S10: User doesn't has insufficient rights to apply update
> > 
> > --> open minor update UI to the Unable to Update page, and set the About Window
> > to read:
> > 
> >  Minefield
> >  4.0b7pre (released 09-15-2010)
> >  An update is available, but you do not have access to apply it
> We currently show the manual update page with a link to download. How about
> using the S6 download failed case or some variant here instead?
> Updates available at www.firefox.com

Well, I was figuring the important thing to signal was that the user didn't have the rights to update, but that an update is available. Didn't see what pointing them to the binary would do as they'd be unlikely to have access to make that work, too.

Comment 21

[Robert Strong (they/them - no direct email)]

Attachment: patch - additional strings

Comment 22

[Robert Strong (they/them - no direct email)]

Comment on attachment 477209 [details] [diff] [review]
patch - additional strings

bah... missed some

Comment 23

[Robert Strong (they/them - no direct email)]

Attachment: patch - additional strings

Comment 24

[Dave Townsend [:mossop]]

Comment on attachment 477216 [details] [diff] [review]
patch - additional strings

>+# LOCALIZATION NOTE (update.manual.label) - followed by www.firefox.com
>+update.manual.label=Updates available at

I'm a little concerned by this one, it might need to be a prefix and suffix string to give localizers the flexibility they need, perhaps spin it past Axel? Otherwise looks ok

Comment 25

[Robert Strong (they/them - no direct email)]

Attachment: patch - additional strings rev2 - add prefix / suffix strings along with notes

Comment 26

[Robert Strong (they/them - no direct email)]

Attachment: screenshots

This is everything but the buttons to open the current app update UI.

Comment 27

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 477700 [details]
screenshots

Looks nice!

Stylistically: I think we want to use a light grey & italic treatment so that it's not as front-and-center.

Also, in the case where the updater isn't built/enabled, we'll want a space between the version number and the paragraph of text.

String nitpicking:

s/This is the latest available version/Firefox is up to date/
s/Updates disabled by administrator/Updates disabled by your system administrator/

Really helps to see it in screenshots, Rob. Thanks!

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #27)
> Comment on attachment 477700 [details]
> screenshots
> 
> Looks nice!

Indeed! But I wonder if the space we have available is enough for localized builds or how much the about dialog has to be stretched. Do we know enough from the current update dialog to make sure to not cut-off elements or destroying the layout by stretching it too much?

Comment 29

[Robert Strong (they/them - no direct email)]

These are new strings and the current update dialog strings should not be used when localizing this. Having said that, it will be important to check up with l10n for these changes as well as the other changes made to the about dialog since either could potentially cause problems for l10n.

Comment 30

[Axel Hecht [:Pike]]

Comment on attachment 477265 [details] [diff] [review]
patch - additional strings rev2 - add prefix / suffix strings along with notes

I think we should have the ' ' explicitly encoded in these strings for languages without them being able to drop them. Needs the unicode escape of it to work, as regular ' ' is cropped at the end of strings.

Where do we actually size this dialog? I see we do something explicit only for the mac. Which makes me think, can we add the potentially longest string into the UI so that that "just works"?

Also, looking forward to the real patch, can we make it so that a mozmill test can explicitly turn each of those strings on? Then we can make automated testing of the various options for l10n.

Many tools won't show the localization note when people actually edit the strings unless it's explicitly bound to the keys with (...), sadly.

Feedback- on the whitespace issue.

Comment 31

[Robert Strong (they/them - no direct email)]

The dialog and its sizing were implemented in bug 579547... probably best to ask there or file a new bug cc'ing the people from bug 579547.

I will probably go with dtd's for each string to avoid many of the issues you bring up and I might be able to add a pref to allow testing of each string.

Comment 32

[Robert Strong (they/them - no direct email)]

Attachment: screenshots with comments addressed

Carrying forward ui-r+ but feel free to ask for changes since I still have some backend work to finish for the final patch.

Still looking to have this finished by tomorrow.

Comment 33

[Robert Strong (they/them - no direct email)]

Attachment: patch - string changes rev3

Axel, thanks for the feedback and I'd appreciate your feedback on this new string patch as soon as you're able.

Comment 34

[Robert Strong (they/them - no direct email)]

Comment on attachment 477859 [details] [diff] [review]
patch - string changes rev3

Axel, I just noticed that the comment in browser.properties isn't correct anymore... guidance in the form of examples on what comments would be the most useful for l10n would be appreciated.

Comment 35

[Robert Strong (they/them - no direct email)]

Axel, with the way it is implemented if an update string is longer than the space available the ui will widen to display it even if the string isn't currently displayed. I see no reasonable way to add code for testing on the client side and hope that adding a l10n note regarding this fact would suffice.

Comment 36

[Axel Hecht [:Pike]]

Comment on attachment 477859 [details] [diff] [review]
patch - string changes rev3

I'll leave you in the competent hands of Henrik when it comes down to testing. I guess if the code is factored such that we can call methods or set data without needing the events, things should be testable.

>+
>+<!-- LOCALIZATION NOTE - these strings must be short since they must fit on a
>+     single line in the about dialog. This is especially important for the
>+     prefix and suffix strings on either side of an url. -->

This note needs to explicitly call out the entities at risk. I'm a bit confused by this comment and comment 35. Does it resize or does it not?
I'd just call out explicitly the most likely candidates for size problems in a ().

<...>

>+<!-- LOCALIZATION NOTE (update.failed.start) - text before updateFailedLink -->
>+<!ENTITY update.failed.start        "Update failed. ">
>+
>+<!-- LOCALIZATION NOTE (update.failed.linkText) - this is the link text that links to http://www.firefox.com -->
>+<!ENTITY update.failed.linkText     "Download the latest version">
>+
>+<!-- LOCALIZATION NOTE (update.failed.end) - text after updateFailedLink -->
>+<!ENTITY update.failed.end          "">
>+
>+<!-- LOCALIZATION NOTE (update.manual.start) - text before link to http://www.firefox.com -->
>+<!ENTITY update.manual.start        "Updates available at ">
>+
>+<!-- LOCALIZATION NOTE (update.manual.end) - text after www.firefox.com -->
>+<!ENTITY update.manual.end          "">
>+
>+<!-- LOCALIZATION NOTE (update.downloading.start) â is the "em dash" (long dash)
>+     and this is followed by the amount downloaded.
>+     example: Downloading update â 111 KB of 13 MB -->
>+<!ENTITY update.downloading.start   "Downloading update â ">
>+
>+<!-- LOCALIZATION NOTE (update.downloading.end) - text after the amount downloaded -->
>+<!ENTITY update.downloading.end     "">

I'd fold the localization notes into one per group, like

<!-- LOCALIZATION NOTE (update.failed.start,update.failed.linkText,update.failed.end) 
 failed.start, failed.linkText, and failed.end all go into one line, linkText being wrapped in an anchor linking to http://www.firefox.com.
 As this is all in one line, try to make the localized text short.
-->
and
<!-- LOCALIZATION NOTE (update.manual.start,update.manual.end) 
 manual.start and manual.end all go into one line, with the amount downloaded inserted in between.
 â is the "em dash" (long dash)
 example: Downloading update â 111 KB of 13 MB
 As this is all in one line, try to make the localized text short.
-->

feedback+ with the comments fixed.

Comment 37

[Robert Strong (they/them - no direct email)]

Attachment: patch - string changes rev4

Comment 38

[Robert Strong (they/them - no direct email)]

(In reply to comment #36)
> Comment on attachment 477859 [details] [diff] [review]
> patch - string changes rev3
> 
> I'll leave you in the competent hands of Henrik when it comes down to testing.
> I guess if the code is factored such that we can call methods or set data
> without needing the events, things should be testable.
This uses a deck and I don't think it is appropriate to add one-off code to the client for testing in this instance since mozmill can go into the dom to unhide elements in decks. To prevent the ui from auto checking mozmill can turn off auto update before testing.

>... 
> This note needs to explicitly call out the entities at risk. I'm a bit confused
> by this comment and comment 35. Does it resize or does it not?
> I'd just call out explicitly the most likely candidates for size problems in aIt does and I cleaned up the notes. Having said that, experience shows that any of these strings could potentially be too long so I added notes for all of them.

>...
> I'd fold the localization notes into one per group, like
> 
> <!-- LOCALIZATION NOTE
> (update.failed.start,update.failed.linkText,update.failed.end) 
>  failed.start, failed.linkText, and failed.end all go into one line, linkText
> being wrapped in an anchor linking to http://www.firefox.com.
>  As this is all in one line, try to make the localized text short.
> -->
> and
> <!-- LOCALIZATION NOTE (update.manual.start,update.manual.end) 
>  manual.start and manual.end all go into one line, with the amount downloaded
> inserted in between.
>  â is the "em dash" (long dash)
>  example: Downloading update â 111 KB of 13 MB
>  As this is all in one line, try to make the localized text short.
> -->
> 
> feedback+ with the comments fixed.
Done and thanks for the review!

Comment 39

[Dave Townsend [:mossop]]

Comment on attachment 477985 [details] [diff] [review]
patch - string changes rev4

r+ with these two comments addressed.

>+<!-- LOCALIZATION NOTE (update.failed.start,update.failed.linkText,update.failed.end):
>+     update.failed.start, update.failed.linkText, and update.failed.end all go into
>+     one line with linkText being wrapped in an anchor that links to a site to download
>+     the latest version of Firefox (e.g. http://www.firefox.com). As this is all in
>+     one line, try to make the localized text short (see bug 596813 for screenshots). -->
>+<!ENTITY update.failed.start        "Update failed. ">
>+<!ENTITY update.failed.linkText     "Download the latest version">
>+<!ENTITY update.failed.end          "">
>+
>+<!-- LOCALIZATION NOTE (update.manual.start,update.manual.end): update.manual.start and
>+     update.manual.end all go into one line and have an anchor in between that links to a site to
>+     download the latest version of Firefox (e.g. http://www.firefox.com). As this is all in one
>+     line, try to make the localized text short (see bug 596813 for screenshots). -->

It is unclear from this what the text of the link that goes in the middle will be, will it be the full URL?

>+<!-- LOCALIZATION NOTE (update.manual.start) - text before link to http://www.firefox.com -->

Don't think the additional note is necessary here.

Comment 40

[Robert Strong (they/them - no direct email)]

Attachment: patch - for checkin

Comment 41

[Mike Beltzner [:beltzner, not reading bugmail]]

If we're going to do this, let's do this.

Comment 42

[Robert Strong (they/them - no direct email)]

Attachment: main patch rev1

I'm going to do another run through of testing the different scenarios.

Comment 43

[Robert Strong (they/them - no direct email)]

Attachment: add safe mode env var

Forgot to include this in the main patch

Comment 44

[Dave Townsend [:mossop]]

Comment on attachment 478088 [details] [diff] [review]
main patch rev1

>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js

>+// Alternative windowtype for an application update user interface window. When
>+// a window with this windowtype is open the application update service won't
>+// open the normal application update user interface windowt.

Damn those windowts!

>diff --git a/browser/base/content/aboutDialog.js b/browser/base/content/aboutDialog.js

>+function onUnload(aEvent) {
>+  if (gAppUpdater.isChecking)
>+    gAppUpdater.checker.stopChecking(Components.interfaces.nsIUpdateChecker.CURRENT_CHECK);
>+  // Safe to call even when there isn't a download in progress.
>+  gAppUpdater.removeDownloadListener();
>+  gAppUpdater = null;
>+}

I think it's worth declaring Cc and Ci at the top of the file. Make sure to do it outside the MOZ_UPDATER block though.

>+function appUpdater()
> {
>+  this.updateDeck = document.getElementById("updateDeck");

... SNIP ...

>+  this.bundle = Services.strings.
>+                createBundle("chrome://browser/locale/browser.properties");
>+
>+  this.updateBtn = document.getElementById("updateButton");
>+  this.updateDeck = document.getElementById("updateDeck");

I suspect the deck won't have changed since you last retrieved it.

>+  setupUpdateButton: function(aKeyPrefix) {
>+    this.updateBtn.label = this.bundle.GetStringFromName(aKeyPrefix + ".label");
>+    this.updateBtn.accessKey = this.bundle.GetStringFromName(aKeyPrefix + ".accesskey");
>+    if (!document.commandDispatcher.focusedElement ||
>+        document.commandDispatcher.focusedElement.nodeName == "button")
>+      this.updateBtn.focus();

I'd be worried about this stealing focus in the event that we ever add any other buttons to this UI. Check that focusedElement == this.updateBtn

>+  buttonOnCommand: function() {
>+    if (this.isPending) {
>+      // Notify all windows that an application quit has been requested.
>+      let cancelQuit = Components.classes["@mozilla.org/supports-PRBool;1"].
>+                       createInstance(Components.interfaces.nsISupportsPRBool);
>+      Services.obs.notifyObservers(cancelQuit, "quit-application-requested", "restart");
>+
>+      // Something aborted the quit process.
>+      if (cancelQuit.data)
>+        return;
>+
>+      // If already in safe mode restart in safe mode (bug 327119)
>+      if (Services.appinfo.inSafeMode) {
>+        let env = Components.classes["@mozilla.org/process/environment;1"].
>+                  getService(Components.interfaces.nsIEnvironment);
>+        env.set("MOZ_SAFE_MODE_RESTART", "1");
>+      }
>+
>+      Application.restart();

Application.restart also sends out quit-application-requested so I think this will cause multiple quit confirmation dialogs to show. Just call nsIAppStartup.quit manually I think.

>+  onUpdateFinished: function(aAddon) {
>+    ++this.addonsCheckedCount;
>+
>+    if (this.addonsCheckedCount < this.addonsTotalCount)
>+      return;
>+
>+    if (this.addons.length == 0) {
>+      // Compatibility updates or new version updates were founf for all add-ons

founf?

>diff --git a/browser/base/content/aboutDialog.xul b/browser/base/content/aboutDialog.xul

>   <vbox>
>     <hbox id="clientBox">
>       <vbox id="leftBox" flex="1"/>
>       <vbox id="rightBox" flex="1">
> #expand <label id="version" value="__MOZ_APP_VERSION__"/>
>         <label id="distribution" class="text-blurb"/>
>         <label id="distributionId" class="text-blurb"/>
>+        <vbox id="updateBox">

Any reason the vbox isn't in the MOZ_UPDATER section?

Comment 45

[Robert Strong (they/them - no direct email)]

(In reply to comment #44)
> Comment on attachment 478088 [details] [diff] [review]
> main patch rev1
> ...
> >diff --git a/browser/base/content/aboutDialog.js b/browser/base/content/aboutDialog.js
> 
> >+function onUnload(aEvent) {
> >+  if (gAppUpdater.isChecking)
> >+    gAppUpdater.checker.stopChecking(Components.interfaces.nsIUpdateChecker.CURRENT_CHECK);
> >+  // Safe to call even when there isn't a download in progress.
> >+  gAppUpdater.removeDownloadListener();
> >+  gAppUpdater = null;
> >+}
> 
> I think it's worth declaring Cc and Ci at the top of the file. Make sure to do
> it outside the MOZ_UPDATER block though.
Last time I tried this it broke because macBrowserOverlay.xul already had them defined as const

> >+function appUpdater()
> > {
> >+  this.updateDeck = document.getElementById("updateDeck");
> 
> ... SNIP ...
> 
> >+  this.bundle = Services.strings.
> >+                createBundle("chrome://browser/locale/browser.properties");
> >+
> >+  this.updateBtn = document.getElementById("updateButton");
> >+  this.updateDeck = document.getElementById("updateDeck");
> 
> I suspect the deck won't have changed since you last retrieved it.
doh!

> >+  setupUpdateButton: function(aKeyPrefix) {
> >+    this.updateBtn.label = this.bundle.GetStringFromName(aKeyPrefix + ".label");
> >+    this.updateBtn.accessKey = this.bundle.GetStringFromName(aKeyPrefix + ".accesskey");
> >+    if (!document.commandDispatcher.focusedElement ||
> >+        document.commandDispatcher.focusedElement.nodeName == "button")
> >+      this.updateBtn.focus();
> 
> I'd be worried about this stealing focus in the event that we ever add any
> other buttons to this UI. Check that focusedElement == this.updateBtn
will do

> >+  buttonOnCommand: function() {
> >+    if (this.isPending) {
> >+      // Notify all windows that an application quit has been requested.
> >+      let cancelQuit = Components.classes["@mozilla.org/supports-PRBool;1"].
> >+                       createInstance(Components.interfaces.nsISupportsPRBool);
> >+      Services.obs.notifyObservers(cancelQuit, "quit-application-requested", "restart");
> >+
> >+      // Something aborted the quit process.
> >+      if (cancelQuit.data)
> >+        return;
> >+
> >+      // If already in safe mode restart in safe mode (bug 327119)
> >+      if (Services.appinfo.inSafeMode) {
> >+        let env = Components.classes["@mozilla.org/process/environment;1"].
> >+                  getService(Components.interfaces.nsIEnvironment);
> >+        env.set("MOZ_SAFE_MODE_RESTART", "1");
> >+      }
> >+
> >+      Application.restart();
> 
> Application.restart also sends out quit-application-requested so I think this
> will cause multiple quit confirmation dialogs to show. Just call
> nsIAppStartup.quit manually I think.
will do

> >+  onUpdateFinished: function(aAddon) {
> >+    ++this.addonsCheckedCount;
> >+
> >+    if (this.addonsCheckedCount < this.addonsTotalCount)
> >+      return;
> >+
> >+    if (this.addons.length == 0) {
> >+      // Compatibility updates or new version updates were founf for all add-ons
> 
> founf?
Yes... founf!!!

will fix

> >diff --git a/browser/base/content/aboutDialog.xul b/browser/base/content/aboutDialog.xul
> 
> >   <vbox>
> >     <hbox id="clientBox">
> >       <vbox id="leftBox" flex="1"/>
> >       <vbox id="rightBox" flex="1">
> > #expand <label id="version" value="__MOZ_APP_VERSION__"/>
> >         <label id="distribution" class="text-blurb"/>
> >         <label id="distributionId" class="text-blurb"/>
> >+        <vbox id="updateBox">
> 
> Any reason the vbox isn't in the MOZ_UPDATER section?
So when the update window is already open and the deck isn't displayed or when building without MOZ_UPDATER it will have the correct margin... see beltner's comment #27

Comment 46

[Robert Strong (they/them - no direct email)]

Attachment: main patch rev2

Addresses current comments except for Cc and Ci. I'll take a look in a few to see if the macBrowserOverlay is still an issue. This also includes the changes to nsUpdateDriver.cpp

Comment 47

[Dave Townsend [:mossop]]

Comment on attachment 478153 [details] [diff] [review]
main patch rev2

Don't worry about the Cc stuff, macBrowserOverlay will indeed pooch you.

Comment 48

[Robert Strong (they/them - no direct email)]

Attachment: main patch for checkin

I renamed the pref from app.update.alt.windowtype to app.update.altwindowtype since I don't see a need for an alt branch and I added a comment to _getAltUpdateWindow. Carrying forward r+ for this simple change / comment addition

Comment 49

[Robert Strong (they/them - no direct email)]

Attachment: main patch as pushed.

Comment 50

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/78c90846f8c7
http://hg.mozilla.org/mozilla-central/rev/7cd8c313dfc5

Created bug 599233 for adding tests to the about window's update check so this bug can be resolved.

Comment 51

[Robert Strong (they/them - no direct email)]

btw: one thing not implemented is when there are compatible and enabled add-ons that are incompatible with the update's version opening the update ui to the incompatible add-ons page of the normal update ui. Instead, it opens to the updates found page and the incompatible add-ons page is the page after that. I ran this by beltzner and he agreed that we could live with that for now to get this in for beta 7.

Comment 52

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #1)
> (Not sure if we want to make "Check for Updates..." in the Application Menu on
> OS X just open the About Firefox window if we do this - let's leave that as a
> separate bug, but might be good to not have the duplicated functionality
> anymore!)

Anyone filed a bug for it? Would be good to know how we want to make progress on OS X.

Comment 53

[José Jeria]

(In reply to comment #52)
> Anyone filed a bug for it? 

bug 599945

Comment 54

[Robert Strong (they/them - no direct email)]

also see bug 599480 and bug 599573

Comment 55

[Thomas Stache]

The experience while a large update is being downloaded is not so great, as the About dialog (at least on Windows) is in the foreground. It's unclear whether it's safe to close the dialog to continue doing anything with Firefox until the download finishes. Is there a bug for that?

Comment 56

[Robert Strong (they/them - no direct email)]

There isn't... please file a new bug.

Comment 57

[Nicolas Mandil (:NicolasWeb)]

I'm not what is the final decision, so :

Once I've been in Russia. There you pay internet depending on the bandwith you use (an amount of data). It's not just like in US or in Europe where you pay for an amount of time.
Every one there disable images, ..., everything they can. Updating/Upgrading has a cost there. Checking for updates too (even if it is a smaller). So no-one has up-to-day software and no-one wished to update/upgrade.
So, not implementing the auto-download of updates/upgrades when avaible is important for firefox market penetration and development.
I know about Russia, but I think that some similar cases could happen in developing countries.

(In reply to comment #20)
> Well, I was figuring the important thing to signal was that the user didn't
> have the rights to update, but that an update is available. Didn't see what
> pointing them to the binary would do as they'd be unlikely to have access to
> make that work, too.

I remember that I've seen somewhere that installing Firefox in the user's folder was an sutied way of solving this issue of install folder (I can't found a bug about that with 'right install folder' : do one exist ?).
If this way is still wanted, we should inform the user that he/she will be able to install the last version in a place where he/she will have the rights. If we don't, I think that the flow isn't understandable for the user (aka Why downloading a new version if the auto-update of Firefox says that it can't be installed ?).

In the right case, I'm thinking about portable apps too. Could a link to this be a good solution ? ;-)

(In reply to comment #55)
> The experience while a large update is being downloaded is not so great, as the
> About dialog (at least on Windows) is in the foreground. It's unclear whether
> it's safe to close the dialog to continue doing anything with Firefox until the
> download finishes. Is there a bug for that?

What about using the same kind of button planed for the new Download Manager and for AMO ? (they become a progress bar like with pause and stop buttons). See Bug 600376

Comment 58

[Ilhan Yumer]

(In reply to comment #57)
> I'm not what is the final decision, so :
> 
> Once I've been in Russia. There you pay internet depending on the bandwith you
> use (an amount of data). It's not just like in US or in Europe where you pay
> for an amount of time.
> Every one there disable images, ..., everything they can. Updating/Upgrading
> has a cost there. Checking for updates too (even if it is a smaller). So no-one
> has up-to-day software and no-one wished to update/upgrade.

Valid for Turkey too.

Comment 59

[Nicolas Mandil (:NicolasWeb)]

Filled Bug 600383

Comment 60

[Robert Strong (they/them - no direct email)]

(In reply to comment #57)
> I'm not what is the final decision, so :
> 
> Once I've been in Russia. There you pay internet depending on the bandwith you
> use (an amount of data). It's not just like in US or in Europe where you pay
> for an amount of time.
> Every one there disable images, ..., everything they can. Updating/Upgrading
> has a cost there. Checking for updates too (even if it is a smaller). So no-one
> has up-to-day software and no-one wished to update/upgrade.
> So, not implementing the auto-download of updates/upgrades when avaible is
> important for firefox market penetration and development.
> I know about Russia, but I think that some similar cases could happen in
> developing countries.
The only difference here is that if you have auto update set in options / preferences then it checks and downloads when an update is available when opening the about window instead of just in the background as it did before. Disabling auto update will not check and download if an update is found when opening the about window or in the background where the background check download has always been the behavior when auto update is set.

The reason this is the default is so we can get security fixes out quickly.

> (In reply to comment #20)
> > Well, I was figuring the important thing to signal was that the user didn't
> > have the rights to update, but that an update is available. Didn't see what
> > pointing them to the binary would do as they'd be unlikely to have access to
> > make that work, too.
> 
> I remember that I've seen somewhere that installing Firefox in the user's
> folder was an sutied way of solving this issue of install folder (I can't found
> a bug about that with 'right install folder' : do one exist ?).
> If this way is still wanted, we should inform the user that he/she will be able
> to install the last version in a place where he/she will have the rights. If we
> don't, I think that the flow isn't understandable for the user (aka Why
> downloading a new version if the auto-update of Firefox says that it can't be
> installed ?).
> 
> In the right case, I'm thinking about portable apps too. Could a link to this
> be a good solution ? ;-)
It breaks other things for desktop builds such as multiple users using Firefox on the same system. I never thought there were a lot of users that did so but we have found with other bugs that affect multiple users on the same system that there are quite a few.

There is also the potential of the application files being compromised.

I am also working on a fix for this that doesn't require installing into a location that the user has write access to.

Comment 61

[Nicolas Mandil (:NicolasWeb)]

(In reply to comment #60)
> (In reply to comment #57)
> > I'm not what is the final decision, so :
> > 
> > Once I've been in Russia. There you pay internet depending on the bandwith you
> > use (an amount of data). It's not just like in US or in Europe where you pay
> > for an amount of time.
> > Every one there disable images, ..., everything they can. Updating/Upgrading
> > has a cost there. Checking for updates too (even if it is a smaller). So no-one
> > has up-to-day software and no-one wished to update/upgrade.
> > So, not implementing the auto-download of updates/upgrades when avaible is
> > important for firefox market penetration and development.
> > I know about Russia, but I think that some similar cases could happen in
> > developing countries.
> The only difference here is that if you have auto update set in options /
> preferences then it checks and downloads when an update is available when
> opening the about window instead of just in the background as it did before.
> Disabling auto update will not check and download if an update is found when
> opening the about window or in the background where the background check
> download has always been the behavior when auto update is set.
> 
> The reason this is the default is so we can get security fixes out quickly.

Everyone agree that we need to get security fixes out quickly. But right now as Cheking and downloading are not independant, what is happening in Russia is that people disable cheking and donwloading and stay like that for years... with Fx 1.0 ;-)

I'm happy with  the 'check and download' when I'm in US/Europe, but I feel like I want to keep check  but disable download when I'm in RU so I can decide what to do instead of just skipping all notifications about updates and upgrades.
I understand that this is inconfortable for maintaining Firefox secure, but I think it's a cost-less price in countries where you pay the internet acces by data amount.

Maybe just keep the actual behavior, but disociate the check and the download option in options/preferences dialog.

Let's speak about that in the related bug 600383, no ?

Comment 62

[Robert Strong (they/them - no direct email)]

(In reply to comment #61)
> (In reply to comment #60)
> > (In reply to comment #57)
> > > I'm not what is the final decision, so :
> > > 
> > > Once I've been in Russia. There you pay internet depending on the bandwith you
> > > use (an amount of data). It's not just like in US or in Europe where you pay
> > > for an amount of time.
> > > Every one there disable images, ..., everything they can. Updating/Upgrading
> > > has a cost there. Checking for updates too (even if it is a smaller). So no-one
> > > has up-to-day software and no-one wished to update/upgrade.
> > > So, not implementing the auto-download of updates/upgrades when avaible is
> > > important for firefox market penetration and development.
> > > I know about Russia, but I think that some similar cases could happen in
> > > developing countries.
> > The only difference here is that if you have auto update set in options /
> > preferences then it checks and downloads when an update is available when
> > opening the about window instead of just in the background as it did before.
> > Disabling auto update will not check and download if an update is found when
> > opening the about window or in the background where the background check
> > download has always been the behavior when auto update is set.
> > 
> > The reason this is the default is so we can get security fixes out quickly.
> 
> Everyone agree that we need to get security fixes out quickly. But right now as
> Cheking and downloading are not independant, what is happening in Russia is
> that people disable cheking and donwloading and stay like that for years...
> with Fx 1.0 ;-)
Are these people really opening the about window or manually checking for updates all that often? see below

> I'm happy with  the 'check and download' when I'm in US/Europe, but I feel like
> I want to keep check  but disable download when I'm in RU so I can decide what
> to do instead of just skipping all notifications about updates and upgrades.
> I understand that this is inconfortable for maintaining Firefox secure, but I
> think it's a cost-less price in countries where you pay the internet acces by
> data amount.
Or just set the preference to notify and let the background check notify you that there is an update available. My anecdotal experience of users show that the vast majority of people don't manually check and instead wait to be notified or have the update downloaded in the background (which has always been how it works) and I highly doubt that is any different in Russia though it is different with the subset of tech people that use nightly builds.

> Maybe just keep the actual behavior, but disociate the check and the download
> option in options/preferences dialog.
> 
> Let's speak about that in the related bug 600383, no ?
Sure but that is different than the points you made above.

Comment 63

[Nicolas Mandil (:NicolasWeb)]

(In reply to comment #62)
> > Everyone agree that we need to get security fixes out quickly. But right now as
> > Cheking and downloading are not independant, what is happening in Russia is
> > that people disable cheking and donwloading and stay like that for years...
> > with Fx 1.0 ;-)
Aka without beeing prompt about upgrades (because it's a all in one 'check and download' pref), that is the worse issue (even if being notified about updates is important too)
> Are these people really opening the about window or manually checking for
> updates all that often? see below

They won't, right. That's why I think that a more global thing should be done. Something that allow to 'Do not make download of updates/upgrades automatic when
avaible'. I think it could be 'Add an option with UI to make checking and donwloading of updates/upgrades independant' as this keeps the actual behaviour that is important for security fixies (then, the user can disable the '*download* in background' aka '*download* without notification' or '*download* as soon as an apdate/upgrade is avaible' option)

> > I'm happy with  the 'check and download' when I'm in US/Europe, but I feel like
> > I want to keep check  but disable download when I'm in RU so I can decide what
> > to do instead of just skipping all notifications about updates and upgrades.
> > I understand that this is inconfortable for maintaining Firefox secure, but I
> > think it's a cost-less price in countries where you pay the internet acces by
> > data amount.
> Or just set the preference to notify and let the background check notify you
> that there is an update available.
If that don't means that a download is made without an user action.
This is not for manual check only, but it's part of auto and manual check.
My specific experience with download in background of updates (security fixies) is that people that pay on data decide to disable the check for update only to prevent the donwload action.
I'ld like that for people that pay on time to keep the default (actual) behaviour, but allow thoses that pay on data to just be notified (so at least we get the chance to convince them to update ; Today, they just disable the all in one 'check and download avaible updates' and they don't get notifications for upgrades).
That's why I'm in favor to set a notify only pref (completed by a download only pref).

I hope to be clear because sometimes I've hard time to speak EN :-(
> My anecdotal experience of users show that
> the vast majority of people don't manually check and instead wait to be
> notified or have the update downloaded in the background (which has always been
> how it works) and I highly doubt that is any different in Russia though it is
> different with the subset of tech people that use nightly builds.

If I'm not too tired (that is not sure), what I describe is not for manual check only, but it's part of auto and manual check.

> > Let's speak about that in the related bug 600383, no ?
> Sure but that is different than the points you made above.
Be pleased to tweak the title to make it better if needed ;-)
I've copied this new comment there too ;-)
If you prefer to speak and fix it in that bug, I don't mind...

Comment 64

[Robert Strong (they/them - no direct email)]

(In reply to comment #63)
> If that don't means that a download is made without an user action.
> This is not for manual check only, but it's part of auto and manual check.
> My specific experience with download in background of updates (security fixies)
> is that people that pay on data decide to disable the check for update only to
> prevent the donwload action.
> I'ld like that for people that pay on time to keep the default (actual)
> behaviour, but allow thoses that pay on data to just be notified (so at least
> we get the chance to convince them to update ; Today, they just disable the all
> in one 'check and download avaible updates' and they don't get notifications
> for upgrades).
> That's why I'm in favor to set a notify only pref (completed by a download only
> pref).
The notify pref is already there. It is labeled "Ask me what I want to do" in English. Nothing besides the check is performed... nothing is downloaded. This is the same behavior as prior to this ui being implemented.

Comment 65

[Nicolas Mandil (:NicolasWeb)]

I tested it on Ubuntu distribution nightly and not a Mozilla nightly : in that
case you don't have all the "When updates to _app-name_ are found :" ;-)

Comment 66

[Robert Strong (they/them - no direct email)]

(In reply to comment #65)
> I tested it on Ubuntu distribution nightly and not a Mozilla nightly : in that
> case you don't have all the "When updates to _app-name_ are found :" ;-)
That is intentional and correct. Linux distros do not use our app update system.

Comment 67

[Dave Garrett]

To chime in on the bandwidth debate which probably needs a new bug:
For +1 dot updates, it does do a smaller patch download instead of a full download, right? I think for people with per-bit charging ISPs we should make a point of saying that upfront. In other words, tell them that if they do all updates they'll actually save bandwidth in comparison to waiting, overall.

Comment 68

[Scott Field]

Not sure if this is a bug or intended behavior, but checking for an update in the "About Minefield" window ignores user preferences. (At least on Mac OS X.) My preferences are set to 1)Automatically check for updates 2)When updates are
found, ask me what I want to do

If I go to Minefield/About Minefield and click on "Check for updates", if an
update if found, it automatically downloads the update.  If I go to
Minefield/Check For Updates, and an update is found, it still obeys the preference and gives me the option to install now or be reminded later.

If checking for updates is going to ignore user preferences, the useless preferences should be removed or there should be a warning that "check for updates" will override the user set preferences.

As "check for updates" implies an informational action - i.e. Firefox will check for updates and let you know if there is one available, the wording should be changed to something that more accurately describes what is going to happen - e.g "Download update if available" 

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20101004 Firefox/4.0b7pre ID:20101004030754

Comment 69

[Jesse Ruderman]

Scott, see bug 600500.

Comment 70

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with builds on all platforms like Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101106 Firefox/4.0b8pre. Now we fully support this path for each platform.

Updated the Litmus test:
https://litmus.mozilla.org/show_test.cgi?id=9940

I think that there are more tests to update for major updates. Assigning this part to Al.