Add histogram to track prerender sessions

Add histogram to track prerender sessions

BUG=70401
TEST=nope

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72800

[cbentzel, 2011-01-23 12:32:47]

LGTM

For test, make sure to at least validate that reasonable values show up in
about:histograms locally.

http://codereview.chromium.org/6340013/diff/1/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6340013/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:1584: UMA_HISTOGRAM_ENUMERATION(
Could you put this in PrefetchFieldTrial so it's more tied with the other
prerender/prefetch?

This is OK as well since near other UMA calls.

http://codereview.chromium.org/6340013/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:1586:
parsed_command_line.HasSwitch(switches::kEnablePagePrerender), 2);
Eventually I think this histogram will want more buckets such as off, manually
disabled, manually enabled, trial-enabled.

[gavinp, 2011-01-23 14:27:10]

On 2011/01/23 12:32:47, cbentzel wrote:
> 
> For test, make sure to at least validate that reasonable values show up in
> about:histograms locally.
> 

I've done that, for both settings, and against the command line & about_flags. 
Sorry if my test message was more glib above: I meant there were no automatic
tests.

> http://codereview.chromium.org/6340013/diff/1/chrome/browser/browser_main.cc
> File chrome/browser/browser_main.cc (right):
> 
>
http://codereview.chromium.org/6340013/diff/1/chrome/browser/browser_main.cc#...
> chrome/browser/browser_main.cc:1584: UMA_HISTOGRAM_ENUMERATION(
> Could you put this in PrefetchFieldTrial so it's more tied with the other
> prerender/prefetch?
> 
> This is OK as well since near other UMA calls.

There were a few places that I thought of.

A) where it is

B) in ProfileImpl::GetPrerenderManager()  (call out to a static function from
here, that
   static function having a guard.)  I thought that was a bit sketchy...

C) Closer to other prerender material in the command line flags.  I weighed the
UMAs as the place, but I can look and see if we can clump all of our prerender
logic in browser_main in one place.
   
> 
>
http://codereview.chromium.org/6340013/diff/1/chrome/browser/browser_main.cc#...
> chrome/browser/browser_main.cc:1586:
> parsed_command_line.HasSwitch(switches::kEnablePagePrerender), 2);
> Eventually I think this histogram will want more buckets such as off, manually
> disabled, manually enabled, trial-enabled.

Hrrrm.  We might; or we might just use the field trial mechanism to split the
histograms we care about (including this one).

- Gavin

[cbentzel, 2011-01-23 14:30:59]

Still LGTM

On Sun, Jan 23, 2011 at 9:27 AM, <gavinp@chromium.org> wrote:

>
> B) in ProfileImpl::GetPrerenderManager()  (call out to a static function
> from
> here, that
>   static function having a guard.)  I thought that was a bit sketchy...
>

This is probably the wrong place, since the configuration is currently
controlled per-browser rather than per-profile.  If it moves to per-profile
configuration, that may be a better location.

[jar (doing other things), 2011-01-23 20:15:53]

Be sure to validate on try bots.  See comment below.

http://codereview.chromium.org/6340013/diff/1/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6340013/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:1586:
parsed_command_line.HasSwitch(switches::kEnablePagePrerender), 2);
I think HasSwitch returns a boolean, but the HISTOGRAM is expected an integer
sample.  I'd like to see a more explicit selection of 0 vs 1.

[gavinp, 2011-01-24 06:01:46]

Thanks for the reviews!

I've now remediated to Jim's review; I created an enumeration to describe the
mapping from bool --> 0, 1.

http://codereview.chromium.org/6340013/diff/1/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6340013/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:1586:
parsed_command_line.HasSwitch(switches::kEnablePagePrerender), 2);
On 2011/01/23 20:15:53, jar wrote:
> I think HasSwitch returns a boolean, but the HISTOGRAM is expected an integer
> sample.  I'd like to see a more explicit selection of 0 vs 1.

OK, I've now created an enumeration (which I put in the prerendering directory),
and I use that to seed the histogram.  That should also allow expansion of the
types in this histogram for different session types.  Chris suggested we'd want
this if we don't use the UMA field trial mechanism, and there might well be
other reasons.

[cbentzel, 2011-01-25 17:49:04]

