Add Finch experiment for selectively bypassing proxies.

Add Finch experiment for selectively bypassing proxies.

Add option to bypass the data compression proxy if the request resource
type (as inferred by the renderer process) is not an image.

For background, see this design doc:

https://docs.google.com/a/google.com/document/d/1Kz92Fmw3lv_R-2aNvLp8jW9lkfKOZciTZtni2qQ_Adc/edit

BUG=391836
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281951

[mmenke, 2014-06-18 14:44:38]

This is pretty ugly.  Can we just attach a LOAD_FLAGS_BYPASS_PROXY when using
flywheel for non-image requests, or do we support using another proxy in
addition to flywheel?

[mmenke, 2014-06-18 14:48:37]

On 2014/06/18 14:44:38, mmenke wrote:
> This is pretty ugly.  Can we just attach a LOAD_FLAGS_BYPASS_PROXY when using
> flywheel for non-image requests, or do we support using another proxy in
> addition to flywheel?

"Ugly" meaning net/ really shouldn't know about this.  I didn't know you would
have net/ read the data, when I supported using SupportsUserData.  That class
exists specifically so net/ doesn't have to know about the data.  You need to
retrieve the information at the proxy level, where we don't have access to the
URLRequest, so I understand the reason, but we need to find another way.

[rcs, 2014-06-18 16:41:41]

On 2014/06/18 14:48:37, mmenke wrote:
> On 2014/06/18 14:44:38, mmenke wrote:
> > This is pretty ugly.  Can we just attach a LOAD_FLAGS_BYPASS_PROXY when
using
> > flywheel for non-image requests, or do we support using another proxy in
> > addition to flywheel?
> 
> "Ugly" meaning net/ really shouldn't know about this.  I didn't know you would
> have net/ read the data, when I supported using SupportsUserData.  That class
> exists specifically so net/ doesn't have to know about the data.  You need to
> retrieve the information at the proxy level, where we don't have access to the
> URLRequest, so I understand the reason, but we need to find another way.


Agreed about the uglyness :-)

Incidentally, the LOAD_FLAGS_BYPASS_PROXY trick was exactly what I used for my
initial prototype. As you mentioned, we do need to support the possibility of
other proxies. And AFAICT, only the ProxyResolverService knows whether the
effective proxy is Flywheel. 

Do you know of other ways to get information about the effective proxy?

I guess one possibility would be to invoke ProxyResolverService before the
URLRequest makes it into net/, then attach the LOAD_FLAG iff the effective proxy
is the data compression proxy. But that seems like a bad separation of concerns
to me. Thoughts?

[mmenke, 2014-06-18 18:10:39]

On 2014/06/18 16:41:41, rcs wrote:
> On 2014/06/18 14:48:37, mmenke wrote:
> > On 2014/06/18 14:44:38, mmenke wrote:
> > > This is pretty ugly.  Can we just attach a LOAD_FLAGS_BYPASS_PROXY when
> using
> > > flywheel for non-image requests, or do we support using another proxy in
> > > addition to flywheel?
> > 
> > "Ugly" meaning net/ really shouldn't know about this.  I didn't know you
would
> > have net/ read the data, when I supported using SupportsUserData.  That
class
> > exists specifically so net/ doesn't have to know about the data.  You need
to
> > retrieve the information at the proxy level, where we don't have access to
the
> > URLRequest, so I understand the reason, but we need to find another way.
> 
> 
> Agreed about the uglyness :-)
> 
> Incidentally, the LOAD_FLAGS_BYPASS_PROXY trick was exactly what I used for my
> initial prototype. As you mentioned, we do need to support the possibility of
> other proxies. And AFAICT, only the ProxyResolverService knows whether the
> effective proxy is Flywheel. 
> 
> Do you know of other ways to get information about the effective proxy?
> 
> I guess one possibility would be to invoke ProxyResolverService before the
> URLRequest makes it into net/, then attach the LOAD_FLAG iff the effective
proxy
> is the data compression proxy. But that seems like a bad separation of
concerns
> to me. Thoughts?

