Create A/B test of SDCH...

Create A/B test of SDCH

To do this, I needed to add the feature that ALL FieldTrials that are
established in the browser process are forwarded and established in
the corresponding renderer processes.  This then allows both DNS impact,
 as well as SDCH inmpact (and any other field tests) to be studied
at the same time in a single binary.

This checkin also establishes a pattern that when we're doing A/B tests
via a histogram such as RequestToFinish, that we produce names for
all groups, rather than leaving one group as the "default" or "empty postfix"
group.  This is critical for naming various sub-groups when a multitude
of tests are taking place at the same time.

BUG=15479
r=mbelshe

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

[jar (doing other things), 2009-06-30 03:34:53]

Dave: Can you take a look at what I did for the renderer histograms?  Since I
now support multiple tests, I can't just leave an "untouched" histogram to show
the default impact.  Instead, now I have a "aggregate" histogram (which covers
all possibilities), as well as several histograms, each of which divides the
samples differently depending on the trial.

Mike: Can you look at the code that passes settings established in the browser
to all renderer instances. There are new unit tests to cover the persisting of
the state, which is passed down as an argument to the renderer.

I refactored the placement of the DNS trial, as it was just getting tooooo large
in the browser_main file.

For the new SDCH trial, it is pretty compact, and so I didn't factor it out.

There is some chance that I'll be able to land this code tomorrow... and get
results this week from the dev build.

Thanks in advance for swift reviews... but if you guys think this needs a lot of
work/discussion, then we should take our time.

Thanks,

Jim

[DaveMoore, 2009-06-30 15:07:58]

http://codereview.chromium.org/150087/diff/92/1066
File chrome/browser/net/dns_global.cc (right):

http://codereview.chromium.org/150087/diff/92/1066#newcode228
Line 228: base::Time start_acrual(base::Time::Now());
acrual->accrual

http://codereview.chromium.org/150087/diff/92/1066#newcode246
Line 246: base::Time::Now() - start_acrual);
Acrual_Delay->Accrual_Dealy

http://codereview.chromium.org/150087/diff/92/1070
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/150087/diff/92/1070#newcode2909
Line 2909: DCHECK(FieldTrialList::Find("GlobalSdch") &&
Since it's important that the trials have the same values you should probably
factor out finish - begin, TimeDelta::FromMilliseconds(10),
TimeDelta::FromMinutes(10) and 100 into variables and pass those variables to
the 3 calls to UMA_HISTOGRAM_CUSTOM_TIMES.

http://codereview.chromium.org/150087/diff/92/1058
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/150087/diff/92/1058#newcode1104
Line 1104: }
What is "silly_counter?"

[Mike Belshe, 2009-06-30 16:04:33]

http://codereview.chromium.org/150087/diff/1084/99
File base/field_trial.h (right):

http://codereview.chromium.org/150087/diff/1084/99#newcode175
Line 175: // resurection in another process.  This allows randomization to be
done in
sp: resurrection

http://codereview.chromium.org/150087/diff/1084/101
File base/tracked.h (right):

http://codereview.chromium.org/150087/diff/1084/101#newcode25
Line 25: #define TRACK_ALL_TASK_OBJECTS  // Force object tracking in production
for Jar......
I think this is accidental?

http://codereview.chromium.org/150087/diff/1084/105
File chrome/browser/net/dns_global.cc (right):

http://codereview.chromium.org/150087/diff/1084/105#newcode228
Line 228: base::Time start_accrual(base::Time::Now());
As per discussion, may not be right?

http://codereview.chromium.org/150087/diff/1084/104
File chrome/browser/net/dns_master.h (right):

http://codereview.chromium.org/150087/diff/1084/104#newcode102
Line 102: size_t max_concurrent_lookups() const { return
max_concurrent_lookups_; }
Need comment

