Implement utility-side host resolver Mojo client.

Implement utility-side host resolver Mojo client.

BUG=11746
Committed: https://crrev.com/6ac3fe5747d81785bc0750e19b4746f3f2570537
Cr-Commit-Position: refs/heads/master@{#317990}

[Sam McNally, 2015-02-11 23:42:07]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2015-02-11 23:42:13]

Patchset #1 (id:20001) has been deleted

[Sam McNally, 2015-02-11 23:46:39]

sammc@chromium.org changed reviewers:
+ amistry@chromium.org, eroman@chromium.org

[eroman, 2015-02-12 23:39:50]

https://codereview.chromium.org/904313003/diff/40001/net/dns/host_resolver_mo...
File net/dns/host_resolver_mojo.cc (right):

https://codereview.chromium.org/904313003/diff/40001/net/dns/host_resolver_mo...
net/dns/host_resolver_mojo.cc:24: void ReportResult(int32_t error,
int

https://codereview.chromium.org/904313003/diff/40001/net/dns/host_resolver_mo...
net/dns/host_resolver_mojo.cc:65: return ERR_FAILED;
Return ERR_DNS_CACHE_MISS instead. That way consumers treat it as a cache miss
rather than an error.

https://codereview.chromium.org/904313003/diff/40001/net/dns/host_resolver_mo...
net/dns/host_resolver_mojo.cc:70: delete static_cast<Job*>(req);
What is the mechanism through which the mojo side is cancelled?

https://codereview.chromium.org/904313003/diff/40001/net/dns/host_resolver_mo...
net/dns/host_resolver_mojo.cc:84: int32_t error,
int

https://codereview.chromium.org/904313003/diff/40001/net/dns/host_resolver_mo...
net/dns/host_resolver_mojo.cc:90: error = ERR_FAILED;
why mask the error? Wouldn't it be sufficient to pass |error| directly to Run()
below?

[Sam McNally, 2015-02-13 00:50:58]

https://codereview.chromium.org/904313003/diff/40001/net/dns/host_resolver_mo...
File net/dns/host_resolver_mojo.cc (right):

https://codereview.chromium.org/904313003/diff/40001/net/dns/host_resolver_mo...
net/dns/host_resolver_mojo.cc:24: void ReportResult(int32_t error,
On 2015/02/12 23:39:50, eroman wrote:
> int

This is implementing the mojo interface.

https://codereview.chromium.org/904313003/diff/40001/net/dns/host_resolver_mo...
net/dns/host_resolver_mojo.cc:65: return ERR_FAILED;
On 2015/02/12 23:39:50, eroman wrote:
> Return ERR_DNS_CACHE_MISS instead. That way consumers treat it as a cache miss
> rather than an error.

Done.

https://codereview.chromium.org/904313003/diff/40001/net/dns/host_resolver_mo...
net/dns/host_resolver_mojo.cc:70: delete static_cast<Job*>(req);
On 2015/02/12 23:39:50, eroman wrote:
> What is the mechanism through which the mojo side is cancelled?

It closes the mojo pipe, which the other side detects in
MojoHostResolverImpl::Job::OnConnectionError().

https://codereview.chromium.org/904313003/diff/40001/net/dns/host_resolver_mo...
net/dns/host_resolver_mojo.cc:84: int32_t error,
On 2015/02/12 23:39:50, eroman wrote:
> int

This is implementing the mojo interface.

https://codereview.chromium.org/904313003/diff/40001/net/dns/host_resolver_mo...
net/dns/host_resolver_mojo.cc:90: error = ERR_FAILED;
On 2015/02/12 23:39:50, eroman wrote:
> why mask the error? Wouldn't it be sufficient to pass |error| directly to
Run()
> below?

It was only when the error was OK, but no results were returned. It doesn't seem
particularly necessary. Removed it.

[Anand Mistry (off Chromium), 2015-02-16 06:32:44]

https://codereview.chromium.org/904313003/diff/100001/net/dns/host_resolver_m...
File net/dns/host_resolver_mojo.cc (right):

https://codereview.chromium.org/904313003/diff/100001/net/dns/host_resolver_m...
net/dns/host_resolver_mojo.cc:52:
mojo::InterfacePtr<interfaces::HostResolverRequestClient> handle;
This is interfaces::HostResolverRequestClientPtr right? Which one should we be
using consistently?

https://codereview.chromium.org/904313003/diff/100001/net/dns/host_resolver_m...
File net/dns/host_resolver_mojo.h (right):

https://codereview.chromium.org/904313003/diff/100001/net/dns/host_resolver_m...
net/dns/host_resolver_mojo.h:9: #include "net/base/address_list.h"
You can forward-declare this one.

https://codereview.chromium.org/904313003/diff/100001/net/dns/host_resolver_m...
net/dns/host_resolver_mojo.h:17: class HostResolverMojo : public HostResolver {
Add a short blurb here about what this is.

https://codereview.chromium.org/904313003/diff/100001/net/dns/host_resolver_m...
net/dns/host_resolver_mojo.h:20: mojo::ErrorHandler* error_handler);
I'm having a hard time liking passing in an error handler. I understand why, but
I still don't like it.

A couple of alternate ideas:
- A disconnection callback instead of ErrorHandler.
- Ignore it. If this pipe goes, the browser side will also detect the
disconnection and also kill everything. Also, the proxy connection is likely
dead as well.

https://codereview.chromium.org/904313003/diff/100001/net/dns/host_resolver_m...
File net/dns/host_resolver_mojo_unittest.cc (right):

https://codereview.chromium.org/904313003/diff/100001/net/dns/host_resolver_m...
net/dns/host_resolver_mojo_unittest.cc:104: explicit MockMojoHostResolver(
Can you add a destructor that does:
ASSERT_EQ(results_returned_, actions_.size());
to be sure that all declared actions are done.

https://codereview.chromium.org/904313003/diff/100001/net/dns/host_resolver_m...
net/dns/host_resolver_mojo_unittest.cc:203: base::Bind(&OnResolveComplete,
&error, run_loop.QuitClosure()),
net::TestCompletionCallback

https://codereview.chromium.org/904313003/diff/100001/net/dns/mojo_type_conve...
File net/dns/mojo_type_converters.cc (right):

https://codereview.chromium.org/904313003/diff/100001/net/dns/mojo_type_conve...
net/dns/mojo_type_converters.cc:51:
result.set_address_family(net::AddressFamilyFromMojo(obj.address_family));
Oops. I missed setting result.set_is_my_ip_address() here. Can you add it while
you're here.

[Sam McNally, 2015-02-16 07:21:43]

Patchset #5 (id:120001) has been deleted

[Sam McNally, 2015-02-16 07:24:44]

https://codereview.chromium.org/904313003/diff/100001/net/dns/host_resolver_m...
File net/dns/host_resolver_mojo.cc (right):

https://codereview.chromium.org/904313003/diff/100001/net/dns/host_resolver_m...
net/dns/host_resolver_mojo.cc:52:
mojo::InterfacePtr<interfaces::HostResolverRequestClient> handle;
On 2015/02/16 06:32:44, Anand Mistry wrote:
> This is interfaces::HostResolverRequestClientPtr right? Which one should we be
> using consistently?

interfaces::HostResolverRequestClientPtr.

https://codereview.chromium.org/904313003/diff/100001/net/dns/host_resolver_m...
File net/dns/host_resolver_mojo.h (right):

https://codereview.chromium.org/904313003/diff/100001/net/dns/host_resolver_m...
net/dns/host_resolver_mojo.h:9: #include "net/base/address_list.h"
On 2015/02/16 06:32:44, Anand Mistry wrote:
> You can forward-declare this one.

Done.

https://codereview.chromium.org/904313003/diff/100001/net/dns/host_resolver_m...
net/dns/host_resolver_mojo.h:17: class HostResolverMojo : public HostResolver {
On 2015/02/16 06:32:44, Anand Mistry wrote:
> Add a short blurb here about what this is.

Done.

https://codereview.chromium.org/904313003/diff/100001/net/dns/host_resolver_m...
net/dns/host_resolver_mojo.h:20: mojo::ErrorHandler* error_handler);
On 2015/02/16 06:32:44, Anand Mistry wrote:
> I'm having a hard time liking passing in an error handler. I understand why,
but
> I still don't like it.
> 
> A couple of alternate ideas:
> - A disconnection callback instead of ErrorHandler.
> - Ignore it. If this pipe goes, the browser side will also detect the
> disconnection and also kill everything. Also, the proxy connection is likely
> dead as well.

Switched to a callback. I'd prefer to avoid leaving this in a silently broken
state.

https://codereview.chromium.org/904313003/diff/100001/net/dns/host_resolver_m...
File net/dns/host_resolver_mojo_unittest.cc (right):

https://codereview.chromium.org/904313003/diff/100001/net/dns/host_resolver_m...
net/dns/host_resolver_mojo_unittest.cc:104: explicit MockMojoHostResolver(
On 2015/02/16 06:32:44, Anand Mistry wrote:
> Can you add a destructor that does:
> ASSERT_EQ(results_returned_, actions_.size());
> to be sure that all declared actions are done.

Done.

https://codereview.chromium.org/904313003/diff/100001/net/dns/host_resolver_m...
net/dns/host_resolver_mojo_unittest.cc:203: base::Bind(&OnResolveComplete,
&error, run_loop.QuitClosure()),
On 2015/02/16 06:32:44, Anand Mistry wrote:
> net::TestCompletionCallback

Done.

https://codereview.chromium.org/904313003/diff/100001/net/dns/mojo_type_conve...
File net/dns/mojo_type_converters.cc (right):

https://codereview.chromium.org/904313003/diff/100001/net/dns/mojo_type_conve...
net/dns/mojo_type_converters.cc:51:
result.set_address_family(net::AddressFamilyFromMojo(obj.address_family));
On 2015/02/16 06:32:44, Anand Mistry wrote:
> Oops. I missed setting result.set_is_my_ip_address() here. Can you add it
while
> you're here.

Done.

[Sam McNally, 2015-02-16 23:54:31]

Patchset #7 (id:180001) has been deleted

[Sam McNally, 2015-02-16 23:57:13]

Patchset #7 (id:200001) has been deleted

[Sam McNally, 2015-02-17 00:00:21]

Patchset #7 (id:220001) has been deleted

[Anand Mistry (off Chromium), 2015-02-17 06:23:04]

lgtm

[eroman, 2015-02-25 00:50:44]

lgtm

https://codereview.chromium.org/904313003/diff/240001/net/dns/host_resolver_m...
File net/dns/host_resolver_mojo_unittest.cc (right):

https://codereview.chromium.org/904313003/diff/240001/net/dns/host_resolver_m...
net/dns/host_resolver_mojo_unittest.cc:50: public:
nit: no need for public

https://codereview.chromium.org/904313003/diff/240001/net/dns/host_resolver_m...
net/dns/host_resolver_mojo_unittest.cc:84: int error = OK;
probably want to use net::Error consistently throughout the code (I should have
pointed this out sooner, but actually didn't realize it had become idiomatic of
net code until recently)

https://codereview.chromium.org/904313003/diff/240001/net/dns/host_resolver_m...
net/dns/host_resolver_mojo_unittest.cc:90: explicit MockMojoHostResolver(
no need for explicit

[Sam McNally, 2015-02-25 02:15:10]

Patchset #8 (id:260001) has been deleted

[Sam McNally, 2015-02-25 02:17:45]

https://codereview.chromium.org/904313003/diff/240001/net/dns/host_resolver_m...
File net/dns/host_resolver_mojo_unittest.cc (right):

https://codereview.chromium.org/904313003/diff/240001/net/dns/host_resolver_m...
net/dns/host_resolver_mojo_unittest.cc:50: public:
On 2015/02/25 00:50:44, eroman wrote:
> nit: no need for public

Done.

https://codereview.chromium.org/904313003/diff/240001/net/dns/host_resolver_m...
net/dns/host_resolver_mojo_unittest.cc:84: int error = OK;
On 2015/02/25 00:50:44, eroman wrote:
> probably want to use net::Error consistently throughout the code (I should
have
> pointed this out sooner, but actually didn't realize it had become idiomatic
of
> net code until recently)

Done.

https://codereview.chromium.org/904313003/diff/240001/net/dns/host_resolver_m...
net/dns/host_resolver_mojo_unittest.cc:90: explicit MockMojoHostResolver(
On 2015/02/25 00:50:44, eroman wrote:
> no need for explicit

Done.

[Sam McNally, 2015-02-25 05:04:20]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2015-02-25 05:04:21]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
amistry@chromium.org
Link to the patchset: https://codereview.chromium.org/904313003/#ps300001
(title: " ")

[commit-bot: I haz the power, 2015-02-25 05:05:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/904313003/300001

[commit-bot: I haz the power, 2015-02-25 06:00:36]

Committed patchset #9 (id:300001)

[commit-bot: I haz the power, 2015-02-25 06:01:16]

Patchset 9 (id:??) landed as
https://crrev.com/6ac3fe5747d81785bc0750e19b4746f3f2570537
Cr-Commit-Position: refs/heads/master@{#317990}