Ben has a CL out to add headers through a callback for specific proxies
(https://codereview.chromium.org/333113002/).  We could do something similar to
let the delegate reject certain proxies for a request, and then just move on to
the next.  I still find the extra callbacks less than ideal, but I think it's a
cleaner approach than this.

[rcs, 2014-06-18 22:07:59]

On 2014/06/18 18:10:39, mmenke wrote:
> On 2014/06/18 16:41:41, rcs wrote:
> > On 2014/06/18 14:48:37, mmenke wrote:
> > > On 2014/06/18 14:44:38, mmenke wrote:
> > > > This is pretty ugly.  Can we just attach a LOAD_FLAGS_BYPASS_PROXY when
> > using
> > > > flywheel for non-image requests, or do we support using another proxy in
> > > > addition to flywheel?
> > > 
> > > "Ugly" meaning net/ really shouldn't know about this.  I didn't know you
> would
> > > have net/ read the data, when I supported using SupportsUserData.  That
> class
> > > exists specifically so net/ doesn't have to know about the data.  You need
> to
> > > retrieve the information at the proxy level, where we don't have access to
> the
> > > URLRequest, so I understand the reason, but we need to find another way.
> > 
> > 
> > Agreed about the uglyness :-)
> > 
> > Incidentally, the LOAD_FLAGS_BYPASS_PROXY trick was exactly what I used for
my
> > initial prototype. As you mentioned, we do need to support the possibility
of
> > other proxies. And AFAICT, only the ProxyResolverService knows whether the
> > effective proxy is Flywheel. 
> > 
> > Do you know of other ways to get information about the effective proxy?
> > 
> > I guess one possibility would be to invoke ProxyResolverService before the
> > URLRequest makes it into net/, then attach the LOAD_FLAG iff the effective
> proxy
> > is the data compression proxy. But that seems like a bad separation of
> concerns
> > to me. Thoughts?
> 
> Ben has a CL out to add headers through a callback for specific proxies
> (https://codereview.chromium.org/333113002/).  We could do something similar
to
> let the delegate reject certain proxies for a request, and then just move on
to
> the next.  I still find the extra callbacks less than ideal, but I think it's
a
> cleaner approach than this.

Double checking that I understand the proposal correctly:
  - Add a callback "OnResolveProxy" to NetworkDelegate
  - Plumb a reference to NetworkDelegate to ProxyService, and have ProxyService
invoke NetworkDelegate.OnResolveProxy before returning from ResolveProxy()

Or did you a different point of invocation in mind?

[mmenke, 2014-06-19 15:27:41]

On 2014/06/18 22:07:59, rcs wrote:
> On 2014/06/18 18:10:39, mmenke wrote:
> > On 2014/06/18 16:41:41, rcs wrote:
> > > On 2014/06/18 14:48:37, mmenke wrote:
> > > > On 2014/06/18 14:44:38, mmenke wrote:
> > > > > This is pretty ugly.  Can we just attach a LOAD_FLAGS_BYPASS_PROXY
when
> > > using
> > > > > flywheel for non-image requests, or do we support using another proxy
in
> > > > > addition to flywheel?
> > > > 
> > > > "Ugly" meaning net/ really shouldn't know about this.  I didn't know you
> > would
> > > > have net/ read the data, when I supported using SupportsUserData.  That
> > class
> > > > exists specifically so net/ doesn't have to know about the data.  You
need
> > to
> > > > retrieve the information at the proxy level, where we don't have access
to
> > the
> > > > URLRequest, so I understand the reason, but we need to find another way.
> > > 
> > > 
> > > Agreed about the uglyness :-)
> > > 
> > > Incidentally, the LOAD_FLAGS_BYPASS_PROXY trick was exactly what I used
for
> my
> > > initial prototype. As you mentioned, we do need to support the possibility
> of
> > > other proxies. And AFAICT, only the ProxyResolverService knows whether the
> > > effective proxy is Flywheel. 
> > > 
> > > Do you know of other ways to get information about the effective proxy?
> > > 
> > > I guess one possibility would be to invoke ProxyResolverService before the
> > > URLRequest makes it into net/, then attach the LOAD_FLAG iff the effective
> > proxy
> > > is the data compression proxy. But that seems like a bad separation of
> > concerns
> > > to me. Thoughts?
> > 
> > Ben has a CL out to add headers through a callback for specific proxies
> > (https://codereview.chromium.org/333113002/).  We could do something similar
> to
> > let the delegate reject certain proxies for a request, and then just move on
> to
> > the next.  I still find the extra callbacks less than ideal, but I think
it's
> a
> > cleaner approach than this.
> 
> Double checking that I understand the proposal correctly:
>   - Add a callback "OnResolveProxy" to NetworkDelegate
>   - Plumb a reference to NetworkDelegate to ProxyService, and have
ProxyService
> invoke NetworkDelegate.OnResolveProxy before returning from ResolveProxy()
> 
> Or did you a different point of invocation in mind?

That's the basic idea, though you should probably run it by eroman, who knows
the proxy code much better than I.

Also, instead of storing request type use SupportsUserData, you might want to
consider moving it into ResourceRequestInfo.  AppCache, at least, needs that
information as well, so seems like others may want it as well, if we're not
already storing it somewhere (No need to modify AppCache to use the new field).

[mmenke, 2014-06-19 15:30:44]

On 2014/06/19 15:27:41, mmenke wrote:
> On 2014/06/18 22:07:59, rcs wrote:
> > On 2014/06/18 18:10:39, mmenke wrote:
> > > On 2014/06/18 16:41:41, rcs wrote:
> > > > On 2014/06/18 14:48:37, mmenke wrote:
> > > > > On 2014/06/18 14:44:38, mmenke wrote:
> > > > > > This is pretty ugly.  Can we just attach a LOAD_FLAGS_BYPASS_PROXY
> when
> > > > using
> > > > > > flywheel for non-image requests, or do we support using another
proxy
> in
> > > > > > addition to flywheel?
> > > > > 
> > > > > "Ugly" meaning net/ really shouldn't know about this.  I didn't know
you
> > > would
> > > > > have net/ read the data, when I supported using SupportsUserData. 
That
> > > class
> > > > > exists specifically so net/ doesn't have to know about the data.  You
> need
> > > to
> > > > > retrieve the information at the proxy level, where we don't have
access
> to
> > > the
> > > > > URLRequest, so I understand the reason, but we need to find another
way.
> > > > 
> > > > 
> > > > Agreed about the uglyness :-)
> > > > 
> > > > Incidentally, the LOAD_FLAGS_BYPASS_PROXY trick was exactly what I used
> for
> > my
> > > > initial prototype. As you mentioned, we do need to support the
possibility
> > of
> > > > other proxies. And AFAICT, only the ProxyResolverService knows whether
the
> > > > effective proxy is Flywheel. 
> > > > 
> > > > Do you know of other ways to get information about the effective proxy?
> > > > 
> > > > I guess one possibility would be to invoke ProxyResolverService before
the
> > > > URLRequest makes it into net/, then attach the LOAD_FLAG iff the
effective
> > > proxy
> > > > is the data compression proxy. But that seems like a bad separation of
> > > concerns
> > > > to me. Thoughts?
> > > 
> > > Ben has a CL out to add headers through a callback for specific proxies
> > > (https://codereview.chromium.org/333113002/).  We could do something
similar
> > to
> > > let the delegate reject certain proxies for a request, and then just move
on
> > to
> > > the next.  I still find the extra callbacks less than ideal, but I think
> it's
> > a
> > > cleaner approach than this.
> > 
> > Double checking that I understand the proposal correctly:
> >   - Add a callback "OnResolveProxy" to NetworkDelegate
> >   - Plumb a reference to NetworkDelegate to ProxyService, and have
> ProxyService
> > invoke NetworkDelegate.OnResolveProxy before returning from ResolveProxy()
> > 
> > Or did you a different point of invocation in mind?
> 
> That's the basic idea, though you should probably run it by eroman, who knows
> the proxy code much better than I.
> 
> Also, instead of storing request type use SupportsUserData, you might want to
> consider moving it into ResourceRequestInfo.  AppCache, at least, needs that
> information as well, so seems like others may want it as well, if we're not
> already storing it somewhere (No need to modify AppCache to use the new
field).

Actually, it looks like ResourceRequestInfo already as the ResourceType, so I
think you can just grab it there, without adding any new code to store it.  See
content::ResourceRequestInfo::ForRequest.

[rcs, 2014-06-19 17:42:23]

On 2014/06/19 15:30:44, mmenke wrote:
> On 2014/06/19 15:27:41, mmenke wrote:
> > On 2014/06/18 22:07:59, rcs wrote:
> > > On 2014/06/18 18:10:39, mmenke wrote:
> > > > On 2014/06/18 16:41:41, rcs wrote:
> > > > > On 2014/06/18 14:48:37, mmenke wrote:
> > > > > > On 2014/06/18 14:44:38, mmenke wrote:
> > > > > > > This is pretty ugly.  Can we just attach a LOAD_FLAGS_BYPASS_PROXY
> > when
> > > > > using
> > > > > > > flywheel for non-image requests, or do we support using another
> proxy
> > in
> > > > > > > addition to flywheel?
> > > > > > 
> > > > > > "Ugly" meaning net/ really shouldn't know about this.  I didn't know
> you
> > > > would
> > > > > > have net/ read the data, when I supported using SupportsUserData. 
> That
> > > > class
> > > > > > exists specifically so net/ doesn't have to know about the data. 
You
> > need
> > > > to
> > > > > > retrieve the information at the proxy level, where we don't have
> access
> > to
> > > > the
> > > > > > URLRequest, so I understand the reason, but we need to find another
> way.
> > > > > 
> > > > > 
> > > > > Agreed about the uglyness :-)
> > > > > 
> > > > > Incidentally, the LOAD_FLAGS_BYPASS_PROXY trick was exactly what I
used
> > for
> > > my
> > > > > initial prototype. As you mentioned, we do need to support the
> possibility
> > > of
> > > > > other proxies. And AFAICT, only the ProxyResolverService knows whether
> the
> > > > > effective proxy is Flywheel. 
> > > > > 
> > > > > Do you know of other ways to get information about the effective
proxy?
> > > > > 
> > > > > I guess one possibility would be to invoke ProxyResolverService before
> the
> > > > > URLRequest makes it into net/, then attach the LOAD_FLAG iff the
> effective
> > > > proxy
> > > > > is the data compression proxy. But that seems like a bad separation of
> > > > concerns
> > > > > to me. Thoughts?
> > > > 
> > > > Ben has a CL out to add headers through a callback for specific proxies
> > > > (https://codereview.chromium.org/333113002/).  We could do something
> similar
> > > to
> > > > let the delegate reject certain proxies for a request, and then just
move
> on
> > > to
> > > > the next.  I still find the extra callbacks less than ideal, but I think
> > it's
> > > a
> > > > cleaner approach than this.
> > > 
> > > Double checking that I understand the proposal correctly:
> > >   - Add a callback "OnResolveProxy" to NetworkDelegate
> > >   - Plumb a reference to NetworkDelegate to ProxyService, and have
> > ProxyService
> > > invoke NetworkDelegate.OnResolveProxy before returning from ResolveProxy()
> > > 
> > > Or did you a different point of invocation in mind?
> > 
> > That's the basic idea, though you should probably run it by eroman, who
knows
> > the proxy code much better than I.
> > 
> > Also, instead of storing request type use SupportsUserData, you might want
to
> > consider moving it into ResourceRequestInfo.  AppCache, at least, needs that
> > information as well, so seems like others may want it as well, if we're not
> > already storing it somewhere (No need to modify AppCache to use the new
> field).
> 
> Actually, it looks like ResourceRequestInfo already as the ResourceType, so I
> think you can just grab it there, without adding any new code to store it. 
See
> content::ResourceRequestInfo::ForRequest.

Incidentally, I started out by using content::ResourceRequestInfo, but realized
that net/ can't depend on it without introducing a layering violation. If we're
invoking it from a callback though, maybe that isn't a problem.

[mmenke, 2014-06-19 17:45:22]

On 2014/06/19 17:42:23, rcs wrote:
> On 2014/06/19 15:30:44, mmenke wrote:
> > On 2014/06/19 15:27:41, mmenke wrote:
> > > On 2014/06/18 22:07:59, rcs wrote:
> > > > On 2014/06/18 18:10:39, mmenke wrote:
> > > > > On 2014/06/18 16:41:41, rcs wrote:
> > > > > > On 2014/06/18 14:48:37, mmenke wrote:
> > > > > > > On 2014/06/18 14:44:38, mmenke wrote:
> > > > > > > > This is pretty ugly.  Can we just attach a
LOAD_FLAGS_BYPASS_PROXY
> > > when
> > > > > > using
> > > > > > > > flywheel for non-image requests, or do we support using another
> > proxy
> > > in
> > > > > > > > addition to flywheel?
> > > > > > > 
> > > > > > > "Ugly" meaning net/ really shouldn't know about this.  I didn't
know
> > you
> > > > > would
> > > > > > > have net/ read the data, when I supported using SupportsUserData. 
> > That
> > > > > class
> > > > > > > exists specifically so net/ doesn't have to know about the data. 
> You
> > > need
> > > > > to
> > > > > > > retrieve the information at the proxy level, where we don't have
> > access
> > > to
> > > > > the
> > > > > > > URLRequest, so I understand the reason, but we need to find
another
> > way.
> > > > > > 
> > > > > > 
> > > > > > Agreed about the uglyness :-)
> > > > > > 
> > > > > > Incidentally, the LOAD_FLAGS_BYPASS_PROXY trick was exactly what I
> used
> > > for
> > > > my
> > > > > > initial prototype. As you mentioned, we do need to support the
> > possibility
> > > > of
> > > > > > other proxies. And AFAICT, only the ProxyResolverService knows
whether
> > the
> > > > > > effective proxy is Flywheel. 
> > > > > > 
> > > > > > Do you know of other ways to get information about the effective
> proxy?
> > > > > > 
> > > > > > I guess one possibility would be to invoke ProxyResolverService
before
> > the
> > > > > > URLRequest makes it into net/, then attach the LOAD_FLAG iff the
> > effective
> > > > > proxy
> > > > > > is the data compression proxy. But that seems like a bad separation
of
> > > > > concerns
> > > > > > to me. Thoughts?
> > > > > 
> > > > > Ben has a CL out to add headers through a callback for specific
proxies
> > > > > (https://codereview.chromium.org/333113002/).  We could do something
> > similar
> > > > to
> > > > > let the delegate reject certain proxies for a request, and then just
> move
> > on
> > > > to
> > > > > the next.  I still find the extra callbacks less than ideal, but I
think
> > > it's
> > > > a
> > > > > cleaner approach than this.
> > > > 
> > > > Double checking that I understand the proposal correctly:
> > > >   - Add a callback "OnResolveProxy" to NetworkDelegate
> > > >   - Plumb a reference to NetworkDelegate to ProxyService, and have
> > > ProxyService
> > > > invoke NetworkDelegate.OnResolveProxy before returning from
ResolveProxy()
> > > > 
> > > > Or did you a different point of invocation in mind?
> > > 
> > > That's the basic idea, though you should probably run it by eroman, who
> knows
> > > the proxy code much better than I.
> > > 
> > > Also, instead of storing request type use SupportsUserData, you might want
> to
> > > consider moving it into ResourceRequestInfo.  AppCache, at least, needs
that
> > > information as well, so seems like others may want it as well, if we're
not
> > > already storing it somewhere (No need to modify AppCache to use the new
> > field).
> > 
> > Actually, it looks like ResourceRequestInfo already as the ResourceType, so
I
> > think you can just grab it there, without adding any new code to store it. 
> See
> > content::ResourceRequestInfo::ForRequest.
> 
> Incidentally, I started out by using content::ResourceRequestInfo, but
realized
> that net/ can't depend on it without introducing a layering violation. If
we're
> invoking it from a callback though, maybe that isn't a problem.

The NetworkDelegate callback should go through the URLRequest or URLRequestJob,
both of which can send the request along.  From the callback, you can grab the
info without causing a layering violation.

[rcs, 2014-06-20 01:13:35]

On 2014/06/19 17:45:22, mmenke wrote:
> On 2014/06/19 17:42:23, rcs wrote:
> > On 2014/06/19 15:30:44, mmenke wrote:
> > > On 2014/06/19 15:27:41, mmenke wrote:
> > > > On 2014/06/18 22:07:59, rcs wrote:
> > > > > On 2014/06/18 18:10:39, mmenke wrote:
> > > > > > On 2014/06/18 16:41:41, rcs wrote:
> > > > > > > On 2014/06/18 14:48:37, mmenke wrote:
> > > > > > > > On 2014/06/18 14:44:38, mmenke wrote:
> > > > > > > > > This is pretty ugly.  Can we just attach a
> LOAD_FLAGS_BYPASS_PROXY
> > > > when
> > > > > > > using
> > > > > > > > > flywheel for non-image requests, or do we support using
another
> > > proxy
> > > > in
> > > > > > > > > addition to flywheel?
> > > > > > > > 
> > > > > > > > "Ugly" meaning net/ really shouldn't know about this.  I didn't
> know
> > > you
> > > > > > would
> > > > > > > > have net/ read the data, when I supported using
SupportsUserData. 
> > > That
> > > > > > class
> > > > > > > > exists specifically so net/ doesn't have to know about the data.

> > You
> > > > need
> > > > > > to
> > > > > > > > retrieve the information at the proxy level, where we don't have
> > > access
> > > > to
> > > > > > the
> > > > > > > > URLRequest, so I understand the reason, but we need to find
> another
> > > way.
> > > > > > > 
> > > > > > > 
> > > > > > > Agreed about the uglyness :-)
> > > > > > > 
> > > > > > > Incidentally, the LOAD_FLAGS_BYPASS_PROXY trick was exactly what I
> > used
> > > > for
> > > > > my
> > > > > > > initial prototype. As you mentioned, we do need to support the
> > > possibility
> > > > > of
> > > > > > > other proxies. And AFAICT, only the ProxyResolverService knows
> whether
> > > the
> > > > > > > effective proxy is Flywheel. 
> > > > > > > 
> > > > > > > Do you know of other ways to get information about the effective
> > proxy?
> > > > > > > 
> > > > > > > I guess one possibility would be to invoke ProxyResolverService
> before
> > > the
> > > > > > > URLRequest makes it into net/, then attach the LOAD_FLAG iff the
> > > effective
> > > > > > proxy
> > > > > > > is the data compression proxy. But that seems like a bad
separation
> of
> > > > > > concerns
> > > > > > > to me. Thoughts?
> > > > > > 
> > > > > > Ben has a CL out to add headers through a callback for specific
> proxies
> > > > > > (https://codereview.chromium.org/333113002/).  We could do something
> > > similar
> > > > > to
> > > > > > let the delegate reject certain proxies for a request, and then just
> > move
> > > on
> > > > > to
> > > > > > the next.  I still find the extra callbacks less than ideal, but I
> think
> > > > it's
> > > > > a
> > > > > > cleaner approach than this.
> > > > > 
> > > > > Double checking that I understand the proposal correctly:
> > > > >   - Add a callback "OnResolveProxy" to NetworkDelegate
> > > > >   - Plumb a reference to NetworkDelegate to ProxyService, and have
> > > > ProxyService
> > > > > invoke NetworkDelegate.OnResolveProxy before returning from
> ResolveProxy()
> > > > > 
> > > > > Or did you a different point of invocation in mind?
> > > > 
> > > > That's the basic idea, though you should probably run it by eroman, who
> > knows
> > > > the proxy code much better than I.
> > > > 
> > > > Also, instead of storing request type use SupportsUserData, you might
want
> > to
> > > > consider moving it into ResourceRequestInfo.  AppCache, at least, needs
> that
> > > > information as well, so seems like others may want it as well, if we're
> not
> > > > already storing it somewhere (No need to modify AppCache to use the new
> > > field).
> > > 
> > > Actually, it looks like ResourceRequestInfo already as the ResourceType,
so
> I
> > > think you can just grab it there, without adding any new code to store it.

> > See
> > > content::ResourceRequestInfo::ForRequest.
> > 
> > Incidentally, I started out by using content::ResourceRequestInfo, but
> realized
> > that net/ can't depend on it without introducing a layering violation. If
> we're
> > invoking it from a callback though, maybe that isn't a problem.
> 
> The NetworkDelegate callback should go through the URLRequest or
URLRequestJob,
> both of which can send the request along.  From the callback, you can grab the
> info without causing a layering violation.

Hey Matt,

I'm currently at a conceptual roadblock. The point where ResolveProxy() is
called is in HTTPStreamFactory:

https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_stre...

HTTPStreamFactory does not have a reference to any URLRequest objects -- only
HTTPRequestInfo objects. And your proposal assumes that we have a reference to
the original URLRequest object (so that we can invoke
content::ResourceRequestInfo::ForRequest to obtain the resource_type).

Are you suggesting that, within URLRequest, we curry the URLRequest reference
into the NetworkDelgate callback, then pass the callback to the
HTTPStreamFactory to be invoked?

[mmenke, 2014-06-20 17:21:17]

On 2014/06/20 01:13:35, rcs wrote:
> On 2014/06/19 17:45:22, mmenke wrote:
> > On 2014/06/19 17:42:23, rcs wrote:
> > > On 2014/06/19 15:30:44, mmenke wrote:
> > > > On 2014/06/19 15:27:41, mmenke wrote:
> > > > > On 2014/06/18 22:07:59, rcs wrote:
> > > > > > On 2014/06/18 18:10:39, mmenke wrote:
> > > > > > > On 2014/06/18 16:41:41, rcs wrote:
> > > > > > > > On 2014/06/18 14:48:37, mmenke wrote:
> > > > > > > > > On 2014/06/18 14:44:38, mmenke wrote:
> > > > > > > > > > This is pretty ugly.  Can we just attach a
> > LOAD_FLAGS_BYPASS_PROXY
> > > > > when
> > > > > > > > using
> > > > > > > > > > flywheel for non-image requests, or do we support using
> another
> > > > proxy
> > > > > in
> > > > > > > > > > addition to flywheel?
> > > > > > > > > 
> > > > > > > > > "Ugly" meaning net/ really shouldn't know about this.  I
didn't
> > know
> > > > you
> > > > > > > would
> > > > > > > > > have net/ read the data, when I supported using
> SupportsUserData. 
> > > > That
> > > > > > > class
> > > > > > > > > exists specifically so net/ doesn't have to know about the
data.
> 
> > > You
> > > > > need
> > > > > > > to
> > > > > > > > > retrieve the information at the proxy level, where we don't
have
> > > > access
> > > > > to
> > > > > > > the
> > > > > > > > > URLRequest, so I understand the reason, but we need to find
> > another
> > > > way.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > Agreed about the uglyness :-)
> > > > > > > > 
> > > > > > > > Incidentally, the LOAD_FLAGS_BYPASS_PROXY trick was exactly what
I
> > > used
> > > > > for
> > > > > > my
> > > > > > > > initial prototype. As you mentioned, we do need to support the
> > > > possibility
> > > > > > of
> > > > > > > > other proxies. And AFAICT, only the ProxyResolverService knows
> > whether
> > > > the
> > > > > > > > effective proxy is Flywheel. 
> > > > > > > > 
> > > > > > > > Do you know of other ways to get information about the effective
> > > proxy?
> > > > > > > > 
> > > > > > > > I guess one possibility would be to invoke ProxyResolverService
> > before
> > > > the
> > > > > > > > URLRequest makes it into net/, then attach the LOAD_FLAG iff the
> > > > effective
> > > > > > > proxy
> > > > > > > > is the data compression proxy. But that seems like a bad
> separation
> > of
> > > > > > > concerns
> > > > > > > > to me. Thoughts?
> > > > > > > 
> > > > > > > Ben has a CL out to add headers through a callback for specific
> > proxies
> > > > > > > (https://codereview.chromium.org/333113002/).  We could do
something
> > > > similar
> > > > > > to
> > > > > > > let the delegate reject certain proxies for a request, and then
just
> > > move
> > > > on
> > > > > > to
> > > > > > > the next.  I still find the extra callbacks less than ideal, but I
> > think
> > > > > it's
> > > > > > a
> > > > > > > cleaner approach than this.
> > > > > > 
> > > > > > Double checking that I understand the proposal correctly:
> > > > > >   - Add a callback "OnResolveProxy" to NetworkDelegate
> > > > > >   - Plumb a reference to NetworkDelegate to ProxyService, and have
> > > > > ProxyService
> > > > > > invoke NetworkDelegate.OnResolveProxy before returning from
> > ResolveProxy()
> > > > > > 
> > > > > > Or did you a different point of invocation in mind?
> > > > > 
> > > > > That's the basic idea, though you should probably run it by eroman,
who
> > > knows
> > > > > the proxy code much better than I.
> > > > > 
> > > > > Also, instead of storing request type use SupportsUserData, you might
> want
> > > to
> > > > > consider moving it into ResourceRequestInfo.  AppCache, at least,
needs
> > that
> > > > > information as well, so seems like others may want it as well, if
we're
> > not
> > > > > already storing it somewhere (No need to modify AppCache to use the
new
> > > > field).
> > > > 
> > > > Actually, it looks like ResourceRequestInfo already as the ResourceType,
> so
> > I
> > > > think you can just grab it there, without adding any new code to store
it.
> 
> > > See
> > > > content::ResourceRequestInfo::ForRequest.
> > > 
> > > Incidentally, I started out by using content::ResourceRequestInfo, but
> > realized
> > > that net/ can't depend on it without introducing a layering violation. If
> > we're
> > > invoking it from a callback though, maybe that isn't a problem.
> > 
> > The NetworkDelegate callback should go through the URLRequest or
> URLRequestJob,
> > both of which can send the request along.  From the callback, you can grab
the
> > info without causing a layering violation.
> 
> Hey Matt,
> 
> I'm currently at a conceptual roadblock. The point where ResolveProxy() is
> called is in HTTPStreamFactory:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_stre...
> 
> HTTPStreamFactory does not have a reference to any URLRequest objects -- only
> HTTPRequestInfo objects. And your proposal assumes that we have a reference to
> the original URLRequest object (so that we can invoke
> content::ResourceRequestInfo::ForRequest to obtain the resource_type).
> 
> Are you suggesting that, within URLRequest, we curry the URLRequest reference
> into the NetworkDelgate callback, then pass the callback to the
> HTTPStreamFactory to be invoked?

Hrm...I was getting confused at what layer we have the proxy information, before
we've tried to connect to the proxy.  I'm going to have to spend some time
digging, and get back to you.  Sorry about that.

[rcs, 2014-06-23 21:38:03]

On 2014/06/20 17:21:17, mmenke wrote:
> On 2014/06/20 01:13:35, rcs wrote:
> > On 2014/06/19 17:45:22, mmenke wrote:
> > > On 2014/06/19 17:42:23, rcs wrote:
> > > > On 2014/06/19 15:30:44, mmenke wrote:
> > > > > On 2014/06/19 15:27:41, mmenke wrote:
> > > > > > On 2014/06/18 22:07:59, rcs wrote:
> > > > > > > On 2014/06/18 18:10:39, mmenke wrote:
> > > > > > > > On 2014/06/18 16:41:41, rcs wrote:
> > > > > > > > > On 2014/06/18 14:48:37, mmenke wrote:
> > > > > > > > > > On 2014/06/18 14:44:38, mmenke wrote:
> > > > > > > > > > > This is pretty ugly.  Can we just attach a
> > > LOAD_FLAGS_BYPASS_PROXY
> > > > > > when
> > > > > > > > > using
> > > > > > > > > > > flywheel for non-image requests, or do we support using
> > another
> > > > > proxy
> > > > > > in
> > > > > > > > > > > addition to flywheel?
> > > > > > > > > > 
> > > > > > > > > > "Ugly" meaning net/ really shouldn't know about this.  I
> didn't
> > > know
> > > > > you
> > > > > > > > would
> > > > > > > > > > have net/ read the data, when I supported using
> > SupportsUserData. 
> > > > > That
> > > > > > > > class
> > > > > > > > > > exists specifically so net/ doesn't have to know about the
> data.
> > 
> > > > You
> > > > > > need
> > > > > > > > to
> > > > > > > > > > retrieve the information at the proxy level, where we don't
> have
> > > > > access
> > > > > > to
> > > > > > > > the
> > > > > > > > > > URLRequest, so I understand the reason, but we need to find
> > > another
> > > > > way.
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Agreed about the uglyness :-)
> > > > > > > > > 
> > > > > > > > > Incidentally, the LOAD_FLAGS_BYPASS_PROXY trick was exactly
what
> I
> > > > used
> > > > > > for
> > > > > > > my
> > > > > > > > > initial prototype. As you mentioned, we do need to support the
> > > > > possibility
> > > > > > > of
> > > > > > > > > other proxies. And AFAICT, only the ProxyResolverService knows
> > > whether
> > > > > the
> > > > > > > > > effective proxy is Flywheel. 
> > > > > > > > > 
> > > > > > > > > Do you know of other ways to get information about the
effective
> > > > proxy?
> > > > > > > > > 
> > > > > > > > > I guess one possibility would be to invoke
ProxyResolverService
> > > before
> > > > > the
> > > > > > > > > URLRequest makes it into net/, then attach the LOAD_FLAG iff
the
> > > > > effective
> > > > > > > > proxy
> > > > > > > > > is the data compression proxy. But that seems like a bad
> > separation
> > > of
> > > > > > > > concerns
> > > > > > > > > to me. Thoughts?
> > > > > > > > 
> > > > > > > > Ben has a CL out to add headers through a callback for specific
> > > proxies
> > > > > > > > (https://codereview.chromium.org/333113002/).  We could do
> something
> > > > > similar
> > > > > > > to
> > > > > > > > let the delegate reject certain proxies for a request, and then
> just
> > > > move
> > > > > on
> > > > > > > to
> > > > > > > > the next.  I still find the extra callbacks less than ideal, but
I
> > > think
> > > > > > it's
> > > > > > > a
> > > > > > > > cleaner approach than this.
> > > > > > > 
> > > > > > > Double checking that I understand the proposal correctly:
> > > > > > >   - Add a callback "OnResolveProxy" to NetworkDelegate
> > > > > > >   - Plumb a reference to NetworkDelegate to ProxyService, and have
> > > > > > ProxyService
> > > > > > > invoke NetworkDelegate.OnResolveProxy before returning from
> > > ResolveProxy()
> > > > > > > 
> > > > > > > Or did you a different point of invocation in mind?
> > > > > > 
> > > > > > That's the basic idea, though you should probably run it by eroman,
> who
> > > > knows
> > > > > > the proxy code much better than I.
> > > > > > 
> > > > > > Also, instead of storing request type use SupportsUserData, you
might
> > want
> > > > to
> > > > > > consider moving it into ResourceRequestInfo.  AppCache, at least,
> needs
> > > that
> > > > > > information as well, so seems like others may want it as well, if
> we're
> > > not
> > > > > > already storing it somewhere (No need to modify AppCache to use the
> new
> > > > > field).
> > > > > 
> > > > > Actually, it looks like ResourceRequestInfo already as the
ResourceType,
> > so
> > > I
> > > > > think you can just grab it there, without adding any new code to store
> it.
> > 
> > > > See
> > > > > content::ResourceRequestInfo::ForRequest.
> > > > 
> > > > Incidentally, I started out by using content::ResourceRequestInfo, but
> > > realized
> > > > that net/ can't depend on it without introducing a layering violation.
If
> > > we're
> > > > invoking it from a callback though, maybe that isn't a problem.
> > > 
> > > The NetworkDelegate callback should go through the URLRequest or
> > URLRequestJob,
> > > both of which can send the request along.  From the callback, you can grab
> the
> > > info without causing a layering violation.
> > 
> > Hey Matt,
> > 
> > I'm currently at a conceptual roadblock. The point where ResolveProxy() is
> > called is in HTTPStreamFactory:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_stre...
> > 
> > HTTPStreamFactory does not have a reference to any URLRequest objects --
only
> > HTTPRequestInfo objects. And your proposal assumes that we have a reference
to
> > the original URLRequest object (so that we can invoke
> > content::ResourceRequestInfo::ForRequest to obtain the resource_type).
> > 
> > Are you suggesting that, within URLRequest, we curry the URLRequest
reference
> > into the NetworkDelgate callback, then pass the callback to the
> > HTTPStreamFactory to be invoked?
> 
> Hrm...I was getting confused at what layer we have the proxy information,
before
> we've tried to connect to the proxy.  I'm going to have to spend some time
> digging, and get back to you.  Sorry about that.

ping

[mmenke, 2014-06-23 21:58:50]

On 2014/06/23 21:38:03, rcs wrote:
> On 2014/06/20 17:21:17, mmenke wrote:
> > On 2014/06/20 01:13:35, rcs wrote:
> > > On 2014/06/19 17:45:22, mmenke wrote:
> > > > On 2014/06/19 17:42:23, rcs wrote:
> > > > > On 2014/06/19 15:30:44, mmenke wrote:
> > > > > > On 2014/06/19 15:27:41, mmenke wrote:
> > > > > > > On 2014/06/18 22:07:59, rcs wrote:
> > > > > > > > On 2014/06/18 18:10:39, mmenke wrote:
> > > > > > > > > On 2014/06/18 16:41:41, rcs wrote:
> > > > > > > > > > On 2014/06/18 14:48:37, mmenke wrote:
> > > > > > > > > > > On 2014/06/18 14:44:38, mmenke wrote:
> > > > > > > > > > > > This is pretty ugly.  Can we just attach a
> > > > LOAD_FLAGS_BYPASS_PROXY
> > > > > > > when
> > > > > > > > > > using
> > > > > > > > > > > > flywheel for non-image requests, or do we support using
> > > another
> > > > > > proxy
> > > > > > > in
> > > > > > > > > > > > addition to flywheel?
> > > > > > > > > > > 
> > > > > > > > > > > "Ugly" meaning net/ really shouldn't know about this.  I
> > didn't
> > > > know
> > > > > > you
> > > > > > > > > would
> > > > > > > > > > > have net/ read the data, when I supported using
> > > SupportsUserData. 
> > > > > > That
> > > > > > > > > class
> > > > > > > > > > > exists specifically so net/ doesn't have to know about the
> > data.
> > > 
> > > > > You
> > > > > > > need
> > > > > > > > > to
> > > > > > > > > > > retrieve the information at the proxy level, where we
don't
> > have
> > > > > > access
> > > > > > > to
> > > > > > > > > the
> > > > > > > > > > > URLRequest, so I understand the reason, but we need to
find
> > > > another
> > > > > > way.
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > Agreed about the uglyness :-)
> > > > > > > > > > 
> > > > > > > > > > Incidentally, the LOAD_FLAGS_BYPASS_PROXY trick was exactly
> what
> > I
> > > > > used
> > > > > > > for
> > > > > > > > my
> > > > > > > > > > initial prototype. As you mentioned, we do need to support
the
> > > > > > possibility
> > > > > > > > of
> > > > > > > > > > other proxies. And AFAICT, only the ProxyResolverService
knows
> > > > whether
> > > > > > the
> > > > > > > > > > effective proxy is Flywheel. 
> > > > > > > > > > 
> > > > > > > > > > Do you know of other ways to get information about the
> effective
> > > > > proxy?
> > > > > > > > > > 
> > > > > > > > > > I guess one possibility would be to invoke
> ProxyResolverService
> > > > before
> > > > > > the
> > > > > > > > > > URLRequest makes it into net/, then attach the LOAD_FLAG iff
> the
> > > > > > effective
> > > > > > > > > proxy
> > > > > > > > > > is the data compression proxy. But that seems like a bad
> > > separation
> > > > of
> > > > > > > > > concerns
> > > > > > > > > > to me. Thoughts?
> > > > > > > > > 
> > > > > > > > > Ben has a CL out to add headers through a callback for
specific
> > > > proxies
> > > > > > > > > (https://codereview.chromium.org/333113002/).  We could do
> > something
> > > > > > similar
> > > > > > > > to
> > > > > > > > > let the delegate reject certain proxies for a request, and
then
> > just
> > > > > move
> > > > > > on
> > > > > > > > to
> > > > > > > > > the next.  I still find the extra callbacks less than ideal,
but
> I
> > > > think
> > > > > > > it's
> > > > > > > > a
> > > > > > > > > cleaner approach than this.
> > > > > > > > 
> > > > > > > > Double checking that I understand the proposal correctly:
> > > > > > > >   - Add a callback "OnResolveProxy" to NetworkDelegate
> > > > > > > >   - Plumb a reference to NetworkDelegate to ProxyService, and
have
> > > > > > > ProxyService
> > > > > > > > invoke NetworkDelegate.OnResolveProxy before returning from
> > > > ResolveProxy()
> > > > > > > > 
> > > > > > > > Or did you a different point of invocation in mind?
> > > > > > > 
> > > > > > > That's the basic idea, though you should probably run it by
eroman,
> > who
> > > > > knows
> > > > > > > the proxy code much better than I.
> > > > > > > 
> > > > > > > Also, instead of storing request type use SupportsUserData, you
> might
> > > want
> > > > > to
> > > > > > > consider moving it into ResourceRequestInfo.  AppCache, at least,
> > needs
> > > > that
> > > > > > > information as well, so seems like others may want it as well, if
> > we're
> > > > not
> > > > > > > already storing it somewhere (No need to modify AppCache to use
the
> > new
> > > > > > field).
> > > > > > 
> > > > > > Actually, it looks like ResourceRequestInfo already as the
> ResourceType,
> > > so
> > > > I
> > > > > > think you can just grab it there, without adding any new code to
store
> > it.
> > > 
> > > > > See
> > > > > > content::ResourceRequestInfo::ForRequest.
> > > > > 
> > > > > Incidentally, I started out by using content::ResourceRequestInfo, but
> > > > realized
> > > > > that net/ can't depend on it without introducing a layering violation.
> If
> > > > we're
> > > > > invoking it from a callback though, maybe that isn't a problem.
> > > > 
> > > > The NetworkDelegate callback should go through the URLRequest or
> > > URLRequestJob,
> > > > both of which can send the request along.  From the callback, you can
grab
> > the
> > > > info without causing a layering violation.
> > > 
> > > Hey Matt,
> > > 
> > > I'm currently at a conceptual roadblock. The point where ResolveProxy() is
> > > called is in HTTPStreamFactory:
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_stre...
> > > 
> > > HTTPStreamFactory does not have a reference to any URLRequest objects --
> only
> > > HTTPRequestInfo objects. And your proposal assumes that we have a
reference
> to
> > > the original URLRequest object (so that we can invoke
> > > content::ResourceRequestInfo::ForRequest to obtain the resource_type).
> > > 
> > > Are you suggesting that, within URLRequest, we curry the URLRequest
> reference
> > > into the NetworkDelgate callback, then pass the callback to the
> > > HTTPStreamFactory to be invoked?
> > 
> > Hrm...I was getting confused at what layer we have the proxy information,
> before
> > we've tried to connect to the proxy.  I'm going to have to spend some time
> > digging, and get back to you.  Sorry about that.
> 
> ping

I'm having trouble coming up with a reasonable way to do this.

[mmenke, 2014-06-23 22:59:59]

On 2014/06/23 21:58:50, mmenke wrote:
> On 2014/06/23 21:38:03, rcs wrote:
> > On 2014/06/20 17:21:17, mmenke wrote:
> > > On 2014/06/20 01:13:35, rcs wrote:
> > > > On 2014/06/19 17:45:22, mmenke wrote:
> > > > > On 2014/06/19 17:42:23, rcs wrote:
> > > > > > On 2014/06/19 15:30:44, mmenke wrote:
> > > > > > > On 2014/06/19 15:27:41, mmenke wrote:
> > > > > > > > On 2014/06/18 22:07:59, rcs wrote:
> > > > > > > > > On 2014/06/18 18:10:39, mmenke wrote:
> > > > > > > > > > On 2014/06/18 16:41:41, rcs wrote:
> > > > > > > > > > > On 2014/06/18 14:48:37, mmenke wrote:
> > > > > > > > > > > > On 2014/06/18 14:44:38, mmenke wrote:
> > > > > > > > > > > > > This is pretty ugly.  Can we just attach a
> > > > > LOAD_FLAGS_BYPASS_PROXY
> > > > > > > > when
> > > > > > > > > > > using
> > > > > > > > > > > > > flywheel for non-image requests, or do we support
using
> > > > another
> > > > > > > proxy
> > > > > > > > in
> > > > > > > > > > > > > addition to flywheel?
> > > > > > > > > > > > 
> > > > > > > > > > > > "Ugly" meaning net/ really shouldn't know about this.  I
> > > didn't
> > > > > know
> > > > > > > you
> > > > > > > > > > would
> > > > > > > > > > > > have net/ read the data, when I supported using
> > > > SupportsUserData. 
> > > > > > > That
> > > > > > > > > > class
> > > > > > > > > > > > exists specifically so net/ doesn't have to know about
the
> > > data.
> > > > 
> > > > > > You
> > > > > > > > need
> > > > > > > > > > to
> > > > > > > > > > > > retrieve the information at the proxy level, where we
> don't
> > > have
> > > > > > > access
> > > > > > > > to
> > > > > > > > > > the
> > > > > > > > > > > > URLRequest, so I understand the reason, but we need to
> find
> > > > > another
> > > > > > > way.
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > Agreed about the uglyness :-)
> > > > > > > > > > > 
> > > > > > > > > > > Incidentally, the LOAD_FLAGS_BYPASS_PROXY trick was
exactly
> > what
> > > I
> > > > > > used
> > > > > > > > for
> > > > > > > > > my
> > > > > > > > > > > initial prototype. As you mentioned, we do need to support
> the
> > > > > > > possibility
> > > > > > > > > of
> > > > > > > > > > > other proxies. And AFAICT, only the ProxyResolverService
> knows
> > > > > whether
> > > > > > > the
> > > > > > > > > > > effective proxy is Flywheel. 
> > > > > > > > > > > 
> > > > > > > > > > > Do you know of other ways to get information about the
> > effective
> > > > > > proxy?
> > > > > > > > > > > 
> > > > > > > > > > > I guess one possibility would be to invoke
> > ProxyResolverService
> > > > > before
> > > > > > > the
> > > > > > > > > > > URLRequest makes it into net/, then attach the LOAD_FLAG
iff
> > the
> > > > > > > effective
> > > > > > > > > > proxy
> > > > > > > > > > > is the data compression proxy. But that seems like a bad
> > > > separation
> > > > > of
> > > > > > > > > > concerns
> > > > > > > > > > > to me. Thoughts?
> > > > > > > > > > 
> > > > > > > > > > Ben has a CL out to add headers through a callback for
> specific
> > > > > proxies
> > > > > > > > > > (https://codereview.chromium.org/333113002/).  We could do
> > > something
> > > > > > > similar
> > > > > > > > > to
> > > > > > > > > > let the delegate reject certain proxies for a request, and
> then
> > > just
> > > > > > move
> > > > > > > on
> > > > > > > > > to
> > > > > > > > > > the next.  I still find the extra callbacks less than ideal,
> but
> > I
> > > > > think
> > > > > > > > it's
> > > > > > > > > a
> > > > > > > > > > cleaner approach than this.
> > > > > > > > > 
> > > > > > > > > Double checking that I understand the proposal correctly:
> > > > > > > > >   - Add a callback "OnResolveProxy" to NetworkDelegate
> > > > > > > > >   - Plumb a reference to NetworkDelegate to ProxyService, and
> have
> > > > > > > > ProxyService
> > > > > > > > > invoke NetworkDelegate.OnResolveProxy before returning from
> > > > > ResolveProxy()
> > > > > > > > > 
> > > > > > > > > Or did you a different point of invocation in mind?
> > > > > > > > 
> > > > > > > > That's the basic idea, though you should probably run it by
> eroman,
> > > who
> > > > > > knows
> > > > > > > > the proxy code much better than I.
> > > > > > > > 
> > > > > > > > Also, instead of storing request type use SupportsUserData, you
> > might
> > > > want
> > > > > > to
> > > > > > > > consider moving it into ResourceRequestInfo.  AppCache, at
least,
> > > needs
> > > > > that
> > > > > > > > information as well, so seems like others may want it as well,
if
> > > we're
> > > > > not
> > > > > > > > already storing it somewhere (No need to modify AppCache to use
> the
> > > new
> > > > > > > field).
> > > > > > > 
> > > > > > > Actually, it looks like ResourceRequestInfo already as the
> > ResourceType,
> > > > so
> > > > > I
> > > > > > > think you can just grab it there, without adding any new code to
> store
> > > it.
> > > > 
> > > > > > See
> > > > > > > content::ResourceRequestInfo::ForRequest.
> > > > > > 
> > > > > > Incidentally, I started out by using content::ResourceRequestInfo,
but
> > > > > realized
> > > > > > that net/ can't depend on it without introducing a layering
violation.
> > If
> > > > > we're
> > > > > > invoking it from a callback though, maybe that isn't a problem.
> > > > > 
> > > > > The NetworkDelegate callback should go through the URLRequest or
> > > > URLRequestJob,
> > > > > both of which can send the request along.  From the callback, you can
> grab
> > > the
> > > > > info without causing a layering violation.
> > > > 
> > > > Hey Matt,
> > > > 
> > > > I'm currently at a conceptual roadblock. The point where ResolveProxy()
is
> > > > called is in HTTPStreamFactory:
> > > > 
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_stre...
> > > > 
> > > > HTTPStreamFactory does not have a reference to any URLRequest objects --
> > only
> > > > HTTPRequestInfo objects. And your proposal assumes that we have a
> reference
> > to
> > > > the original URLRequest object (so that we can invoke
> > > > content::ResourceRequestInfo::ForRequest to obtain the resource_type).
> > > > 
> > > > Are you suggesting that, within URLRequest, we curry the URLRequest
> > reference
> > > > into the NetworkDelgate callback, then pass the callback to the
> > > > HTTPStreamFactory to be invoked?
> > > 
> > > Hrm...I was getting confused at what layer we have the proxy information,
> > before
> > > we've tried to connect to the proxy.  I'm going to have to spend some time
> > > digging, and get back to you.  Sorry about that.
> > 
> > ping
> 
> I'm having trouble coming up with a reasonable way to do this.

And just for the record, talked to Michael Piatek a bit about it.  If we don't
come up with anything better by tomorrow afternoon, we'll add a temporary load
flag for this.  I expect this will not be a major change from this CL.

You can basically just add an entry to net/base/load_flags.h
(LOAD_BYPASS_FLYWHEEL or somesuch), set it on non-image requests (Or whatever),
and then it already makes its way all the way down to the
HttpStreamFactoryImpl::Job, you just need to pass it on to the proxy service
from there, like you are doing for ResourceType.

Sorry I've been slow on this.

[rcs, 2014-06-23 23:24:11]

On 2014/06/23 22:59:59, mmenke wrote:
> On 2014/06/23 21:58:50, mmenke wrote:
> > On 2014/06/23 21:38:03, rcs wrote:
> > > On 2014/06/20 17:21:17, mmenke wrote:
> > > > On 2014/06/20 01:13:35, rcs wrote:
> > > > > On 2014/06/19 17:45:22, mmenke wrote:
> > > > > > On 2014/06/19 17:42:23, rcs wrote:
> > > > > > > On 2014/06/19 15:30:44, mmenke wrote:
> > > > > > > > On 2014/06/19 15:27:41, mmenke wrote:
> > > > > > > > > On 2014/06/18 22:07:59, rcs wrote:
> > > > > > > > > > On 2014/06/18 18:10:39, mmenke wrote:
> > > > > > > > > > > On 2014/06/18 16:41:41, rcs wrote:
> > > > > > > > > > > > On 2014/06/18 14:48:37, mmenke wrote:
> > > > > > > > > > > > > On 2014/06/18 14:44:38, mmenke wrote:
> > > > > > > > > > > > > > This is pretty ugly.  Can we just attach a
> > > > > > LOAD_FLAGS_BYPASS_PROXY
> > > > > > > > > when
> > > > > > > > > > > > using
> > > > > > > > > > > > > > flywheel for non-image requests, or do we support
> using
> > > > > another
> > > > > > > > proxy
> > > > > > > > > in
> > > > > > > > > > > > > > addition to flywheel?
> > > > > > > > > > > > > 
> > > > > > > > > > > > > "Ugly" meaning net/ really shouldn't know about this. 
I
> > > > didn't
> > > > > > know
> > > > > > > > you
> > > > > > > > > > > would
> > > > > > > > > > > > > have net/ read the data, when I supported using
> > > > > SupportsUserData. 
> > > > > > > > That
> > > > > > > > > > > class
> > > > > > > > > > > > > exists specifically so net/ doesn't have to know about
> the
> > > > data.
> > > > > 
> > > > > > > You
> > > > > > > > > need
> > > > > > > > > > > to
> > > > > > > > > > > > > retrieve the information at the proxy level, where we
> > don't
> > > > have
> > > > > > > > access
> > > > > > > > > to
> > > > > > > > > > > the
> > > > > > > > > > > > > URLRequest, so I understand the reason, but we need to
> > find
> > > > > > another
> > > > > > > > way.
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > Agreed about the uglyness :-)
> > > > > > > > > > > > 
> > > > > > > > > > > > Incidentally, the LOAD_FLAGS_BYPASS_PROXY trick was
> exactly
> > > what
> > > > I
> > > > > > > used
> > > > > > > > > for
> > > > > > > > > > my
> > > > > > > > > > > > initial prototype. As you mentioned, we do need to
support
> > the
> > > > > > > > possibility
> > > > > > > > > > of
> > > > > > > > > > > > other proxies. And AFAICT, only the ProxyResolverService
> > knows
> > > > > > whether
> > > > > > > > the
> > > > > > > > > > > > effective proxy is Flywheel. 
> > > > > > > > > > > > 
> > > > > > > > > > > > Do you know of other ways to get information about the
> > > effective
> > > > > > > proxy?
> > > > > > > > > > > > 
> > > > > > > > > > > > I guess one possibility would be to invoke
> > > ProxyResolverService
> > > > > > before
> > > > > > > > the
> > > > > > > > > > > > URLRequest makes it into net/, then attach the LOAD_FLAG
> iff
> > > the
> > > > > > > > effective
> > > > > > > > > > > proxy
> > > > > > > > > > > > is the data compression proxy. But that seems like a bad
> > > > > separation
> > > > > > of
> > > > > > > > > > > concerns
> > > > > > > > > > > > to me. Thoughts?
> > > > > > > > > > > 
> > > > > > > > > > > Ben has a CL out to add headers through a callback for
> > specific
> > > > > > proxies
> > > > > > > > > > > (https://codereview.chromium.org/333113002/).  We could do
> > > > something
> > > > > > > > similar
> > > > > > > > > > to
> > > > > > > > > > > let the delegate reject certain proxies for a request, and
> > then
> > > > just
> > > > > > > move
> > > > > > > > on
> > > > > > > > > > to
> > > > > > > > > > > the next.  I still find the extra callbacks less than
ideal,
> > but
> > > I
> > > > > > think
> > > > > > > > > it's
> > > > > > > > > > a
> > > > > > > > > > > cleaner approach than this.
> > > > > > > > > > 
> > > > > > > > > > Double checking that I understand the proposal correctly:
> > > > > > > > > >   - Add a callback "OnResolveProxy" to NetworkDelegate
> > > > > > > > > >   - Plumb a reference to NetworkDelegate to ProxyService,
and
> > have
> > > > > > > > > ProxyService
> > > > > > > > > > invoke NetworkDelegate.OnResolveProxy before returning from
> > > > > > ResolveProxy()
> > > > > > > > > > 
> > > > > > > > > > Or did you a different point of invocation in mind?
> > > > > > > > > 
> > > > > > > > > That's the basic idea, though you should probably run it by
> > eroman,
> > > > who
> > > > > > > knows
> > > > > > > > > the proxy code much better than I.
> > > > > > > > > 
> > > > > > > > > Also, instead of storing request type use SupportsUserData,
you
> > > might
> > > > > want
> > > > > > > to
> > > > > > > > > consider moving it into ResourceRequestInfo.  AppCache, at
> least,
> > > > needs
> > > > > > that
> > > > > > > > > information as well, so seems like others may want it as well,
> if
> > > > we're
> > > > > > not
> > > > > > > > > already storing it somewhere (No need to modify AppCache to
use
> > the
> > > > new
> > > > > > > > field).
> > > > > > > > 
> > > > > > > > Actually, it looks like ResourceRequestInfo already as the
> > > ResourceType,
> > > > > so
> > > > > > I
> > > > > > > > think you can just grab it there, without adding any new code to
> > store
> > > > it.
> > > > > 
> > > > > > > See
> > > > > > > > content::ResourceRequestInfo::ForRequest.
> > > > > > > 
> > > > > > > Incidentally, I started out by using content::ResourceRequestInfo,
> but
> > > > > > realized
> > > > > > > that net/ can't depend on it without introducing a layering
> violation.
> > > If
> > > > > > we're
> > > > > > > invoking it from a callback though, maybe that isn't a problem.
> > > > > > 
> > > > > > The NetworkDelegate callback should go through the URLRequest or
> > > > > URLRequestJob,
> > > > > > both of which can send the request along.  From the callback, you
can
> > grab
> > > > the
> > > > > > info without causing a layering violation.
> > > > > 
> > > > > Hey Matt,
> > > > > 
> > > > > I'm currently at a conceptual roadblock. The point where
ResolveProxy()
> is
> > > > > called is in HTTPStreamFactory:
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_stre...
> > > > > 
> > > > > HTTPStreamFactory does not have a reference to any URLRequest objects
--
> > > only
> > > > > HTTPRequestInfo objects. And your proposal assumes that we have a
> > reference
> > > to
> > > > > the original URLRequest object (so that we can invoke
> > > > > content::ResourceRequestInfo::ForRequest to obtain the resource_type).
> > > > > 
> > > > > Are you suggesting that, within URLRequest, we curry the URLRequest
> > > reference
> > > > > into the NetworkDelgate callback, then pass the callback to the
> > > > > HTTPStreamFactory to be invoked?
> > > > 
> > > > Hrm...I was getting confused at what layer we have the proxy
information,
> > > before
> > > > we've tried to connect to the proxy.  I'm going to have to spend some
time
> > > > digging, and get back to you.  Sorry about that.
> > > 
> > > ping
> > 
> > I'm having trouble coming up with a reasonable way to do this.
> 
> And just for the record, talked to Michael Piatek a bit about it.  If we don't
> come up with anything better by tomorrow afternoon, we'll add a temporary load
> flag for this.  I expect this will not be a major change from this CL.
> 
> You can basically just add an entry to net/base/load_flags.h
> (LOAD_BYPASS_FLYWHEEL or somesuch), set it on non-image requests (Or
whatever),
> and then it already makes its way all the way down to the
> HttpStreamFactoryImpl::Job, you just need to pass it on to the proxy service
> from there, like you are doing for ResourceType.
> 
> Sorry I've been slow on this.

No worries. 

Let me know what you think of these two proposals.

Proposal one: "global dictionary"
---
Keep a dictionary from GURL (raw string) to resource type.

Then within ResolveProxy(), invoke a network delegate callback that looks at
that dictionary, and makes a bypass decision based on the resource type and the
effective proxy. The key insight there is that the GURL is already plumbed all
the way down, and that GURL uniquely identifies a resource type.
 
Manage that dictionary within components/data_compression_proxy/, and have
ResourceDispatcherHost invoke a components/data_compression_proxy/ method that
populates the dictionary.
---

Proposal two: "callback object per request"
---
At the point within net/ where URLRequest objects are converted into
HTTPRequestInfo objects, create a callback object (e.g.,
base::bind(NetworkDelegate.OnResolveProxy). Then, attach the resource type to
the callback. Finally, plumb that callback object all the way into
HttpStreamFactoryImpl::Job.

I think this approach will work, but it's messy IMO, both because there is a lot
of plumbing involved, and because it's a complicated pattern (normally,
callbacks are invoked directly on the NetworkDelegate object rather than being
bound and then passed around.)
---

[rcs, 2014-06-23 23:30:24]

bengr@ also pointed out that, in ~3 weeks, he'll be creating a
data-compression-proxy-specific ProxyService subclass. Within that subclass,
we'll be able to add pretty much any logic we need, assuming we can still get
access to the resource type somehow. Unfortunately this experiment is somewhat
time critical, so it'd be preferable to get something out before then, even if
we revert it later.

[mmenke, 2014-06-24 17:01:48]

On 2014/06/23 23:24:11, rcs wrote:
> Let me know what you think of these two proposals.
> 
> Proposal one: "global dictionary"
> ---
> Keep a dictionary from GURL (raw string) to resource type.
> 
> Then within ResolveProxy(), invoke a network delegate callback that looks at
> that dictionary, and makes a bypass decision based on the resource type and
the
> effective proxy. The key insight there is that the GURL is already plumbed all
> the way down, and that GURL uniquely identifies a resource type.
>  
> Manage that dictionary within components/data_compression_proxy/, and have
> ResourceDispatcherHost invoke a components/data_compression_proxy/ method that
> populates the dictionary.
> ---

This works, though you could end up having one resource requested with multiple
resource types.  Also, one URLRequest could request the same resource multiple
times, for a variety of reasons (network errors, auth, etc).  The latter you
could deal with by keeping requests in the table until the request is redirected
or the ResourceLoader is destroyed (Simplest way to do this may be by attaching
a ResourceThrottle to image requests, and have it do all the updating of a
central service that lives on the IO Thread on redirect and cancellation).

The sources of ugliness here are the map, which doesn't perfectly match requests
that come in to the proxy service, adding a new callback to the delegate, and
pushing the NetworkDelegate in the HttpStreamFactory.  I think that from a
network stack perspective, this may be a little nicer than the load flag
approach, but it's a lot more work.  If we go with a new ProxyService for
Flywheel, this may well be the probably the approach we'll need to take, unless
we can come up with a better way to get resource type down to the ProxyService.

Since we're agreed that this is a temporary change, I suggest going with the
load flag for testing purposes since it's much simpler, but with the agreement
that we'll remove it within a two major revisions, in favor of another approach.
 If you want to go ahead and go with this approach now, though, I'm completely
fine with it, and it will be less effort later when we switch to a proxy service
based approach.

> Proposal two: "callback object per request"
> ---
> At the point within net/ where URLRequest objects are converted into
> HTTPRequestInfo objects, create a callback object (e.g.,
> base::bind(NetworkDelegate.OnResolveProxy). Then, attach the resource type to
> the callback. Finally, plumb that callback object all the way into
> HttpStreamFactoryImpl::Job.
> 
> I think this approach will work, but it's messy IMO, both because there is a
lot
> of plumbing involved, and because it's a complicated pattern (normally,
> callbacks are invoked directly on the NetworkDelegate object rather than being
> bound and then passed around.)
> ---

Yea, like you, I really don't like this.  Pushing callbacks that far down just
seems really ugly.

[mmenke, 2014-06-24 18:51:53]

Just saw a comment from bengr elsewhere...  "We plan change Flywheel support to
configure Flywheel iff the effective proxy configuration resolves to direct." 
If we do that, we can just use the load flag to disable the proxy, which makes
life much simpler, longer term.

[mmenke, 2014-06-25 18:00:27]

On 2014/06/24 18:51:53, mmenke wrote:
> Just saw a comment from bengr elsewhere...  "We plan change Flywheel support
to
> configure Flywheel iff the effective proxy configuration resolves to direct." 
> If we do that, we can just use the load flag to disable the proxy, which makes
> life much simpler, longer term.

And I misinterpreted ben's comment.  I was thinking "when there's no proxy
configured", but ben means "When a particular request won't go through a proxy".

[rcs, 2014-06-25 23:50:59]

On 2014/06/25 18:00:27, mmenke wrote:
> On 2014/06/24 18:51:53, mmenke wrote:
> > Just saw a comment from bengr elsewhere...  "We plan change Flywheel support
> to
> > configure Flywheel iff the effective proxy configuration resolves to
direct." 
> > If we do that, we can just use the load flag to disable the proxy, which
makes
> > life much simpler, longer term.
> 
> And I misinterpreted ben's comment.  I was thinking "when there's no proxy
> configured", but ben means "When a particular request won't go through a
proxy".

Hey Matt,

I'm still in the process of writing tests, and there are obviously some layering
violations I need to fix (need to head out for the day!), but would you mind
letting me know if this second patch is the design you had in mind?

Thanks,
Colin

[mmenke, 2014-06-26 16:36:55]

On 2014/06/25 23:50:59, rcs wrote:
> On 2014/06/25 18:00:27, mmenke wrote:
> > On 2014/06/24 18:51:53, mmenke wrote:
> > > Just saw a comment from bengr elsewhere...  "We plan change Flywheel
support
> > to
> > > configure Flywheel iff the effective proxy configuration resolves to
> direct." 
> > > If we do that, we can just use the load flag to disable the proxy, which
> makes
> > > life much simpler, longer term.
> > 
> > And I misinterpreted ben's comment.  I was thinking "when there's no proxy
> > configured", but ben means "When a particular request won't go through a
> proxy".
> 
> Hey Matt,
> 
> I'm still in the process of writing tests, and there are obviously some
layering
> violations I need to fix (need to head out for the day!), but would you mind
> letting me know if this second patch is the design you had in mind?
> 
> Thanks,
> Colin

Not quite what I had in mind (I was thinking either a load flag with logic in
net or a NetworkDelegate callback + dictionary, hadn't considered a load flag
with a NetworkDelegate callback), I think this should be fine, but I want to
spend some more time digging through it this afternoon.

[mmenke, 2014-06-27 18:59:36]

Sorry, dropped off my radar for a day.  I think this is fine.  Some quick
comments, know it's not yet ready for full review.

https://codereview.chromium.org/332313003/diff/20001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/332313003/diff/20001/net/proxy/proxy_service....
net/proxy/proxy_service.cc:1000: // TODO(rcs): add load_flags to PacRequest.
Flywheel requests always go through a PAC currently, don't they?

https://codereview.chromium.org/332313003/diff/20001/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

https://codereview.chromium.org/332313003/diff/20001/net/proxy/proxy_service....
net/proxy/proxy_service.h:99: NetworkDelegate* network_delegate,
Suggest making this an argument to ResolveProxy instead.  Allows one proxy
service to be reused by multiple URLRequestContexts.

https://codereview.chromium.org/332313003/diff/20001/net/proxy/proxy_service....
net/proxy/proxy_service.h:450: // Network Delegate manages callbacks to be
invoked for upon Proxy Resolution.
nit:  "NetworkDelegate", "proxy resolution"

[rcs, 2014-06-27 19:30:33]

On 2014/06/27 18:59:36, mmenke wrote:
> Sorry, dropped off my radar for a day.  I think this is fine.  Some quick
> comments, know it's not yet ready for full review.
> 
>
https://codereview.chromium.org/332313003/diff/20001/net/proxy/proxy_service.cc
> File net/proxy/proxy_service.cc (right):
> 
>
https://codereview.chromium.org/332313003/diff/20001/net/proxy/proxy_service....
> net/proxy/proxy_service.cc:1000: // TODO(rcs): add load_flags to PacRequest.
> Flywheel requests always go through a PAC currently, don't they?
> 
> https://codereview.chromium.org/332313003/diff/20001/net/proxy/proxy_service.h
> File net/proxy/proxy_service.h (right):
> 
>
https://codereview.chromium.org/332313003/diff/20001/net/proxy/proxy_service....
> net/proxy/proxy_service.h:99: NetworkDelegate* network_delegate,
> Suggest making this an argument to ResolveProxy instead.  Allows one proxy
> service to be reused by multiple URLRequestContexts.
> 
>
https://codereview.chromium.org/332313003/diff/20001/net/proxy/proxy_service....
> net/proxy/proxy_service.h:450: // Network Delegate manages callbacks to be
> invoked for upon Proxy Resolution.
> nit:  "NetworkDelegate", "proxy resolution"

Thanks Matt! Will ping you when I have a complete patch set

[rcs, 2014-06-28 03:53:38]

https://codereview.chromium.org/332313003/diff/20001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/332313003/diff/20001/net/proxy/proxy_service....
net/proxy/proxy_service.cc:1000: // TODO(rcs): add load_flags to PacRequest.
On 2014/06/27 18:59:35, mmenke wrote:
> Flywheel requests always go through a PAC currently, don't they?

I ran this (a few days) without adding load_flags to PacRequest, and verified
(via netlog and console logging) that it's bypassing flywheel correctly.

Will verify again once I'm back in Seattle

https://codereview.chromium.org/332313003/diff/20001/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

https://codereview.chromium.org/332313003/diff/20001/net/proxy/proxy_service....
net/proxy/proxy_service.h:99: NetworkDelegate* network_delegate,
On 2014/06/27 18:59:35, mmenke wrote:
> Suggest making this an argument to ResolveProxy instead.  Allows one proxy
> service to be reused by multiple URLRequestContexts.

Done.

https://codereview.chromium.org/332313003/diff/20001/net/proxy/proxy_service....
net/proxy/proxy_service.h:450: // Network Delegate manages callbacks to be
invoked for upon Proxy Resolution.
On 2014/06/27 18:59:35, mmenke wrote:
> nit:  "NetworkDelegate", "proxy resolution"

Done.

[rcs, 2014-06-28 04:01:04]

Reviewers -= darin
Reviewers += ajwong, jam

https://codereview.chromium.org/332313003/diff/20001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/332313003/diff/20001/net/proxy/proxy_service....
net/proxy/proxy_service.cc:1000: // TODO(rcs): add load_flags to PacRequest.
On 2014/06/28 03:53:38, rcs wrote:
> On 2014/06/27 18:59:35, mmenke wrote:
> > Flywheel requests always go through a PAC currently, don't they?
> 
> I ran this (a few days) without adding load_flags to PacRequest, and verified
> (via netlog and console logging) that it's bypassing flywheel correctly.
> 
> Will verify again once I'm back in Seattle

*a few days ago

[awong, 2014-06-28 06:28:55]

There's enough reviewers on this that I'm not sure who's responsible for what.
Which part did you want me to review?

[rcs, 2014-06-30 17:54:31]

On 2014/06/28 06:28:55, awong wrote:
> There's enough reviewers on this that I'm not sure who's responsible for what.
> Which part did you want me to review?

ajwong:
  chrome/browser/profiles/profile_io_data.cc
  jingle/glue/proxy_resolving_client_socket.cc

jam:
  chrome/browser/io_thread.cc
  chrome/browser/io_thread.h
  chrome/browser/net/chrome_network_delegate.cc
  chrome/browser/net/chrome_network_delegate.h
  chrome/browser/net/network_stats.cc
  chrome/browser/profiles/profile_io_data.cc
  content/browser/loader/resource_dispatcher_host_impl.cc
  content/browser/renderer_host/pepper/pepper_network_proxy_host.cc
  content/browser/resolve_proxy_msg_helper.cc

zea:
  google_apis/gcm/engine/connection_factory_impl.cc
  jingle/glue/proxy_resolving_client_socket.cc

bengr and mmenke as overall reviewers. Thanks!

[Nicolas Zea, 2014-06-30 17:57:29]

google_apis and jingle LGTM

[rcs, 2014-06-30 19:46:31]

https://codereview.chromium.org/332313003/diff/20001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/332313003/diff/20001/net/proxy/proxy_service....
net/proxy/proxy_service.cc:1000: // TODO(rcs): add load_flags to PacRequest.
On 2014/06/28 04:01:04, rcs wrote:
> On 2014/06/28 03:53:38, rcs wrote:
> > On 2014/06/27 18:59:35, mmenke wrote:
> > > Flywheel requests always go through a PAC currently, don't they?
> > 
> > I ran this (a few days) without adding load_flags to PacRequest, and
verified
> > (via netlog and console logging) that it's bypassing flywheel correctly.
> > 
> > Will verify again once I'm back in Seattle
> 
> *a few days ago

I verified that data compression proxy requests do *not* go through PacRequests.
bengr@ mentioned that our requests used to go through PacRequests, but no
longer.

[mmenke, 2014-06-30 21:19:59]

On 2014/06/30 19:46:31, rcs wrote:
> I verified that data compression proxy requests do *not* go through
PacRequests.
> bengr@ mentioned that our requests used to go through PacRequests, but no
> longer.

Thanks for checking!  Ben would certainly know better than I.

Is this ready for a full review now, or are you still working on it?  Just want
to make sure you aren't blocked on me.

[awong, 2014-06-30 22:04:37]

profile_io_data and jingle LGTM

[jam, 2014-06-30 22:07:18]

rubberstamp lgtm for content and chrome

[rcs1, 2014-06-30 23:42:50]

On 2014/06/30 21:19:59, mmenke wrote:
> On 2014/06/30 19:46:31, rcs wrote:
> > I verified that data compression proxy requests do *not* go through
> PacRequests.
> > bengr@ mentioned that our requests used to go through PacRequests, but no
> > longer.
> 
> Thanks for checking!  Ben would certainly know better than I.
> 
> Is this ready for a full review now, or are you still working on it?  Just
want
> to make sure you aren't blocked on me.

It's ready for a full review. Thanks!

[mmenke, 2014-07-01 19:45:13]

https://codereview.chromium.org/332313003/diff/60001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/332313003/diff/60001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate.cc:482: }
nit:  Preferred style in this file seems to be not to use braces on single line
ifs.

https://codereview.chromium.org/332313003/diff/60001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_network_delegate.h (right):

https://codereview.chromium.org/332313003/diff/60001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate.h:55: class ProxyInfo;
nit:  Already forward declared.

https://codereview.chromium.org/332313003/diff/60001/chrome/browser/net/netwo...
File chrome/browser/net/network_stats.cc (right):

https://codereview.chromium.org/332313003/diff/60001/chrome/browser/net/netwo...
chrome/browser/net/network_stats.cc:823: 0,
LOAD_NORMAL.

https://codereview.chromium.org/332313003/diff/60001/net/base/network_delegate.h
File net/base/network_delegate.h (right):

https://codereview.chromium.org/332313003/diff/60001/net/base/network_delegat...
net/base/network_delegate.h:41: class ProxyInfo;
nit:  Alphabetize.

https://codereview.chromium.org/332313003/diff/60001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/332313003/diff/60001/net/proxy/proxy_service....
net/proxy/proxy_service.cc:774: int rv =
service_->TryToCompleteSynchronously(url_, 0, NULL, results_);
Rather than using 0 for load flags, should use LOAD_NORMAL (Goes for all files)

https://codereview.chromium.org/332313003/diff/60001/net/proxy/proxy_service....
net/proxy/proxy_service.cc:999: // TODO(rcs): add load_flags and
network_delegate to PacRequest.
Since we aren't using flywheel with PACs, and are planning to get rid of this
code when we create a flywheel proxy service, I don't think we need this TODO. 
Instead, can just add a comment to the modified PAC calls that the data
reduction proxy isn't used with PACs.

https://codereview.chromium.org/332313003/diff/60001/net/proxy/proxy_service....
net/proxy/proxy_service.cc:1051: }
nit:  To be consistent with the rest of the code in this file, don't use braces
here.

