Add ProxyResolverFactoryV8Tracing.

Add ProxyResolverFactoryV8Tracing.

This changes ProxyResolverV8Tracing to use the new ProxyResolver and
ProxyResolverFactory interfaces.

BUG=467403
NOPRESUBMIT=true

Committed: https://crrev.com/3b81bc1ac3ea712d448562dbcd7f5b35b58dbcf3
Cr-Commit-Position: refs/heads/master@{#330074}

[Sam McNally, 2015-05-04 04:05:14]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2015-05-04 06:24:59]

Patchset #1 (id:20001) has been deleted

[Sam McNally, 2015-05-04 06:26:01]

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

[eroman, 2015-05-05 00:04:55]

I started reviewing this, but then had a bigger design-level question to ask:

ProxyResolverFactoryForV8Resolver doesn't seem like a good fit for inheriting
from ProxyResolverFactory.

All the consumers which use this derive their own subclass, however exposing the
base factory is not super clean.

How about instead just having a global factory method like:

scoped_ptr<XXXRequest> CreateProxyResolverV8Tracing(
    HostResolver* host_resolver,	
    ProxyResolverErrorObserver* error_observer,	
    NetLog* net_log,	
    const LoadStateChangedCallback& on_load_state_changed,
    const CreationCallback& callback);

Where CreationCallback() is called with a net::Error and a
scoped_ptr<ProxyResolver>.

Or the regular net style of returning ERR_IO_PENDING (however in this case there
is only the possiblity of async completion so can probably simplify).

WDYT?

Probably also possible to re-use more code from the existing SetPacScript()  /
CancelPacScript() code paths with the shim for the factory, but I haven't read
that in too much detail yet.

https://codereview.chromium.org/1126513002/diff/40001/net/data/proxy_resolver...
File net/data/proxy_resolver_v8_tracing_unittest/error_on_load.js (right):

https://codereview.chromium.org/1126513002/diff/40001/net/data/proxy_resolver...
net/data/proxy_resolver_v8_tracing_unittest/error_on_load.js:1: alert('Prepare
to DIE!');
Note: You may hit some presubmit warning about lack of copyright (we have
something baked in that enforces it on all *.js). If you need to add a copyright
line don't worry about getting another review pass for it.

https://codereview.chromium.org/1126513002/diff/40001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/1126513002/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.cc:71: // SetPacScript(), since both of
these operations share similar code.
SetPacScript() no longer applies. please change this comment.

https://codereview.chromium.org/1126513002/diff/40001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_v8_tracing.h (right):

https://codereview.chromium.org/1126513002/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.h:23: // ProxyResolverV8Tracing is a
non-blocking ProxyResolver. It executes
This reads a bit funny because the class below is the Factory. Can you change
this in a way to clarify?

https://codereview.chromium.org/1126513002/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.h:29: // Note that the constructor takes
ownership of |error_observer|, whereas
remove reference to |error_observer|

https://codereview.chromium.org/1126513002/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.h:35: const
ProxyResolver::LoadStateChangedCallback& callback);
This seems weird to me -- one callback that applies to every instance created.
While it works for the mojo usecase, feels odd to me.

https://codereview.chromium.org/1126513002/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.h:52: virtual
scoped_ptr<ProxyResolverErrorObserver> CreateErrorObserver() = 0;
Why private and not protected? C++ lets you change the visibility in subclasses,
but still feels like this is conceptually a "protected" method or a public one
even.

[Sam McNally, 2015-05-05 01:31:37]

On 2015/05/05 00:04:55, eroman wrote:
> I started reviewing this, but then had a bigger design-level question to ask:
> 
> ProxyResolverFactoryForV8Resolver doesn't seem like a good fit for inheriting
> from ProxyResolverFactory.
> 
> All the consumers which use this derive their own subclass, however exposing
the
> base factory is not super clean.
> 
> How about instead just having a global factory method like:
> 
> scoped_ptr<XXXRequest> CreateProxyResolverV8Tracing(
>     HostResolver* host_resolver,	
>     ProxyResolverErrorObserver* error_observer,	
>     NetLog* net_log,	
>     const LoadStateChangedCallback& on_load_state_changed,
>     const CreationCallback& callback);
> 
> Where CreationCallback() is called with a net::Error and a
> scoped_ptr<ProxyResolver>.
> 
> Or the regular net style of returning ERR_IO_PENDING (however in this case
there
> is only the possiblity of async completion so can probably simplify).
> 
> WDYT?
> 
Looking at this some more, I think a error observer factory callback is a nicer
approach than each client writing a subclass. This avoids the need for
ProxyResolverFactoryForV8Resolver entirely.

