Clean up Prerender field trials.

Clean up Prerender field trials.

Remove the second comparison group from the prerender field trials, and reorganize the code so that we can compile assert on the summation. The second group has already shown us our numbers are good. The compile time assertion should avoid fat finger errors that we've caught in other reviews (which would otherwise have been a _beta crasher_!).

I think we should also give consideration to renaming these histograms for ease of understanding. I know that makes a historic comparison cost, but it makes arriving new in this area so much easier.

I'll rebase the multiple prerender CL on top of this.

R=dominich@chromium.org
BUG=None

TEST=


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

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

[gavinp, 2012-07-22 17:03:32]

Dominic, WDYT?

[dominich, 2012-07-23 15:04:35]

LGTM

Don't forget to change the histograms.xml file. I'm not sure if you want to add
new entries and keep the old ones marked deprecated, or replace the old entries
entirely. We can always go to dremel for access if we need to.

I suggest also getting buy-in from other Prerender team members. I completely
support this change but I think others may be more conservative.

[gavinp, 2012-07-23 15:11:36]

On 2012/07/23 15:04:35, dominich wrote:
> LGTM
> 
> Don't forget to change the histograms.xml file. I'm not sure if you want to
add
> new entries and keep the old ones marked deprecated, or replace the old
entries
> entirely. We can always go to dremel for access if we need to.

There's no new entries to add. An abundance of conservatism led me to keep these
all named as for field trial one. I'd like to rename all of these, particularly
to just put everything under Prerender.* for simplicity.

> 
> I suggest also getting buy-in from other Prerender team members. I completely
> support this change but I think others may be more conservative.

I'll email the list rather than add reviewers, and hold off on landing this
until fair notice is past.

[dominich, 2012-07-23 16:05:44]

On Mon, Jul 23, 2012 at 8:11 AM, <gavinp@chromium.org> wrote:

> On 2012/07/23 15:04:35, dominich wrote:
>
>> LGTM
>>
>
>  Don't forget to change the histograms.xml file. I'm not sure if you want
>> to
>>
> add
>
>> new entries and keep the old ones marked deprecated, or replace the old
>>
> entries
>
>> entirely. We can always go to dremel for access if we need to.
>>
>
> There's no new entries to add. An abundance of conservatism led me to keep
> these
> all named as for field trial one. I'd like to rename all of these,
> particularly
> to just put everything under Prerender.* for simplicity.


Ah, but you do could remove the *2 entries from the histograms.xml to be a
good citizen.


>
>
>
>  I suggest also getting buy-in from other Prerender team members. I
>> completely
>> support this change but I think others may be more conservative.
>>
>
> I'll email the list rather than add reviewers, and hold off on landing this
> until fair notice is past.
>
>
>
>
>
http://codereview.chromium.**org/10802062/<http://codereview.chromium.org/108...
>

[gavinp, 2012-07-23 16:49:55]

On 2012/07/23 16:05:44, dominich wrote:
> On Mon, Jul 23, 2012 at 8:11 AM, <mailto:gavinp@chromium.org> wrote:
> 
> > On 2012/07/23 15:04:35, dominich wrote:
> >
> >> LGTM
> >>
> >
> >  Don't forget to change the histograms.xml file. I'm not sure if you want
> >> to
> >>
> > add
> >
> >> new entries and keep the old ones marked deprecated, or replace the old
> >>
> > entries
> >
> >> entirely. We can always go to dremel for access if we need to.
> >>
> >
> > There's no new entries to add. An abundance of conservatism led me to keep
> > these
> > all named as for field trial one. I'd like to rename all of these,
> > particularly
> > to just put everything under Prerender.* for simplicity.
> 
> 
> Ah, but you do could remove the *2 entries from the histograms.xml to be a
> good citizen.
> 

I just did another upload, and I got more aggressive: I actually renamed them,
based on support in the thread in the mailing list. Could you take another peek
and LGTM this if you're OK with it? I'll let it sit a while before CQing so
other prerendering concerned people can comment.

I'll edit histograms.xml after this lands.

> 
> >
> >
> >
> >  I suggest also getting buy-in from other Prerender team members. I
> >> completely
> >> support this change but I think others may be more conservative.
> >>
> >
> > I'll email the list rather than add reviewers, and hold off on landing this
> > until fair notice is past.
> >
> >
> >
> >
> >
>
http://codereview.chromium.**org/10802062/%3Chttp://codereview.chromium.org/1...>
> >

[dominich, 2012-07-23 16:56:41]

LGTM modulo minor renaming suggestions

http://codereview.chromium.org/10802062/diff/4002/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_field_trial.cc (right):

http://codereview.chromium.org/10802062/diff/4002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_field_trial.cc:98: "Prerender", divisor,
"PrerenderOn",
PrerenderEnabled?

http://codereview.chromium.org/10802062/diff/4002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_field_trial.cc:101:
trial->AppendGroup("PrerenderControlGroup",
drop the Group. Just PrerenderControl. It's cleaner.

http://codereview.chromium.org/10802062/diff/4002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_field_trial.cc:111: if (trial_group ==
prerender_on_group) {
switch statement

[gavinp, 2012-07-23 17:01:04]

http://codereview.chromium.org/10802062/diff/4002/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_field_trial.cc (right):

http://codereview.chromium.org/10802062/diff/4002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_field_trial.cc:98: "Prerender", divisor,
"PrerenderOn",
On 2012/07/23 16:56:41, dominich wrote:
> PrerenderEnabled?

Done.

http://codereview.chromium.org/10802062/diff/4002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_field_trial.cc:101:
trial->AppendGroup("PrerenderControlGroup",
On 2012/07/23 16:56:41, dominich wrote:
> drop the Group. Just PrerenderControl. It's cleaner.

Done.

http://codereview.chromium.org/10802062/diff/4002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_field_trial.cc:111: if (trial_group ==
prerender_on_group) {
On 2012/07/23 16:56:41, dominich wrote:
> switch statement

No can do without some ungainly plumbing, since case statements take only
constant expressions.

[dominich, 2012-07-23 17:10:04]

lgtm

http://codereview.chromium.org/10802062/diff/12001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_field_trial.cc (right):

http://codereview.chromium.org/10802062/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_field_trial.cc:99: 2013, 6, 30,
&prerender_on_group));
nit: prerender_enabled_group.

[commit-bot: I haz the power, 2012-07-26 21:21:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/10802062/14001

[gavinp, 2012-07-26 21:55:21]

http://codereview.chromium.org/10802062/diff/12001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_field_trial.cc (right):

http://codereview.chromium.org/10802062/diff/12001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_field_trial.cc:99: 2013, 6, 30,
&prerender_on_group));
On 2012/07/23 17:10:05, dominich wrote:
> nit: prerender_enabled_group.

Done.

[commit-bot: I haz the power, 2012-07-26 22:45:00]

Change committed as 148639

[commit-bot: I haz the power, 2012-07-26 23:28:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/10802062/25001

[gavinp, 2012-07-26 23:31:47]

On 2012/07/26 22:45:00, I haz the power (commit-bot) wrote:
> Change committed as 148639

Removed a top level unreferenced variable; the per channel versions still use it
in their DCHECK.

[commit-bot: I haz the power, 2012-07-27 09:06:25]

Try job failure for 10802062-25001 (retry) on linux_rel for steps
"interactive_ui_tests, browser_tests".
It's a second try, previously, steps "interactive_ui_tests, browser_tests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-07-27 09:10:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/10802062/25001

[commit-bot: I haz the power, 2012-07-27 11:21:32]

Change committed as 148743