https://codereview.chromium.org/332313003/diff/60001/net/proxy/proxy_service....
net/proxy/proxy_service.cc:1186: const BoundNetLog& net_log) {
When this is called from http_stream_factory_impl_job, you can get the load
flags and the NetworkDelegate, right?

Don't worry about the call from the SyncProxyServiceHelper - turns out that
class isn't used anymore, and I've just sent out a CL that deletes it.

https://codereview.chromium.org/332313003/diff/60001/net/proxy/proxy_service_...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/332313003/diff/60001/net/proxy/proxy_service_...
net/proxy/proxy_service_unittest.cc:210:
EXPECT_TRUE(delegate.on_resolve_proxy_called);
Should have the delegate modify the list, both by adding and removing a proxy
(Different tests), and make sure neither has an effect on future resolve proxy
requests.

https://codereview.chromium.org/332313003/diff/60001/net/proxy/proxy_service_...
net/proxy/proxy_service_unittest.cc:229: url, 0, &info, callback.callback(),
&request, NULL, log.bound());
These should all use LOAD_NORMAL.

[rcs, 2014-07-02 00:56:21]

Addressed mmenke's feedback.

https://codereview.chromium.org/332313003/diff/60001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/332313003/diff/60001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate.cc:482: }
On 2014/07/01 19:45:12, mmenke wrote:
> nit:  Preferred style in this file seems to be not to use braces on single
line
> ifs.

