[net] Change factory methods for HostResolver and HostCache to return a scoped_ptr

[net] Change factory methods for HostResolver and HostCache to return a scoped_ptr.

Move HostResolver factory methods to host_resolver.cc.

This also fixes a double-free in ShellURLRequestContextGetter.

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

[szym, 2012-08-16 16:23:44]

I have followed the pattern of Spdy field trial. I have not added
field-trial-based histogram names since our TotalTime histogram is already split
AsyncDNS/DNS. Should I also update the PLT histograms (in
page_load_histograms.cc) with AsyncDns field-trial names?

[szym, 2012-08-16 16:25:26]

On 2012/08/16 16:23:44, szym wrote:
> I have followed the pattern of Spdy field trial. I have not added
> field-trial-based histogram names since our TotalTime histogram is already
split
> AsyncDNS/DNS. Should I also update the PLT histograms (in
> page_load_histograms.cc) with AsyncDns field-trial names?

Also, let me know if you'd like to see the FieldTrial installed in a separate CL
after the factory method change lands.

[Jamie, 2012-08-16 16:40:04]

remoting/ lgtm.

[cbentzel, 2012-08-16 20:09:20]

Some quick comments. I'll do a full pass before I take off today.

http://codereview.chromium.org/10831277/diff/7003/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_field_trials.cc (right):

http://codereview.chromium.org/10831277/diff/7003/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_field_trials.cc:587: // When --enable-async-dns is
not set, users will be in A/B test for DnsClient.
Do you want to move this to a ternary (enabled/disabled/auto)? We did that for
prerender trials, and then didn't count metrics for sessions in the enabled +
disabled buckets.

http://codereview.chromium.org/10831277/diff/7003/chrome/service/net/service_...
File chrome/service/net/service_url_request_context.cc (right):

