Split ProxyResolverV8Tracing into an implementation and a wrapper.

Split ProxyResolverV8Tracing into an implementation and a wrapper.

The implementation-specific interfaces use a Bindings interface for
reporting alerts and errors and making DNS requests. This allows a user
to provide an implementation instead of forwarding to a fixed NetLog
and HostResolver.

The wrappers implement the ProxyResolver and ProxyResolverFactory
interfaces, mostly matching the previous behavior. The one exception is
that LoadState change reporting has been removed; the one user
(MojoProxyResolverImpl) will soon use the implementation interface
directly instead.

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

[Sam McNally, 2015-06-02 07:00:06]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2015-06-03 01:20:55]

Patchset #1 (id:20001) has been deleted

[Sam McNally, 2015-06-03 03:01:03]

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

[eroman, 2015-06-03 03:10:37]

Oh btw a heads up: I will be on vacation starting Friday and until June 23rd.

[eroman, 2015-06-05 01:11:49]

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

https://codereview.chromium.org/1145153004/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.h:19: // ProxyResolverV8Tracing is a
non-blocking ProxyResolver. It executes
ProxyResolver --> proxy resolver (no longer implements ProxyResolver)

https://codereview.chromium.org/1145153004/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.h:20: // ProxyResolverV8 on a single helper
thread, and does some magic to avoid
This comment reads strangely now, since this is not actually an implementation
but an interface. Technically all of this only applies to the factory.

https://codereview.chromium.org/1145153004/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.h:28: virtual ~Bindings() {}
It would also be useful to give some comments on what thread destruction
happens, and when.

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

https://codereview.chromium.org/1145153004/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:64: {
no need for this extra scope

https://codereview.chromium.org/1145153004/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:70: {
is this scope needed?
seems fine if the signal is sent while lock_ is held.

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

https://codereview.chromium.org/1145153004/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing_wrapper.cc:133: class
ProxyResolverFactoryV8TracingWrapper::BindingsImpl
This is the same as BindingsImpl above.

