Add "SessionRestore.ForegroundTabFirstPaint2" histogram.

Add "SessionRestore.ForegroundTabFirstPaint2" histogram.

This commit keeps the old histogram for history and adds a new one that is resized with newer dimmensions so that the overflow bucket isn't so large.

BUG=467238
Committed: https://crrev.com/403f352e301204dd68986ad1f0af315573d89988
Cr-Commit-Position: refs/heads/master@{#320839}

[danduong, 2015-03-14 00:23:32]

danduong@chromium.org changed reviewers:
+ asvitkine@chromium.org

[danduong, 2015-03-14 00:24:32]

danduong@chromium.org changed reviewers:
+ sky@chromium.org

[danduong, 2015-03-14 00:40:42]

Alexei and Sky PTAL. This is something that I may want to merge into M42 to
support the Code Purple with better data.

[Alexei Svitkine (slow), 2015-03-14 13:23:43]

https://codereview.chromium.org/1004033006/diff/1/chrome/browser/sessions/ses...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/1004033006/diff/1/chrome/browser/sessions/ses...
chrome/browser/sessions/session_restore.cc:419: time_to_paint,
Is there a reason you're still keeping the old one?

https://codereview.chromium.org/1004033006/diff/1/chrome/browser/sessions/ses...
chrome/browser/sessions/session_restore.cc:424: time_to_paint,
Nit: Align params.

https://codereview.chromium.org/1004033006/diff/1/chrome/browser/sessions/ses...
chrome/browser/sessions/session_restore.cc:426:
base::TimeDelta::FromSeconds(1000),
Nit: Use FromMinutes to make the range more intuitive to a reader.

https://codereview.chromium.org/1004033006/diff/1/chrome/browser/sessions/ses...
chrome/browser/sessions/session_restore.cc:427: 100);
I wonder if it's actually useful to have 100 buckets here?

Looking at the data, it seems its pretty smooth transition between buckets, so
my intuition is the extra granularity doesn't give us much.

(50 is a more typical bucket count for histograms).

https://codereview.chromium.org/1004033006/diff/1/chrome/browser/sessions/ses...
chrome/browser/sessions/session_restore.cc:431:
"SessionRestore.ForegroundTabFirstPaint_%d", tab_count_);
Should we also be changing these ones?

[danduong, 2015-03-16 19:22:01]

https://codereview.chromium.org/1004033006/diff/1/chrome/browser/sessions/ses...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/1004033006/diff/1/chrome/browser/sessions/ses...
chrome/browser/sessions/session_restore.cc:419: time_to_paint,
On 2015/03/14 13:23:42, Alexei Svitkine (slow) wrote:
> Is there a reason you're still keeping the old one?

This was a suggestion from pawliger relayed from jar so that we can temporarily
continue to track existing data, but move off to the newer histogram eventually.

https://codereview.chromium.org/1004033006/diff/1/chrome/browser/sessions/ses...
chrome/browser/sessions/session_restore.cc:424: time_to_paint,
On 2015/03/14 13:23:42, Alexei Svitkine (slow) wrote:
> Nit: Align params.

Done.

https://codereview.chromium.org/1004033006/diff/1/chrome/browser/sessions/ses...
chrome/browser/sessions/session_restore.cc:426:
base::TimeDelta::FromSeconds(1000),
On 2015/03/14 13:23:42, Alexei Svitkine (slow) wrote:
> Nit: Use FromMinutes to make the range more intuitive to a reader.

Done.

https://codereview.chromium.org/1004033006/diff/1/chrome/browser/sessions/ses...
chrome/browser/sessions/session_restore.cc:427: 100);
On 2015/03/14 13:23:42, Alexei Svitkine (slow) wrote:
> I wonder if it's actually useful to have 100 buckets here?
> 
> Looking at the data, it seems its pretty smooth transition between buckets, so
> my intuition is the extra granularity doesn't give us much.
> 
> (50 is a more typical bucket count for histograms).

Done.

https://codereview.chromium.org/1004033006/diff/1/chrome/browser/sessions/ses...
chrome/browser/sessions/session_restore.cc:431:
"SessionRestore.ForegroundTabFirstPaint_%d", tab_count_);
On 2015/03/14 13:23:42, Alexei Svitkine (slow) wrote:
> Should we also be changing these ones?

Good catch. Yes.

[Alexei Svitkine (slow), 2015-03-16 19:32:11]

Thanks, a few more comments.

https://codereview.chromium.org/1004033006/diff/40001/chrome/browser/sessions...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/1004033006/diff/40001/chrome/browser/sessions...
chrome/browser/sessions/session_restore.cc:418:
UMA_HISTOGRAM_CUSTOM_TIMES("SessionRestore.ForegroundTabFirstPaint",
Please add a TODO(dangduong): to remove this with an associated (new) bug, to
make sure we don't forget to clean this up.

https://codereview.chromium.org/1004033006/diff/40001/chrome/browser/sessions...
chrome/browser/sessions/session_restore.cc:443:
"SessionRestore.ForegroundTabFirstPaint2_%d", tab_count_);
Please update histogram_suffixes element SessionRestoreTabCounts to include the
new histogram name.

https://codereview.chromium.org/1004033006/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1004033006/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:32812: +    Deprecated 2015-03-13 in
favor of SessionRestore.ForegroundTabFirstPaint2
Nit: mention why and terminate with a .

[danduong, 2015-03-16 20:42:40]

https://codereview.chromium.org/1004033006/diff/40001/chrome/browser/sessions...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/1004033006/diff/40001/chrome/browser/sessions...
chrome/browser/sessions/session_restore.cc:418:
UMA_HISTOGRAM_CUSTOM_TIMES("SessionRestore.ForegroundTabFirstPaint",
On 2015/03/16 19:32:11, Alexei Svitkine (slow) wrote:
> Please add a TODO(dangduong): to remove this with an associated (new) bug, to
> make sure we don't forget to clean this up.

Done.

https://codereview.chromium.org/1004033006/diff/40001/chrome/browser/sessions...
chrome/browser/sessions/session_restore.cc:443:
"SessionRestore.ForegroundTabFirstPaint2_%d", tab_count_);
On 2015/03/16 19:32:11, Alexei Svitkine (slow) wrote:
> Please update histogram_suffixes element SessionRestoreTabCounts to include
the
> new histogram name.

Done.

https://codereview.chromium.org/1004033006/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1004033006/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:32812: +    Deprecated 2015-03-13 in
favor of SessionRestore.ForegroundTabFirstPaint2
On 2015/03/16 19:32:11, Alexei Svitkine (slow) wrote:
> Nit: mention why and terminate with a .

Done.

[Alexei Svitkine (slow), 2015-03-16 21:09:51]

lgtm

[danduong, 2015-03-16 21:11:35]

sky, PTAL.

[sky, 2015-03-16 21:33:30]

LGTM with the following change

https://codereview.chromium.org/1004033006/diff/80001/chrome/browser/sessions...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/1004033006/diff/80001/chrome/browser/sessions...
chrome/browser/sessions/session_restore.cc:444: time_for_count =
base::StringPrintf(
Reusing a variable is often error prone, please create a new one.

[danduong, 2015-03-16 22:55:23]

The CQ bit was checked by danduong@chromium.org

[danduong, 2015-03-16 22:55:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1004033006/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-03-16 22:56:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1004033006/100001

[commit-bot: I haz the power, 2015-03-17 01:05:26]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-17 01:07:19]

Patchset 6 (id:??) landed as
https://crrev.com/403f352e301204dd68986ad1f0af315573d89988
Cr-Commit-Position: refs/heads/master@{#320839}