Updating histograms to allow for experiments & log origin-based histograms.

Updating histograms to allow for experiments & log origin-based histograms.
R=cbentzel@chromium.org, dominich@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91765

[tburkard, 2011-06-30 17:44:04]

Here is a first cut.  Not tested yet.

Thanks.

Timo

[dominich, 2011-06-30 18:11:15]

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

http://codereview.chromium.org/7289020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_contents.h:65: char experiment_id) = 0;
A uint8 would be more appropriate than a char. This suggests it might be 'A' or
'B' for example. A uint8 makes it clear it's an integer.

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

http://codereview.chromium.org/7289020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.h:71: static const char kNoExperiment
= -1;
A uint8 set to static_cast<uint8>(-1) if you agree to use uint8 for the type. I
wonder if an enum is even better in this case.

http://codereview.chromium.org/7289020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.h:340: // Composes a histogram name
based on a histogram type.
It doesn't look like these really need to be in the PrerenderManager. Can we
have a PrerenderHistogram or PrerenderExperimentTracker class that manages this
stuff?

[cbentzel, 2011-06-30 18:15:39]

I share similar concerns with dominic it turns out. 

Unfortunately, the additional macro is probably better over the
struct-of-histogram-pointers, as we discussed over chat.

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

http://codereview.chromium.org/7289020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:249: char experiment_id)
OVERRIDE {
uint8 is better than char.

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

http://codereview.chromium.org/7289020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.h:216: // Extracts an experiment
stored in the query parameter
Can we just combine this with MaybeGetQueryStringBasedAliasURL?

I'd also consider moving it to a separate file and just make it a top-level
function, rather than a static on PrernederManager.

[tburkard, 2011-06-30 19:25:43]

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

http://codereview.chromium.org/7289020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:249: char experiment_id)
OVERRIDE {
On 2011/06/30 18:15:39, cbentzel wrote:
> uint8 is better than char. 

So use 255 then as "no experiment"?  I think -1 is more intuitive..

[dominich, 2011-06-30 19:28:25]

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

http://codereview.chromium.org/7289020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:249: char experiment_id)
OVERRIDE {
On 2011/06/30 19:25:43, tburkard wrote:
> On 2011/06/30 18:15:39, cbentzel wrote:
> > uint8 is better than char. 
> 
> So use 255 then as "no experiment"?  I think -1 is more intuitive..

You can cast -1 to a uint8.

Are the experiments going to be named? If so, an enum with 10 entries might be
better.

[cbentzel, 2011-06-30 19:34:23]

Experiments could be 1-based as well.

On Thu, Jun 30, 2011 at 3:28 PM,  <dominich@chromium.org> wrote:
>
>
http://codereview.chromium.org/7289020/diff/1/chrome/browser/prerender/preren...
> File chrome/browser/prerender/prerender_browsertest.cc (right):
>
>
http://codereview.chromium.org/7289020/diff/1/chrome/browser/prerender/preren...
> chrome/browser/prerender/prerender_browsertest.cc:249: char
> experiment_id) OVERRIDE {
> On 2011/06/30 19:25:43, tburkard wrote:
>>
>> On 2011/06/30 18:15:39, cbentzel wrote:
>> > uint8 is better than char.
>
>> So use 255 then as "no experiment"?  I think -1 is more intuitive..
>
> You can cast -1 to a uint8.
>
> Are the experiments going to be named? If so, an enum with 10 entries
> might be better.
>
> http://codereview.chromium.org/7289020/
>

[tburkard, 2011-06-30 19:45:48]

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

http://codereview.chromium.org/7289020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:249: char experiment_id)
OVERRIDE {
On 2011/06/30 18:15:39, cbentzel wrote:
> uint8 is better than char. 

Done.

http://codereview.chromium.org/7289020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:249: char experiment_id)
OVERRIDE {
On 2011/06/30 19:28:25, dominich wrote:
> On 2011/06/30 19:25:43, tburkard wrote:
> > On 2011/06/30 18:15:39, cbentzel wrote:
> > > uint8 is better than char. 
> > 
> > So use 255 then as "no experiment"?  I think -1 is more intuitive..
> 
> You can cast -1 to a uint8.
> 
> Are the experiments going to be named? If so, an enum with 10 entries might be
> better.

Done.

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

http://codereview.chromium.org/7289020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_contents.h:65: char experiment_id) = 0;
On 2011/06/30 18:11:15, dominich wrote:
> A uint8 would be more appropriate than a char. This suggests it might be 'A'
or
> 'B' for example. A uint8 makes it clear it's an integer.

Done.

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

http://codereview.chromium.org/7289020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.h:71: static const char kNoExperiment
= -1;
On 2011/06/30 18:11:15, dominich wrote:
> A uint8 set to static_cast<uint8>(-1) if you agree to use uint8 for the type.
I
> wonder if an enum is even better in this case.

Done.

http://codereview.chromium.org/7289020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.h:216: // Extracts an experiment
stored in the query parameter
Since it's a local check that only prerendermanager needs to perform, I think it
makes sense to make it part of that class.  Would you prefer it being a full
member as opposed to static?

On 2011/06/30 18:15:39, cbentzel wrote:
> Can we just combine this with MaybeGetQueryStringBasedAliasURL?
> 
> I'd also consider moving it to a separate file and just make it a top-level
> function, rather than a static on PrernederManager.

http://codereview.chromium.org/7289020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.h:340: // Composes a histogram name
based on a histogram type.
Can I do the cleanup in a separate CL?  This CL is already complex, I'd rather
get in the functionality first, and then do a more straightforward cleanup.

On 2011/06/30 18:11:15, dominich wrote:
> It doesn't look like these really need to be in the PrerenderManager. Can we
> have a PrerenderHistogram or PrerenderExperimentTracker class that manages
this
> stuff?

[cbentzel, 2011-06-30 19:51:27]

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

http://codereview.chromium.org/7289020/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.h:216: // Extracts an experiment
stored in the query parameter
On 2011/06/30 19:45:48, tburkard wrote:
> Since it's a local check that only prerendermanager needs to perform, I think
it
> makes sense to make it part of that class.  Would you prefer it being a full
> member as opposed to static?
> 
> On 2011/06/30 18:15:39, cbentzel wrote:
> > Can we just combine this with MaybeGetQueryStringBasedAliasURL?
> > 
> > I'd also consider moving it to a separate file and just make it a top-level
> > function, rather than a static on PrernederManager. 
> 

The reason to move it out is the prerender_manager.[h|cc] are pretty big. Easier
to read and test if top-level function that this includes. I also noticed that
the new method is not tested.

[dominich.google, 2011-06-30 19:55:15]

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

http://codereview.chromium.org/7289020/diff/5002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:95: static char experiment_id =
kNoExperiment; \
This should be a uint8 now.

If you have two histograms in the same method, does this work (being static)?

http://codereview.chromium.org/7289020/diff/5002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:1136: std::string
PrerenderManager::ComposeHistogramName(std::string prefix_type,
const references here

http://codereview.chromium.org/7289020/diff/5002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:1145: std::string name)  const{
const reference.

http://codereview.chromium.org/7289020/diff/5002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:1166: std::string
PrerenderManager::GetDefaultHistogramName(std::string name) const {
const reference.

http://codereview.chromium.org/7289020/diff/5002/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/7289020/diff/5002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.h:219: // is not an integer in the
range 0 to 9.
1 to 9

[tburkard, 2011-06-30 21:40:20]

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

http://codereview.chromium.org/7289020/diff/5002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:95: static char experiment_id =
kNoExperiment; \
On 2011/06/30 19:55:15, dominich.google wrote:
> This should be a uint8 now.
> 
> If you have two histograms in the same method, does this work (being static)?

Done.

http://codereview.chromium.org/7289020/diff/5002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:1136: std::string
PrerenderManager::ComposeHistogramName(std::string prefix_type,
On 2011/06/30 19:55:15, dominich.google wrote:
> const references here

Done.

http://codereview.chromium.org/7289020/diff/5002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:1145: std::string name)  const{
On 2011/06/30 19:55:15, dominich.google wrote:
> const reference.

Done.

http://codereview.chromium.org/7289020/diff/5002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:1166: std::string
PrerenderManager::GetDefaultHistogramName(std::string name) const {
On 2011/06/30 19:55:15, dominich.google wrote:
> const reference.

Done.

http://codereview.chromium.org/7289020/diff/5002/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/7289020/diff/5002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.h:219: // is not an integer in the
range 0 to 9.
On 2011/06/30 19:55:15, dominich.google wrote:
> 1 to 9

Done.

[dominich, 2011-06-30 21:44:59]

http://codereview.chromium.org/7289020/diff/3006/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/7289020/diff/3006/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.h:328: // Composes a histogram name
based on a histogram type.
Do these need to be in PrerenderManager? It seems they should be in a
prerender_histogram file in the top-level prerender namespace, or at least in a
class to track the last experiment/wash stuff.

[tburkard, 2011-06-30 21:48:59]

I will look into turning that into a separate class once this is committed.

I don't want to refactor code & add new features in the same chagne.

Timo

On Thu, Jun 30, 2011 at 2:44 PM, <dominich@chromium.org> wrote:

>
> http://codereview.chromium.**org/7289020/diff/3006/chrome/**
>
browser/prerender/prerender_**manager.h<http://codereview.chromium.org/7289020/diff/3006/chrome/browser/prerender/prerender_manager.h>
>
> File chrome/browser/prerender/**prerender_manager.h (right):
>
> http://codereview.chromium.**org/7289020/diff/3006/chrome/**
>
browser/prerender/prerender_**manager.h#newcode328<http://codereview.chromium.org/7289020/diff/3006/chrome/browser/prerender/prerender_manager.h#newcode328>
> chrome/browser/prerender/**prerender_manager.h:328: // Composes a
>
> histogram name based on a histogram type.
> Do these need to be in PrerenderManager? It seems they should be in a
> prerender_histogram file in the top-level prerender namespace, or at
> least in a class to track the last experiment/wash stuff.
>
>
>
http://codereview.chromium.**org/7289020/<http://codereview.chromium.org/7289...
>

[dominich, 2011-06-30 21:50:49]

On 2011/06/30 21:48:59, tburkard wrote:
> I will look into turning that into a separate class once this is committed.
> 
> I don't want to refactor code & add new features in the same chagne.
> 

But you'll only be refactoring the code that you're adding, no? It seems pretty
self-contained already so moving it out shouldn't be a big step. My concern is
that if it isn't done as you're adding it it is the sort of thing that can slip
down the priority list. We have a good thing going with cleaning up
PrerenderManager and this takes it a step backwards.

> 
> On Thu, Jun 30, 2011 at 2:44 PM, <mailto:dominich@chromium.org> wrote:
> 
> >
> > http://codereview.chromium.**org/7289020/diff/3006/chrome/**
> >
>
browser/prerender/prerender_**manager.h<http://codereview.chromium.org/7289020/diff/3006/chrome/browser/prerender/prerender_manager.h>
> >
> > File chrome/browser/prerender/**prerender_manager.h (right):
> >
> > http://codereview.chromium.**org/7289020/diff/3006/chrome/**
> >
>
browser/prerender/prerender_**manager.h#newcode328<http://codereview.chromium.org/7289020/diff/3006/chrome/browser/prerender/prerender_manager.h#newcode328>
> > chrome/browser/prerender/**prerender_manager.h:328: // Composes a
> >
> > histogram name based on a histogram type.
> > Do these need to be in PrerenderManager? It seems they should be in a
> > prerender_histogram file in the top-level prerender namespace, or at
> > least in a class to track the last experiment/wash stuff.
> >
> >
> >
>
http://codereview.chromium.**org/7289020/%3Chttp://codereview.chromium.org/72...>
> >

[tburkard, 2011-06-30 21:54:38]

I think its messy to have histogram related things not all at one place.
 The UMA calls are in prerender_manager.cc, which is why I added these
functions to prerender_manager.c as well.

Timo

On Thu, Jun 30, 2011 at 2:50 PM, <dominich@chromium.org> wrote:

> On 2011/06/30 21:48:59, tburkard wrote:
>
>> I will look into turning that into a separate class once this is
>> committed.
>>
>
>  I don't want to refactor code & add new features in the same chagne.
>>
>
>
> But you'll only be refactoring the code that you're adding, no? It seems
> pretty
> self-contained already so moving it out shouldn't be a big step. My concern
> is
> that if it isn't done as you're adding it it is the sort of thing that can
> slip
> down the priority list. We have a good thing going with cleaning up
> PrerenderManager and this takes it a step backwards.
>
>
>
>  On Thu, Jun 30, 2011 at 2:44 PM, <mailto:dominich@chromium.org> wrote:
>>
>
>  >
>> > http://codereview.chromium.****org/7289020/diff/3006/chrome/****
>> >
>>
>
> browser/prerender/prerender_****manager.h<http://codereview.**
> chromium.org/7289020/diff/**3006/chrome/browser/prerender/**
>
prerender_manager.h<http://codereview.chromium.org/7289020/diff/3006/chrome/browser/prerender/prerender_manager.h>
> >
>
>  >
>> > File chrome/browser/prerender/****prerender_manager.h (right):
>> >
>> > http://codereview.chromium.****org/7289020/diff/3006/chrome/****
>> >
>>
>
> browser/prerender/prerender_****manager.h#newcode328<http://**
> codereview.chromium.org/**7289020/diff/3006/chrome/**
>
browser/prerender/prerender_**manager.h#newcode328<http://codereview.chromium.org/7289020/diff/3006/chrome/browser/prerender/prerender_manager.h#newcode328>
> >
>
>  > chrome/browser/prerender/****prerender_manager.h:328: // Composes a
>> >
>> > histogram name based on a histogram type.
>> > Do these need to be in PrerenderManager? It seems they should be in a
>> > prerender_histogram file in the top-level prerender namespace, or at
>> > least in a class to track the last experiment/wash stuff.
>> >
>> >
>> >
>>
>
> http://codereview.chromium.****org/7289020/%3Chttp://coderevi**
> ew.chromium.org/7289020/ <http://codereview.chromium.org/7289020/>>
>
>  >
>>
>
>
>
>
http://codereview.chromium.**org/7289020/<http://codereview.chromium.org/7289...
>

[dominich, 2011-06-30 22:06:02]

I don't buy that logic. Just because you have calls there doesn't mean
everything should be in there. The UMA calls could be move out, or the macros
could be in a header of another file. There's no reason for the new methods to
be a member of PrerenderManager. Unless I've missed something, the only thing
they access from PrerenderManager are members that you've added.

It sounds like you're pretty adamant to leave it, but I think it's a mistake to
introduce something that you know you want to change later when right now is the
easiest time to change it.

On 2011/06/30 21:54:38, tburkard wrote:
> I think its messy to have histogram related things not all at one place.
>  The UMA calls are in prerender_manager.cc, which is why I added these
> functions to prerender_manager.c as well.
> 
> 
> On Thu, Jun 30, 2011 at 2:50 PM, <mailto:dominich@chromium.org> wrote:
> 
> > On 2011/06/30 21:48:59, tburkard wrote:
> >
> >> I will look into turning that into a separate class once this is
> >> committed.
> >>
> >
> >  I don't want to refactor code & add new features in the same chagne.
> >>
> >
> >
> > But you'll only be refactoring the code that you're adding, no? It seems
> > pretty
> > self-contained already so moving it out shouldn't be a big step. My concern
> > is
> > that if it isn't done as you're adding it it is the sort of thing that can
> > slip
> > down the priority list. We have a good thing going with cleaning up
> > PrerenderManager and this takes it a step backwards.
> >
> >
> >
> >  On Thu, Jun 30, 2011 at 2:44 PM, <mailto:dominich@chromium.org> wrote:
> >>
> >
> >  >
> >> > http://codereview.chromium.****org/7289020/diff/3006/chrome/****
> >> >
> >>
> >
> > browser/prerender/prerender_****manager.h<http://codereview.**
> > chromium.org/7289020/diff/**3006/chrome/browser/prerender/**
> >
>
prerender_manager.h<http://codereview.chromium.org/7289020/diff/3006/chrome/browser/prerender/prerender_manager.h>
> > >
> >
> >  >
> >> > File chrome/browser/prerender/****prerender_manager.h (right):
> >> >
> >> > http://codereview.chromium.****org/7289020/diff/3006/chrome/****
> >> >
> >>
> >
> > browser/prerender/prerender_****manager.h#newcode328<http://**
> > codereview.chromium.org/**7289020/diff/3006/chrome/**
> >
>
browser/prerender/prerender_**manager.h#newcode328<http://codereview.chromium.org/7289020/diff/3006/chrome/browser/prerender/prerender_manager.h#newcode328>
> > >
> >
> >  > chrome/browser/prerender/****prerender_manager.h:328: // Composes a
> >> >
> >> > histogram name based on a histogram type.
> >> > Do these need to be in PrerenderManager? It seems they should be in a
> >> > prerender_histogram file in the top-level prerender namespace, or at
> >> > least in a class to track the last experiment/wash stuff.
> >> >
> >> >
> >> >
> >>
> >
> > http://codereview.chromium.****org/7289020/%253Chttp://coderevi**
> > ew.chromium.org/7289020/ <http://codereview.chromium.org/7289020/>>
> >
> >  >
> >>
> >
> >
> >
> >
>
http://codereview.chromium.**org/7289020/%3Chttp://codereview.chromium.org/72...>
> >

[tburkard, 2011-06-30 22:19:49]

Let's ask Chris and see what he thinks.

On Thu, Jun 30, 2011 at 3:06 PM, <dominich@chromium.org> wrote:

> I don't buy that logic. Just because you have calls there doesn't mean
> everything should be in there. The UMA calls could be move out, or the
> macros
> could be in a header of another file. There's no reason for the new methods
> to
> be a member of PrerenderManager. Unless I've missed something, the only
> thing
> they access from PrerenderManager are members that you've added.
>
> It sounds like you're pretty adamant to leave it, but I think it's a
> mistake to
> introduce something that you know you want to change later when right now
> is the
> easiest time to change it.
>
>
> On 2011/06/30 21:54:38, tburkard wrote:
>
>> I think its messy to have histogram related things not all at one place.
>>  The UMA calls are in prerender_manager.cc, which is why I added these
>> functions to prerender_manager.c as well.
>>
>
>
>  On Thu, Jun 30, 2011 at 2:50 PM, <mailto:dominich@chromium.org> wrote:
>>
>
>  > On 2011/06/30 21:48:59, tburkard wrote:
>> >
>> >> I will look into turning that into a separate class once this is
>> >> committed.
>> >>
>> >
>> >  I don't want to refactor code & add new features in the same chagne.
>> >>
>> >
>> >
>> > But you'll only be refactoring the code that you're adding, no? It seems
>> > pretty
>> > self-contained already so moving it out shouldn't be a big step. My
>> concern
>> > is
>> > that if it isn't done as you're adding it it is the sort of thing that
>> can
>> > slip
>> > down the priority list. We have a good thing going with cleaning up
>> > PrerenderManager and this takes it a step backwards.
>> >
>> >
>> >
>> >  On Thu, Jun 30, 2011 at 2:44 PM, <mailto:dominich@chromium.org> wrote:
>> >>
>> >
>> >  >
>> >> > http://codereview.chromium.******org/7289020/diff/3006/chrome/******
>> >> >
>> >>
>> >
>> > browser/prerender/prerender_******manager.h<http://codereview.****
>> >
chromium.org/7289020/diff/****3006/chrome/browser/prerender/****<http://chromium.org/7289020/diff/**3006/chrome/browser/prerender/**>
>> >
>>
>
> prerender_manager.h<http://**codereview.chromium.org/**
>
7289020/diff/3006/chrome/**browser/prerender/prerender_**manager.h<http://codereview.chromium.org/7289020/diff/3006/chrome/browser/prerender/prerender_manager.h>
> >
>
>  > >
>> >
>> >  >
>> >> > File chrome/browser/prerender/******prerender_manager.h (right):
>> >> >
>> >> > http://codereview.chromium.******org/7289020/diff/3006/chrome/******
>> >> >
>> >>
>> >
>> > browser/prerender/prerender_******manager.h#newcode328<http://****
>> >
codereview.chromium.org/****7289020/diff/3006/chrome/**<http://codereview.chromium.org/**7289020/diff/3006/chrome/**>
>> >
>>
>
> browser/prerender/prerender_****manager.h#newcode328<http://**
> codereview.chromium.org/**7289020/diff/3006/chrome/**
>
browser/prerender/prerender_**manager.h#newcode328<http://codereview.chromium.org/7289020/diff/3006/chrome/browser/prerender/prerender_manager.h#newcode328>
> >
>
>> > >
>> >
>> >  > chrome/browser/prerender/******prerender_manager.h:328: // Composes
>> a
>> >> >
>> >> > histogram name based on a histogram type.
>> >> > Do these need to be in PrerenderManager? It seems they should be in a
>> >> > prerender_histogram file in the top-level prerender namespace, or at
>> >> > least in a class to track the last experiment/wash stuff.
>> >> >
>> >> >
>> >> >
>> >>
>> >
>> > http://codereview.chromium.******org/7289020/%253Chttp://**coderevi**
>>
>> > ew.chromium.org/7289020/
<http://codereview.chromium.**org/7289020/<http://codereview.chromium.org/7289...
>> >>
>> >
>> >  >
>> >>
>> >
>> >
>> >
>> >
>>
>
> http://codereview.chromium.****org/7289020/%3Chttp://coderevi**
> ew.chromium.org/7289020/ <http://codereview.chromium.org/7289020/>>
>
>> >
>>
>
>
>
>
http://codereview.chromium.**org/7289020/<http://codereview.chromium.org/7289...
>

[tburkard, 2011-07-06 17:15:22]

Chris, what do you think?

Timo

On Thu, Jun 30, 2011 at 3:19 PM, Timo Burkard <tburkard@chromium.org> wrote:

> Let's ask Chris and see what he thinks.
>
>
> On Thu, Jun 30, 2011 at 3:06 PM, <dominich@chromium.org> wrote:
>
>> I don't buy that logic. Just because you have calls there doesn't mean
>> everything should be in there. The UMA calls could be move out, or the
>> macros
>> could be in a header of another file. There's no reason for the new
>> methods to
>> be a member of PrerenderManager. Unless I've missed something, the only
>> thing
>> they access from PrerenderManager are members that you've added.
>>
>> It sounds like you're pretty adamant to leave it, but I think it's a
>> mistake to
>> introduce something that you know you want to change later when right now
>> is the
>> easiest time to change it.
>>
>>
>> On 2011/06/30 21:54:38, tburkard wrote:
>>
>>> I think its messy to have histogram related things not all at one place.
>>>  The UMA calls are in prerender_manager.cc, which is why I added these
>>> functions to prerender_manager.c as well.
>>>
>>
>>
>>  On Thu, Jun 30, 2011 at 2:50 PM, <mailto:dominich@chromium.org> wrote:
>>>
>>
>>  > On 2011/06/30 21:48:59, tburkard wrote:
>>> >
>>> >> I will look into turning that into a separate class once this is
>>> >> committed.
>>> >>
>>> >
>>> >  I don't want to refactor code & add new features in the same chagne.
>>> >>
>>> >
>>> >
>>> > But you'll only be refactoring the code that you're adding, no? It
>>> seems
>>> > pretty
>>> > self-contained already so moving it out shouldn't be a big step. My
>>> concern
>>> > is
>>> > that if it isn't done as you're adding it it is the sort of thing that
>>> can
>>> > slip
>>> > down the priority list. We have a good thing going with cleaning up
>>> > PrerenderManager and this takes it a step backwards.
>>> >
>>> >
>>> >
>>> >  On Thu, Jun 30, 2011 at 2:44 PM, <mailto:dominich@chromium.org>
>>> wrote:
>>> >>
>>> >
>>> >  >
>>> >> > http://codereview.chromium.******org/7289020/diff/3006/chrome/**
>>> ****
>>> >> >
>>> >>
>>> >
>>> > browser/prerender/prerender_******manager.h<http://codereview.****
>>> >
chromium.org/7289020/diff/****3006/chrome/browser/prerender/****<http://chromium.org/7289020/diff/**3006/chrome/browser/prerender/**>
>>> >
>>>
>>
>> prerender_manager.h<http://**codereview.chromium.org/**
>>
7289020/diff/3006/chrome/**browser/prerender/prerender_**manager.h<http://codereview.chromium.org/7289020/diff/3006/chrome/browser/prerender/prerender_manager.h>
>> >
>>
>>  > >
>>> >
>>> >  >
>>> >> > File chrome/browser/prerender/******prerender_manager.h (right):
>>> >> >
>>> >> > http://codereview.chromium.******org/7289020/diff/3006/chrome/**
>>> ****
>>> >> >
>>> >>
>>> >
>>> > browser/prerender/prerender_******manager.h#newcode328<http://****
>>> >
codereview.chromium.org/****7289020/diff/3006/chrome/**<http://codereview.chromium.org/**7289020/diff/3006/chrome/**>
>>> >
>>>
>>
>> browser/prerender/prerender_****manager.h#newcode328<http://**
>> codereview.chromium.org/**7289020/diff/3006/chrome/**
>>
browser/prerender/prerender_**manager.h#newcode328<http://codereview.chromium.org/7289020/diff/3006/chrome/browser/prerender/prerender_manager.h#newcode328>
>> >
>>
>>> > >
>>> >
>>> >  > chrome/browser/prerender/******prerender_manager.h:328: // Composes
>>> a
>>> >> >
>>> >> > histogram name based on a histogram type.
>>> >> > Do these need to be in PrerenderManager? It seems they should be in
>>> a
>>> >> > prerender_histogram file in the top-level prerender namespace, or at
>>> >> > least in a class to track the last experiment/wash stuff.
>>> >> >
>>> >> >
>>> >> >
>>> >>
>>> >
>>> > http://codereview.chromium.******org/7289020/%253Chttp://**coderevi**
>>>
>>> > ew.chromium.org/7289020/
<http://codereview.chromium.**org/7289020/<http://codereview.chromium.org/7289...
>>> >>
>>> >
>>> >  >
>>> >>
>>> >
>>> >
>>> >
>>> >
>>>
>>
>> http://codereview.chromium.****org/7289020/%3Chttp://coderevi**
>> ew.chromium.org/7289020/ <http://codereview.chromium.org/7289020/>>
>>
>>> >
>>>
>>
>>
>>
>>
http://codereview.chromium.**org/7289020/<http://codereview.chromium.org/7289...
>>
>
>

[tburkard, 2011-07-06 19:59:02]

Hi Dominic,

some of the methods that you would like to have out of PrerenderManager are
actually using prerender manager state, and the few others that don't are
directly related ot that and only really apply in the context of the
PrerenderManager.

Therefore, I think having them in prerendermanager makes more sense.

Like I said, I am willing to revisit refactoring the histogram stuff later,
but I wanted to keep the histogram stuff in PrerenderManager for now as it
was before to avoid doing major refactoring & new functionality in the same
CL.

Thanks.

Timo

On Wed, Jul 6, 2011 at 10:15 AM, Timo Burkard <tburkard@chromium.org> wrote:

> Chris, what do you think?
>
> Timo
>
>
> On Thu, Jun 30, 2011 at 3:19 PM, Timo Burkard <tburkard@chromium.org>wrote:
>
>> Let's ask Chris and see what he thinks.
>>
>>
>> On Thu, Jun 30, 2011 at 3:06 PM, <dominich@chromium.org> wrote:
>>
>>> I don't buy that logic. Just because you have calls there doesn't mean
>>> everything should be in there. The UMA calls could be move out, or the
>>> macros
>>> could be in a header of another file. There's no reason for the new
>>> methods to
>>> be a member of PrerenderManager. Unless I've missed something, the only
>>> thing
>>> they access from PrerenderManager are members that you've added.
>>>
>>> It sounds like you're pretty adamant to leave it, but I think it's a
>>> mistake to
>>> introduce something that you know you want to change later when right now
>>> is the
>>> easiest time to change it.
>>>
>>>
>>> On 2011/06/30 21:54:38, tburkard wrote:
>>>
>>>> I think its messy to have histogram related things not all at one place.
>>>>  The UMA calls are in prerender_manager.cc, which is why I added these
>>>> functions to prerender_manager.c as well.
>>>>
>>>
>>>
>>>  On Thu, Jun 30, 2011 at 2:50 PM, <mailto:dominich@chromium.org> wrote:
>>>>
>>>
>>>  > On 2011/06/30 21:48:59, tburkard wrote:
>>>> >
>>>> >> I will look into turning that into a separate class once this is
>>>> >> committed.
>>>> >>
>>>> >
>>>> >  I don't want to refactor code & add new features in the same chagne.
>>>> >>
>>>> >
>>>> >
>>>> > But you'll only be refactoring the code that you're adding, no? It
>>>> seems
>>>> > pretty
>>>> > self-contained already so moving it out shouldn't be a big step. My
>>>> concern
>>>> > is
>>>> > that if it isn't done as you're adding it it is the sort of thing that
>>>> can
>>>> > slip
>>>> > down the priority list. We have a good thing going with cleaning up
>>>> > PrerenderManager and this takes it a step backwards.
>>>> >
>>>> >
>>>> >
>>>> >  On Thu, Jun 30, 2011 at 2:44 PM, <mailto:dominich@chromium.org>
>>>> wrote:
>>>> >>
>>>> >
>>>> >  >
>>>> >> > http://codereview.chromium.******org/7289020/diff/3006/chrome/**
>>>> ****
>>>> >> >
>>>> >>
>>>> >
>>>> > browser/prerender/prerender_******manager.h<http://codereview.****
>>>> >
chromium.org/7289020/diff/****3006/chrome/browser/prerender/****<http://chromium.org/7289020/diff/**3006/chrome/browser/prerender/**>
>>>> >
>>>>
>>>
>>> prerender_manager.h<http://**codereview.chromium.org/**
>>>
7289020/diff/3006/chrome/**browser/prerender/prerender_**manager.h<http://codereview.chromium.org/7289020/diff/3006/chrome/browser/prerender/prerender_manager.h>
>>> >
>>>
>>>  > >
>>>> >
>>>> >  >
>>>> >> > File chrome/browser/prerender/******prerender_manager.h (right):
>>>> >> >
>>>> >> > http://codereview.chromium.******org/7289020/diff/3006/chrome/**
>>>> ****
>>>> >> >
>>>> >>
>>>> >
>>>> > browser/prerender/prerender_******manager.h#newcode328<http://****
>>>> >
codereview.chromium.org/****7289020/diff/3006/chrome/**<http://codereview.chromium.org/**7289020/diff/3006/chrome/**>
>>>> >
>>>>
>>>
>>> browser/prerender/prerender_****manager.h#newcode328<http://**
>>> codereview.chromium.org/**7289020/diff/3006/chrome/**
>>>
browser/prerender/prerender_**manager.h#newcode328<http://codereview.chromium.org/7289020/diff/3006/chrome/browser/prerender/prerender_manager.h#newcode328>
>>> >
>>>
>>>> > >
>>>> >
>>>> >  > chrome/browser/prerender/******prerender_manager.h:328: //
>>>> Composes a
>>>> >> >
>>>> >> > histogram name based on a histogram type.
>>>> >> > Do these need to be in PrerenderManager? It seems they should be in
>>>> a
>>>> >> > prerender_histogram file in the top-level prerender namespace, or
>>>> at
>>>> >> > least in a class to track the last experiment/wash stuff.
>>>> >> >
>>>> >> >
>>>> >> >
>>>> >>
>>>> >
>>>> > http://codereview.chromium.******org/7289020/%253Chttp://**coderevi**
>>>>
>>>> > ew.chromium.org/7289020/
<http://codereview.chromium.**org/7289020/<http://codereview.chromium.org/7289...
>>>> >>
>>>> >
>>>> >  >
>>>> >>
>>>> >
>>>> >
>>>> >
>>>> >
>>>>
>>>
>>> http://codereview.chromium.****org/7289020/%3Chttp://coderevi**
>>> ew.chromium.org/7289020/ <http://codereview.chromium.org/7289020/>>
>>>
>>>> >
>>>>
>>>
>>>
>>>
>>>
http://codereview.chromium.**org/7289020/<http://codereview.chromium.org/7289...
>>>
>>
>>
>

[cbentzel, 2011-07-06 20:01:03]

I will take a more detailed look at the CL later tonight. I agree in
theory on keeping the logic in PrerenderManager if lots of state needs
to be shared between UMA and PrerenderManager internals, but haven't
looked if that's actually the case here.

On Wed, Jul 6, 2011 at 3:58 PM, Timo Burkard <tburkard@chromium.org> wrote:
> Hi Dominic,
> some of the methods that you would like to have out of PrerenderManager are
> actually using prerender manager state, and the few others that don't are
> directly related ot that and only really apply in the context of the
> PrerenderManager.
> Therefore, I think having them in prerendermanager makes more sense.
> Like I said, I am willing to revisit refactoring the histogram stuff later,
> but I wanted to keep the histogram stuff in PrerenderManager for now as it
> was before to avoid doing major refactoring & new functionality in the same
> CL.
> Thanks.
> Timo
> On Wed, Jul 6, 2011 at 10:15 AM, Timo Burkard <tburkard@chromium.org> wrote:
>>
>> Chris, what do you think?
>> Timo
>>
>> On Thu, Jun 30, 2011 at 3:19 PM, Timo Burkard <tburkard@chromium.org>
>> wrote:
>>>
>>> Let's ask Chris and see what he thinks.
>>>
>>> On Thu, Jun 30, 2011 at 3:06 PM, <dominich@chromium.org> wrote:
>>>>
>>>> I don't buy that logic. Just because you have calls there doesn't mean
>>>> everything should be in there. The UMA calls could be move out, or the
>>>> macros
>>>> could be in a header of another file. There's no reason for the new
>>>> methods to
>>>> be a member of PrerenderManager. Unless I've missed something, the only
>>>> thing
>>>> they access from PrerenderManager are members that you've added.
>>>>
>>>> It sounds like you're pretty adamant to leave it, but I think it's a
>>>> mistake to
>>>> introduce something that you know you want to change later when right
>>>> now is the
>>>> easiest time to change it.
>>>>
>>>> On 2011/06/30 21:54:38, tburkard wrote:
>>>>>
>>>>> I think its messy to have histogram related things not all at one
>>>>> place.
>>>>>  The UMA calls are in prerender_manager.cc, which is why I added these
>>>>> functions to prerender_manager.c as well.
>>>>
>>>>
>>>>> On Thu, Jun 30, 2011 at 2:50 PM, <mailto:dominich@chromium.org> wrote:
>>>>
>>>>> > On 2011/06/30 21:48:59, tburkard wrote:
>>>>> >
>>>>> >> I will look into turning that into a separate class once this is
>>>>> >> committed.
>>>>> >>
>>>>> >
>>>>> >  I don't want to refactor code & add new features in the same chagne.
>>>>> >>
>>>>> >
>>>>> >
>>>>> > But you'll only be refactoring the code that you're adding, no? It
>>>>> > seems
>>>>> > pretty
>>>>> > self-contained already so moving it out shouldn't be a big step. My
>>>>> > concern
>>>>> > is
>>>>> > that if it isn't done as you're adding it it is the sort of thing
>>>>> > that can
>>>>> > slip
>>>>> > down the priority list. We have a good thing going with cleaning up
>>>>> > PrerenderManager and this takes it a step backwards.
>>>>> >
>>>>> >
>>>>> >
>>>>> >  On Thu, Jun 30, 2011 at 2:44 PM, <mailto:dominich@chromium.org>
>>>>> > wrote:
>>>>> >>
>>>>> >
>>>>> >  >
>>>>> >> > http://codereview.chromium.****org/7289020/diff/3006/chrome/****
>>>>> >> >
>>>>> >>
>>>>> >
>>>>> > browser/prerender/prerender_****manager.h<http://codereview.**
>>>>> > chromium.org/7289020/diff/**3006/chrome/browser/prerender/**
>>>>> >
>>>>
>>>>
>>>>
prerender_manager.h<http://codereview.chromium.org/7289020/diff/3006/chrome/browser/prerender/prerender_manager.h>
>>>>>
>>>>> > >
>>>>> >
>>>>> >  >
>>>>> >> > File chrome/browser/prerender/****prerender_manager.h (right):
>>>>> >> >
>>>>> >> > http://codereview.chromium.****org/7289020/diff/3006/chrome/****
>>>>> >> >
>>>>> >>
>>>>> >
>>>>> > browser/prerender/prerender_****manager.h#newcode328<http://**
>>>>> > codereview.chromium.org/**7289020/diff/3006/chrome/**
>>>>> >
>>>>
>>>>
>>>>
browser/prerender/prerender_**manager.h#newcode328<http://codereview.chromium.org/7289020/diff/3006/chrome/browser/prerender/prerender_manager.h#newcode328>
>>>>>
>>>>> > >
>>>>> >
>>>>> >  > chrome/browser/prerender/****prerender_manager.h:328: // Composes
>>>>> > a
>>>>> >> >
>>>>> >> > histogram name based on a histogram type.
>>>>> >> > Do these need to be in PrerenderManager? It seems they should be
>>>>> >> > in a
>>>>> >> > prerender_histogram file in the top-level prerender namespace, or
>>>>> >> > at
>>>>> >> > least in a class to track the last experiment/wash stuff.
>>>>> >> >
>>>>> >> >
>>>>> >> >
>>>>> >>
>>>>> >
>>>>> > http://codereview.chromium.****org/7289020/%253Chttp://coderevi**
>>>>> > ew.chromium.org/7289020/ <http://codereview.chromium.org/7289020/>>
>>>>> >
>>>>> >  >
>>>>> >>
>>>>> >
>>>>> >
>>>>> >
>>>>> >
>>>>
>>>>
>>>>
http://codereview.chromium.**org/7289020/%3Chttp://codereview.chromium.org/72...>
>>>>>
>>>>> >
>>>>
>>>>
>>>>
>>>> http://codereview.chromium.org/7289020/
>>>
>>
>
>

[tburkard, 2011-07-07 20:45:31]

Dominic, per our discussion, can you lgtm this now?
Thanks.

Timo

On Wed, Jul 6, 2011 at 1:00 PM, Chris Bentzel <cbentzel@chromium.org> wrote:

> I will take a more detailed look at the CL later tonight. I agree in
> theory on keeping the logic in PrerenderManager if lots of state needs
> to be shared between UMA and PrerenderManager internals, but haven't
> looked if that's actually the case here.
>
> On Wed, Jul 6, 2011 at 3:58 PM, Timo Burkard <tburkard@chromium.org>
> wrote:
> > Hi Dominic,
> > some of the methods that you would like to have out of PrerenderManager
> are
> > actually using prerender manager state, and the few others that don't are
> > directly related ot that and only really apply in the context of the
> > PrerenderManager.
> > Therefore, I think having them in prerendermanager makes more sense.
> > Like I said, I am willing to revisit refactoring the histogram stuff
> later,
> > but I wanted to keep the histogram stuff in PrerenderManager for now as
> it
> > was before to avoid doing major refactoring & new functionality in the
> same
> > CL.
> > Thanks.
> > Timo
> > On Wed, Jul 6, 2011 at 10:15 AM, Timo Burkard <tburkard@chromium.org>
> wrote:
> >>
> >> Chris, what do you think?
> >> Timo
> >>
> >> On Thu, Jun 30, 2011 at 3:19 PM, Timo Burkard <tburkard@chromium.org>
> >> wrote:
> >>>
> >>> Let's ask Chris and see what he thinks.
> >>>
> >>> On Thu, Jun 30, 2011 at 3:06 PM, <dominich@chromium.org> wrote:
> >>>>
> >>>> I don't buy that logic. Just because you have calls there doesn't mean
> >>>> everything should be in there. The UMA calls could be move out, or the
> >>>> macros
> >>>> could be in a header of another file. There's no reason for the new
> >>>> methods to
> >>>> be a member of PrerenderManager. Unless I've missed something, the
> only
> >>>> thing
> >>>> they access from PrerenderManager are members that you've added.
> >>>>
> >>>> It sounds like you're pretty adamant to leave it, but I think it's a
> >>>> mistake to
> >>>> introduce something that you know you want to change later when right
> >>>> now is the
> >>>> easiest time to change it.
> >>>>
> >>>> On 2011/06/30 21:54:38, tburkard wrote:
> >>>>>
> >>>>> I think its messy to have histogram related things not all at one
> >>>>> place.
> >>>>>  The UMA calls are in prerender_manager.cc, which is why I added
> these
> >>>>> functions to prerender_manager.c as well.
> >>>>
> >>>>
> >>>>> On Thu, Jun 30, 2011 at 2:50 PM, <mailto:dominich@chromium.org>
> wrote:
> >>>>
> >>>>> > On 2011/06/30 21:48:59, tburkard wrote:
> >>>>> >
> >>>>> >> I will look into turning that into a separate class once this is
> >>>>> >> committed.
> >>>>> >>
> >>>>> >
> >>>>> >  I don't want to refactor code & add new features in the same
> chagne.
> >>>>> >>
> >>>>> >
> >>>>> >
> >>>>> > But you'll only be refactoring the code that you're adding, no? It
> >>>>> > seems
> >>>>> > pretty
> >>>>> > self-contained already so moving it out shouldn't be a big step. My
> >>>>> > concern
> >>>>> > is
> >>>>> > that if it isn't done as you're adding it it is the sort of thing
> >>>>> > that can
> >>>>> > slip
> >>>>> > down the priority list. We have a good thing going with cleaning up
> >>>>> > PrerenderManager and this takes it a step backwards.
> >>>>> >
> >>>>> >
> >>>>> >
> >>>>> >  On Thu, Jun 30, 2011 at 2:44 PM, <mailto:dominich@chromium.org>
> >>>>> > wrote:
> >>>>> >>
> >>>>> >
> >>>>> >  >
> >>>>> >> > http://codereview.chromium.
> ****org/7289020/diff/3006/chrome/****
> >>>>> >> >
> >>>>> >>
> >>>>> >
> >>>>> > browser/prerender/prerender_****manager.h<http://codereview.**
> >>>>> > chromium.org/7289020/diff/**3006/chrome/browser/prerender/**
> >>>>> >
> >>>>
> >>>>
> >>>> prerender_manager.h<
>
http://codereview.chromium.org/7289020/diff/3006/chrome/browser/prerender/pre...
> >
> >>>>>
> >>>>> > >
> >>>>> >
> >>>>> >  >
> >>>>> >> > File chrome/browser/prerender/****prerender_manager.h (right):
> >>>>> >> >
> >>>>> >> > http://codereview.chromium.
> ****org/7289020/diff/3006/chrome/****
> >>>>> >> >
> >>>>> >>
> >>>>> >
> >>>>> > browser/prerender/prerender_****manager.h#newcode328<http://**
> >>>>> > codereview.chromium.org/**7289020/diff/3006/chrome/**
> >>>>> >
> >>>>
> >>>>
> >>>> browser/prerender/prerender_**manager.h#newcode328<
>
http://codereview.chromium.org/7289020/diff/3006/chrome/browser/prerender/pre...
> >
> >>>>>
> >>>>> > >
> >>>>> >
> >>>>> >  > chrome/browser/prerender/****prerender_manager.h:328: //
> Composes
> >>>>> > a
> >>>>> >> >
> >>>>> >> > histogram name based on a histogram type.
> >>>>> >> > Do these need to be in PrerenderManager? It seems they should be
> >>>>> >> > in a
> >>>>> >> > prerender_histogram file in the top-level prerender namespace,
> or
> >>>>> >> > at
> >>>>> >> > least in a class to track the last experiment/wash stuff.
> >>>>> >> >
> >>>>> >> >
> >>>>> >> >
> >>>>> >>
> >>>>> >
> >>>>> > http://codereview.chromium.****org/7289020/%253Chttp://coderevi**
> >>>>> > ew.chromium.org/7289020/ <http://codereview.chromium.org/7289020/
> >>
> >>>>> >
> >>>>> >  >
> >>>>> >>
> >>>>> >
> >>>>> >
> >>>>> >
> >>>>> >
> >>>>
> >>>>
> >>>> http://codereview.chromium.**org/7289020/%3Chttp://
> codereview.chromium.org/7289020/>
> >>>>>
> >>>>> >
> >>>>
> >>>>
> >>>>
> >>>> http://codereview.chromium.org/7289020/
> >>>
> >>
> >
> >
>

[dominich, 2011-07-07 20:48:03]

LGTM