Can you extract this to a single impelementation? (I think all you need to do is
change the |factory| dependency to |host_resolver| + |net_log|

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

https://codereview.chromium.org/1145153004/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing_wrapper.h:5: #ifndef
NET_PROXY_PROXY_RESOLVER_V8_TRACING_WRAPPER_H_
technically this file should be proxy_resolver_v8_tracing_wrapper

https://codereview.chromium.org/1145153004/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing_wrapper.h:24: class NET_EXPORT
ProxyResolverFactoryV8TracingWrapper
I am not enamored with the terminology "Wrapper" for this. Do you have any other
suggestions on how to do the namings?

[Sam McNally, 2015-06-16 07:13:38]

Patchset #2 (id:60001) has been deleted

[Sam McNally, 2015-06-16 07:13:45]

Patchset #2 (id:80001) has been deleted

[Sam McNally, 2015-06-17 00:44:34]

Patchset #3 (id:110001) has been deleted

[Sam McNally, 2015-06-23 00:32:47]

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

https://codereview.chromium.org/1145153004/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.h:19: // ProxyResolverV8Tracing is a
non-blocking ProxyResolver. It executes
On 2015/06/05 01:11:47, eroman (vacation till june 24) wrote:
> ProxyResolver --> proxy resolver (no longer implements ProxyResolver)

Done.

https://codereview.chromium.org/1145153004/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.h:20: // ProxyResolverV8 on a single helper
thread, and does some magic to avoid
On 2015/06/05 01:11:47, eroman (vacation till june 24) wrote:
> This comment reads strangely now, since this is not actually an implementation
> but an interface. Technically all of this only applies to the factory.

Done.

https://codereview.chromium.org/1145153004/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing.h:28: virtual ~Bindings() {}
On 2015/06/05 01:11:47, eroman (vacation till june 24) wrote:
> It would also be useful to give some comments on what thread destruction
> happens, and when.

Done.

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

https://codereview.chromium.org/1145153004/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:64: {
On 2015/06/05 01:11:47, eroman (vacation till june 24) wrote:
> no need for this extra scope

Done.

https://codereview.chromium.org/1145153004/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:70: {
On 2015/06/05 01:11:47, eroman (vacation till june 24) wrote:
> is this scope needed?
> seems fine if the signal is sent while lock_ is held.

Done.

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

https://codereview.chromium.org/1145153004/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing_wrapper.cc:133: class
ProxyResolverFactoryV8TracingWrapper::BindingsImpl
On 2015/06/05 01:11:47, eroman (vacation till june 24) wrote:
> This is the same as BindingsImpl above.
> 
> Can you extract this to a single impelementation? (I think all you need to do
is
> change the |factory| dependency to |host_resolver| + |net_log|

Done.

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

https://codereview.chromium.org/1145153004/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing_wrapper.h:5: #ifndef
NET_PROXY_PROXY_RESOLVER_V8_TRACING_WRAPPER_H_
On 2015/06/05 01:11:47, eroman (vacation till june 24) wrote:
> technically this file should be proxy_resolver_v8_tracing_wrapper

I think it already is.

https://codereview.chromium.org/1145153004/diff/40001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8_tracing_wrapper.h:24: class NET_EXPORT
ProxyResolverFactoryV8TracingWrapper
On 2015/06/05 01:11:47, eroman (vacation till june 24) wrote:
> I am not enamored with the terminology "Wrapper" for this. Do you have any
other
> suggestions on how to do the namings?

One alternative I considered was making this ProxyResolver(Factory)V8Tracing
since it actually implements ProxyResolver(Factory), and going with something
else (e.g. TracingV8ProxyResolver(Factory)) for the impl class.

[eroman, 2015-06-26 15:37:25]

lgtm

https://codereview.chromium.org/1145153004/diff/130001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/1145153004/diff/130001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_v8_tracing.cc:19: #include "base/values.h"
Can this be deleted now?

https://codereview.chromium.org/1145153004/diff/130001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_v8_tracing.cc:23: #include "net/log/net_log.h"
can this be deleted?

https://codereview.chromium.org/1145153004/diff/130001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_v8_tracing_wrapper_unittest.cc (right):

https://codereview.chromium.org/1145153004/diff/130001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_v8_tracing_wrapper_unittest.cc:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
Any way to get this file to show up with better diffs? (as it draws from
proxy_resolver_v8_tracing_unittest.cc)

Maybe try re-uploading with "git cl upload --similarity=5" or some such thing.

I didn't review this file since there are too many diffs.

I am guessing this is a duplication of the original
proxy_resolver_v8_tracing_unittest.cc, so unlike the new
proxy_resolver_v8_tracing_unittest.cc it tests the netlog stuff?

Not thrilled about the duplication, but probably OK. Will get a better sense
when the diffs are cleaner, but we might simplify the testing. We can iterate on
this later.

[Sam McNally, 2015-06-29 00:29:06]

Patchset #4 (id:150001) has been deleted

[Sam McNally, 2015-06-29 01:33:51]

https://codereview.chromium.org/1145153004/diff/130001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/1145153004/diff/130001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_v8_tracing.cc:19: #include "base/values.h"
On 2015/06/26 15:37:25, eroman (back in office slow) wrote:
> Can this be deleted now?

Done.

https://codereview.chromium.org/1145153004/diff/130001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_v8_tracing.cc:23: #include "net/log/net_log.h"
On 2015/06/26 15:37:25, eroman (back in office slow) wrote:
> can this be deleted?

Done.

https://codereview.chromium.org/1145153004/diff/130001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_v8_tracing_wrapper_unittest.cc (right):

https://codereview.chromium.org/1145153004/diff/130001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_v8_tracing_wrapper_unittest.cc:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
On 2015/06/26 15:37:25, eroman (back in office slow) wrote:
> Any way to get this file to show up with better diffs? (as it draws from
> proxy_resolver_v8_tracing_unittest.cc)
> 
> Maybe try re-uploading with "git cl upload --similarity=5" or some such thing.

Done.
> 
> I didn't review this file since there are too many diffs.
> 
> I am guessing this is a duplication of the original
> proxy_resolver_v8_tracing_unittest.cc, so unlike the new
> proxy_resolver_v8_tracing_unittest.cc it tests the netlog stuff?

Yes.
> 
> Not thrilled about the duplication, but probably OK. Will get a better sense
> when the diffs are cleaner, but we might simplify the testing. We can iterate
on
> this later.

[Sam McNally, 2015-06-29 01:36:31]

Patchset #5 (id:190001) has been deleted

[Sam McNally, 2015-06-29 01:37:01]

Patchset #5 (id:210001) has been deleted

[Sam McNally, 2015-06-29 01:37:41]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2015-06-29 01:37:41]

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/1145153004/#ps230001
(title: " ")

[commit-bot: I haz the power, 2015-06-29 01:37:51]

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

[commit-bot: I haz the power, 2015-06-29 02:58:53]

Committed patchset #5 (id:230001)

[commit-bot: I haz the power, 2015-06-29 02:59:52]

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