Refactor DNS A/B experient, and add test of congestion time limits...

Refactor DNS A/B experient, and add test of congestion time limits

I added another option in the DNS experiment: user 2 seconds or 500ms
for the congestion limit (that causes pre-resolutions to be discarded
from the queue.

The code in browser_main.cc was getting toooo large, so I pulled all
the experiment code into dns_global.cc.

BUG=15479
r=eroman

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

[eroman, 2009-06-26 22:02:14]

LGTM

Good cleanup!

Just be sure to read comment
<http://codereview.chromium.org/147215/diff/1032/36#newcode392> since that looks
like an unintentional change.

http://codereview.chromium.org/147215/diff/1032/38
File chrome/browser/net/dns_global.cc (right):

http://codereview.chromium.org/147215/diff/1032/38#newcode285
Line 285: DCHECK_EQ(startup_list->GetSize(), 0u);
reverse the parameter order (expected first, then actual).

http://codereview.chromium.org/147215/diff/1032/38#newcode570
Line 570: FieldTrial::Probability k_PROBABILITY_PER_GROUP = 10;  // 10%
probability.
i am not sure about this naming style... i believe for a constant you want:
kProbabilityPerGroup

http://codereview.chromium.org/147215/diff/1032/38#newcode574
Line 574: // First option is to disable prefetching completele.
typo: completely

http://codereview.chromium.org/147215/diff/1032/39
File chrome/browser/net/dns_global.h (right):

http://codereview.chromium.org/147215/diff/1032/39#newcode87
Line 87: DnsPrefetcherInit(PrefService* user_prefs, PrefService* local_state);
Can you re-order these further up? (methods should be ordered before data
members).

See
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...>

http://codereview.chromium.org/147215/diff/1032/36
File chrome/browser/net/dns_master.cc (right):

http://codereview.chromium.org/147215/diff/1032/36#newcode392
Line 392: DCHECK_EQ(0u, hostname.length());
I believe you want the opposite direction:

DCHECK_NE(0U, hostname.length().

http://codereview.chromium.org/147215/diff/1032/35
File chrome/browser/net/dns_master_unittest.cc (right):

http://codereview.chromium.org/147215/diff/1032/35#newcode68
Line 68: static TimeDelta kMaxQueueingDelay;
move data members after methods:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

[eroman, 2009-06-26 22:14:34]

Ok, so I was wrong about the declaration order suggestion (in this case you are
right to put the data member before ctor since it is a constant).

However my follow-up concern is, I don't believe the style guide permits statics
that are not plain old data. So instead of having this constant be a TimeDelta,
it probably needs to be a size_t.

[jar (doing other things), 2009-06-26 22:18:12]

Changes made per your corrections.

Thanks!

http://codereview.chromium.org/147215/diff/1032/38
File chrome/browser/net/dns_global.cc (right):

http://codereview.chromium.org/147215/diff/1032/38#newcode285
Line 285: DCHECK_EQ(startup_list->GetSize(), 0u);
On 2009/06/26 22:02:16, eroman wrote:
> reverse the parameter order (expected first, then actual).
> 

Done.

http://codereview.chromium.org/147215/diff/1032/38#newcode570
Line 570: FieldTrial::Probability k_PROBABILITY_PER_GROUP = 10;  // 10%
probability.
On 2009/06/26 22:02:16, eroman wrote:
> i am not sure about this naming style... i believe for a constant you want:
> kProbabilityPerGroup

Done.

http://codereview.chromium.org/147215/diff/1032/38#newcode574
Line 574: // First option is to disable prefetching completele.
On 2009/06/26 22:02:16, eroman wrote:
> typo: completely

Done.

http://codereview.chromium.org/147215/diff/1032/39
File chrome/browser/net/dns_global.h (right):

http://codereview.chromium.org/147215/diff/1032/39#newcode87
Line 87: DnsPrefetcherInit(PrefService* user_prefs, PrefService* local_state);
Constants get to come first.

On 2009/06/26 22:02:16, eroman wrote:
> Can you re-order these further up? (methods should be ordered before data
> members).
> 
> See
>
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...>

http://codereview.chromium.org/147215/diff/1032/36
File chrome/browser/net/dns_master.cc (right):

http://codereview.chromium.org/147215/diff/1032/36#newcode392
Line 392: DCHECK_EQ(0u, hostname.length());
On 2009/06/26 22:02:16, eroman wrote:
> I believe you want the opposite direction:
> 
> DCHECK_NE(0U, hostname.length().

Done.

http://codereview.chromium.org/147215/diff/1032/35
File chrome/browser/net/dns_master_unittest.cc (right):

http://codereview.chromium.org/147215/diff/1032/35#newcode68
Line 68: static TimeDelta kMaxQueueingDelay;
They get special dispensation for being constants.

On 2009/06/26 22:02:16, eroman wrote:
> move data members after methods:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

[eroman, 2009-06-26 22:21:52]

lgtm