http://codereview.chromium.org/150087/diff/1084/97
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/150087/diff/1084/97#newcode1095
Line 1095: if (!reused_socket_) {
accidental file inclusion?

[jar (doing other things), 2009-06-30 16:12:55]

http://codereview.chromium.org/150087/diff/92/1066
File chrome/browser/net/dns_global.cc (right):

http://codereview.chromium.org/150087/diff/92/1066#newcode228
Line 228: base::Time start_acrual(base::Time::Now());
On 2009/06/30 15:07:58, davemoore wrote:
> acrual->accrual

Done.

http://codereview.chromium.org/150087/diff/92/1066#newcode246
Line 246: base::Time::Now() - start_acrual);
On 2009/06/30 15:07:58, davemoore wrote:
> Acrual_Delay->Accrual_Dealy

Changing typos in histogram names causes numbers to not be as obviously
comparable... so I'd prefer not to fix this typo (until I evolve to another
version of the histogram etc.)

http://codereview.chromium.org/150087/diff/92/1070
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/150087/diff/92/1070#newcode2909
Line 2909: DCHECK(FieldTrialList::Find("GlobalSdch") &&
On 2009/06/30 15:07:58, davemoore wrote:
> Since it's important that the trials have the same values you should probably
> factor out finish - begin, TimeDelta::FromMilliseconds(10),
> TimeDelta::FromMinutes(10) and 100 into variables and pass those variables to
> the 3 calls to UMA_HISTOGRAM_CUSTOM_TIMES.

Done.

http://codereview.chromium.org/150087/diff/92/1058
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/150087/diff/92/1058#newcode1104
Line 1104: }
On 2009/06/30 15:07:58, davemoore wrote:
> What is "silly_counter?"

This file should not have been included :-(

I removed it.

http://codereview.chromium.org/150087/diff/1084/99
File base/field_trial.h (right):

http://codereview.chromium.org/150087/diff/1084/99#newcode175
Line 175: // resurection in another process.  This allows randomization to be
done in
On 2009/06/30 16:04:33, Mike Belshe wrote:
> sp: resurrection

Done.

http://codereview.chromium.org/150087/diff/1084/101
File base/tracked.h (right):

http://codereview.chromium.org/150087/diff/1084/101#newcode25
Line 25: #define TRACK_ALL_TASK_OBJECTS  // Force object tracking in production
for Jar......
On 2009/06/30 16:04:33, Mike Belshe wrote:
> I think this is accidental?

File removed.

http://codereview.chromium.org/150087/diff/1084/105
File chrome/browser/net/dns_global.cc (right):

http://codereview.chromium.org/150087/diff/1084/105#newcode228
Line 228: base::Time start_accrual(base::Time::Now());
On 2009/06/30 16:04:33, Mike Belshe wrote:
> As per discussion, may not be right?

Lines 227-229, and 245-246 removed (part of another CL)

http://codereview.chromium.org/150087/diff/1084/104
File chrome/browser/net/dns_master.h (right):

http://codereview.chromium.org/150087/diff/1084/104#newcode102
Line 102: size_t max_concurrent_lookups() const { return
max_concurrent_lookups_; }
On 2009/06/30 16:04:33, Mike Belshe wrote:
> Need comment

Done.

http://codereview.chromium.org/150087/diff/1084/97
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/150087/diff/1084/97#newcode1095
Line 1095: if (!reused_socket_) {
On 2009/06/30 16:04:33, Mike Belshe wrote:
> accidental file inclusion?

Done.

[jar (doing other things), 2009-06-30 16:15:36]

http://codereview.chromium.org/150087/diff/92/1066
File chrome/browser/net/dns_global.cc (right):

http://codereview.chromium.org/150087/diff/92/1066#newcode228
Line 228: base::Time start_acrual(base::Time::Now());
All of this (227-229 and 246-246) was deleted... and should not have been in
this CL.  Hence spelling errors are gone.


On 2009/06/30 16:12:56, jar wrote:
> On 2009/06/30 15:07:58, davemoore wrote:
> > acrual->accrual
> 
> Done.

[Mike Belshe, 2009-06-30 16:30:13]

lgtm again