Add UMA to measure potential impact of Fast Tab Close.

Add UMA metrics to measure potential impact of Fast Tab Close.

TEST=Verified Close.Tab.UnloadTime is correct in about:histograms.
BUG=142458
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158627

[slamm, 2012-09-20 00:13:01]

Hi Sreeram,

I would like to add UMAs for Fast Tab Close.

Here are the ones in the CL:

Tab.Close.MinusDialog - Remove time to show/close beforeunload dialogs.
Tab.Close.UpToDialog - Best time we could get with backgrounding tab after
dialogs.
Tab.Close.PostDialog - The time that could potentially be moved to the
background.

We could get by with just the last one. Do you have a suggestion for what
metrics we should track?

Eventually, we would test would something like

Tab.Close.HideUnload

-slamm

[sreeram, 2012-09-20 17:34:05]

Is this meant to measure only the time spent waiting for the dialog to be
dismissed, or to also measure the time to run the beforeunload handlers? For
example, the page may run slow handlers, but ultimately not popup a dialog.

Your code needs to handle the cases where the beforeunload dialog doesn't show
during the close. Currently, you're always recording these times; that's wrong:
1. dialog_start_time_ may be zero, indicating that we never showed a
beforeunload dialog.
2. dialog_start_time_ may be non-zero, but still earlier than close_start_time_,
indicating that we showed the dialog once before, and the user chose to stay on
the page. Now, during the actual close, there was no dialog shown.

[slamm_google, 2012-09-20 18:05:25]

On Thu, Sep 20, 2012 at 10:34 AM, <sreeram@chromium.org> wrote:

> Is this meant to measure only the time spent waiting for the dialog to be
> dismissed, or to also measure the time to run the beforeunload handlers?
> For
> example, the page may run slow handlers, but ultimately not popup a dialog.
>

The latter. I want to know about long beforeunload. The time to show and
respond to the dialog is exactly what I want to take out of consideration.

If a tab-close took 10 seconds but 8 seconds of that had the dialog up, I
am only interested in the 2 seconds without the dialog. Further, I want to
know how those 2 seconds split between before the dialog (onbeforeunload)
and after the dialog (onunload).

Your code needs to handle the cases where the beforeunload dialog doesn't
> show
> during the close. Currently, you're always recording these times; that's
> wrong:
> 1. dialog_start_time_ may be zero, indicating that we never showed a
> beforeunload dialog.
>

I thought the timer was set regardless of whether the dialog is shown (I
still need to test all this).
However, I updated the CL to clear the timers with no dialog, and check if
they are set before using them.

2. dialog_start_time_ may be non-zero, but still earlier than
> close_start_time_,
> indicating that we showed the dialog once before, and the user chose to
> stay on
> the page. Now, during the actual close, there was no dialog shown.
>

Good point. CL updated.


>
>
http://codereview.chromium.**org/10945042/<http://codereview.chromium.org/109...
>

[slamm, 2012-09-21 16:41:07]

I would like to tweak the proposed UMAs to make the more straight-forward.
I had proposed the following:

    Tab.Close.MinusDialog
    Tab.Close.UpToDialog
    Tab.Close.PostDialog

How about these instead:

    Tab.Close.NoDialog (does not record closes that have a dialog)
    Tab.Close.AfterOnbeforeunload (the part we hope to speed up)

Both of these will show the current state, as well as the impact of
backgrounding part of the close.

[sreeram, 2012-09-21 17:55:46]

On Fri, Sep 21, 2012 at 9:41 AM,  <slamm@chromium.org> wrote:
> I would like to tweak the proposed UMAs to make the more straight-forward.
> I had proposed the following:
>
>     Tab.Close.MinusDialog
>     Tab.Close.UpToDialog
>     Tab.Close.PostDialog
>
> How about these instead:
>
>     Tab.Close.NoDialog (does not record closes that have a dialog)
>     Tab.Close.AfterOnbeforeunload (the part we hope to speed up)
>
> Both of these will show the current state, as well as the impact of
> backgrounding part of the close.

Tab.Close.NoDialog won't be useful. It doesn't tell you which part of
the time can be saved (since you still need to run the beforeunload
handler). So, I'd recommend just storing a single histogram:
Tab.Close.AfterBeforeUnload, which records (regardless of whether the
beforeunload caused a dialog or not), after beforeunload has run and
dialogs have been dismissed, the time till the tab is removed from the
tabstrip.

[slamm, 2012-09-21 23:10:32]

I added some printfs (temporary) and a handler for ViewHostMsg_ShouldClose_ACK.

Unfortunately, my patch breaks closing tabs that have an unload handler. I will
have investigate further.

[sreeram, 2012-09-24 18:35:55]

On 2012/09/21 23:10:32, slamm1 wrote:
> I added some printfs (temporary) and a handler for
ViewHostMsg_ShouldClose_ACK.
> 
> Unfortunately, my patch breaks closing tabs that have an unload handler. I
will
> have investigate further.

Naturally :) The RVH now doesn't get the message, since you've already handled
it. You should instead continue to let the RVH handle the message, and call
delegate_->Foo() from within the handler
(RenderViewHostImpl::OnMsgShouldCloseACK). (The delegate is the
WebContentsImpl.)

[slamm, 2012-09-24 22:14:51]

PTAL

[sreeram, 2012-09-24 22:45:29]

LGTM, but the real lgtm has to come from content/OWNERS (say, @jam).

[slamm, 2012-09-24 23:41:06]

Hi John,

I would like to add a UMA, Tab.Close.UnloadTime, to measure what could be saved
if onunload ran in the background.

This is for the issue on Fast Tab Closure (http://crbug.com/142458).

-slamm

[jam, 2012-09-25 16:19:24]

lgtm

[commit-bot: I haz the power, 2012-09-25 17:04:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/slamm@chromium.org/10945042/11001

[commit-bot: I haz the power, 2012-09-25 19:12:56]

Change committed as 158627