Add support for ProxyResolverErrorObserver to ProxyResolverMojo.

Add support for ProxyResolverErrorObserver to ProxyResolverMojo.

BUG=467832
Committed: https://crrev.com/359a8b7489247d20a4d83d5cc78ae684625f5f13
Cr-Commit-Position: refs/heads/master@{#330081}

[Sam McNally, 2015-03-17 07:20:11]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2015-03-17 07:24:34]

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

[Anand Mistry (off Chromium), 2015-03-20 03:12:32]

https://codereview.chromium.org/1017453005/diff/60001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_error_observer_mojo.cc (right):

https://codereview.chromium.org/1017453005/diff/60001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_error_observer_mojo.cc:16: class
ProxyResolverErrorObserverMojo::Core
I think you can simplify this code and eliminate this class by using a WeakPtr<>
to post to the task runner in OnPACScriptError().

[Sam McNally, 2015-03-20 03:52:41]

https://codereview.chromium.org/1017453005/diff/60001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_error_observer_mojo.cc (right):

https://codereview.chromium.org/1017453005/diff/60001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_error_observer_mojo.cc:16: class
ProxyResolverErrorObserverMojo::Core
On 2015/03/20 03:12:32, Anand Mistry wrote:
> I think you can simplify this code and eliminate this class by using a
WeakPtr<>
> to post to the task runner in OnPACScriptError().

Done.

[Anand Mistry (off Chromium), 2015-03-20 07:48:56]

lgtm

Definitely cleaner.

[eroman, 2015-03-24 00:54:28]

Have you considered instead merging this into the response from
ResolveProxy/SetProxy?

Seems to me like both the error observer, and the netlog can be buffered and
packaged into the final response for easier to understand code and fewer message
pipes.

In the case of the error observer there is no additional delay since the request
is about to fail anyway so I think this is a good fit.

In the case of the NetLog there are more complicated tradeoffs which we should
discuss separately on its design (since buffering of NetLog has some
consequences).

[eroman, 2015-03-24 07:39:49]

Actually my suggestion doesn't work that well, I forgot that the error observer
is global and not per request

[Sam McNally, 2015-05-05 05:57:05]

Patchset #9 (id:180001) has been deleted

[Sam McNally, 2015-05-05 05:59:00]

Patchset #9 (id:200001) has been deleted

[Sam McNally, 2015-05-05 05:59:42]

Patchset #8 (id:160001) has been deleted

[Sam McNally, 2015-05-05 06:25:01]

Patchset #8 (id:210001) has been deleted

[Sam McNally, 2015-05-11 23:32:03]

ping

[eroman, 2015-05-15 07:22:03]

lgtm

https://codereview.chromium.org/1017453005/diff/230001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_error_observer_mojo.cc (right):

https://codereview.chromium.org/1017453005/diff/230001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_error_observer_mojo.cc:29: FROM_HERE,
base::Bind(&ProxyResolverErrorObserverMojo::OnPACScriptError,
So the reason for this is that the mojo interface expects calls on task_runner_
thread?

[Sam McNally, 2015-05-15 09:44:17]

Patchset #10 (id:270001) has been deleted

[Sam McNally, 2015-05-15 09:44:25]

Patchset #10 (id:290001) has been deleted

[Sam McNally, 2015-05-15 09:44:59]

https://codereview.chromium.org/1017453005/diff/230001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_error_observer_mojo.cc (right):

https://codereview.chromium.org/1017453005/diff/230001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_error_observer_mojo.cc:29: FROM_HERE,
base::Bind(&ProxyResolverErrorObserverMojo::OnPACScriptError,
On 2015/05/15 07:22:03, eroman (sick slow) wrote:
> So the reason for this is that the mojo interface expects calls on
task_runner_
> thread?

Yes.

[Sam McNally, 2015-05-15 12:02:09]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2015-05-15 12:02:09]

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

[commit-bot: I haz the power, 2015-05-15 12:02:16]

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

[commit-bot: I haz the power, 2015-05-15 12:21:08]

Committed patchset #10 (id:310001)

[commit-bot: I haz the power, 2015-05-15 12:21:58]

Patchset 10 (id:??) landed as
https://crrev.com/359a8b7489247d20a4d83d5cc78ae684625f5f13
Cr-Commit-Position: refs/heads/master@{#330081}