Move AsyncDns field trial to server-side control.

Move AsyncDns field trial to server-side control.

BUG=143454
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=164375

[SteveT, 2012-10-25 03:00:27]

lgtm

FYI I am OOO tomorrow so please direct reviews to asvitkine@, if there are any
more. Thanks!

http://codereview.chromium.org/11262018/diff/1/chrome/browser/net/async_dns_f...
File chrome/browser/net/async_dns_field_trial.cc (right):

http://codereview.chromium.org/11262018/diff/1/chrome/browser/net/async_dns_f...
chrome/browser/net/async_dns_field_trial.cc:8: #include
"base/memory/ref_counted.h"
nit: Header out of order?

http://codereview.chromium.org/11262018/diff/1/chrome/browser/net/async_dns_f...
chrome/browser/net/async_dns_field_trial.cc:12: 
check: BTW the header comment is still applies in the exact same way, yeah?

http://codereview.chromium.org/11262018/diff/1/chrome/browser/net/async_dns_f...
chrome/browser/net/async_dns_field_trial.cc:22: 
nit: Maybe have a comment here on how the two choices are AsyncDnsA and
AsyncDnsB so this is a little less voodoo-looking.

[szym, 2012-10-25 03:03:13]

http://codereview.chromium.org/11262018/diff/1/chrome/browser/net/async_dns_f...
File chrome/browser/net/async_dns_field_trial.cc (right):

http://codereview.chromium.org/11262018/diff/1/chrome/browser/net/async_dns_f...
chrome/browser/net/async_dns_field_trial.cc:12: 
On 2012/10/25 03:00:27, SteveT wrote:
> check: BTW the header comment is still applies in the exact same way, yeah?

Thanks for catching that. The header comment is 2 CLs out of date.

[Alexei Svitkine (slow), 2012-10-25 14:37:46]

http://codereview.chromium.org/11262018/diff/1/chrome/browser/net/async_dns_f...
File chrome/browser/net/async_dns_field_trial.cc (right):

http://codereview.chromium.org/11262018/diff/1/chrome/browser/net/async_dns_f...
chrome/browser/net/async_dns_field_trial.cc:18: scoped_refptr<base::FieldTrial>
trial(base::FieldTrialList::Find("AsyncDns"));
You could simplify all of this by just getting the group name via:

base::FieldTrialList::FindFullName("AsyncDns")

If the trial doesn't exist, it will return an empty string.

[szym, 2012-10-25 17:17:45]

cbentzel: please approve

http://codereview.chromium.org/11262018/diff/1/chrome/browser/net/async_dns_f...
File chrome/browser/net/async_dns_field_trial.cc (right):

http://codereview.chromium.org/11262018/diff/1/chrome/browser/net/async_dns_f...
chrome/browser/net/async_dns_field_trial.cc:8: #include
"base/memory/ref_counted.h"
On 2012/10/25 03:00:27, SteveT wrote:
> nit: Header out of order?

Done.

http://codereview.chromium.org/11262018/diff/1/chrome/browser/net/async_dns_f...
chrome/browser/net/async_dns_field_trial.cc:12: 
On 2012/10/25 03:03:13, szym wrote:
> On 2012/10/25 03:00:27, SteveT wrote:
> > check: BTW the header comment is still applies in the exact same way, yeah?
> 
> Thanks for catching that. The header comment is 2 CLs out of date.

Done.

http://codereview.chromium.org/11262018/diff/1/chrome/browser/net/async_dns_f...
chrome/browser/net/async_dns_field_trial.cc:18: scoped_refptr<base::FieldTrial>
trial(base::FieldTrialList::Find("AsyncDns"));
On 2012/10/25 14:37:46, Alexei Svitkine wrote:
> You could simplify all of this by just getting the group name via:
> 
> base::FieldTrialList::FindFullName("AsyncDns")
> 
> If the trial doesn't exist, it will return an empty string.
> 

Done.

http://codereview.chromium.org/11262018/diff/1/chrome/browser/net/async_dns_f...
chrome/browser/net/async_dns_field_trial.cc:22: 
On 2012/10/25 03:00:27, SteveT wrote:
> nit: Maybe have a comment here on how the two choices are AsyncDnsA and
> AsyncDnsB so this is a little less voodoo-looking.

Done.

[Alexei Svitkine (slow), 2012-10-25 17:34:41]

