[net/dns] When serving AF_UNSPEC addresses from DnsHosts, serve one from each family.

[net/dns] When serving AF_UNSPEC addresses from DnsHosts, serve one from each family.

DnsHosts stores at most one address for each hostname per family. When resolving
requests for unspecified family, serve both of them (with IPv6 first and
IPv4 second) if present.

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

[szym, 2013-03-27 19:47:58]

Moving review to mmenke, i.e., engaging review afterburner ;)

[mmenke, 2013-03-27 19:58:02]

https://codereview.chromium.org/12681017/diff/1/net/dns/host_resolver_impl.cc
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/12681017/diff/1/net/dns/host_resolver_impl.cc...
net/dns/host_resolver_impl.cc:1963: // We prefer IPv6, "happy eyeballs" will
fallback to IPv4 if needed.
nit:  "fall back" (fallback is a noun, not a verb)

https://codereview.chromium.org/12681017/diff/1/net/dns/host_resolver_impl.cc...
net/dns/host_resolver_impl.cc:1963: // We prefer IPv6, "happy eyeballs" will
fallback to IPv4 if needed.
nit:  Also, that comma should be a semi-colon, or there should be a conjunction.

https://codereview.chromium.org/12681017/diff/1/net/dns/host_resolver_impl.cc...
net/dns/host_resolver_impl.cc:1975: DnsHostsKey(hostname, key.address_family));
optional:  This may be a little prettier with both cases merged:

addresses->clear();
if (key.address_family == IPV4 || key.address_family == UNSPEC) {
  DoStuff
}

etc.

https://codereview.chromium.org/12681017/diff/1/net/dns/host_resolver_impl.cc...
net/dns/host_resolver_impl.cc:1979: *addresses =
AddressList::CreateFromIPAddress(it->second, info.port());
I think it's a mistake to clear addresses on failure in one path, but not the
other.  This can result in code that always assumes on behavior or the other if
people fail to test the non-UNSPEC case, or only the UNSPEC case.

[szym, 2013-03-27 21:29:04]

all done

[mmenke, 2013-03-27 21:30:57]

On 2013/03/27 21:29:04, szym wrote:
> all done

LGTM!

[szym, 2013-03-27 21:31:15]

On 2013/03/27 21:30:57, mmenke wrote:
> On 2013/03/27 21:29:04, szym wrote:
> > all done
> 
> LGTM!

Thanks!

[mmenke, 2013-03-27 21:31:48]

quick nit

https://codereview.chromium.org/12681017/diff/9001/net/dns/host_resolver_impl.cc
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/12681017/diff/9001/net/dns/host_resolver_impl...
net/dns/host_resolver_impl.cc:1963: // necessary.
nit:  May want to move this just above the IPV6 block

[commit-bot: I haz the power, 2013-03-27 21:49:51]

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

[commit-bot: I haz the power, 2013-03-28 07:30:10]

Change committed as 191119