Closed Bug 719947 Opened 9 years ago Closed 9 years ago

Add an indeterminate progress bar for updates applied from service

Categories

(Toolkit :: Application Update, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12
Tracking Status
firefox11 --- unaffected
firefox12 --- verified
firefox13 --- verified

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

(Whiteboard: [qa+])

Attachments

(2 files, 3 obsolete files)

When the service applies updates, it currently does not show a progress bar since the updater process runs as the system account in session 0.  Because of session 0 isolation in Vista and above, you cannot display a UI anymore in those cases from session 0, and to keep Windows XP consistent we did the same there.

This task is to show an indeterminate progress bar from the unelevated updater.exe in cases that the service applies the update so that the user is not left in the dark about the slow startup.

There are cases even when background updates land that we may apply updates on startup, so it is good to have this UI anyway.
Thanks for filing this bug.  

The recommendation from Product/UX is that we display this progress bar if Fx doesn't start up within 5 seconds.  Ideally, we won't need to show this progress bar very often (even if the startup is slightly longer than normal) after an update has been applied.
So if an update takes 6 seconds total we should display this dialog for 1 second only right?
I know based on Comment 1 that should hold true :) I just want to make sure that the flashing dialog in/out is ok.
Correct.  The only downside to this is the fact that a user may not know what was in the dialog.

Is the plan to re-use the existing dialog we had before?

Limi, any concern with the flashing in and out if the dialog is only up for ~1 sec?
Yes we'll use exactly the same UI but the progress bar will be a marquee (going back and forth)
If the progress is indeterminate, and it shows for just a second after 5 seconds, that's ok, yes.
Attached image New progress bar UI.
Here is a screenshot of the new UI.  It's the same as the old except for the progress bar.
Attachment #590569 - Flags: feedback?(robert.bugzilla)
Attached patch Patch v1. (obsolete) — Splinter Review
- UI is only shown after 5 seconds of waiting for the service operation to complete.
- Progress bar is an indeterminate progress bar now (aka marquee progress bar) as per screenshot.
Attachment #590570 - Flags: review?(robert.bugzilla)
Will this be fixed on time for Firefox 12?
Yes
Attachment #590569 - Flags: feedback?(robert.bugzilla) → feedback+
Comment on attachment 590570 [details] [diff] [review]
Patch v1.

Looks good overall but the service path displays the ui when the updater.ini doesn't exist or the strings aren't present in the updater.ini unlike the non-service path.
Attachment #590570 - Flags: review?(robert.bugzilla) → review-
Attached patch Patch v2. (obsolete) — Splinter Review
Only show the progress UI if strings can be loaded.
Attachment #590570 - Attachment is obsolete: true
Attachment #591806 - Flags: review?(robert.bugzilla)
Comment on attachment 591806 [details] [diff] [review]
Patch v2.

>diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
>--- a/toolkit/mozapps/update/updater/updater.cpp
>+++ b/toolkit/mozapps/update/updater/updater.cpp
>@@ -1795,17 +1805,35 @@ int NS_main(int argc, NS_tchar **argv)
>       // comamnd for the update.
>       if (useService) {
>         // If the update couldn't be started, then set useService to false so
>         // we do the update the old way.
>         DWORD ret = LaunchServiceSoftwareUpdateCommand(argc, (LPCWSTR *)argv);
>         useService = (ret == ERROR_SUCCESS);
>         // If the command was launched then wait for the service to be done.
>         if (useService) {
>-          DWORD lastState = WaitForServiceStop(SVC_NAME, 600);
>+          // We need to call this separately instead of allowing ShowProgressUI
>+          // to initialize the strings because the service will move the
>+          // ini file out of the way when running updater.
>+          bool showProgressUI = !InitProgressUIStrings();
>+
>+          // Wait for the service to stop for 5 seconds.  If the service
>+          // has still not stopped then show an indeterminate progress bar.
>+          DWORD lastState = WaitForServiceStop(SVC_NAME, 5);
>+          if (lastState != SERVICE_STOPPED) {
>+            Thread t1;
>+            if (t1.Run(WaitForServiceFinishThread, NULL) == 0) {
>+              if (showProgressUI) {
nit: any reason not to just check showProgressUI in the previous if?
Attachment #591806 - Flags: review?(robert.bugzilla) → review+
Attached patch Patch v3. (obsolete) — Splinter Review
Implemented review nit.  Carrying forward r+.
Attachment #591806 - Attachment is obsolete: true
Attachment #593090 - Flags: review+
So, will this land in Aurora now?
> So, will this land in Aurora now?

That is correct, this will be requested to move onto Aurora soon.
Attached patch Patch v4.Splinter Review
Same as last patch but needed #ifdef XP_WIN around the thread function. (Caused a problem on the try run).  Carrying forward r+.
Attachment #593090 - Attachment is obsolete: true
Attachment #593893 - Flags: review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/d85d87f791b5

I'll be requesting Aurora once this lands on m-c + 1 day to test it on m-c.
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/d85d87f791b5
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment on attachment 593893 [details] [diff] [review]
Patch v4.

[Approval Request Comment]
Regression caused by (bug #): bug 481815, not a regression but should land with service work.
User impact if declined: Users may perceive that updates are slower because there is no UI without this patch.
Testing completed (on m-c, etc.): I verified on m-c.
Risk to taking this patch (and alternatives if risky): Lowish
String changes made by this patch: None
Attachment #593893 - Flags: approval-mozilla-aurora?
(In reply to Brian R. Bondy [:bbondy] from comment #20)
> Risk to taking this patch (and alternatives if risky): Lowish

Chris - would you mind weighing in on the product criticality of landing this updater change given the lowish risk evaluation?
This would be valuable to land with the UAC work as Brian mentions above -- users may perceive updates are slower or not sure what's happening when an update takes that is >5 seconds.
Also this task was mainly done for the sole purpose of going to Aurora since we will have background updates after it making startup time always < 5 seconds and hence no UI.
Comment on attachment 593893 [details] [diff] [review]
Patch v4.

[Triage Comment]
Thanks for the clarification guys - just wanted to make sure this was needed since we don't typically take updater changes on Aurora. Approved.
Attachment #593893 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Target Milestone: mozilla13 → mozilla12
Whiteboard: [qa+]
Verified on Win XP, Win Vista and Win 7 that when updating from Firefox 12 beta 2 to Firefox 12 beta 3 and the startup time is >5 seconds - the new progress bar is displayed.

Mozilla/5.0 (Windows NT 5.1; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (Windows NT 6.0; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (Windows NT 6.1; rv:12.0) Gecko/20100101 Firefox/12.0
Verified on Win XP, Win Vista and Win 7 that when updating from Firefox 13 beta 1 to Firefox 13 beta 2 and the startup time is >5 seconds - the new progress bar is displayed.

Mozilla/5.0 (Windows NT 5.1; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Windows NT 6.1; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Windows NT 6.0; rv:13.0) Gecko/20100101 Firefox/13.0
You need to log in before you can comment on or make changes to this bug.