http://codereview.chromium.org/11262018/diff/9001/chrome/browser/net/async_dn...
File chrome/browser/net/async_dns_field_trial.cc (right):

http://codereview.chromium.org/11262018/diff/9001/chrome/browser/net/async_dn...
chrome/browser/net/async_dns_field_trial.cc:20: return
base::FieldTrialList::FindFullName("AsyncDns").compare(
Nit: Use StartsWith() from base/string_util.h.

[Alexei Svitkine (slow), 2012-10-25 17:35:11]

http://codereview.chromium.org/11262018/diff/9001/chrome/browser/net/async_dn...
File chrome/browser/net/async_dns_field_trial.cc (right):

http://codereview.chromium.org/11262018/diff/9001/chrome/browser/net/async_dn...
chrome/browser/net/async_dns_field_trial.cc:7: #include
"base/memory/ref_counted.h"
This isn't needed anymore, I think.

[szym, 2012-10-25 17:45:15]

http://codereview.chromium.org/11262018/diff/9001/chrome/browser/net/async_dn...
File chrome/browser/net/async_dns_field_trial.cc (right):

http://codereview.chromium.org/11262018/diff/9001/chrome/browser/net/async_dn...
chrome/browser/net/async_dns_field_trial.cc:7: #include
"base/memory/ref_counted.h"
On 2012/10/25 17:35:11, Alexei Svitkine wrote:
> This isn't needed anymore, I think.

Done.

http://codereview.chromium.org/11262018/diff/9001/chrome/browser/net/async_dn...
chrome/browser/net/async_dns_field_trial.cc:20: return
base::FieldTrialList::FindFullName("AsyncDns").compare(
On 2012/10/25 17:34:41, Alexei Svitkine wrote:
> Nit: Use StartsWith() from base/string_util.h.

Oh wow! Thanks!

[Alexei Svitkine (slow), 2012-10-25 17:49:56]

LGTM

[cbentzel, 2012-10-25 20:37:16]

LGTM

Out of curiosity - when do we pick up the server-side specified field trials?
This is called during IOThread::Init. Will it be ready in time?

http://codereview.chromium.org/11262018/diff/11002/chrome/browser/net/async_d...
File chrome/browser/net/async_dns_field_trial.h (right):

http://codereview.chromium.org/11262018/diff/11002/chrome/browser/net/async_d...
chrome/browser/net/async_dns_field_trial.h:10: // Returns true if should set
|HostResolver::Options::enable_async| to true.
This comment is a bit unclear.

Perhaps

Returns true when the async resolver should be used.

The groups that are assigned to it feel more like an implementation issue rather
than part of the interface.

[Alexei Svitkine (slow), 2012-10-25 20:40:38]

On 2012/10/25 20:37:16, cbentzel wrote:
> LGTM
> 
> Out of curiosity - when do we pick up the server-side specified field trials?
> This is called during IOThread::Init. Will it be ready in time?

Yes. Server-side field trials are saved in prefs and applied (on the next
browser start) in ChromeBrowserMainParts::SetupMetricsAndFieldTrials() which
happens before the IOThread is created.

[szym, 2012-10-25 20:41:39]

On 2012/10/25 20:37:16, cbentzel wrote:
> LGTM
> 
> Out of curiosity - when do we pick up the server-side specified field trials?
> This is called during IOThread::Init. Will it be ready in time?

I asked the same question in CL/36365061
stevet says: "this configuration is fetched on startup, but won't take effect
until after the next startup... so on first run, you will run without the
config."

> 
>
http://codereview.chromium.org/11262018/diff/11002/chrome/browser/net/async_d...
> File chrome/browser/net/async_dns_field_trial.h (right):
> 
>
http://codereview.chromium.org/11262018/diff/11002/chrome/browser/net/async_d...
> chrome/browser/net/async_dns_field_trial.h:10: // Returns true if should set
> |HostResolver::Options::enable_async| to true.
> This comment is a bit unclear.
> 
> Perhaps
> 
> Returns true when the async resolver should be used.

Done.

> 
> The groups that are assigned to it feel more like an implementation issue
rather
> than part of the interface.

I moved the comment to implementation.

[commit-bot: I haz the power, 2012-10-25 20:51:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/11262018/12011

[commit-bot: I haz the power, 2012-10-26 18:29:14]

Commit queue patch verification failed without an error message.
Something went wrong, probably a crash, a hickup or simply
the monkeys went out for dinner.
Ping the relevant dude on a on-needed basis.