Done.

https://codereview.chromium.org/332313003/diff/60001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_network_delegate.h (right):

https://codereview.chromium.org/332313003/diff/60001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate.h:55: class ProxyInfo;
On 2014/07/01 19:45:12, mmenke wrote:
> nit:  Already forward declared.

Done.

https://codereview.chromium.org/332313003/diff/60001/chrome/browser/net/netwo...
File chrome/browser/net/network_stats.cc (right):

https://codereview.chromium.org/332313003/diff/60001/chrome/browser/net/netwo...
chrome/browser/net/network_stats.cc:823: 0,
On 2014/07/01 19:45:12, mmenke wrote:
> LOAD_NORMAL.

Done.

https://codereview.chromium.org/332313003/diff/60001/net/base/network_delegate.h
File net/base/network_delegate.h (right):

https://codereview.chromium.org/332313003/diff/60001/net/base/network_delegat...
net/base/network_delegate.h:41: class ProxyInfo;
On 2014/07/01 19:45:12, mmenke wrote:
> nit:  Alphabetize.

Done.

https://codereview.chromium.org/332313003/diff/60001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/332313003/diff/60001/net/proxy/proxy_service....
net/proxy/proxy_service.cc:774: int rv =
service_->TryToCompleteSynchronously(url_, 0, NULL, results_);
On 2014/07/01 19:45:12, mmenke wrote:
> Rather than using 0 for load flags, should use LOAD_NORMAL (Goes for all
files)