https://codereview.chromium.org/1126513002/diff/40001/net/data/proxy_resolver...
File net/data/proxy_resolver_v8_tracing_unittest/error_on_load.js (right):

https://codereview.chromium.org/1126513002/diff/40001/net/data/proxy_resolver...
net/data/proxy_resolver_v8_tracing_unittest/error_on_load.js:1: alert('Prepare
to DIE!');
On 2015/05/05 00:04:55, eroman wrote:
> Note: You may hit some presubmit warning about lack of copyright (we have
> something baked in that enforces it on all *.js). If you need to add a
copyright
> line don't worry about getting another review pass for it.

Done. The presubmit's already unhappy about the extra ScopedAllowIO.

https://codereview.chromium.org/1126513002/diff/40001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/1126513002/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.cc:71: // SetPacScript(), since both of
these operations share similar code.
On 2015/05/05 00:04:55, eroman wrote:
> SetPacScript() no longer applies. please change this comment.

Done.

https://codereview.chromium.org/1126513002/diff/40001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_v8_tracing.h (right):

https://codereview.chromium.org/1126513002/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.h:23: // ProxyResolverV8Tracing is a
non-blocking ProxyResolver. It executes
On 2015/05/05 00:04:55, eroman wrote:
> This reads a bit funny because the class below is the Factory. Can you change
> this in a way to clarify?

Done.

https://codereview.chromium.org/1126513002/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.h:29: // Note that the constructor takes
ownership of |error_observer|, whereas
On 2015/05/05 00:04:55, eroman wrote:
> remove reference to |error_observer|

Done.

https://codereview.chromium.org/1126513002/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.h:35: const
ProxyResolver::LoadStateChangedCallback& callback);
On 2015/05/05 00:04:55, eroman wrote:
> This seems weird to me -- one callback that applies to every instance created.
> While it works for the mojo usecase, feels odd to me.

This is another place where it feels like this might belong on the interface
instead. I think it isn't too bad given how this factory is used: at most one
ProxyResolver instance from each factory instance exists at a time.

[eroman, 2015-05-15 07:39:38]

lgtm

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

https://codereview.chromium.org/1126513002/diff/60001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.cc:112: // |parent| is non-owned. It is the
ProxyResolverV8Tracing that spawned this
comment no longer applicable

https://codereview.chromium.org/1126513002/diff/60001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.cc:232: // The ProxyResolverV8Tracing which
spawned this Job.
comment no longer applicable

[Sam McNally, 2015-05-15 09:20:19]

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

https://codereview.chromium.org/1126513002/diff/60001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.cc:112: // |parent| is non-owned. It is the
ProxyResolverV8Tracing that spawned this
On 2015/05/15 07:39:37, eroman (sick slow) wrote:
> comment no longer applicable

Done.

https://codereview.chromium.org/1126513002/diff/60001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.cc:232: // The ProxyResolverV8Tracing which
spawned this Job.
On 2015/05/15 07:39:38, eroman (sick slow) wrote:
> comment no longer applicable

Done.

[Sam McNally, 2015-05-15 11:10:59]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2015-05-15 11:11:00]

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

[commit-bot: I haz the power, 2015-05-15 11:11:05]

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

[commit-bot: I haz the power, 2015-05-15 11:11:57]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-05-15 11:12:55]

Patchset 5 (id:??) landed as
https://crrev.com/3b81bc1ac3ea712d448562dbcd7f5b35b58dbcf3
Cr-Commit-Position: refs/heads/master@{#330074}