http://codereview.chromium.org/10831277/diff/7003/chrome/service/net/service_...
chrome/service/net/service_url_request_context.cc:112:
storage_.set_host_resolver(
Could |set_host_resolver| change to take a scoped_ptr argument?

[szym, 2012-08-16 20:38:54]

http://codereview.chromium.org/10831277/diff/7003/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_field_trials.cc (right):

http://codereview.chromium.org/10831277/diff/7003/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_field_trials.cc:587: // When --enable-async-dns is
not set, users will be in A/B test for DnsClient.
On 2012/08/16 20:09:21, cbentzel wrote:
> Do you want to move this to a ternary (enabled/disabled/auto)? We did that for
> prerender trials, and then didn't count metrics for sessions in the enabled +
> disabled buckets.

I'm okay with that. I'm not sure what this means for the DNS.TotalTime and
AsyncDNS.TotalTime histograms. I suspect they will be merged into
DNS.TotalTime_async/sync/default?

http://codereview.chromium.org/10831277/diff/7003/chrome/service/net/service_...
File chrome/service/net/service_url_request_context.cc (right):

http://codereview.chromium.org/10831277/diff/7003/chrome/service/net/service_...
chrome/service/net/service_url_request_context.cc:112:
storage_.set_host_resolver(
On 2012/08/16 20:09:21, cbentzel wrote:
> Could |set_host_resolver| change to take a scoped_ptr argument?

Ultimately, I'd strongly prefer that. If you can tolerate a small (and hopefully
temporary) inconsistency between set_host_resolver and all the other setters in
URLRequestContextStorage then I'd definitely do that.

[cbentzel, 2012-08-16 21:20:26]

http://codereview.chromium.org/10831277/diff/7003/chrome/service/net/service_...
File chrome/service/net/service_url_request_context.cc (right):

http://codereview.chromium.org/10831277/diff/7003/chrome/service/net/service_...
chrome/service/net/service_url_request_context.cc:112:
storage_.set_host_resolver(
On 2012/08/16 20:38:55, szym wrote:
> On 2012/08/16 20:09:21, cbentzel wrote:
> > Could |set_host_resolver| change to take a scoped_ptr argument?
> 
> Ultimately, I'd strongly prefer that. If you can tolerate a small (and
hopefully
> temporary) inconsistency between set_host_resolver and all the other setters
in
> URLRequestContextStorage then I'd definitely do that.

I can. May want to get mmenke or willchan to chime in.

[cbentzel, 2012-08-17 13:41:52]

L G T M with one question below.

http://codereview.chromium.org/10831277/diff/15001/net/base/host_resolver.cc
File net/base/host_resolver.cc (right):

http://codereview.chromium.org/10831277/diff/15001/net/base/host_resolver.cc#...
net/base/host_resolver.cc:53: HostResolver::CreateSystemResolver(size_t
max_concurrent_resolves,
This used to be in host_resolver_impl.cc - and meant that this file did not
depend on the impl. Are you OK with this change?

[szym, 2012-08-17 15:03:30]

Right, this brings in a number of headers into this file. I have no
strong opinion about this although host_resolver_impl.cc seems very
large to me -- hence the few "TODO(szym): move to another file"
comments, on which I've been punting for a while.


On Fri, Aug 17, 2012 at 9:41 AM,  <cbentzel@chromium.org> wrote:
> L G T M with one question below.
>
>
> http://codereview.chromium.org/10831277/diff/15001/net/base/host_resolver.cc
> File net/base/host_resolver.cc (right):
>
>
http://codereview.chromium.org/10831277/diff/15001/net/base/host_resolver.cc#...
> net/base/host_resolver.cc:53: HostResolver::CreateSystemResolver(size_t
> max_concurrent_resolves,
> This used to be in host_resolver_impl.cc - and meant that this file did
> not depend on the impl. Are you OK with this change?
>
> http://codereview.chromium.org/10831277/

[cbentzel, 2012-08-17 17:59:15]

On 2012/08/17 15:03:30, szym wrote:
> Right, this brings in a number of headers into this file. I have no
> strong opinion about this although host_resolver_impl.cc seems very
> large to me -- hence the few "TODO(szym): move to another file"
> comments, on which I've been punting for a while.
> 
> 
> On Fri, Aug 17, 2012 at 9:41 AM,  <mailto:cbentzel@chromium.org> wrote:
> > L G T M with one question below.
> >
> >
> > http://codereview.chromium.org/10831277/diff/15001/net/base/host_resolver.cc
> > File net/base/host_resolver.cc (right):
> >
> >
>
http://codereview.chromium.org/10831277/diff/15001/net/base/host_resolver.cc#...
> > net/base/host_resolver.cc:53: HostResolver::CreateSystemResolver(size_t
> > max_concurrent_resolves,
> > This used to be in host_resolver_impl.cc - and meant that this file did
> > not depend on the impl. Are you OK with this change?
> >
> > http://codereview.chromium.org/10831277/

OK

LGTM

[szym, 2012-08-17 18:44:03]

Thanks for the review. I still plan to change
URLRequestContext{Storage,Builder}::set_host_resolver to accept
scoped_ptr<HostResolver> before asking other OWNERS to review.

On 2012/08/17 17:59:15, cbentzel wrote:
> On 2012/08/17 15:03:30, szym wrote:
> > Right, this brings in a number of headers into this file. I have no
> > strong opinion about this although host_resolver_impl.cc seems very
> > large to me -- hence the few "TODO(szym): move to another file"
> > comments, on which I've been punting for a while.
> > 
> > 
> > On Fri, Aug 17, 2012 at 9:41 AM,  <mailto:cbentzel@chromium.org> wrote:
> > > L G T M with one question below.
> > >
> > >
> > >
http://codereview.chromium.org/10831277/diff/15001/net/base/host_resolver.cc
> > > File net/base/host_resolver.cc (right):
> > >
> > >
> >
>
http://codereview.chromium.org/10831277/diff/15001/net/base/host_resolver.cc#...
> > > net/base/host_resolver.cc:53: HostResolver::CreateSystemResolver(size_t
> > > max_concurrent_resolves,
> > > This used to be in host_resolver_impl.cc - and meant that this file did
> > > not depend on the impl. Are you OK with this change?
> > >
> > > http://codereview.chromium.org/10831277/
> 
> OK
> 
> LGTM

[szym, 2012-08-20 20:58:10]

brettw: please approve changes in chrome/ content/ and webkit/
akalin: please approve changes in sync/

[akalin, 2012-08-20 21:01:13]

sync lgtm

[szym, 2012-08-27 21:03:45]

brettw: ping

[brettw, 2012-08-29 06:57:30]

overloaded, please find another reviewer

[akalin, 2012-09-25 21:31:04]

On 2012/08/29 06:57:30, brettw wrote:
> overloaded, please find another reviewer

will this be checked in?

[szym, 2012-09-25 21:32:45]

On 2012/09/25 21:31:04, akalin wrote:
> On 2012/08/29 06:57:30, brettw wrote:
> > overloaded, please find another reviewer
> 
> will this be checked in?

Sorry for sleeping on this. Yes, I plan to rebase it and get an owner review for
chrome/ content/ and webkit/. ETA next week.

[szym, 2012-09-27 15:44:20]

darin: OWNER approval requested for chrome/, content/ and webkit/
PTAL

[szym, 2012-10-08 20:18:43]

On 2012/09/27 15:44:20, szym wrote:
> darin: OWNER approval requested for chrome/, content/ and webkit/
> PTAL

darin: ping?

[darin (slow to review), 2012-10-09 17:38:34]

http://codereview.chromium.org/10831277/diff/37009/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/10831277/diff/37009/chrome/browser/io_thread.c...
chrome/browser/io_thread.cc:164: true /* use_cache */,
I'm not a big fan of functions that take multiples bool parameters.  It is
really easy to make a mistake at the callsite.  You might consider defining an
Options struct with bool member variables that can be set.  See for example
Thread::StartWithOptions.  You could also use bit masks that you OR together.

http://codereview.chromium.org/10831277/diff/37009/chrome/browser/net/connect...
File chrome/browser/net/connection_tester.cc (right):

http://codereview.chromium.org/10831277/diff/37009/chrome/browser/net/connect...
chrome/browser/net/connection_tester.cc:171: return net::ERR_UNEXPECTED;
It looks like it is possible for this function to return an error and a non-null
HostResolver instance!!  That's generally a bad idea as it can create flaky
code.

I recommend changing to a pattern like this:

  scoped_ptr<net::HostResolver> result = ...Create...
  switch (experiment) {
    case ...
      break;
    case ...
      break;
    default:
      NOTREACHED();
      return net::ERR_UNEXPECTED;
  }
  host_resolver->swap(result);
  return net::OK;

http://codereview.chromium.org/10831277/diff/37009/net/base/host_resolver.cc
File net/base/host_resolver.cc (right):

http://codereview.chromium.org/10831277/diff/37009/net/base/host_resolver.cc#...
net/base/host_resolver.cc:14: namespace {
nit: new line after open of namespace

http://codereview.chromium.org/10831277/diff/37009/net/base/host_resolver.cc#...
net/base/host_resolver.cc:20: static const size_t kDefaultMaxProcTasks = 6u;
static and anonymous namespace are redundant.  you only need one.

http://codereview.chromium.org/10831277/diff/37009/net/base/host_resolver.cc#...
net/base/host_resolver.cc:53: HostResolver::CreateSystemResolver(size_t
max_concurrent_resolves,
nit: looks like you can format like this:

  scoped_ptr<HostResolver> HostResolver::CreateSystemResolver(
      size_t max_concurrent_resolves,
      ...) {
  }

http://codereview.chromium.org/10831277/diff/37009/net/base/host_resolver.cc#...
net/base/host_resolver.cc:83: scoped_ptr<HostResolver>
ditto

http://codereview.chromium.org/10831277/diff/37009/net/base/host_resolver.h
File net/base/host_resolver.h (right):

http://codereview.chromium.org/10831277/diff/37009/net/base/host_resolver.h#n...
net/base/host_resolver.h:183: size_t max_concurrent_resolves,
an Options struct containing these four fields would be nice here.

http://codereview.chromium.org/10831277/diff/37009/net/base/host_resolver_imp...
File net/base/host_resolver_impl_unittest.cc (right):

http://codereview.chromium.org/10831277/diff/37009/net/base/host_resolver_imp...
net/base/host_resolver_impl_unittest.cc:761: scoped_ptr<HostCache>(NULL),
nit: NULL parameter is unnecessary.  just call the default ctor.

http://codereview.chromium.org/10831277/diff/37009/net/base/host_resolver_imp...
net/base/host_resolver_impl_unittest.cc:764: scoped_ptr<DnsClient>(NULL),
same nit here

http://codereview.chromium.org/10831277/diff/37009/net/url_request/url_reques...
File net/url_request/url_request_context_storage.h (right):

http://codereview.chromium.org/10831277/diff/37009/net/url_request/url_reques...
net/url_request/url_request_context_storage.h:46: void set_net_log(NetLog*
net_log);
nit: all of these functions are incorrectly named using lower_case.  They should
be formatted as CamelCase since they are not implemented inline.  Obviously,
this is out of scope for this CL.  Just mentioning since I'm seeing it here.

[szym, 2012-10-09 21:40:42]

http://codereview.chromium.org/10831277/diff/37009/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/10831277/diff/37009/chrome/browser/io_thread.c...
chrome/browser/io_thread.cc:164: true /* use_cache */,
On 2012/10/09 17:38:34, darin wrote:
> I'm not a big fan of functions that take multiples bool parameters.  It is
> really easy to make a mistake at the callsite.  You might consider defining an
> Options struct with bool member variables that can be set.  See for example
> Thread::StartWithOptions.  You could also use bit masks that you OR together.

Done.

http://codereview.chromium.org/10831277/diff/37009/chrome/browser/net/connect...
File chrome/browser/net/connection_tester.cc (right):

http://codereview.chromium.org/10831277/diff/37009/chrome/browser/net/connect...
chrome/browser/net/connection_tester.cc:171: return net::ERR_UNEXPECTED;
On 2012/10/09 17:38:34, darin wrote:
> It looks like it is possible for this function to return an error and a
non-null
> HostResolver instance!!  That's generally a bad idea as it can create flaky
> code.
> 
> I recommend changing to a pattern like this:
> 
>   scoped_ptr<net::HostResolver> result = ...Create...
>   switch (experiment) {
>     case ...
>       break;
>     case ...
>       break;
>     default:
>       NOTREACHED();
>       return net::ERR_UNEXPECTED;
>   }
>   host_resolver->swap(result);
>   return net::OK;

Done.

http://codereview.chromium.org/10831277/diff/37009/net/base/host_resolver.cc
File net/base/host_resolver.cc (right):

http://codereview.chromium.org/10831277/diff/37009/net/base/host_resolver.cc#...
net/base/host_resolver.cc:14: namespace {
On 2012/10/09 17:38:34, darin wrote:
> nit: new line after open of namespace

Done.

http://codereview.chromium.org/10831277/diff/37009/net/base/host_resolver.cc#...
net/base/host_resolver.cc:20: static const size_t kDefaultMaxProcTasks = 6u;
On 2012/10/09 17:38:34, darin wrote:
> static and anonymous namespace are redundant.  you only need one.

Done.

http://codereview.chromium.org/10831277/diff/37009/net/base/host_resolver.cc#...
net/base/host_resolver.cc:53: HostResolver::CreateSystemResolver(size_t
max_concurrent_resolves,
On 2012/10/09 17:38:34, darin wrote:
> nit: looks like you can format like this:
> 
>   scoped_ptr<HostResolver> HostResolver::CreateSystemResolver(
>       size_t max_concurrent_resolves,
>       ...) {
>   }

Done.

http://codereview.chromium.org/10831277/diff/37009/net/base/host_resolver.cc#...
net/base/host_resolver.cc:61: scoped_ptr<HostCache> cache(NULL);
Replaced those NULLs with default constructors.

http://codereview.chromium.org/10831277/diff/37009/net/base/host_resolver.cc#...
net/base/host_resolver.cc:83: scoped_ptr<HostResolver>
On 2012/10/09 17:38:34, darin wrote:
> ditto

Done. But now that HostResolver::Options has a default ctor, is it really needed
or should it be replaced by:
  CreateSystemResolver(HostResolver::Options(), net_log);

http://codereview.chromium.org/10831277/diff/37009/net/base/host_resolver.h
File net/base/host_resolver.h (right):

http://codereview.chromium.org/10831277/diff/37009/net/base/host_resolver.h#n...
net/base/host_resolver.h:183: size_t max_concurrent_resolves,
On 2012/10/09 17:38:34, darin wrote:
> an Options struct containing these four fields would be nice here.

Done.

http://codereview.chromium.org/10831277/diff/37009/net/base/host_resolver_imp...
File net/base/host_resolver_impl_unittest.cc (right):

http://codereview.chromium.org/10831277/diff/37009/net/base/host_resolver_imp...
net/base/host_resolver_impl_unittest.cc:761: scoped_ptr<HostCache>(NULL),
On 2012/10/09 17:38:34, darin wrote:
> nit: NULL parameter is unnecessary.  just call the default ctor.

Done.

http://codereview.chromium.org/10831277/diff/37009/net/base/host_resolver_imp...
net/base/host_resolver_impl_unittest.cc:764: scoped_ptr<DnsClient>(NULL),
On 2012/10/09 17:38:34, darin wrote:
> same nit here

Done.

[szym, 2012-10-10 18:45:40]

jam: please review changes in content/shell
As explained in https://chromiumcodereview.appspot.com/11053007/, the current
code causes double-free.

[szym, 2012-10-10 18:46:10]

jam: please review changes in content/shell
As explained in https://chromiumcodereview.appspot.com/11053007/, the current
code causes double-free.

[jam, 2012-10-10 20:12:46]

content lgtm

[szym, 2012-10-11 21:29:44]

darin: ping

[szym, 2012-10-22 16:36:55]

On 2012/10/11 21:29:44, szym wrote:
> darin: ping

darin: please review.

[darin (slow to review), 2012-10-22 17:27:17]

Sorry, I failed to publish these comments back when I wrote them :-(

http://codereview.chromium.org/10831277/diff/52004/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/10831277/diff/52004/chrome/browser/io_thread.c...
chrome/browser/io_thread.cc:126: options.max_concurrent_resolves =
net::HostResolver::kDefaultParallelism;
nit: Shouldn't the Options struct have a constructor that initializes
max_concurrent_resolves to kDefaultParallelism?  I don't think everyone should
have to replicate this line of code.

http://codereview.chromium.org/10831277/diff/52004/chrome/browser/io_thread.c...
chrome/browser/io_thread.cc:143: options.max_retry_attempts =
net::HostResolver::kDefaultRetryAttempts;
ditto.  seems like Options constructor should initialize this.

http://codereview.chromium.org/10831277/diff/52004/chrome/browser/net/connect...
File chrome/browser/net/connection_tester.cc (right):

http://codereview.chromium.org/10831277/diff/52004/chrome/browser/net/connect...
chrome/browser/net/connection_tester.cc:174: (*host_resolver).swap(resolver);
host_resolver->swap(resolver);

[szym, 2012-10-22 17:48:42]

http://codereview.chromium.org/10831277/diff/52004/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/10831277/diff/52004/chrome/browser/io_thread.c...
chrome/browser/io_thread.cc:126: options.max_concurrent_resolves =
net::HostResolver::kDefaultParallelism;
On 2012/10/22 17:27:17, darin wrote:
> nit: Shouldn't the Options struct have a constructor that initializes
> max_concurrent_resolves to kDefaultParallelism?  I don't think everyone should
> have to replicate this line of code.

It already does have a default constructor which does all that. I removed the
explicit initialization.

http://codereview.chromium.org/10831277/diff/52004/chrome/browser/net/connect...
File chrome/browser/net/connection_tester.cc (right):

http://codereview.chromium.org/10831277/diff/52004/chrome/browser/net/connect...
chrome/browser/net/connection_tester.cc:174: (*host_resolver).swap(resolver);
On 2012/10/22 17:27:17, darin wrote:
> host_resolver->swap(resolver);

Done.

[darin (slow to review), 2012-10-22 17:54:16]

LGTM

[commit-bot: I haz the power, 2012-10-22 20:05:11]

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