Done.

https://codereview.chromium.org/332313003/diff/60001/net/proxy/proxy_service....
net/proxy/proxy_service.cc:999: // TODO(rcs): add load_flags and
network_delegate to PacRequest.
On 2014/07/01 19:45:12, mmenke wrote:
> Since we aren't using flywheel with PACs, and are planning to get rid of this
> code when we create a flywheel proxy service, I don't think we need this TODO.

> Instead, can just add a comment to the modified PAC calls that the data
> reduction proxy isn't used with PACs.

Done.

https://codereview.chromium.org/332313003/diff/60001/net/proxy/proxy_service....
net/proxy/proxy_service.cc:1051: }
On 2014/07/01 19:45:12, mmenke wrote:
> nit:  To be consistent with the rest of the code in this file, don't use
braces
> here.

Done.

https://codereview.chromium.org/332313003/diff/60001/net/proxy/proxy_service....
net/proxy/proxy_service.cc:1186: const BoundNetLog& net_log) {
On 2014/07/01 19:45:12, mmenke wrote:
> When this is called from http_stream_factory_impl_job, you can get the load
> flags and the NetworkDelegate, right?
> 
> Don't worry about the call from the SyncProxyServiceHelper - turns out that
> class isn't used anymore, and I've just sent out a CL that deletes it.

Done.

https://codereview.chromium.org/332313003/diff/60001/net/proxy/proxy_service_...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/332313003/diff/60001/net/proxy/proxy_service_...
net/proxy/proxy_service_unittest.cc:210:
EXPECT_TRUE(delegate.on_resolve_proxy_called);
On 2014/07/01 19:45:12, mmenke wrote:
> Should have the delegate modify the list, both by adding and removing a proxy
> (Different tests), and make sure neither has an effect on future resolve proxy
> requests.

Wrote a test, but it seems vaguely odd to me: it's mostly (though not entirely)
testing the behavior of my mock network delegate implementation.

https://codereview.chromium.org/332313003/diff/60001/net/proxy/proxy_service_...
net/proxy/proxy_service_unittest.cc:229: url, 0, &info, callback.callback(),
&request, NULL, log.bound());
On 2014/07/01 19:45:12, mmenke wrote:
> These should all use LOAD_NORMAL.

