Refactorings surrounding HostResolver:...

Refactorings surrounding HostResolver:

(1) Extract HostResolver to an interface.

The existing concrete implementation is now named HostResolverImpl. This makes it possible to create mocks with more complex behavior (i.e. choose via rules if response will be sync vs async).

(2) Transform HostMapper into HostResolverProc.

Conceptually HostResolverProc maps a hostname to a socket address, whereas HostMapper mapped a hostname to another hostname (so you were still at the mercy of the system's host resolver). With HostResolverProc you can specify the exact AddressList, making it possible to run tests requiring IPv6 socketaddrs on systems (like WinXP) that don't actually support it.

(3) Add a MockHostResolver implementation of HostResolver.

This replaces the [ScopedHostMapper + RuleBasedHostMapper + HostResolver] combo. It is less clunky and a bit more expressive.

BUG=http://crbug.com/16452
R=willchan
TEST=existing


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

[willchan no longer on Chromium, 2009-07-12 04:26:55]

I haven't read through the whole thing yet.  I have some general high level
comments first.

http://codereview.chromium.org/149511/diff/1/23
File net/base/mock_host_resolver.h (right):

http://codereview.chromium.org/149511/diff/1/23#newcode36
Line 36: class MockHostResolver : public HostResolverImpl {
Why are you inheriting from this?  The Google style guide says to prefer
composition over inheritance (for some very good reasons).  The ui team has seen
the problems with inheritance (see
http://sites.google.com/a/chromium.org/dev/developers/design-documents/conven...)
and have emphasized not to use it.

It seems like you inherit and then call set_resolver_proc().  Why not compose
HostResolverImpl and change its constructor to take the HostResolverProc as an
argument?

http://codereview.chromium.org/149511/diff/1016/1047
File net/base/host_resolver_impl.h (right):

http://codereview.chromium.org/149511/diff/1016/1047#newcode154
Line 154: HostResolverProc* SetGlobalHostResolverProc(HostResolverProc* value);
I haven't read through it yet, but is it possible to get rid of this?  Now that
you've made it possible to use a MockHostResolver, I would think that mucking
around with the global host resolver proc should be unnecessary.

[eroman, 2009-07-12 10:56:09]

http://codereview.chromium.org/149511/diff/1/23
File net/base/mock_host_resolver.h (right):

http://codereview.chromium.org/149511/diff/1/23#newcode36
Line 36: class MockHostResolver : public HostResolverImpl {
On 2009/07/12 04:26:55, willchan wrote:
> Why are you inheriting from this?  The Google style guide says to prefer
> composition over inheritance (for some very good reasons).  The ui team has
seen
> the problems with inheritance (see
>
http://sites.google.com/a/chromium.org/dev/developers/design-documents/conven...)
> and have emphasized not to use it.
> 
> It seems like you inherit and then call set_resolver_proc().  Why not compose
> HostResolverImpl and change its constructor to take the HostResolverProc as an
> argument?

Done.

This was laziness on my part (didn't want to stub out all the methods); I agree
that composition is a better solution.

http://codereview.chromium.org/149511/diff/1016/1047
File net/base/host_resolver_impl.h (right):

http://codereview.chromium.org/149511/diff/1016/1047#newcode154
Line 154: HostResolverProc* SetGlobalHostResolverProc(HostResolverProc* value);
On 2009/07/12 04:26:55, willchan wrote:
> I haven't read through it yet, but is it possible to get rid of this?  Now
that
> you've made it possible to use a MockHostResolver, I would think that mucking
> around with the global host resolver proc should be unnecessary.

Almost all usages of ScopedHostMapper are gone (they now use MockHostResolver,
or a HostResolverImpl with an injected HostResolverProc* dependency).

However, there was a remaining use-case which couldn't be converted: The test
harnesses for various unit-tests (net, in-process-ui, unit, browser) use
ScopedHostMapper to set a blanket policy on external DNS resolves.

[willchan no longer on Chromium, 2009-07-14 17:04:57]

LGTM, this is a clear improvement over the existing test setup.

http://codereview.chromium.org/149511/diff/1133/211
File chrome/browser/net/dns_global.cc (right):

http://codereview.chromium.org/149511/diff/1133/211#newcode482
Line 482: global_host_resolver = new net::HostResolverImpl(
I might be getting too pedantic, but it seems to be that HostResolverImpl is
sort of an "internal" implementation class within the networking stack.  It's
not something that I'd want exposed to outside consumers.  I wonder if using a
factory method would be better here?

http://codereview.chromium.org/149511/diff/1133/194
File net/base/host_resolver.h (right):

http://codereview.chromium.org/149511/diff/1133/194#newcode129
Line 129: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/149511/diff/1133/197
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/149511/diff/1133/197#newcode335
Line 335: *out_req = reinterpret_cast<RequestHandle>(req);
Hm, I think it's nice to avoid reinterpret_cast<> when possible.  What do you
think about simply having a HostResolverRequest class?  It can be forward
declared in host_resolver.h and defined in host_resolver_impl.cc.

http://codereview.chromium.org/149511/diff/1133/204
File net/base/host_resolver_proc.cc (right):

http://codereview.chromium.org/149511/diff/1133/204#newcode14
Line 14: #include <sys/socket.h>
Are all these headers necessary?  I haven't checked, but I'd be suspicious is
socket.h were necessary.

http://codereview.chromium.org/149511/diff/1133/195
File net/base/host_resolver_proc.h (right):

http://codereview.chromium.org/149511/diff/1133/195#newcode28
Line 28: virtual int Resolve(const std::string& host, AddressList* addrlist) =
0;
Can this member function be const?

http://codereview.chromium.org/149511/diff/1133/195#newcode32
Line 32: int ResolveUsingPrevious(const std::string& host, AddressList*
addrlist);
Can this member function be const?

http://codereview.chromium.org/149511/diff/1133/195#newcode53
Line 53: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/149511/diff/1133/196
File net/base/mock_host_resolver.h (right):

http://codereview.chromium.org/149511/diff/1133/196#newcode130
Line 130: ScopedDefaultHostResolverProc(HostResolverProc* proc);
explicit

[eroman, 2009-07-14 20:32:27]

http://codereview.chromium.org/149511/diff/1133/211
File chrome/browser/net/dns_global.cc (right):

http://codereview.chromium.org/149511/diff/1133/211#newcode482
Line 482: global_host_resolver = new net::HostResolverImpl(
On 2009/07/14 17:04:57, willchan wrote:
> I might be getting too pedantic, but it seems to be that HostResolverImpl is
> sort of an "internal" implementation class within the networking stack.  It's
> not something that I'd want exposed to outside consumers.  I wonder if using a
> factory method would be better here?

Done.

I fully agree with this suggestion (it is definitely not "too pedantic").

Added this function to "host_resolver.h":

  HostResolver* CreateSystemHostResolver();

I very much like how this makes the non-mock users more explicit, hides the
various caching parameters, avoids inclusions of host_resolver_impl.h, and
avoids all the lame repetitions of "new HostResolverImpl(NULL, x, x)".

http://codereview.chromium.org/149511/diff/1133/194
File net/base/host_resolver.h (right):

http://codereview.chromium.org/149511/diff/1133/194#newcode129
Line 129: };
On 2009/07/14 17:04:57, willchan wrote:
> DISALLOW_COPY_AND_ASSIGN

I don't think this is necessary:

HostResolver is an interface (abstract class) -- if I call
DISALLOW_COPY_AND_ASSIGN it means I will need to additionally define a default
ctor to get things compiling. Since this isn't a concrete class, the
copy/assignment constructors will not be used anyway.

http://codereview.chromium.org/149511/diff/1133/197
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/149511/diff/1133/197#newcode335
Line 335: *out_req = reinterpret_cast<RequestHandle>(req);
On 2009/07/14 17:04:57, willchan wrote:
> Hm, I think it's nice to avoid reinterpret_cast<> when possible.  What do you
> think about simply having a HostResolverRequest class?  It can be forward
> declared in host_resolver.h and defined in host_resolver_impl.cc.

The issue is that different subclasses of HostResolver may want different
internal classes as their RequestHandle. To do away with reinterpret cast means
I would need a shared base class among implementations (and virtual dtor in case
of abuse). I think the semantics of naming it "RequestHandle" are a bit cleaner
since that way each implementation can use whatever class hiearchy it wants (no
need for multiple-inheritance).

http://codereview.chromium.org/149511/diff/1133/204
File net/base/host_resolver_proc.cc (right):

http://codereview.chromium.org/149511/diff/1133/204#newcode14
Line 14: #include <sys/socket.h>
On 2009/07/14 17:04:57, willchan wrote:
> Are all these headers necessary?  I haven't checked, but I'd be suspicious is
> socket.h were necessary.

Not sure.

For this refactor, I am hoping to leave these as-in to avoid potentially
breaking any builds.

I will follow-up and see if there is a smaller minimum set of includes that will
work here for mac/linux

http://codereview.chromium.org/149511/diff/1133/195
File net/base/host_resolver_proc.h (right):

http://codereview.chromium.org/149511/diff/1133/195#newcode28
Line 28: virtual int Resolve(const std::string& host, AddressList* addrlist) =
0;
On 2009/07/14 17:04:57, willchan wrote:
> Can this member function be const?

There are existing non-const implementations, so I would prefer to avoid needing
casts/mutable in them.

I will explore making this change as a follow-up patch, since it is a good
safety-net for implementations that are non-threadsafe.

For now I have added ad fat comment to the class documentation reinforcing that
implementations of HostResolverProc must be threadsafe.

http://codereview.chromium.org/149511/diff/1133/195#newcode32
Line 32: int ResolveUsingPrevious(const std::string& host, AddressList*
addrlist);
On 2009/07/14 17:04:57, willchan wrote:
> Can this member function be const?

See above.

http://codereview.chromium.org/149511/diff/1133/195#newcode53
Line 53: };
On 2009/07/14 17:04:57, willchan wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/149511/diff/1133/196
File net/base/mock_host_resolver.h (right):

http://codereview.chromium.org/149511/diff/1133/196#newcode130
Line 130: ScopedDefaultHostResolverProc(HostResolverProc* proc);
On 2009/07/14 17:04:57, willchan wrote:
> explicit

Done.

[willchan no longer on Chromium, 2009-07-14 22:24:45]

still lgtm

http://codereview.chromium.org/149511/diff/1133/194
File net/base/host_resolver.h (right):

http://codereview.chromium.org/149511/diff/1133/194#newcode129
Line 129: };
On 2009/07/14 20:32:27, eroman wrote:
> On 2009/07/14 17:04:57, willchan wrote:
> > DISALLOW_COPY_AND_ASSIGN
> 
> I don't think this is necessary:
> 
> HostResolver is an interface (abstract class) -- if I call
> DISALLOW_COPY_AND_ASSIGN it means I will need to additionally define a default
> ctor to get things compiling. Since this isn't a concrete class, the
> copy/assignment constructors will not be used anyway.

You're right, but if you put the DISALLOW_COPY_AND_ASSIGN here, then all derived
classes with inherit the declaration (with the missing definition), so even if
they forget DISALLOW_COPY_AND_ASSIGN, a call to the derived class's copy
constructor will fail since it will automatically call the parent class's.

That said, if you still disagree, I don't feel strongly, so go ahead and commit
this anyway.

http://codereview.chromium.org/149511/diff/1133/197
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/149511/diff/1133/197#newcode335
Line 335: *out_req = reinterpret_cast<RequestHandle>(req);
On 2009/07/14 20:32:27, eroman wrote:
> On 2009/07/14 17:04:57, willchan wrote:
> > Hm, I think it's nice to avoid reinterpret_cast<> when possible.  What do
you
> > think about simply having a HostResolverRequest class?  It can be forward
> > declared in host_resolver.h and defined in host_resolver_impl.cc.
> 
> The issue is that different subclasses of HostResolver may want different
> internal classes as their RequestHandle. To do away with reinterpret cast
means
> I would need a shared base class among implementations (and virtual dtor in
case
> of abuse). I think the semantics of naming it "RequestHandle" are a bit
cleaner
> since that way each implementation can use whatever class hiearchy it wants
(no
> need for multiple-inheritance).

I see.  I didn't see multiple subclasses using the RequestHandle, but if you
want to handle this case, then you're right, reinterpret_cast<> is the only
solution.  Fine by me.

[eroman, 2009-07-15 02:10:56]

http://codereview.chromium.org/149511/diff/1133/194
File net/base/host_resolver.h (right):

http://codereview.chromium.org/149511/diff/1133/194#newcode129
Line 129: };
On 2009/07/14 22:24:46, willchan wrote:
> On 2009/07/14 20:32:27, eroman wrote:
> > On 2009/07/14 17:04:57, willchan wrote:
> > > DISALLOW_COPY_AND_ASSIGN
> > 
> > I don't think this is necessary:
> > 
> > HostResolver is an interface (abstract class) -- if I call
> > DISALLOW_COPY_AND_ASSIGN it means I will need to additionally define a
default
> > ctor to get things compiling. Since this isn't a concrete class, the
> > copy/assignment constructors will not be used anyway.
> 
> You're right, but if you put the DISALLOW_COPY_AND_ASSIGN here, then all
derived
> classes with inherit the declaration (with the missing definition), so even if
> they forget DISALLOW_COPY_AND_ASSIGN, a call to the derived class's copy
> constructor will fail since it will automatically call the parent class's.
> 
> That said, if you still disagree, I don't feel strongly, so go ahead and
commit
> this anyway.

Done.

http://codereview.chromium.org/149511/diff/1133/197
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/149511/diff/1133/197#newcode335
Line 335: *out_req = reinterpret_cast<RequestHandle>(req);
On 2009/07/14 22:24:46, willchan wrote:
> On 2009/07/14 20:32:27, eroman wrote:
> > On 2009/07/14 17:04:57, willchan wrote:
> > > Hm, I think it's nice to avoid reinterpret_cast<> when possible.  What do
> you
> > > think about simply having a HostResolverRequest class?  It can be forward
> > > declared in host_resolver.h and defined in host_resolver_impl.cc.
> > 
> > The issue is that different subclasses of HostResolver may want different
> > internal classes as their RequestHandle. To do away with reinterpret cast
> means
> > I would need a shared base class among implementations (and virtual dtor in
> case
> > of abuse). I think the semantics of naming it "RequestHandle" are a bit
> cleaner
> > since that way each implementation can use whatever class hiearchy it wants
> (no
> > need for multiple-inheritance).
> 
> I see.  I didn't see multiple subclasses using the RequestHandle, but if you
> want to handle this case, then you're right, reinterpret_cast<> is the only
> solution.  Fine by me.

OK, left as is.