Move prerender histograms out of prerender manager.

Move prerender histograms out of prerender manager.
Add a separate histogram label for GWS based prerendering.
R=cbentzel@chromium.org, dominich@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96227

[dominich, 2011-08-10 00:32:46]

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_histograms.cc:53:
last_origin_(ORIGIN_LINK_REL_PRERENDER),
Initialize to something invalid, like ORIGIN_MAX.

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_histograms.cc:155: std::string
PrerenderHistograms::ComposeHistogramName(
This does not need to be a member. Make it a function in an anonymous namespace
in this file.

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_histograms.cc:163: std::string
PrerenderHistograms::GetHistogramName(
This does not need to be a member. Make it a function in an anonymous namespace
in this file.

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_histograms.h (right):

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_histograms.h:18: // currently underway (based
on the PrerenderOrigin of the most recent
nit: close this (

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.cc:179: histograms_(new
PrerenderHistograms(config_.max_age)) {
If max_age is ever changed dynamically we should reset this on the histograms.
Or the histograms should take a pointer to the config and look it up from there
every time instead of taking a copy.

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.cc:224:
StartsWithASCII(referrer.host(), std::string("www.google."), true)) {
Is the referrer ever google.* without the www.?

[tburkard, 2011-08-10 01:05:12]

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_histograms.cc:53:
last_origin_(ORIGIN_LINK_REL_PRERENDER),
I don't think that's right, because for the PLT's independent of any prerender,
a valid origin is needed.  So I think using the most neutral thing (a web-based
prerender) makes the most sense.

On 2011/08/10 00:32:46, dominich wrote:
> Initialize to something invalid, like ORIGIN_MAX.

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_histograms.cc:155: std::string
PrerenderHistograms::ComposeHistogramName(
On 2011/08/10 00:32:46, dominich wrote:
> This does not need to be a member. Make it a function in an anonymous
namespace
> in this file.

Done.

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_histograms.cc:163: std::string
PrerenderHistograms::GetHistogramName(
On 2011/08/10 00:32:46, dominich wrote:
> This does not need to be a member. Make it a function in an anonymous
namespace
> in this file.

Done.

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_histograms.h (right):

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_histograms.h:18: // currently underway (based
on the PrerenderOrigin of the most recent
On 2011/08/10 00:32:46, dominich wrote:
> nit: close this (

Done.

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.cc:179: histograms_(new
PrerenderHistograms(config_.max_age)) {
Only an issue for tests, in which case histograms not matter.  Not an issue in
the production setup.

On 2011/08/10 00:32:46, dominich wrote:
> If max_age is ever changed dynamically we should reset this on the histograms.
> Or the histograms should take a pointer to the config and look it up from
there
> every time instead of taking a copy.

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.cc:224:
StartsWithASCII(referrer.host(), std::string("www.google."), true)) {
No.

On 2011/08/10 00:32:46, dominich wrote:
> Is the referrer ever google.* without the http://www.?

[dominich, 2011-08-10 15:53:37]

There are lint errors too.

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_histograms.cc:53:
last_origin_(ORIGIN_LINK_REL_PRERENDER),
That makes sense, but then I would try to make the PLT histograms not require a
valid origin. Is that possible?

On 2011/08/10 01:05:13, tburkard wrote:
> I don't think that's right, because for the PLT's independent of any
prerender,
> a valid origin is needed.  So I think using the most neutral thing (a
web-based
> prerender) makes the most sense.
> 
> On 2011/08/10 00:32:46, dominich wrote:
> > Initialize to something invalid, like ORIGIN_MAX.
>

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.cc:179: histograms_(new
PrerenderHistograms(config_.max_age)) {
On 2011/08/10 01:05:13, tburkard wrote:
> Only an issue for tests, in which case histograms not matter.  Not an issue in
> the production setup.
> 

For now, but there's nothing stopping someone from deciding to change this on
the fly. Currently it would work correctly as the PrerenderManager checks the
max_age every time. This change breaks the consistency between the Config and
the Histograms. To be correct it should take a reference/pointer to the config
or max_age so it can reflect any changes.

> On 2011/08/10 00:32:46, dominich wrote:
> > If max_age is ever changed dynamically we should reset this on the
histograms.
> > Or the histograms should take a pointer to the config and look it up from
> there
> > every time instead of taking a copy.
>

[tburkard, 2011-08-10 17:11:33]

Fixed all the lint issues.

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_histograms.cc:53:
last_origin_(ORIGIN_LINK_REL_PRERENDER),
I could easily do that.. make another histogram category "none".  However, since
we already have so many categories the way it is, i'd rather use a "default"
value, and the most neutral one to me seems LINK_REL_PRERENDER.

On 2011/08/10 15:53:37, dominich wrote:
> That makes sense, but then I would try to make the PLT histograms not require
a
> valid origin. Is that possible?
> 
> On 2011/08/10 01:05:13, tburkard wrote:
> > I don't think that's right, because for the PLT's independent of any
> prerender,
> > a valid origin is needed.  So I think using the most neutral thing (a
> web-based
> > prerender) makes the most sense.
> > 
> > On 2011/08/10 00:32:46, dominich wrote:
> > > Initialize to something invalid, like ORIGIN_MAX.
> > 
>

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/7605020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.cc:179: histograms_(new
PrerenderHistograms(config_.max_age)) {
Ok I changed it by passing in the value into the only method needing it every
time (rather than in the constructor).

On 2011/08/10 15:53:37, dominich wrote:
> On 2011/08/10 01:05:13, tburkard wrote:
> > Only an issue for tests, in which case histograms not matter.  Not an issue
in
> > the production setup.
> > 
> 
> For now, but there's nothing stopping someone from deciding to change this on
> the fly. Currently it would work correctly as the PrerenderManager checks the
> max_age every time. This change breaks the consistency between the Config and
> the Histograms. To be correct it should take a reference/pointer to the config
> or max_age so it can reflect any changes.
> 
> > On 2011/08/10 00:32:46, dominich wrote:
> > > If max_age is ever changed dynamically we should reset this on the
> histograms.
> > > Or the histograms should take a pointer to the config and look it up from
> > there
> > > every time instead of taking a copy.
> > 
>

[dominich, 2011-08-10 17:16:22]

Getting close. Are you going to add unit tests?

http://codereview.chromium.org/7605020/diff/7001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_histograms.h (right):

http://codereview.chromium.org/7605020/diff/7001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_histograms.h:28: explicit
PrerenderHistograms();
no need for explicit here now.

http://codereview.chromium.org/7605020/diff/7001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_histograms.h:58: virtual base::TimeTicks
GetCurrentTimeTicks() const;
This is virtual so that browser tests could override it in the PrerenderManager.
Does this need to be virtual? If not, then you don't need a virtual destructor
either.

[tburkard, 2011-08-10 17:49:14]

There weren't any unit tests before.  I would liek to get this in first, I will
make tests in a separate chagne later this week.

Thanks.

http://codereview.chromium.org/7605020/diff/7001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_histograms.h (right):

http://codereview.chromium.org/7605020/diff/7001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_histograms.h:28: explicit
PrerenderHistograms();
On 2011/08/10 17:16:22, dominich wrote:
> no need for explicit here now.

Done.

http://codereview.chromium.org/7605020/diff/7001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_histograms.h:58: virtual base::TimeTicks
GetCurrentTimeTicks() const;
On 2011/08/10 17:16:22, dominich wrote:
> This is virtual so that browser tests could override it in the
PrerenderManager.
> Does this need to be virtual? If not, then you don't need a virtual destructor
> either.

Done.

[dominich, 2011-08-10 17:51:37]

LGTM

Please add a bug about adding unit tests so that we don't forget ;)