Implement utility-side proxy resolver Mojo service.

Implement utility-side proxy resolver Mojo service.

BUG=11746
Committed: https://crrev.com/5403aa1dc4fa777f106c7c311644abf3d3d576ba
Cr-Commit-Position: refs/heads/master@{#317976}

[Sam McNally, 2015-02-09 06:51:05]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2015-02-09 06:58:51]

Patchset #1 (id:20001) has been deleted

[Sam McNally, 2015-02-09 07:07:28]

Patchset #1 (id:40001) has been deleted

[Sam McNally, 2015-02-09 07:13:08]

Patchset #1 (id:60001) has been deleted

[Sam McNally, 2015-02-09 07:13:14]

Patchset #1 (id:80001) has been deleted

[Sam McNally, 2015-02-09 07:13:29]

Patchset #1 (id:100001) has been deleted

[Sam McNally, 2015-02-09 07:14:08]

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

[Anand Mistry (off Chromium), 2015-02-10 03:54:09]

https://codereview.chromium.org/896203003/diff/120001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_impl.cc (right):

https://codereview.chromium.org/896203003/diff/120001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:57: set_pac_script_callback_ = callback;
DCHECK(set_pac_script_callback_.is_null());

https://codereview.chromium.org/896203003/diff/120001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:66: void
MojoProxyResolverImpl::GetProxyForUrl(
I think it would be nice to have a few DVLOGs scattered around to help debug
when we run this out-of-process. They can always be removed later.

https://codereview.chromium.org/896203003/diff/120001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:72: job->Start(url.To<GURL>());
URL is a property of the job. Shouldn't it be in the constructor? I know, memory
usage, blah, blah, blah. But I'd argue this is insignificant and transient.

https://codereview.chromium.org/896203003/diff/120001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:84: set_pac_script_callback_ =
mojo::Callback<void(int32_t)>();
I've been using .reset() instead. Not sure which one is more correct, but we
should standardise on one.

https://codereview.chromium.org/896203003/diff/120001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/896203003/diff/120001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:24: class TestRequestClient :
public interfaces::ProxyResolverRequestClient,
I wonder if we can combine this with the host resolver test client to factor out
some basic stuff, like waiting for a connection error.

[Sam McNally, 2015-02-10 06:27:42]

https://codereview.chromium.org/896203003/diff/120001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_impl.cc (right):

https://codereview.chromium.org/896203003/diff/120001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:57: set_pac_script_callback_ = callback;
On 2015/02/10 03:54:08, Anand Mistry wrote:
> DCHECK(set_pac_script_callback_.is_null());

Done.

https://codereview.chromium.org/896203003/diff/120001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:66: void
MojoProxyResolverImpl::GetProxyForUrl(
On 2015/02/10 03:54:08, Anand Mistry wrote:
> I think it would be nice to have a few DVLOGs scattered around to help debug
> when we run this out-of-process. They can always be removed later.

Done.

https://codereview.chromium.org/896203003/diff/120001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:72: job->Start(url.To<GURL>());
On 2015/02/10 03:54:08, Anand Mistry wrote:
> URL is a property of the job. Shouldn't it be in the constructor? I know,
memory
> usage, blah, blah, blah. But I'd argue this is insignificant and transient.

Done.

https://codereview.chromium.org/896203003/diff/120001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:84: set_pac_script_callback_ =
mojo::Callback<void(int32_t)>();
On 2015/02/10 03:54:08, Anand Mistry wrote:
> I've been using .reset() instead. Not sure which one is more correct, but we
> should standardise on one.

Changed to reset(). I got used to mojo::Callbacks not having a reset.

https://codereview.chromium.org/896203003/diff/120001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/896203003/diff/120001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:24: class TestRequestClient :
public interfaces::ProxyResolverRequestClient,
On 2015/02/10 03:54:08, Anand Mistry wrote:
> I wonder if we can combine this with the host resolver test client to factor
out
> some basic stuff, like waiting for a connection error.

As discussed, let's wait until we have another similar test client.

[Anand Mistry (off Chromium), 2015-02-12 00:00:22]

lgtm

[Sam McNally, 2015-02-12 01:46:29]

New patchsets have been uploaded after l-g-t-m from amistry@chromium.org

[eroman, 2015-02-12 19:00:42]

Haven't reviewed all files yet, but some initial feedback (will finish after
next meeting).

Overall looks good

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_impl.cc (right):

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:29: void OnConnectionError() override;
Is this also how requests are cancelled from the browser side? (seen as a
connection error by utility side)

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:31: void GetProxyDone(int32_t error);
Same comment as on Anand's CL:
  In the network stack network errors are "int", so code interfacing it should
be specifying "int" and not "int32".

In practice int32 == int for Chrome builds, however let's try to match the net
signatures. (Even better net should have a typedef for NetError, but that is a
separate issue).

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:52:
set_pac_script_callback_.Run(ERR_ABORTED);
Why is the callback run in this case?

Typically in cancellation paths we do not run callbacks, especially deletion, as
it can make things hard to reason about.

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:117: client_.set_error_handler(this);
Is it intentional that the error handler is only set in the async completion
case?

I believe so, but just want to verify.

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:120: void
MojoProxyResolverImpl::Job::GetProxyDone(int32_t error) {
int

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:128: if (error == OK)
Use curly braces around this if statement (more than 1 line).

[eroman, 2015-02-12 22:51:31]

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_impl.h (right):

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.h:40: mojo::Callback<void(int32_t)>
set_pac_script_callback_;
int32_t --> int

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:37: void ReportResult(int32_t
error,
int

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:43: bool done_ = false;
c++11? didn't know you could do that

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:45: int32_t error_ = ERR_FAILED;
int

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:99: base::Callback<void(int32_t)>
CreateCallback();
int

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:103: void ReportResult(int32_t
error);
int

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:105: int32_t error_ = ERR_FAILED;
int

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:108:
base::Callback<void(int32_t)> SetPacScriptClient::CreateCallback() {
int

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_type_con...
File net/proxy/mojo_type_converters.cc (right):

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_type_con...
net/proxy/mojo_type_converters.cc:30: default:
How about omitting default, so will get compiler error when new schemes are
added

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_type_con...
net/proxy/mojo_type_converters.cc:52: default:
Same comment as above.

[Sam McNally, 2015-02-13 00:03:20]

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_impl.cc (right):

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:29: void OnConnectionError() override;
On 2015/02/12 19:00:42, eroman wrote:
> Is this also how requests are cancelled from the browser side? (seen as a
> connection error by utility side)

Yes. Added a comment to make it more obvious.

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:31: void GetProxyDone(int32_t error);
On 2015/02/12 19:00:41, eroman wrote:
> Same comment as on Anand's CL:
>   In the network stack network errors are "int", so code interfacing it should
> be specifying "int" and not "int32".
> 
> In practice int32 == int for Chrome builds, however let's try to match the net
> signatures. (Even better net should have a typedef for NetError, but that is a
> separate issue).

Done.

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:52:
set_pac_script_callback_.Run(ERR_ABORTED);
On 2015/02/12 19:00:42, eroman wrote:
> Why is the callback run in this case?
> 
> Typically in cancellation paths we do not run callbacks, especially deletion,
as
> it can make things hard to reason about.

It was to avoid leaving the client with a hanging callback. Hoever, on further
thought, either the client disconnected, in which case it doesn't care, or we're
disconnecting, in which case the client will find out when it gets an
OnConnectionError call. Removed it.

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:117: client_.set_error_handler(this);
On 2015/02/12 19:00:42, eroman wrote:
> Is it intentional that the error handler is only set in the async completion
> case?
> 
> I believe so, but just want to verify.

Yes, there's no need to set it in the sync case.

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:120: void
MojoProxyResolverImpl::Job::GetProxyDone(int32_t error) {
On 2015/02/12 19:00:42, eroman wrote:
> int

Done.

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:128: if (error == OK)
On 2015/02/12 19:00:41, eroman wrote:
> Use curly braces around this if statement (more than 1 line).

Done.

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_impl.h (right):

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.h:40: mojo::Callback<void(int32_t)>
set_pac_script_callback_;
On 2015/02/12 22:51:30, eroman wrote:
> int32_t --> int

This is a callback to the mojo interface, so I think it should be int32_t.

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:37: void ReportResult(int32_t
error,
On 2015/02/12 22:51:30, eroman wrote:
> int

This is implementing the mojo interfaces, which uses int32_t.

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:43: bool done_ = false;
On 2015/02/12 22:51:30, eroman wrote:
> c++11? didn't know you could do that

Yes!

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:45: int32_t error_ = ERR_FAILED;
On 2015/02/12 22:51:30, eroman wrote:
> int

Done.

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:99: base::Callback<void(int32_t)>
CreateCallback();
On 2015/02/12 22:51:30, eroman wrote:
> int

The callback is passed to a mojo interface.

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:103: void ReportResult(int32_t
error);
On 2015/02/12 22:51:30, eroman wrote:
> int

This is used as a callback passed to a mojo interfaces.

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:105: int32_t error_ = ERR_FAILED;
On 2015/02/12 22:51:30, eroman wrote:
> int

Done.

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl_unittest.cc:108:
base::Callback<void(int32_t)> SetPacScriptClient::CreateCallback() {
On 2015/02/12 22:51:30, eroman wrote:
> int

The callback is passed to a mojo interface.

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_type_con...
File net/proxy/mojo_type_converters.cc (right):

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_type_con...
net/proxy/mojo_type_converters.cc:30: default:
On 2015/02/12 22:51:30, eroman wrote:
> How about omitting default, so will get compiler error when new schemes are
> added

Done.

https://codereview.chromium.org/896203003/diff/180001/net/proxy/mojo_type_con...
net/proxy/mojo_type_converters.cc:52: default:
On 2015/02/12 22:51:30, eroman wrote:
> Same comment as above.

Done.

[Sam McNally, 2015-02-17 02:51:51]

Patchset #9 (id:280001) has been deleted

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

lgtm

[Sam McNally, 2015-02-25 01:44:42]

sammc@chromium.org changed reviewers:
+ jamesr@chromium.org

[Sam McNally, 2015-02-25 01:44:43]

+jamesr for the DEPS change.

[jamesr, 2015-02-25 01:48:04]

lgtm

https://codereview.chromium.org/896203003/diff/360001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_impl.cc (right):

https://codereview.chromium.org/896203003/diff/360001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:46: public:
since you said "struct" the default visibility is "public" already

[Sam McNally, 2015-02-25 02:01:58]

https://codereview.chromium.org/896203003/diff/360001/net/proxy/mojo_proxy_re...
File net/proxy/mojo_proxy_resolver_impl.cc (right):

https://codereview.chromium.org/896203003/diff/360001/net/proxy/mojo_proxy_re...
net/proxy/mojo_proxy_resolver_impl.cc:46: public:
On 2015/02/25 01:48:04, jamesr wrote:
> since you said "struct" the default visibility is "public" already

Done.

[Sam McNally, 2015-02-25 02:02:18]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2015-02-25 02:02:18]

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

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

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

[Sam McNally, 2015-02-25 02:50:33]

Patchset #14 (id:400001) has been deleted

[Sam McNally, 2015-02-25 02:51:31]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2015-02-25 02:51:31]

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

[commit-bot: I haz the power, 2015-02-25 02:52:48]

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

[commit-bot: I haz the power, 2015-02-25 04:59:37]

Committed patchset #14 (id:420001)

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

Patchset 14 (id:??) landed as
https://crrev.com/5403aa1dc4fa777f106c7c311644abf3d3d576ba
Cr-Commit-Position: refs/heads/master@{#317976}