1376597 - Delay exiting stub installer until after browser has started

Status: VERIFIED FIXED

(Firefox :: Installer, enhancement, P2)

Description

[Molly Howell (she/her) (mostly inactive)]

Currently, the installers run the newly installed browser when installation is complete, and then immediately exit. This means there is some delay between the installer window going away and a browser window appearing, so the user has to wait with no indication that anything is happening. We could make browser startup feel faster if we delay exiting the installer until browser startup is substantially complete (which we could most simply define as being when a window has appeared).

We should only do this in the stub installer, because it isn't really conceptually appropriate for the full installer, and because the stub is still running a progress bar when it exits, but the full installer has a finish page.

Comment 1

[Romain Testard [:RT]]

Matt, can you please confirm the proposed user experience here:
1 Will the progress bar remains at 100% for a little longer until the Firefox window opens?
2 Will the progress bar get to 100% only when the Firefox Window opens (would require progress bar adjustment tricks)

Michael would rather we do (2) but I'm unsure what's feasible.

Comment 2

[Molly Howell (she/her) (mostly inactive)]

We could do option 2, but keep in mind that the installer can't really track the progress of browser startup, so the the bar would have to either a) sit at not-quite-the-end until the browser window is up and then jump to 100% right before the installer window closes, or b) fill up the rest of the way on a timer (e.g. 1% every second or so), meaning we'll be guessing at how long the browser takes to start, and we would almost always get it wrong and end up jumping forward a bit anyway, or filling up the progress bar before the browser is open (as in option 1).

Comment 3

[Romain Testard [:RT]]

It sounds like a UX consideration, Michael do you have a preference between a) or b)?

Comment 4

[Verdi [:verdi] Best to slack me]

(In reply to Romain Testard [:RT] from comment #3)
> It sounds like a UX consideration, Michael do you have a preference between
> a) or b)?

I mocked this up and ran it by Bryant Mao and Cindy Hsiang and we all agreed that option 2b is considerably better than the other options. 

Do we have any data to help us here? Do we know what the median time between the installer finishing and the Firefox window opening? 

My off the cuff guess is something like 8 seconds. So if we use that as an example, the progress bar could hit 92% width when the installation is done and then progress 1% of the width each of the next 8 seconds. Then if the gap on my computer falls at 4 seconds, then the progress bar jumps 4% at the end. Or if the gap on your computer is 12 seconds, then the progress bar sits at 100% for 4 seconds.

Comment 5

[Molly Howell (she/her) (mostly inactive)]

I *think* the histogram SIMPLE_MEASURES_CREATETOPLEVELWINDOW measures the time we're talking about; the graph at [0] shows its distribution as of release 54. I'm not entirely sure because I'm having a hard time finding where that gets set, but it sounds right.

The 75th percentile by that distribution is at ~ 5 seconds, so I think that's where we should set our guess. As in, do as Verdi described, but with 5 instead of 8. Does that look right to you guys?

We might have to reduce the value a bit once we've seen how 57 performs; the median seems to already be trending down since the middle of the nightly 55 cycle [1].

[0] https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-06-27&keys=__none__!__none__!__none__&max_channel_version=release%252F54&measure=SIMPLE_MEASURES_CREATETOPLEVELWINDOW&min_channel_version=null&os=Windows_NT&processType=parent&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-06-07&table=0&trim=1&use_submission_date=0
[1] https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=median&cumulative=0&end_date=2017-08-01&keys=!__none__!__none__&max_channel_version=nightly%252F56&measure=SIMPLE_MEASURES_CREATETOPLEVELWINDOW&min_channel_version=nightly%252F53&os=Windows_NT&processType=parent&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-06-12&trim=1&use_submission_date=0

Comment 6

[Romain Testard [:RT]]