Done.

[rcs, 2014-07-02 06:08:55]

https://codereview.chromium.org/332313003/diff/80001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/332313003/diff/80001/net/proxy/proxy_service....
net/proxy/proxy_service.cc:1640: url, 0, &proxy_info_, callback_, NULL, NULL,
net_log);
Just noticed that I missed a LOAD_FLAGS -- will change this tomorrow morning

https://codereview.chromium.org/332313003/diff/80001/net/proxy/proxy_service_...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/332313003/diff/80001/net/proxy/proxy_service_...
net/proxy/proxy_service_unittest.cc:295: // Verify that network delegate is
invoked.
Oops, will remove

[mmenke, 2014-07-02 12:00:34]

I'm off for the rest of the week - I'll get to this on Monday.

[rcs, 2014-07-02 17:15:03]

https://codereview.chromium.org/332313003/diff/80001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/332313003/diff/80001/net/proxy/proxy_service....
net/proxy/proxy_service.cc:1640: url, 0, &proxy_info_, callback_, NULL, NULL,
net_log);
On 2014/07/02 06:08:55, rcs wrote:
> Just noticed that I missed a LOAD_FLAGS -- will change this tomorrow morning

Done.

https://codereview.chromium.org/332313003/diff/80001/net/proxy/proxy_service_...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/332313003/diff/80001/net/proxy/proxy_service_...
net/proxy/proxy_service_unittest.cc:295: // Verify that network delegate is
invoked.
On 2014/07/02 06:08:55, rcs wrote:
> Oops, will remove

