Add histograms for session startup enum and number of URLs to open.

Add histograms for session startup enum and number of URLs to open.

This CL adds two histograms to UMA:
- Settings.SessionRestoreOnStartup to enumerate page load settings
on session startup.
- Settings.SessionRestoreOnStartupURLs to record the number of URLs
if page load setting is set to load URLs on session startup.

BUG=262977
R=battre@chromium.org, bbudge@chromium.org, mpearson@chromium.org, rockot@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214426

[meacer, 2013-07-30 00:42:05]

Sorry for multiple CLs for the same bug. This is the "separate histograms"
version of https://codereview.chromium.org/20582004/

Please let me know if you prefer one over the other.

[Ken Rockot(use gerrit already), 2013-07-30 01:14:17]

On 2013/07/30 00:42:05, Mustafa Emre Acer wrote:
> Sorry for multiple CLs for the same bug. This is the "separate histograms"
> version of https://codereview.chromium.org/20582004/
> 
> Please let me know if you prefer one over the other.

I prefer this one; slightly more flexible data and simpler code. Winning all
around!

LGTM

[bbudge, 2013-07-30 16:55:28]

I also prefer this one. LGTM

[meacer, 2013-07-30 17:12:59]

Let's go with this one then.

Adding OWNERS reviewers:

battre: pref_metrics_service.cc
mpearson: histograms.xml

Thanks.

[Mark P, 2013-07-30 17:19:58]

SessionRestoreOnStartup is a misleading histogram name.  Can we do
something like StartupSettings or StartupPrefs?

You add two histograms in the .cc change.  Why do you only add one to
histograms.xml?

--mark


On Tue, Jul 30, 2013 at 10:12 AM, <meacer@chromium.org> wrote:

> Let's go with this one then.
>
> Adding OWNERS reviewers:
>
> battre: pref_metrics_service.cc
> mpearson: histograms.xml
>
> Thanks.
>
>
https://codereview.chromium.**org/21121007/<https://codereview.chromium.org/2...
>

[Mark P, 2013-07-30 17:25:21]

Also, re the 6 in the .cc file:
You should add comments somewhere (session_startup_pref.h?) that tells
someone to update this value when adding a new pref value.  Or add a MAX to
the list of pref values and just use that the metrics code.

--mark



On Tue, Jul 30, 2013 at 10:19 AM, Mark Pearson <mpearson@chromium.org>wrote:

>
> SessionRestoreOnStartup is a misleading histogram name.  Can we do
> something like StartupSettings or StartupPrefs?
>
> You add two histograms in the .cc change.  Why do you only add one to
> histograms.xml?
>
> --mark
>
>
> On Tue, Jul 30, 2013 at 10:12 AM, <meacer@chromium.org> wrote:
>
>> Let's go with this one then.
>>
>> Adding OWNERS reviewers:
>>
>> battre: pref_metrics_service.cc
>> mpearson: histograms.xml
>>
>> Thanks.
>>
>>
https://codereview.chromium.**org/21121007/<https://codereview.chromium.org/2...
>>
>
>

[battre, 2013-07-30 17:31:53]

I'll just defer my LGTM to Mark to make it easier w.r.t. timezones. I think the
principle approach is good and you can figure out the naming questions.

[meacer, 2013-07-30 17:49:08]

> SessionRestoreOnStartup is a misleading histogram name.  Can we do
> something like StartupSettings or StartupPrefs?

Changed it to StartupPageLoadSettings and StartupPageLoadURLs.

> You add two histograms in the .cc change.  Why do you only add one to
> histograms.xml?

Sorry missed the second one. Added.

> Or add a MAX to the list of pref values and just use that the metrics code.

Done.

[Mark P, 2013-07-30 17:56:57]

https://codereview.chromium.org/21121007/diff/10001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/21121007/diff/10001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:34: int url_list_size =
profile_->GetPrefs()->GetList(
nit: const int or use the function directly in the UMA_HISTOGRAM call

https://codereview.chromium.org/21121007/diff/10001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:36:
UMA_HISTOGRAM_COUNTS_100("Settings.StartupPageLoadURLs",
minor comment: this creates a histogram with 50 buckets.  Do you need that many?
 Perhaps you'd be fine with a histogram that maxes out at 50 and has only 20
buckets.

https://codereview.chromium.org/21121007/diff/10001/chrome/browser/prefs/sess...
File chrome/browser/prefs/session_startup_pref.h (right):

https://codereview.chromium.org/21121007/diff/10001/chrome/browser/prefs/sess...
chrome/browser/prefs/session_startup_pref.h:43: // so make sure to update the
histogram enum if you change these.
nit: the histogram enum -> histograms.xml
(that's all that needs to change, right?  Let's be precise.)

https://codereview.chromium.org/21121007/diff/10001/tools/metrics/histograms/...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/21121007/diff/10001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:12832: <summary>Records startup page
settings when a profile is loaded.</summary>
nit: Records -> The
(most histogram summaries are simple noun phrases)

https://codereview.chromium.org/21121007/diff/10001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:12837: Records the number of URLs to be
loaded on startup when a profile is loaded,
nit: Records the -> The

https://codereview.chromium.org/21121007/diff/10001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:20525: <int value="5" label="Open New
tab page"/>
nit: odd capitalization

[meacer, 2013-07-30 18:41:50]

Thanks, can you please take another look?

https://codereview.chromium.org/21121007/diff/10001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/21121007/diff/10001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:34: int url_list_size =
profile_->GetPrefs()->GetList(
On 2013/07/30 17:56:57, Mark P wrote:
> nit: const int or use the function directly in the UMA_HISTOGRAM call

Done.

https://codereview.chromium.org/21121007/diff/10001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:36:
UMA_HISTOGRAM_COUNTS_100("Settings.StartupPageLoadURLs",
On 2013/07/30 17:56:57, Mark P wrote:
> minor comment: this creates a histogram with 50 buckets.  Do you need that
many?
>  Perhaps you'd be fine with a histogram that maxes out at 50 and has only 20
> buckets.

Done.

https://codereview.chromium.org/21121007/diff/10001/chrome/browser/prefs/sess...
File chrome/browser/prefs/session_startup_pref.h (right):

https://codereview.chromium.org/21121007/diff/10001/chrome/browser/prefs/sess...
chrome/browser/prefs/session_startup_pref.h:43: // so make sure to update the
histogram enum if you change these.
On 2013/07/30 17:56:57, Mark P wrote:
> nit: the histogram enum -> histograms.xml
> (that's all that needs to change, right?  Let's be precise.)

Done.

https://codereview.chromium.org/21121007/diff/10001/tools/metrics/histograms/...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/21121007/diff/10001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:12832: <summary>Records startup page
settings when a profile is loaded.</summary>
On 2013/07/30 17:56:57, Mark P wrote:
> nit: Records -> The
> (most histogram summaries are simple noun phrases)

Done.

https://codereview.chromium.org/21121007/diff/10001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:12837: Records the number of URLs to be
loaded on startup when a profile is loaded,
On 2013/07/30 17:56:57, Mark P wrote:
> nit: Records the -> The

Done.

[Mark P, 2013-07-30 18:48:59]

lgtm

[meacer, 2013-07-30 18:50:22]

On 2013/07/30 18:48:59, Mark P wrote:
> lgtm

Thanks Mark! Committing soon.

[meacer, 2013-07-30 20:24:28]

Committed patchset #4 manually as r214426 (presubmit successful).