(In reply to Matt Howell [:mhowell] from comment #5)
> I *think* the histogram SIMPLE_MEASURES_CREATETOPLEVELWINDOW measures the
> time we're talking about; the graph at [0] shows its distribution as of
> release 54. I'm not entirely sure because I'm having a hard time finding
> where that gets set, but it sounds right.
> 
> The 75th percentile by that distribution is at ~ 5 seconds, so I think
> that's where we should set our guess. As in, do as Verdi described, but with
> 5 instead of 8. Does that look right to you guys?
This sounds good to me and yes we can probably adjust the value if we see big changes with FF57

Comment 7

[Molly Howell (she/her) (mostly inactive)]

I am currently working on this.

Comment 8

[Molly Howell (she/her) (mostly inactive)]

Attachment: Bug 1376597 Part 1 - Wait for the browser to start before exiting the stub installer.

Review commit: https://reviewboard.mozilla.org/r/169738/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169738/

Comment 9

[Molly Howell (she/her) (mostly inactive)]

Attachment: Bug 1376597 Part 2 - Simplify progress bar handling.

Working on the main patch for this bug (part 1), it took me longer than seemed
reasonable to understand how the stub installer progress bar worked and to fit
the new stage into it. So I thought I would take the opportunity to attempt a
refactor and simplify the whole thing.

Review commit: https://reviewboard.mozilla.org/r/169740/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169740/

Comment 10

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 8898371 [details]
Bug 1376597 Part 1 - Wait for the browser to start before exiting the stub installer.

https://reviewboard.mozilla.org/r/169738/#review175110

::: browser/installer/windows/nsis/stub.nsi:1699
(Diff revision 1)
> +  FindWindow $0 "${MainWindowClass}"
> +  ${If} $0 <> 0
> +    ${NSD_KillTimer} WaitForAppLaunch
> +    StrCpy $ProgressCompleted "$ProgressTotal"
> +    Call SetProgressBars
> +    Call SendPing

Return after the Call SendPing here and below?

Comment 11

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 8898371 [details]
Bug 1376597 Part 1 - Wait for the browser to start before exiting the stub installer.

https://reviewboard.mozilla.org/r/169738/#review175592

::: browser/installer/windows/nsis/stub.nsi:1694
(Diff revision 1)
>      Exec "$\"$INSTDIR\${FileMainEXE}$\""
>    ${EndIf}
>  FunctionEnd
>  
> +Function WaitForAppLaunch
> +  FindWindow $0 "${MainWindowClass}"

How will this interact with the Import Wizard?

Comment 12

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 8898371 [details]
Bug 1376597 Part 1 - Wait for the browser to start before exiting the stub installer.

https://reviewboard.mozilla.org/r/169738/#review175624

This mostly looks good, just the two issues I mentioned.

Comment 13

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 8898372 [details]
Bug 1376597 Part 2 - Simplify progress bar handling.

https://reviewboard.mozilla.org/r/169740/#review175582

::: browser/installer/windows/nsis/stub.nsi:50
(Diff revision 1)
>  
>  Var CanWriteToInstallDir
>  Var HasRequiredSpaceAvailable
>  Var IsDownloadFinished
>  Var DownloadSizeBytes
>  Var HalfOfDownload

You could get rid of HalfOfDownload now

Comment 14

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8898371 [details]
Bug 1376597 Part 1 - Wait for the browser to start before exiting the stub installer.

https://reviewboard.mozilla.org/r/169738/#review175994

::: browser/installer/windows/nsis/stub.nsi:1694
(Diff revision 1)
>      Exec "$\"$INSTDIR\${FileMainEXE}$\""
>    ${EndIf}
>  FunctionEnd
>  
> +Function WaitForAppLaunch
> +  FindWindow $0 "${MainWindowClass}"

That is an excellent question. I will look into that.

::: browser/installer/windows/nsis/stub.nsi:1699
(Diff revision 1)
> +  FindWindow $0 "${MainWindowClass}"
> +  ${If} $0 <> 0
> +    ${NSD_KillTimer} WaitForAppLaunch
> +    StrCpy $ProgressCompleted "$ProgressTotal"
> +    Call SetProgressBars
> +    Call SendPing

I didn't do that because SendPing doesn't return, it always exits the installer, so those would be redundant. But that also means they don't hurt anything, so I don't mind adding them in for readability/consistency.

Comment 15

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8898371 [details]
Bug 1376597 Part 1 - Wait for the browser to start before exiting the stub installer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169738/diff/1-2/

Comment 16

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8898372 [details]
Bug 1376597 Part 2 - Simplify progress bar handling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169740/diff/1-2/

Comment 17

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

(In reply to Matt Howell [:mhowell] from comment #14)
> I didn't do that because SendPing doesn't return, it always exits the
> installer, so those would be redundant. But that also means they don't hurt
> anything, so I don't mind adding them in for readability/consistency.

Initially I thought that as well, but when actually sending the ping SendPing does return, the exit is done by OnPing after at least DownloadIntervalMS

Comment 18

[Molly Howell (she/her) (mostly inactive)]

(In reply to Adam Gashlin [:agashlin] from comment #17)
> (In reply to Matt Howell [:mhowell] from comment #14)
> > I didn't do that because SendPing doesn't return, it always exits the
> > installer, so those would be redundant. But that also means they don't hurt
> > anything, so I don't mind adding them in for readability/consistency.
> 
> Initially I thought that as well, but when actually sending the ping
> SendPing does return, the exit is done by OnPing after at least
> DownloadIntervalMS

Ah, good point, I'll fix it.

Comment 19

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8898371 [details]
Bug 1376597 Part 1 - Wait for the browser to start before exiting the stub installer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169738/diff/2-3/

Comment 20

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8898372 [details]
Bug 1376597 Part 2 - Simplify progress bar handling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169740/diff/2-3/

Comment 21

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 8898371 [details]
Bug 1376597 Part 1 - Wait for the browser to start before exiting the stub installer.

https://reviewboard.mozilla.org/r/169738/#review176316

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c170eca4275
Part 1 - Wait for the browser to start before exiting the stub installer. r=agashlin
https://hg.mozilla.org/integration/autoland/rev/c96e332ad836
Part 2 - Simplify progress bar handling. r=agashlin

Comment 23

[Verdi [:verdi] Best to slack me]

(In reply to Adam Gashlin [:agashlin] from comment #11)
> How will this interact with the Import Wizard?

FWIW, beginning with 57 we won't show the import wizard on firstrun and instead will have a message about it on newtab.

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7c170eca4275
https://hg.mozilla.org/mozilla-central/rev/c96e332ad836

Comment 25

[Abe - QA (:Abe_LV)]

I tested this on Windows OSes and it is verified as fixed.
Test runs are here: https://public.etherpad-mozilla.org/p/1376597