Done.

[bengr, 2014-07-02 18:46:44]

https://codereview.chromium.org/332313003/diff/100001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc
(right):

https://codereview.chromium.org/332313003/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:90:
net::ProxyInfo* result) {
move up a line.

https://codereview.chromium.org/332313003/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:95:
result->proxy_server().isDataReductionProxy()) {
We're trying to deprecate ProxyServer::IsDatareductionProxy() in favor of using
DataReductionProxyParams::IsDataReductionProxy(). Can you pass params in to
here?

https://codereview.chromium.org/332313003/diff/100001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_protocol.h
(right):

https://codereview.chromium.org/332313003/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol.h:19:
class ProxyInfo;
alphabetize.

https://codereview.chromium.org/332313003/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol.h:38: //
reduction proxy, the net::LOAD_BYPASS_DATA_REDUCTION_PROXY load_flag is
Put code references in ||.

https://codereview.chromium.org/332313003/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol.h:40: void
OnResolveProxyHandler(const GURL& url, int load_flags,
Put each param on its own line if they cant all fit on one line.

https://codereview.chromium.org/332313003/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol.h:40: void
OnResolveProxyHandler(const GURL& url, int load_flags,
Forward declare GURL.

https://codereview.chromium.org/332313003/diff/100001/components/data_reducti...
File
components/data_reduction_proxy/browser/data_reduction_proxy_protocol_unittest.cc
(right):

https://codereview.chromium.org/332313003/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol_unittest.cc:633:
info1.UseNamedProxy("proxy.googlezip.net:443");
It would be better if we didn't have to rely on using a real data reduction
proxy name.

I've added a TestDataReductionProxyParams to simplify that.

https://codereview.chromium.org/332313003/diff/100001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/332313003/diff/100001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1050: network_delegate->NotifyResolveProxy(url,
load_flags, result);
If you move this to DidFinishResolvingProxy, then you can handle the
asynchronous case as well. This will be useful for some work I'm doing now. Can
you do that?

[rcs, 2014-07-02 22:53:13]

https://codereview.chromium.org/332313003/diff/100001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc
(right):

https://codereview.chromium.org/332313003/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:90:
net::ProxyInfo* result) {
On 2014/07/02 18:46:43, bengr1 wrote:
> move up a line.

Breaks 80 chars. I changed it to have one arg per line.

https://codereview.chromium.org/332313003/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:95:
result->proxy_server().isDataReductionProxy()) {
On 2014/07/02 18:46:43, bengr1 wrote:
> We're trying to deprecate ProxyServer::IsDatareductionProxy() in favor of
using
> DataReductionProxyParams::IsDataReductionProxy(). Can you pass params in to
> here?

Done.

https://codereview.chromium.org/332313003/diff/100001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_protocol.h
(right):

https://codereview.chromium.org/332313003/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol.h:19:
class ProxyInfo;
On 2014/07/02 18:46:43, bengr1 wrote:
> alphabetize.

Done.

https://codereview.chromium.org/332313003/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol.h:38: //
reduction proxy, the net::LOAD_BYPASS_DATA_REDUCTION_PROXY load_flag is
On 2014/07/02 18:46:43, bengr1 wrote:
> Put code references in ||.

Done.

https://codereview.chromium.org/332313003/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol.h:40: void
OnResolveProxyHandler(const GURL& url, int load_flags,
On 2014/07/02 18:46:43, bengr1 wrote:
> Forward declare GURL.

Done.

https://codereview.chromium.org/332313003/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol.h:40: void
OnResolveProxyHandler(const GURL& url, int load_flags,
On 2014/07/02 18:46:43, bengr1 wrote:
> Put each param on its own line if they cant all fit on one line.

Done.

https://codereview.chromium.org/332313003/diff/100001/components/data_reducti...
File
components/data_reduction_proxy/browser/data_reduction_proxy_protocol_unittest.cc
(right):

https://codereview.chromium.org/332313003/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol_unittest.cc:633:
info1.UseNamedProxy("proxy.googlezip.net:443");
On 2014/07/02 18:46:43, bengr1 wrote:
> It would be better if we didn't have to rely on using a real data reduction
> proxy name.
> 
> I've added a TestDataReductionProxyParams to simplify that.

Done.

https://codereview.chromium.org/332313003/diff/100001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/332313003/diff/100001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1050: network_delegate->NotifyResolveProxy(url,
load_flags, result);
On 2014/07/02 18:46:43, bengr1 wrote:
> If you move this to DidFinishResolvingProxy, then you can handle the
> asynchronous case as well. This will be useful for some work I'm doing now.
Can
> you do that?

Done.

[bengr, 2014-07-07 16:28:14]

components/data_reduction_proxy: lgtm, with nit.

https://codereview.chromium.org/332313003/diff/120001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_protocol.h
(right):

https://codereview.chromium.org/332313003/diff/120001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol.h:37: //
Upon |ProxyService.ResolveProxy()|, bypass if the effective proxy is the data
Say something about when this method is called. E.g., is it meant to be called
only from a particular method of NetworkDelegate? Also, be more specific about
what "bypass" means, because the meaning here is different from how "bypass" is
used in other data reduction proxy contexts. (Except for here, "bypass" means
put the proxy on the ProxyRetryInfo map.)

[bengr, 2014-07-07 18:08:55]

On 2014/07/07 16:28:14, bengr1 wrote:
> components/data_reduction_proxy: lgtm, with nit.
> 
>
https://codereview.chromium.org/332313003/diff/120001/components/data_reducti...
> File components/data_reduction_proxy/browser/data_reduction_proxy_protocol.h
> (right):
> 
>
https://codereview.chromium.org/332313003/diff/120001/components/data_reducti...
> components/data_reduction_proxy/browser/data_reduction_proxy_protocol.h:37: //
> Upon |ProxyService.ResolveProxy()|, bypass if the effective proxy is the data
> Say something about when this method is called. E.g., is it meant to be called
> only from a particular method of NetworkDelegate? Also, be more specific about
> what "bypass" means, because the meaning here is different from how "bypass"
is
> used in other data reduction proxy contexts. (Except for here, "bypass" means
> put the proxy on the ProxyRetryInfo map.)

Please add a BUG number.

[rcs, 2014-07-07 18:44:16]

Reviewers -= eroman

Address bengr's final feedback

[mmenke, 2014-07-07 20:22:00]

Just minor comments

https://codereview.chromium.org/332313003/diff/140001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/332313003/diff/140001/chrome/browser/io_threa...
chrome/browser/io_thread.h:200: on_resolve_proxy_handler;
No need for a scoped_ptr here, callbacks have their own method to check if
they're NULL.

https://codereview.chromium.org/332313003/diff/140001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/332313003/diff/140001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:482: data_reduction_proxy_params_,
result);
nit:  Use braces when if body is more than one line.

https://codereview.chromium.org/332313003/diff/140001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.h (right):

https://codereview.chromium.org/332313003/diff/140001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.h:152: net::ProxyInfo* result)>
OnResolveProxyHandler;
nit:  Per Google style guide, typedefs (and enums) go first.

https://codereview.chromium.org/332313003/diff/140001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.h:298: OnResolveProxyHandler*
on_resolve_proxy_handler_;
Suggest not making this a pointer.  Callbacks have a method...is_empty(), I
believe (Or maybe is_null() or is_valid() or somesuch).

As-is, this is perfectly safe, just keeping a pointer to a callback seems a bit
ugly to me.

https://codereview.chromium.org/332313003/diff/140001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc
(right):

https://codereview.chromium.org/332313003/diff/140001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:21: 
nit:  Remove extra blank line.

https://codereview.chromium.org/332313003/diff/140001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:23:
bool SetProxyServerFromGURL(const GURL& gurl,
nit:  blank line after start and before end of namespaces.

https://codereview.chromium.org/332313003/diff/140001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:93: if
((load_flags & net::LOAD_BYPASS_DATA_REDUCTION_PROXY) &&
No need to do anything in this CL in response to this since I'm suggesting a
significant rework here.  Since Ben's changing how things works, this may not
matter, long term, anyways.

It would probably be simpler to go the other way - switch to
LOAD_ALLOW_DATA_REDUCTION_PROXY, and then use:

if (result->is_direct_only()) {
  // Modify result to try the proxy first, and then use direct as a fallback.
}

Then you could probably get rid of some of the other DRP hooks as well.

https://codereview.chromium.org/332313003/diff/140001/net/base/load_flags_list.h
File net/base/load_flags_list.h (right):

https://codereview.chromium.org/332313003/diff/140001/net/base/load_flags_lis...
net/base/load_flags_list.h:130: //
https://code.google.com/p/chromium/issues/detail?id=339237
nit:  Suggest just using "http://crbug.com/339237" instead, for shorter links.

https://codereview.chromium.org/332313003/diff/140001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/332313003/diff/140001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1335: network_delegate->NotifyResolveProxy(url,
load_flags, result);
Suggest getting rid of the redundant copy, and just putting:

if (result_code == OK && network_delegate)
  network_delegate->NotifyResolveProxy(url, load_flags, result);

at the end.

https://codereview.chromium.org/332313003/diff/140001/net/proxy/proxy_service...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/332313003/diff/140001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:162: : NetworkDelegate(),
nit:  NetworkDelegatE() not needed.

https://codereview.chromium.org/332313003/diff/140001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:169: const GURL& url, int load_flags,
ProxyInfo* result) {
nit:  OVERRIDE

https://codereview.chromium.org/332313003/diff/140001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:171: DCHECK(!(add_proxy && remove_proxy));
nit:  I think

DCHECK(!add_proxy || !remove_proxy) would be clearer.

https://codereview.chromium.org/332313003/diff/140001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:181: bool remove_proxy;
These should be private, with public getters/setters as needed.

https://codereview.chromium.org/332313003/diff/140001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:251: // and checking that subsequent
requests are not effected.
Nit:  affected.

https://codereview.chromium.org/332313003/diff/140001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:253: // Callback should interpose:
nit:  Suggest a blank line before each of these 3 requests, for easier reading. 
Same goes for the other test.

[rcs, 2014-07-07 22:08:50]

https://codereview.chromium.org/332313003/diff/140001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/332313003/diff/140001/chrome/browser/io_threa...
chrome/browser/io_thread.h:200: on_resolve_proxy_handler;
On 2014/07/07 20:21:59, mmenke wrote:
> No need for a scoped_ptr here, callbacks have their own method to check if
> they're NULL.

Done.

https://codereview.chromium.org/332313003/diff/140001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/332313003/diff/140001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:482: data_reduction_proxy_params_,
result);
On 2014/07/07 20:21:59, mmenke wrote:
> nit:  Use braces when if body is more than one line.

Done.

https://codereview.chromium.org/332313003/diff/140001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.h (right):

https://codereview.chromium.org/332313003/diff/140001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.h:152: net::ProxyInfo* result)>
OnResolveProxyHandler;
On 2014/07/07 20:21:59, mmenke wrote:
> nit:  Per Google style guide, typedefs (and enums) go first.

Done.

https://codereview.chromium.org/332313003/diff/140001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.h:298: OnResolveProxyHandler*
on_resolve_proxy_handler_;
On 2014/07/07 20:21:59, mmenke wrote:
> Suggest not making this a pointer.  Callbacks have a method...is_empty(), I
> believe (Or maybe is_null() or is_valid() or somesuch).
> 
> As-is, this is perfectly safe, just keeping a pointer to a callback seems a
bit
> ugly to me.

Done.

https://codereview.chromium.org/332313003/diff/140001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc
(right):

https://codereview.chromium.org/332313003/diff/140001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:21: 
On 2014/07/07 20:21:59, mmenke wrote:
> nit:  Remove extra blank line.

Done.

https://codereview.chromium.org/332313003/diff/140001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:23:
bool SetProxyServerFromGURL(const GURL& gurl,
On 2014/07/07 20:21:59, mmenke wrote:
> nit:  blank line after start and before end of namespaces.

Done.

https://codereview.chromium.org/332313003/diff/140001/net/base/load_flags_list.h
File net/base/load_flags_list.h (right):

https://codereview.chromium.org/332313003/diff/140001/net/base/load_flags_lis...
net/base/load_flags_list.h:130: //
https://code.google.com/p/chromium/issues/detail?id=339237
On 2014/07/07 20:21:59, mmenke wrote:
> nit:  Suggest just using "http://crbug.com/339237" instead, for shorter links.

Done.

https://codereview.chromium.org/332313003/diff/140001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/332313003/diff/140001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1335: network_delegate->NotifyResolveProxy(url,
load_flags, result);
On 2014/07/07 20:21:59, mmenke wrote:
> Suggest getting rid of the redundant copy, and just putting:
> 
> if (result_code == OK && network_delegate)
>   network_delegate->NotifyResolveProxy(url, load_flags, result);
> 
> at the end.

That was my original implementation, but I noticed that the net_log.AddEvent()
invocation on line 1312 references the result pointer. If I put my conditional
at the end, it would log the wrong resolver if the callback decides to bypass
the DCP.

https://codereview.chromium.org/332313003/diff/140001/net/proxy/proxy_service...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/332313003/diff/140001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:162: : NetworkDelegate(),
On 2014/07/07 20:22:00, mmenke wrote:
> nit:  NetworkDelegatE() not needed.

Done.

https://codereview.chromium.org/332313003/diff/140001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:169: const GURL& url, int load_flags,
ProxyInfo* result) {
On 2014/07/07 20:22:00, mmenke wrote:
> nit:  OVERRIDE

Done.

https://codereview.chromium.org/332313003/diff/140001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:171: DCHECK(!(add_proxy && remove_proxy));
On 2014/07/07 20:22:00, mmenke wrote:
> nit:  I think
> 
> DCHECK(!add_proxy || !remove_proxy) would be clearer.

Done.

https://codereview.chromium.org/332313003/diff/140001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:181: bool remove_proxy;
On 2014/07/07 20:22:00, mmenke wrote:
> These should be private, with public getters/setters as needed.

Done.

https://codereview.chromium.org/332313003/diff/140001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:251: // and checking that subsequent
requests are not effected.
On 2014/07/07 20:21:59, mmenke wrote:
> Nit:  affected.

Done.

https://codereview.chromium.org/332313003/diff/140001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:253: // Callback should interpose:
On 2014/07/07 20:22:00, mmenke wrote:
> nit:  Suggest a blank line before each of these 3 requests, for easier
reading. 
> Same goes for the other test.

Done.

[mmenke, 2014-07-08 15:27:53]

LGTM!

https://codereview.chromium.org/332313003/diff/160001/net/base/load_flags_list.h
File net/base/load_flags_list.h (right):

https://codereview.chromium.org/332313003/diff/160001/net/base/load_flags_lis...
net/base/load_flags_list.h:129: // TODO(rcs): remove this flag as soon as
http://crbug.com/339237 is resolved.
nit:  Capitalize remove

https://codereview.chromium.org/332313003/diff/160001/net/proxy/proxy_service...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/332313003/diff/160001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:178: bool on_resolve_proxy_called() {
nit:   bool on_resolve_proxy_called() const {

[rcs, 2014-07-08 17:31:13]

wahoo!

https://codereview.chromium.org/332313003/diff/160001/net/base/load_flags_list.h
File net/base/load_flags_list.h (right):

https://codereview.chromium.org/332313003/diff/160001/net/base/load_flags_lis...
net/base/load_flags_list.h:129: // TODO(rcs): remove this flag as soon as
http://crbug.com/339237 is resolved.
On 2014/07/08 15:27:53, mmenke wrote:
> nit:  Capitalize remove

Done.

https://codereview.chromium.org/332313003/diff/160001/net/proxy/proxy_service...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/332313003/diff/160001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:178: bool on_resolve_proxy_called() {
On 2014/07/08 15:27:53, mmenke wrote:
> nit:   bool on_resolve_proxy_called() const {

Done.

[mmenke, 2014-07-08 18:18:19]

I think you're going to have to merge again - my CL that deleted the
SyncProxyServiceHelper landed last night.

[rcs, 2014-07-08 18:19:32]

On 2014/07/08 18:18:19, mmenke wrote:
> I think you're going to have to merge again - my CL that deleted the
> SyncProxyServiceHelper landed last night.

Thanks, just bumped into that a few minutes ago :-) the latest CL accounts for
that. Bengr's running the try bot now

[rcs, 2014-07-08 22:41:49]

The CQ bit was checked by rcs@chromium.org

[rcs, 2014-07-08 22:42:16]

committing

[commit-bot: I haz the power, 2014-07-08 22:43:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rcs@chromium.org/332313003/240001

[commit-bot: I haz the power, 2014-07-09 02:36:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-09 05:23:57]

Change committed as 281951