So, I had a misunderstanding for how about:flags worked.

Basically, there is a per-profile preference which stores the value of enabled
flags. In the ProfileImpl constructor, the shared switches are overwritten with
the value in the local preferences. This means that we should probably create
the PrerenderManager then, and the histogram for this should be done per-profile
rather than per-session [with the exception of off-the-record profiles].

[cbentzel, 2011-01-25 18:02:04]

And, it looks like r72404 landed yesterday which moves about:flags eval to
browser_main [using browser local state] rather than per-profile.

[gavinp, 2011-01-26 01:54:10]

Jar: *ping* ?

Chris, I think r72404 is a good thing.  Also, this patch still applies to trunk,
and I've tested all combinations just now of the about:flags & the command line
option, it seems to capture correctly.

[cbentzel, 2011-01-26 16:55:46]

http://codereview.chromium.org/6340013/diff/8001/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6340013/diff/8001/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1585: UMA_HISTOGRAM_ENUMERATION(
I'd really like this to be placed in PrefetchFieldTrial now so flag reading
happens in one centralized place, rather than both in PrefetchFieldTrial and
GetSessionType

[jar (doing other things), 2011-01-26 21:31:32]

http://codereview.chromium.org/6340013/diff/8001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/6340013/diff/8001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:21:
switches::kEnablePagePrerender))
nit: (I think) indent only 4 spaces from previous line.

[gavinp, 2011-01-27 04:29:29]

Thanks for the reviews everyone!  This latest upload changes the way the
enumeration is initialized, again.  I've also merged in some changes of Timo's
that were touching on similar work.

http://codereview.chromium.org/6340013/diff/8001/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6340013/diff/8001/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1585: UMA_HISTOGRAM_ENUMERATION(
On 2011/01/26 16:55:46, cbentzel wrote:
> I'd really like this to be placed in PrefetchFieldTrial now so flag reading
> happens in one centralized place, rather than both in PrefetchFieldTrial and
> GetSessionType

Done.

http://codereview.chromium.org/6340013/diff/8001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/6340013/diff/8001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:21:
switches::kEnablePagePrerender))
On 2011/01/26 21:31:32, jar wrote:
> nit: (I think) indent only 4 spaces from previous line.

jar & I followed up on this: brettw said that indent by 8 here is acceptable, as
is indent by four based on readability.  However, this is informational only, as
this function doesn't exist in the new upload.

[cbentzel, 2011-01-27 04:38:34]

LGTM

I wish there was a way we could avoid the static'ness of mode, but I couldn't
think of a good alternative.

http://codereview.chromium.org/6340013/diff/21001/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6340013/diff/21001/chrome/browser/browser_main...
chrome/browser/browser_main.cc:453: PrerenderManager::PrerenderManagerMode
prerender_mode;
Uninitialized values always scare me. Maybe change if/else to ternary, or
initialize to DISABLED.

http://codereview.chromium.org/6340013/diff/21001/chrome/browser/browser_main...
chrome/browser/browser_main.cc:460:
UMA_HISTOGRAM_ENUMERATION("Prerender.Sessions", prerender_mode,
Do we have other Prerender-prefixed histograms at this point, or is this the
first one?

[gavinp, 2011-01-27 05:25:26]

This latest upload fixes the initialization, and it's what I'll commit tomorrow
morning if the tree's green.  :)

http://codereview.chromium.org/6340013/diff/21001/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6340013/diff/21001/chrome/browser/browser_main...
chrome/browser/browser_main.cc:453: PrerenderManager::PrerenderManagerMode
prerender_mode;
On 2011/01/27 04:38:34, cbentzel wrote:
> Uninitialized values always scare me. Maybe change if/else to ternary, or
> initialize to DISABLED. 

Done.

http://codereview.chromium.org/6340013/diff/21001/chrome/browser/browser_main...
chrome/browser/browser_main.cc:460:
UMA_HISTOGRAM_ENUMERATION("Prerender.Sessions", prerender_mode,
On 2011/01/27 04:38:34, cbentzel wrote:
> Do we have other Prerender-prefixed histograms at this point, or is this the
> first one?

This is the first one.  I couldn't think of another group to put it in reviewing
them: ultimately I guess our histograms will be split between PLT and Prerender?