Add MaybeInterceptRedirect/Response to URLRequestInterceptor

Adds MaybeInterceptRedirect and MaybeInterceptResponse to the URLRequestInterceptor interface as well as to URLRequestJobFactory.

BUG=429505
Committed: https://crrev.com/1bf8e9456a103d086f53400bdc169c437a1590a4
Cr-Commit-Position: refs/heads/master@{#303154}

[bengr, 2014-10-30 01:24:44]

Patchset #1 (id:1) has been deleted

[bengr, 2014-10-30 01:38:41]

Patchset #1 (id:20001) has been deleted

[mmenke, 2014-10-30 19:59:23]

mmenke@chromium.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2014-10-30 19:59:24]

Only looked at the net/ code, but this is just what I had in mind (Modulo not
getting rid of the deprecated API).

https://codereview.chromium.org/686343002/diff/40001/net/url_request/url_requ...
File net/url_request/url_request_intercepting_job_factory.cc (right):

https://codereview.chromium.org/686343002/diff/40001/net/url_request/url_requ...
net/url_request/url_request_intercepting_job_factory.cc:39: network_delegate);
These aren't right.  These should work just like
MaybeCreateJobWithProtocolHandler - if the interceptor doesn't do anything, pass
the request on to job_factory_.

https://codereview.chromium.org/686343002/diff/40001/net/url_request/url_requ...
File net/url_request/url_request_job_manager.cc (right):

https://codereview.chromium.org/686343002/diff/40001/net/url_request/url_requ...
net/url_request/url_request_job_manager.cc:132: request, network_delegate,
location);
These should go before the old interceptors, for consistency.

https://codereview.chromium.org/686343002/diff/40001/net/url_request/url_requ...
net/url_request/url_request_job_manager.cc:132: request, network_delegate,
location);
Ideally, these should ignore the LOAD_DISABLE_INTERCEPT flag.  AppCache should
just track the URLRequests itself, probably by attaching UserData to them (And
it actually already does this, I believe).

Suppose that should probably be in its own CL, though, after AppCache has been
moved to the new API, so not really something you should worry about (Just don't
use the load flag in your new code).

[bengr, 2014-10-31 00:45:31]

https://codereview.chromium.org/686343002/diff/40001/net/url_request/url_requ...
File net/url_request/url_request_intercepting_job_factory.cc (right):

https://codereview.chromium.org/686343002/diff/40001/net/url_request/url_requ...
net/url_request/url_request_intercepting_job_factory.cc:39: network_delegate);
On 2014/10/30 19:59:24, mmenke wrote:
> These aren't right.  These should work just like
> MaybeCreateJobWithProtocolHandler - if the interceptor doesn't do anything,
pass
> the request on to job_factory_.

Right. Done.

https://codereview.chromium.org/686343002/diff/40001/net/url_request/url_requ...
File net/url_request/url_request_job_manager.cc (right):

https://codereview.chromium.org/686343002/diff/40001/net/url_request/url_requ...
net/url_request/url_request_job_manager.cc:132: request, network_delegate,
location);
On 2014/10/30 19:59:24, mmenke wrote:
> These should go before the old interceptors, for consistency.

Done.

https://codereview.chromium.org/686343002/diff/40001/net/url_request/url_requ...
net/url_request/url_request_job_manager.cc:132: request, network_delegate,
location);
On 2014/10/30 19:59:24, mmenke wrote:
> Ideally, these should ignore the LOAD_DISABLE_INTERCEPT flag.  AppCache should
> just track the URLRequests itself, probably by attaching UserData to them (And
> it actually already does this, I believe).
> 
> Suppose that should probably be in its own CL, though, after AppCache has been
> moved to the new API, so not really something you should worry about (Just
don't
> use the load flag in your new code).

Acknowledged.

[michaeln1, 2014-10-31 00:47:09]

michaeln@google.com changed reviewers:
+ michaeln@google.com

[michaeln1, 2014-10-31 00:47:10]

https://codereview.chromium.org/686343002/diff/40001/net/url_request/url_requ...
File net/url_request/url_request_job_manager.cc (right):

https://codereview.chromium.org/686343002/diff/40001/net/url_request/url_requ...
net/url_request/url_request_job_manager.cc:132: request, network_delegate,
location);
On 2014/10/30 19:59:24, mmenke wrote:
> These should go before the old interceptors, for consistency.

Consistency with what?

The old URLRequest::Interceptor's MaybeIntercept method gets called prior to the
corresponding method of the new URLReqeustInterceptor.

https://codereview.chromium.org/686343002/diff/60001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.h
(right):

https://codereview.chromium.org/686343002/diff/60001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.h:24:
net::URLRequestJob* MaybeInterceptResponse(
i'm surprised to see these additions to the non-deprecated api

[mmenke, 2014-10-31 00:55:42]

On 2014/10/31 00:47:10, michaeln1 wrote:
>
https://codereview.chromium.org/686343002/diff/40001/net/url_request/url_requ...
> File net/url_request/url_request_job_manager.cc (right):
> 
>
https://codereview.chromium.org/686343002/diff/40001/net/url_request/url_requ...
> net/url_request/url_request_job_manager.cc:132: request, network_delegate,
> location);
> On 2014/10/30 19:59:24, mmenke wrote:
> > These should go before the old interceptors, for consistency.
> 
> Consistency with what?
> 
> The old URLRequest::Interceptor's MaybeIntercept method gets called prior to
the
> corresponding method of the new URLReqeustInterceptor.

Oops, you're right...saw us dereferencing the job_factory, and just assumed we
were using it first.

https://codereview.chromium.org/686343002/diff/60001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.h
(right):

https://codereview.chromium.org/686343002/diff/60001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.h:24:
net::URLRequestJob* MaybeInterceptResponse(
On 2014/10/31 00:47:10, michaeln1 wrote:
> i'm surprised to see these additions to the non-deprecated api

Sadly, to support appcache, we have no choice to do this, or something
equivalent (And we have 2 other teams interested in retrying requests in some
circumstances).

There are alternatives.  Could have a URLRequest wrap another URLRequest, but
after experimenting with an implementation of it, there were just way too many
corner cases to deal with, and it still exposed other weirdness to other
consumers.  Could do it at a higher level, starting a new URLRequest, but that
also has a lot of issues.  Could let listeners force a URLRequest start with a
new job, but that really doesn't make anything better.

[mmenke, 2014-10-31 01:05:05]

On 2014/10/31 00:55:42, mmenke wrote:
> On 2014/10/31 00:47:10, michaeln1 wrote:
> >
>
https://codereview.chromium.org/686343002/diff/40001/net/url_request/url_requ...
> > File net/url_request/url_request_job_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/686343002/diff/40001/net/url_request/url_requ...
> > net/url_request/url_request_job_manager.cc:132: request, network_delegate,
> > location);
> > On 2014/10/30 19:59:24, mmenke wrote:
> > > These should go before the old interceptors, for consistency.
> > 
> > Consistency with what?
> > 
> > The old URLRequest::Interceptor's MaybeIntercept method gets called prior to
> the
> > corresponding method of the new URLReqeustInterceptor.
> 
> Oops, you're right...saw us dereferencing the job_factory, and just assumed we
> were using it first.
> 
>
https://codereview.chromium.org/686343002/diff/60001/components/data_reductio...
> File
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.h
> (right):
> 
>
https://codereview.chromium.org/686343002/diff/60001/components/data_reductio...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.h:24:
> net::URLRequestJob* MaybeInterceptResponse(
> On 2014/10/31 00:47:10, michaeln1 wrote:
> > i'm surprised to see these additions to the non-deprecated api
> 
> Sadly, to support appcache, we have no choice to do this, or something
> equivalent (And we have 2 other teams interested in retrying requests in some
> circumstances).
> 
> There are alternatives.  Could have a URLRequest wrap another URLRequest, but
> after experimenting with an implementation of it, there were just way too many
> corner cases to deal with, and it still exposed other weirdness to other
> consumers.  Could do it at a higher level, starting a new URLRequest, but that
> also has a lot of issues.  Could let listeners force a URLRequest start with a
> new job, but that really doesn't make anything better.

Oh, and I should add, if you have a better idea on how to handle this, I'm all
ears.

[mmenke, 2014-10-31 01:28:00]

On 2014/10/31 01:05:05, mmenke wrote:
> On 2014/10/31 00:55:42, mmenke wrote:
> > On 2014/10/31 00:47:10, michaeln1 wrote:
> > >
> >
>
https://codereview.chromium.org/686343002/diff/40001/net/url_request/url_requ...
> > > File net/url_request/url_request_job_manager.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/686343002/diff/40001/net/url_request/url_requ...
> > > net/url_request/url_request_job_manager.cc:132: request, network_delegate,
> > > location);
> > > On 2014/10/30 19:59:24, mmenke wrote:
> > > > These should go before the old interceptors, for consistency.
> > > 
> > > Consistency with what?
> > > 
> > > The old URLRequest::Interceptor's MaybeIntercept method gets called prior
to
> > the
> > > corresponding method of the new URLReqeustInterceptor.
> > 
> > Oops, you're right...saw us dereferencing the job_factory, and just assumed
we
> > were using it first.
> > 
> >
>
https://codereview.chromium.org/686343002/diff/60001/components/data_reductio...
> > File
> >
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.h
> > (right):
> > 
> >
>
https://codereview.chromium.org/686343002/diff/60001/components/data_reductio...
> >
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.h:24:
> > net::URLRequestJob* MaybeInterceptResponse(
> > On 2014/10/31 00:47:10, michaeln1 wrote:
> > > i'm surprised to see these additions to the non-deprecated api
> > 
> > Sadly, to support appcache, we have no choice to do this, or something
> > equivalent (And we have 2 other teams interested in retrying requests in
some
> > circumstances).
> > 
> > There are alternatives.  Could have a URLRequest wrap another URLRequest,
but
> > after experimenting with an implementation of it, there were just way too
many
> > corner cases to deal with, and it still exposed other weirdness to other
> > consumers.  Could do it at a higher level, starting a new URLRequest, but
that
> > also has a lot of issues.  Could let listeners force a URLRequest start with
a
> > new job, but that really doesn't make anything better.
> 
> Oh, and I should add, if you have a better idea on how to handle this, I'm all
> ears.

Hrm...One other option may be URLRequest::Restart, but we may have to allow it
to be called a little later than we currently do.  Haven't really thought about
it.

[mmenke, 2014-10-31 02:23:02]

On 2014/10/31 01:28:00, mmenke wrote:
> On 2014/10/31 01:05:05, mmenke wrote:
> > On 2014/10/31 00:55:42, mmenke wrote:
> > > On 2014/10/31 00:47:10, michaeln1 wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/686343002/diff/40001/net/url_request/url_requ...
> > > > File net/url_request/url_request_job_manager.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/686343002/diff/40001/net/url_request/url_requ...
> > > > net/url_request/url_request_job_manager.cc:132: request,
network_delegate,
> > > > location);
> > > > On 2014/10/30 19:59:24, mmenke wrote:
> > > > > These should go before the old interceptors, for consistency.
> > > > 
> > > > Consistency with what?
> > > > 
> > > > The old URLRequest::Interceptor's MaybeIntercept method gets called
prior
> to
> > > the
> > > > corresponding method of the new URLReqeustInterceptor.
> > > 
> > > Oops, you're right...saw us dereferencing the job_factory, and just
assumed
> we
> > > were using it first.
> > > 
> > >
> >
>
https://codereview.chromium.org/686343002/diff/60001/components/data_reductio...
> > > File
> > >
> >
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.h
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/686343002/diff/60001/components/data_reductio...
> > >
> >
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.h:24:
> > > net::URLRequestJob* MaybeInterceptResponse(
> > > On 2014/10/31 00:47:10, michaeln1 wrote:
> > > > i'm surprised to see these additions to the non-deprecated api
> > > 
> > > Sadly, to support appcache, we have no choice to do this, or something
> > > equivalent (And we have 2 other teams interested in retrying requests in
> some
> > > circumstances).
> > > 
> > > There are alternatives.  Could have a URLRequest wrap another URLRequest,
> but
> > > after experimenting with an implementation of it, there were just way too
> many
> > > corner cases to deal with, and it still exposed other weirdness to other
> > > consumers.  Could do it at a higher level, starting a new URLRequest, but
> that
> > > also has a lot of issues.  Could let listeners force a URLRequest start
with
> a
> > > new job, but that really doesn't make anything better.
> > 
> > Oh, and I should add, if you have a better idea on how to handle this, I'm
all
> > ears.
> 
> Hrm...One other option may be URLRequest::Restart, but we may have to allow it
> to be called a little later than we currently do.  Haven't really thought
about
> it.

And that gets us into pretty much the same situation this does...Though it seems
weird to have two different ways to do the same thing.  Should probably look
into getting rid of the RestartRequest function, rather than the other way
around.

[mmenke, 2014-10-31 17:52:06]

On 2014/10/31 02:23:02, mmenke wrote:
> On 2014/10/31 01:28:00, mmenke wrote:
> > On 2014/10/31 01:05:05, mmenke wrote:
> > > On 2014/10/31 00:55:42, mmenke wrote:
> > > > On 2014/10/31 00:47:10, michaeln1 wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/686343002/diff/40001/net/url_request/url_requ...
> > > > > File net/url_request/url_request_job_manager.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/686343002/diff/40001/net/url_request/url_requ...
> > > > > net/url_request/url_request_job_manager.cc:132: request,
> network_delegate,
> > > > > location);
> > > > > On 2014/10/30 19:59:24, mmenke wrote:
> > > > > > These should go before the old interceptors, for consistency.
> > > > > 
> > > > > Consistency with what?
> > > > > 
> > > > > The old URLRequest::Interceptor's MaybeIntercept method gets called
> prior
> > to
> > > > the
> > > > > corresponding method of the new URLReqeustInterceptor.
> > > > 
> > > > Oops, you're right...saw us dereferencing the job_factory, and just
> assumed
> > we
> > > > were using it first.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/686343002/diff/60001/components/data_reductio...
> > > > File
> > > >
> > >
> >
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.h
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/686343002/diff/60001/components/data_reductio...
> > > >
> > >
> >
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.h:24:
> > > > net::URLRequestJob* MaybeInterceptResponse(
> > > > On 2014/10/31 00:47:10, michaeln1 wrote:
> > > > > i'm surprised to see these additions to the non-deprecated api
> > > > 
> > > > Sadly, to support appcache, we have no choice to do this, or something
> > > > equivalent (And we have 2 other teams interested in retrying requests in
> > some
> > > > circumstances).
> > > > 
> > > > There are alternatives.  Could have a URLRequest wrap another
URLRequest,
> > but
> > > > after experimenting with an implementation of it, there were just way
too
> > many
> > > > corner cases to deal with, and it still exposed other weirdness to other
> > > > consumers.  Could do it at a higher level, starting a new URLRequest,
but
> > that
> > > > also has a lot of issues.  Could let listeners force a URLRequest start
> with
> > a
> > > > new job, but that really doesn't make anything better.
> > > 
> > > Oh, and I should add, if you have a better idea on how to handle this, I'm
> all
> > > ears.
> > 
> > Hrm...One other option may be URLRequest::Restart, but we may have to allow
it
> > to be called a little later than we currently do.  Haven't really thought
> about
> > it.
> 
> And that gets us into pretty much the same situation this does...Though it
seems
> weird to have two different ways to do the same thing.  Should probably look
> into getting rid of the RestartRequest function, rather than the other way
> around.

So I do think this is the way to go.

On unit tests:  I think that we want something like the interceptor tests, but
more thorough.


In addition to the current tests, we should check that when something intercepts
real URLRequestHTTPJobs (Not mock jobs) on errors or redirects, the
NetworkDelegate and URLRequest::Delegate callbacks get reasonable results.  The
basic idea is the URLRequest::Delegate should never see the "real" error, or the
restart.  The NetworkDelegate should get NotifyHeadersReceived for the original
URL in the redirect case (Which is ugly), and in both cases it should get
NotifyBeforeURLRequest/NotifyStart/NotifyBeforeSendHeaders for both the retry
and the original request.

TestNetworkDelegate and TestDelegate in url_request_test_util.h should make
event counting fairly easy, and should check the headers you get in the retry
case, before the request is intercepted.

To set up a test server, you can look at the tests that use LocalHttpTestServer.
 To get an error on a real HTTP request, you can make a response header file
(See files in net/data/url_request_unittest/) with two Content-Length headers
with different values.  We already have files to mimic redirects.


I think it makes sense to move the tests from url_request_unittest.cc to
url_request_job_factory_impl_unittest.cc, given how huge and bloated the former
is, but think for easier review, should keep them in url_request_unittest.cc for
now.

[chromium-reviews, 2014-10-31 19:19:11]

I've never fully understood the motivation to get rid of ability to restart
or the intercept upon receipt of the response head? I think the
capabilities these enable are more difficult/costly to pull off in the
absence of a mechanism like this.

Interesting idea about providing that intercept on response ability by
providing a URLRequest::Restart method which could be called from within
resp/redir received delegate callbacks. That sounds plausible but its not
clear to me if it would be an upgrade ,downgrade, or sidegrade from the
current mechanism?


On Fri, Oct 31, 2014 at 10:52 AM, <mmenke@chromium.org> wrote:

> On 2014/10/31 02:23:02, mmenke wrote:
>
>> On 2014/10/31 01:28:00, mmenke wrote:
>> > On 2014/10/31 01:05:05, mmenke wrote:
>> > > On 2014/10/31 00:55:42, mmenke wrote:
>> > > > On 2014/10/31 00:47:10, michaeln1 wrote:
>> > > > >
>> > > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/686343002/diff/40001/net/
> url_request/url_request_job_manager.cc
>
>> > > > > File net/url_request/url_request_job_manager.cc (right):
>> > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/686343002/diff/40001/net/
> url_request/url_request_job_manager.cc#newcode132
>
>> > > > > net/url_request/url_request_job_manager.cc:132: request,
>> network_delegate,
>> > > > > location);
>> > > > > On 2014/10/30 19:59:24, mmenke wrote:
>> > > > > > These should go before the old interceptors, for consistency.
>> > > > >
>> > > > > Consistency with what?
>> > > > >
>> > > > > The old URLRequest::Interceptor's MaybeIntercept method gets
>> called
>> prior
>> > to
>> > > > the
>> > > > > corresponding method of the new URLReqeustInterceptor.
>> > > >
>> > > > Oops, you're right...saw us dereferencing the job_factory, and just
>> assumed
>> > we
>> > > > were using it first.
>> > > >
>> > > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/686343002/diff/60001/
> components/data_reduction_proxy/core/browser/data_
> reduction_proxy_interceptor.h
>
>> > > > File
>> > > >
>> > >
>> >
>>
>
> components/data_reduction_proxy/core/browser/data_
> reduction_proxy_interceptor.h
>
>> > > > (right):
>> > > >
>> > > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/686343002/diff/60001/
> components/data_reduction_proxy/core/browser/data_
> reduction_proxy_interceptor.h#newcode24
>
>> > > >
>> > >
>> >
>>
>
> components/data_reduction_proxy/core/browser/data_
> reduction_proxy_interceptor.h:24:
>
>> > > > net::URLRequestJob* MaybeInterceptResponse(
>> > > > On 2014/10/31 00:47:10, michaeln1 wrote:
>> > > > > i'm surprised to see these additions to the non-deprecated api
>> > > >
>> > > > Sadly, to support appcache, we have no choice to do this, or
>> something
>> > > > equivalent (And we have 2 other teams interested in retrying
>> requests in
>> > some
>> > > > circumstances).
>> > > >
>> > > > There are alternatives.  Could have a URLRequest wrap another
>>
> URLRequest,
>
>> > but
>> > > > after experimenting with an implementation of it, there were just
>> way
>>
> too
>
>> > many
>> > > > corner cases to deal with, and it still exposed other weirdness to
>> other
>> > > > consumers.  Could do it at a higher level, starting a new
>> URLRequest,
>>
> but
>
>> > that
>> > > > also has a lot of issues.  Could let listeners force a URLRequest
>> start
>> with
>> > a
>> > > > new job, but that really doesn't make anything better.
>> > >
>> > > Oh, and I should add, if you have a better idea on how to handle
>> this, I'm
>> all
>> > > ears.
>> >
>> > Hrm...One other option may be URLRequest::Restart, but we may have to
>> allow
>>
> it
>
>> > to be called a little later than we currently do.  Haven't really
>> thought
>> about
>> > it.
>>
>
>  And that gets us into pretty much the same situation this does...Though it
>>
> seems
>
>> weird to have two different ways to do the same thing.  Should probably
>> look
>> into getting rid of the RestartRequest function, rather than the other way
>> around.
>>
>
> So I do think this is the way to go.
>
> On unit tests:  I think that we want something like the interceptor tests,
> but
> more thorough.
>
>
> In addition to the current tests, we should check that when something
> intercepts
> real URLRequestHTTPJobs (Not mock jobs) on errors or redirects, the
> NetworkDelegate and URLRequest::Delegate callbacks get reasonable
> results.  The
> basic idea is the URLRequest::Delegate should never see the "real" error,
> or the
> restart.  The NetworkDelegate should get NotifyHeadersReceived for the
> original
> URL in the redirect case (Which is ugly), and in both cases it should get
> NotifyBeforeURLRequest/NotifyStart/NotifyBeforeSendHeaders for both the
> retry
> and the original request.
>
> TestNetworkDelegate and TestDelegate in url_request_test_util.h should make
> event counting fairly easy, and should check the headers you get in the
> retry
> case, before the request is intercepted.
>
> To set up a test server, you can look at the tests that use
> LocalHttpTestServer.
>  To get an error on a real HTTP request, you can make a response header
> file
> (See files in net/data/url_request_unittest/) with two Content-Length
> headers
> with different values.  We already have files to mimic redirects.
>
>
> I think it makes sense to move the tests from url_request_unittest.cc to
> url_request_job_factory_impl_unittest.cc, given how huge and bloated the
> former
> is, but think for easier review, should keep them in
> url_request_unittest.cc for
> now.
>
> https://codereview.chromium.org/686343002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mmenke, 2014-10-31 19:27:52]

On 2014/10/31 19:19:11, chromium-reviews wrote:
> I've never fully understood the motivation to get rid of ability to restart
> or the intercept upon receipt of the response head? I think the
> capabilities these enable are more difficult/costly to pull off in the
> absence of a mechanism like this.
> 
> Interesting idea about providing that intercept on response ability by
> providing a URLRequest::Restart method which could be called from within
> resp/redir received delegate callbacks. That sounds plausible but its not
> clear to me if it would be an upgrade ,downgrade, or sidegrade from the
> current mechanism?

The main reason is they result in the NetworkDelegates seeing odd events, which
can have very real consequences for other code (Particularly external
extensions, but also code inside chrome).  We really don't want to expose the
weirdness of these cases to the world.

Restart() results in NetworkDelegates seeing no response to the initial request,
but seeing the same request go through the startup process again, with a new
URL.

Restarting on error looks the same - NetworkDelegates never see the error, but
do see the restart.

When restarting on redirect, the NetworkDelegates see the redirect, but then
they see a new request...for a new redirect, that wasn't the URL the redirect
was really for.

[mmenke, 2014-10-31 19:29:15]

On 2014/10/31 19:27:52, mmenke wrote:
> On 2014/10/31 19:19:11, chromium-reviews wrote:
> > I've never fully understood the motivation to get rid of ability to restart
> > or the intercept upon receipt of the response head? I think the
> > capabilities these enable are more difficult/costly to pull off in the
> > absence of a mechanism like this.
> > 
> > Interesting idea about providing that intercept on response ability by
> > providing a URLRequest::Restart method which could be called from within
> > resp/redir received delegate callbacks. That sounds plausible but its not
> > clear to me if it would be an upgrade ,downgrade, or sidegrade from the
> > current mechanism?
> 
> The main reason is they result in the NetworkDelegates seeing odd events,
which
> can have very real consequences for other code (Particularly external
> extensions, but also code inside chrome).  We really don't want to expose the
> weirdness of these cases to the world.
> 
> Restart() results in NetworkDelegates seeing no response to the initial
request,
> but seeing the same request go through the startup process again, with a new
> URL.
> 
> Restarting on error looks the same - NetworkDelegates never see the error, but
> do see the restart.
> 
> When restarting on redirect, the NetworkDelegates see the redirect, but then
> they see a new request...for a new redirect, that wasn't the URL the redirect
> was really for.

The other issue is these are fairly uncommon paths through the network stack -
and the more paths there are, and the less common they are, the more likely they
have bugs (Or they cause bugs in other modules outside the network stack).

[michaeln1, 2014-10-31 20:00:46]

I see... oddness with NetworkDelegates.

> The main reason is they result in the NetworkDelegates seeing odd events,
which
> can have very real consequences for other code (Particularly external
> extensions, but also code inside chrome).  We really don't want to expose the
> weirdness of these cases to the world.

URLRequest::Delegates don't see oddities like that.

> Restart() results in NetworkDelegates seeing no response to the initial
request,
> but seeing the same request go through the startup process again, with a new
> URL.

Restart changes the url? How's that?

> Restarting on error looks the same - NetworkDelegates never see the error, but
> do see the restart.
> 
> When restarting on redirect, the NetworkDelegates see the redirect, but then
> they see a new request...for a new redirect, that wasn't the URL the redirect
> was really for.

Would it make sense to either expose the restart'ification to NetworkDelegates
unambiguously or to hide it from them entirely (as is done for
URLRequest::Delegates)? If there's no use case / need for a net delegate to
observe the restart, maybe hide them... if there is a need, make it explicit in
the interface.

[mmenke, 2014-10-31 20:22:40]

On 2014/10/31 20:00:46, michaeln1 wrote:
> I see... oddness with NetworkDelegates.
> 
> > The main reason is they result in the NetworkDelegates seeing odd events,
> which
> > can have very real consequences for other code (Particularly external
> > extensions, but also code inside chrome).  We really don't want to expose
the
> > weirdness of these cases to the world.
> 
> URLRequest::Delegates don't see oddities like that.
> 
> > Restart() results in NetworkDelegates seeing no response to the initial
> request,
> > but seeing the same request go through the startup process again, with a new
> > URL.
> 
> Restart changes the url? How's that?

You're right - it can't.

> > Restarting on error looks the same - NetworkDelegates never see the error,
but
> > do see the restart.
> > 
> > When restarting on redirect, the NetworkDelegates see the redirect, but then
> > they see a new request...for a new redirect, that wasn't the URL the
redirect
> > was really for.
> 
> Would it make sense to either expose the restart'ification to NetworkDelegates
> unambiguously or to hide it from them entirely (as is done for
> URLRequest::Delegates)? If there's no use case / need for a net delegate to
> observe the restart, maybe hide them... if there is a need, make it explicit
in
> the interface.

The problem:  In the redirect case, we can't hide it.  We want to let extensions
muck with cookies (Don't ask me why), so they need to be called before cookie
processing.  We don't want to ignore cookies when something chooses to restart a
request, so interception has to happen afterwards, in the redirect case. 
Extensions can also turn a redirect into a non-redirect case, and vice-versa.

I also forgot to mention a case...requests can also be restarted on non-error,
non-redirects, and in that case, the NetworkDelegates also see the headers for
the same reason as in the redirect case.

I believe there are also currently a couple cases where we decide there's an
error after telling the NetworkDelegate about the headers.

Ok...so, ignoring all the cases above, that leaves us with
NotifyBeforeURLRequest, NotifyResolveProxy, NotifyProxyFallback,
NotifyBeforeSendHeaders, NotifyBeforeSendProxyHeaders, and NotifySendHeaders
possible being re-sent.  The proxy ones should move down into a lower layer
delegate that doesn't even know about the URLRequest object, so let's ignore
those for the moment.

They leaves us with NotifyBeforeURLRequest, NotifyBeforeSendHeaders, and
NotifySendHeaders.

* OnBeforeURLRequest is where URLs blocked by policy are handled, do not track
is added, referrer hiding is done, the predictor is hooked up, we force "safe
search (https?)", and a call to extensions is made.

* OnBeforeSendHeaders also has youtube safesearch logic, and calls into
extensions.

* OnSendHeaders lets extensions look at the actual headers we send (Which may
change when resending requests, I assume, due to cookies and caching logic).

If we re-request the same URL with the same request object, barring any change
in group policy, we should be fine for all that stuff except maybe
OnSendHeaders, assuming nothing adds headers/removes, modifies the referer,
changes load flags, or modifies the URL (Well...I'm just assuming for the
youtube logic, didn't check that)...  but will that always be the case?  Is
there any way to really guarantee that?  Regressions in that, made by code
making incorrect assumptions, could very easily go unnoticed for years.

[bengr, 2014-11-05 01:50:31]

Patchset #4 (id:100001) has been deleted

[bengr, 2014-11-05 01:50:50]

Patchset #3 (id:80001) has been deleted

[bengr, 2014-11-05 02:03:36]

bengr@chromium.org changed reviewers:
+ cpu@chromium.org, gavinp@chromium.org, joaodasilva@chromium.org,
mtomasz@chromium.org, sgurun@chromium.org, thestig@chromium.org,
yoz@chromium.org

[bengr, 2014-11-05 02:03:37]

sgurun: android_webview/*

mtomasz: chrome/browser/chromeos/*

thestig: chrome/browser/custom_handlers/*,
chrome_browser_errorpage_browsertest.cc

yoz: chrome/browser/extensions/*

mmenke: chrome/browser/net/*, content/browser/loader/*, net/*

joaodasilva: chrome/browser/policy/*

gavinp: chrome/browser/prefetch/*, chrome/browser/prerender/*

cpu: components/component_updater/*

michaeln: content/browser/appcache/*, content/browser/fileapi/*,
content/browser/service_worker/*

[mtomasz, 2014-11-05 02:06:25]

On 2014/11/05 02:03:37, bengr wrote:
> sgurun: android_webview/*
> 
> mtomasz: chrome/browser/chromeos/*
> 
> thestig: chrome/browser/custom_handlers/*,
> chrome_browser_errorpage_browsertest.cc
> 
> yoz: chrome/browser/extensions/*
> 
> mmenke: chrome/browser/net/*, content/browser/loader/*, net/*
> 
> joaodasilva: chrome/browser/policy/*
> 
> gavinp: chrome/browser/prefetch/*, chrome/browser/prerender/*
> 
> cpu: components/component_updater/*
> 
> michaeln: content/browser/appcache/*, content/browser/fileapi/*,
> content/browser/service_worker/*

Is there any issue number related to this CL?

[mtomasz, 2014-11-05 02:08:34]

https://codereview.chromium.org/686343002/diff/120001/net/data/url_request_un...
File net/data/url_request_unittest/two-content-lengths.html.mock-http-headers
(right):

https://codereview.chromium.org/686343002/diff/120001/net/data/url_request_un...
net/data/url_request_unittest/two-content-lengths.html.mock-http-headers:4:
Content-Length: 41
The header seems doubled. Is it OK?

[bengr, 2014-11-05 02:13:20]

https://codereview.chromium.org/686343002/diff/120001/net/data/url_request_un...
File net/data/url_request_unittest/two-content-lengths.html.mock-http-headers
(right):

https://codereview.chromium.org/686343002/diff/120001/net/data/url_request_un...
net/data/url_request_unittest/two-content-lengths.html.mock-http-headers:4:
Content-Length: 41
On 2014/11/05 02:08:34, mtomasz wrote:
> The header seems doubled. Is it OK?

Yes. The intention was to create an invalid response.

[Lei Zhang, 2014-11-05 02:39:00]

Have you considered providing a default implementation for MaybeInterceptFoo(),
instead of having many impls where the majority of them just have "return NULL;"
anyway?

[bengr, 2014-11-05 03:14:07]

On 2014/11/05 02:39:00, Lei Zhang wrote:
> Have you considered providing a default implementation for
MaybeInterceptFoo(),
> instead of having many impls where the majority of them just have "return
NULL;"
> anyway?

I had. I believe mmenke@ suggested not providing one. I don't remember his
rationale. mmenke?

[bengr, 2014-11-05 05:20:19]

Patchset #4 (id:140001) has been deleted

[bengr, 2014-11-05 05:32:53]

Patchset #4 (id:160001) has been deleted

[Joao da Silva, 2014-11-05 09:22:17]

policy/ lgtm

[mmenke, 2014-11-05 16:25:38]

On 2014/11/05 03:14:07, bengr wrote:
> On 2014/11/05 02:39:00, Lei Zhang wrote:
> > Have you considered providing a default implementation for
> MaybeInterceptFoo(),
> > instead of having many impls where the majority of them just have "return
> NULL;"
> > anyway?
> 
> I had. I believe mmenke@ suggested not providing one. I don't remember his
> rationale. mmenke?

Sorry, I may not have been clear - I think default implementations in
URLRequestInterceptor are reasonable.  I think default implementations in
URLRequestJobFactory are a really bad idea, and are likely to lead to
regressions.

[bengr, 2014-11-05 18:12:57]

Patchset #5 (id:200001) has been deleted

[bengr, 2014-11-05 18:17:06]

On 2014/11/05 16:25:38, mmenke wrote:
> On 2014/11/05 03:14:07, bengr wrote:
> > On 2014/11/05 02:39:00, Lei Zhang wrote:
> > > Have you considered providing a default implementation for
> > MaybeInterceptFoo(),
> > > instead of having many impls where the majority of them just have "return
> > NULL;"
> > > anyway?
> > 
> > I had. I believe mmenke@ suggested not providing one. I don't remember his
> > rationale. mmenke?
> 
> Sorry, I may not have been clear - I think default implementations in
> URLRequestInterceptor are reasonable.  I think default implementations in
> URLRequestJobFactory are a really bad idea, and are likely to lead to
> regressions.

Updates to reviewers:

sgurun: android_webview/*

mtomasz: chrome/browser/chromeos/*

thestig: chrome/browser/custom_handlers/*

mmenke: content/browser/loader/*, net/*

michaeln: content/browser/appcache/*, content/browser/fileapi/*

[bengr, 2014-11-05 18:17:50]

bengr@chromium.org changed reviewers:
- cpu@chromium.org, gavinp@chromium.org, joaodasilva@chromium.org,
yoz@chromium.org

[Lei Zhang, 2014-11-05 18:41:17]

chrome/ lgtm

[mmenke, 2014-11-05 18:43:45]

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
File net/url_request/url_request_test_util.h (right):

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_test_util.h:376: bool can_be_intercepted_on_error_;
Maybe rename to will_be_intercepted_on_next_error_, and set 
next_states_[req_id] to kStageResponseStarted when it's set in
OnHeadersReceived, and then clear it?

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:640: 
nit:  Remove extra line break.  Looks like you tried to be systematic in where
you added them, but doesn't seem to get us much, and the prevailing style is not
to use them.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1580: // An Interceptor for use with
interceptor tests
nit:  +.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1599: 
nit:  // URLRequestInterceptor implementation:

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1600: virtual URLRequestJob*
MaybeInterceptRequest(
nit:  -virtual

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1621: // will error since the requeted
url is not one of its canned urls
nit:  Comments should all be complete sentences (Capitalize + period).

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1686: bool intercept_main_request_;
Per google style guide, these should all be private.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1691: // Other actions we take at
MaybeIntercept time
Comments should all be capitalized, end with periods.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1754: class URLRequestInterceptorTest :
public PlatformTest {
Could you add a TODO about merging this with the URLRequestInterceptorHTTPTest
fixture?  I'm not a big fan of URLRequestTestJob, so would like to remove the
dependency on it at some point (And maybe move these tests into the factory
tests - they're fine as-is for the moment, though).

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1765: #endif
Know you're just copying here, but is this SetProtocolHandler call needed?  I
don't see any tests ifdef-ed out, so I don't think it's used?

Or better, can we just inherit from URLRequestTest, and add a method:

virtual scoped_ptr<URLRequestFactory> SetUpFactory(scoped_ptr<URLRequestFactory
base_factory)?

And have the other tests inherit from URLRequestHTTP test.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1772: ~URLRequestInterceptorTest()
override {
nit:  Blank line before destructor definition.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1777: protected:
These should all be private.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1786: 
nit:  Remove blank line.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1788: // intercept the main request and
respond with a simple response
These should all be sentences (Start with capital, end with period).

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1844: }
nit:  Don't use braces with one line ifs.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1932: }
nit:  Remove braces.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1972: // setup to intercept final
response and override it with an OK response
nit:  Set up

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2042: 
nit:  Remove extra blank line.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2047:
interceptor.main_request_load_timing_info_ = job_load_timing;
How do these tests work?  You aren't injecting the interceptor into the context.

Since the context is already initialized, it seems a bad idea to do that here,
actually.  I think it's simplest just to make this a method of
URLRequestInterceptorTest and use its interceptor_.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2119: TEST_F(URLRequestTest,
InterceptLoadTiming) {
We don't need two sets of load timing tests - these are just using the
interceptors to test load timing behavior, as opposed to testing the load timing
behavior of the interceptors.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:7142: protected:
nit:  Protected not needed.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:7262: EXPECT_TRUE(r->status().error() ==
ERR_RESPONSE_HEADERS_TOO_BIG);
EXPECT_EQ(ERR_..., ...);

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:7272: LocalHttpTestServer test_server_;
Per Google style guide, class variables should all be private.  Can just add an
accessor.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:7369: 
nit:  Remove extra line break

[sgurun-gerrit only, 2014-11-05 19:06:52]

On 2014/11/05 18:17:51, bengr wrote:

aw lgtm

[mmenke, 2014-11-05 20:52:49]

Think this is looking really good.

https://codereview.chromium.org/686343002/diff/220001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/686343002/diff/220001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:7150: void HTTPRedirectMethodTest(const
GURL& redirect_url,
Per other comment, I'd like to see this test fixture inherit from
URLRequestTestHTTP and reuse these methods.

https://codereview.chromium.org/686343002/diff/220001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:7276:
NetworkDelegateNotificationOnRedirectIntercept) {
Should probably intercept the request and replace it with a real HTTP request,
so we count both the old and the new network_delegate calls.  Same goes for the
next one as well.

[bengr, 2014-11-05 22:51:39]

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
File net/url_request/url_request_test_util.h (right):

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_test_util.h:376: bool can_be_intercepted_on_error_;
On 2014/11/05 18:43:43, mmenke wrote:
> Maybe rename to will_be_intercepted_on_next_error_, and set 
> next_states_[req_id] to kStageResponseStarted when it's set in
> OnHeadersReceived, and then clear it?

I'm not sure what you mean. I set next_states_[req_id] to kStageResponseStarted
in OnSendHeaders. Should I be setting next_states_ elsewhere as well? I can
clear the will_be_intercepted_on_next_error_ flag in OnSendHeaders. I've done
that.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:640: 
On 2014/11/05 18:43:44, mmenke wrote:
> nit:  Remove extra line break.  Looks like you tried to be systematic in where
> you added them, but doesn't seem to get us much, and the prevailing style is
not
> to use them.

Done.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1580: // An Interceptor for use with
interceptor tests
On 2014/11/05 18:43:44, mmenke wrote:
> nit:  +.

Done.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1599: 
On 2014/11/05 18:43:44, mmenke wrote:
> nit:  // URLRequestInterceptor implementation:

Done.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1600: virtual URLRequestJob*
MaybeInterceptRequest(
On 2014/11/05 18:43:45, mmenke wrote:
> nit:  -virtual

Done.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1621: // will error since the requeted
url is not one of its canned urls
On 2014/11/05 18:43:44, mmenke wrote:
> nit:  Comments should all be complete sentences (Capitalize + period).

Done.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1686: bool intercept_main_request_;
On 2014/11/05 18:43:44, mmenke wrote:
> Per google style guide, these should all be private.

Done.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1691: // Other actions we take at
MaybeIntercept time
On 2014/11/05 18:43:44, mmenke wrote:
> Comments should all be capitalized, end with periods.

Done.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1754: class URLRequestInterceptorTest :
public PlatformTest {
On 2014/11/05 18:43:44, mmenke wrote:
> Could you add a TODO about merging this with the URLRequestInterceptorHTTPTest
> fixture?  I'm not a big fan of URLRequestTestJob, so would like to remove the
> dependency on it at some point (And maybe move these tests into the factory
> tests - they're fine as-is for the moment, though).

Done.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1765: #endif
On 2014/11/05 18:43:44, mmenke wrote:
> Know you're just copying here, but is this SetProtocolHandler call needed?  I
> don't see any tests ifdef-ed out, so I don't think it's used?
> 
> Or better, can we just inherit from URLRequestTest, and add a method:
> 
> virtual scoped_ptr<URLRequestFactory>
SetUpFactory(scoped_ptr<URLRequestFactory
> base_factory)?
> 
> And have the other tests inherit from URLRequestHTTP test.

Done.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1772: ~URLRequestInterceptorTest()
override {
On 2014/11/05 18:43:44, mmenke wrote:
> nit:  Blank line before destructor definition.

Done.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1777: protected:
On 2014/11/05 18:43:44, mmenke wrote:
> These should all be private.

Done.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1786: 
On 2014/11/05 18:43:44, mmenke wrote:
> nit:  Remove blank line.

Done.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1788: // intercept the main request and
respond with a simple response
On 2014/11/05 18:43:44, mmenke wrote:
> These should all be sentences (Start with capital, end with period).

Done.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1844: }
On 2014/11/05 18:43:44, mmenke wrote:
> nit:  Don't use braces with one line ifs.

Done.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1932: }
On 2014/11/05 18:43:44, mmenke wrote:
> nit:  Remove braces.

Done.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1972: // setup to intercept final
response and override it with an OK response
On 2014/11/05 18:43:44, mmenke wrote:
> nit:  Set up

Done.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2042: 
On 2014/11/05 18:43:45, mmenke wrote:
> nit:  Remove extra blank line.

Done.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2047:
interceptor.main_request_load_timing_info_ = job_load_timing;
On 2014/11/05 18:43:44, mmenke wrote:
> How do these tests work?  You aren't injecting the interceptor into the
context.
> 
> Since the context is already initialized, it seems a bad idea to do that here,
> actually.  I think it's simplest just to make this a method of
> URLRequestInterceptorTest and use its interceptor_.

Acknowledged.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2119: TEST_F(URLRequestTest,
InterceptLoadTiming) {
On 2014/11/05 18:43:44, mmenke wrote:
> We don't need two sets of load timing tests - these are just using the
> interceptors to test load timing behavior, as opposed to testing the load
timing
> behavior of the interceptors.

Done.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:7142: protected:
On 2014/11/05 18:43:44, mmenke wrote:
> nit:  Protected not needed.

Done.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:7262: EXPECT_TRUE(r->status().error() ==
ERR_RESPONSE_HEADERS_TOO_BIG);
On 2014/11/05 18:43:44, mmenke wrote:
> EXPECT_EQ(ERR_..., ...);

Done.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:7272: LocalHttpTestServer test_server_;
On 2014/11/05 18:43:44, mmenke wrote:
> Per Google style guide, class variables should all be private.  Can just add
an
> accessor.

Done.

https://codereview.chromium.org/686343002/diff/180001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:7369: 
On 2014/11/05 18:43:44, mmenke wrote:
> nit:  Remove extra line break

Done.

[mmenke, 2014-11-05 23:14:34]

Just a quick followup to your question about what I meant by my
will_be_intercepted_on_next_error_ suggestion.

I'll do a full pass tomorrow.

https://codereview.chromium.org/686343002/diff/240001/net/url_request/url_req...
File net/url_request/url_request_test_util.cc (right):

https://codereview.chromium.org/686343002/diff/240001/net/url_request/url_req...
net/url_request/url_request_test_util.cc:415:
(will_be_intercepted_on_next_error_ ? kStageResponseStarted : 0);
Replace this with:

if (!will_be_intercepted_on_next_error_) {
  next_states_[req_id] = kStageHeadersReceived | kStageCompletedError;
} else {
  next_states_[req_id] = kStageResponseStarted;
}

That makes the flag more definitive about what will happen next.

[bengr, 2014-11-06 00:10:51]

https://codereview.chromium.org/686343002/diff/240001/net/url_request/url_req...
File net/url_request/url_request_test_util.cc (right):

https://codereview.chromium.org/686343002/diff/240001/net/url_request/url_req...
net/url_request/url_request_test_util.cc:415:
(will_be_intercepted_on_next_error_ ? kStageResponseStarted : 0);
On 2014/11/05 23:14:34, mmenke wrote:
> Replace this with:
> 
> if (!will_be_intercepted_on_next_error_) {
>   next_states_[req_id] = kStageHeadersReceived | kStageCompletedError;
> } else {
>   next_states_[req_id] = kStageResponseStarted;
> }
> 
> That makes the flag more definitive about what will happen next.

Done.

[mtomasz, 2014-11-06 00:20:10]

https://codereview.chromium.org/686343002/diff/260001/chrome/browser/chromeos...
File chrome/browser/chromeos/fileapi/external_file_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/686343002/diff/260001/chrome/browser/chromeos...
chrome/browser/chromeos/fileapi/external_file_url_request_job_unittest.cc:70:
return NULL;
nit: NULL -> nullptr per http://chromium-cpp.appspot.com/?

[mtomasz, 2014-11-06 00:21:26]

On 2014/11/06 00:20:10, mtomasz wrote:
>
https://codereview.chromium.org/686343002/diff/260001/chrome/browser/chromeos...
> File chrome/browser/chromeos/fileapi/external_file_url_request_job_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/686343002/diff/260001/chrome/browser/chromeos...
> chrome/browser/chromeos/fileapi/external_file_url_request_job_unittest.cc:70:
> return NULL;
> nit: NULL -> nullptr per http://chromium-cpp.appspot.com/

lgtm with a nit.

[bengr, 2014-11-06 17:14:49]

https://codereview.chromium.org/686343002/diff/260001/chrome/browser/chromeos...
File chrome/browser/chromeos/fileapi/external_file_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/686343002/diff/260001/chrome/browser/chromeos...
chrome/browser/chromeos/fileapi/external_file_url_request_job_unittest.cc:70:
return NULL;
On 2014/11/06 00:20:10, mtomasz wrote:
> nit: NULL -> nullptr per http://chromium-cpp.appspot.com/

Done.

[mmenke, 2014-11-06 18:49:37]

LGTM

https://codereview.chromium.org/686343002/diff/320001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/686343002/diff/320001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:7071: 
nit:  Remove blank line.

https://codereview.chromium.org/686343002/diff/320001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:7094: }
nit:  Don't use braces when both parts of an if take up one line.

https://codereview.chromium.org/686343002/diff/320001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:7126: EXPECT_EQ(200,
req->response_headers()->response_code());
Should probably check if the response was a success before dereferencing
response_headers().

https://codereview.chromium.org/686343002/diff/320001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:7158: EXPECT_EQ(200,
req->response_headers()->response_code());
Should probably check if the response was a success before dereferencing
response_headers().

[michaeln, 2014-11-06 21:07:44]

michaeln@chromium.org changed reviewers:
+ michaeln@chromium.org

[michaeln, 2014-11-06 21:07:46]

https://codereview.chromium.org/686343002/diff/320001/android_webview/browser...
File android_webview/browser/net/aw_url_request_job_factory.h (right):

https://codereview.chromium.org/686343002/diff/320001/android_webview/browser...
android_webview/browser/net/aw_url_request_job_factory.h:37: virtual
net::URLRequestJob* MaybeInterceptRedirect(
per new c++ 11 guidelines, just annotate with 'override' (generally use only one
of [virtual, override, final] )

https://codereview.chromium.org/686343002/diff/320001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/686343002/diff/320001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_unittest.cc:498: const GURL&
location) const override;
maybe inline these one-liners to return nullptr

https://codereview.chromium.org/686343002/diff/320001/net/url_request/url_req...
File net/url_request/url_request_job_manager.cc (right):

https://codereview.chromium.org/686343002/diff/320001/net/url_request/url_req...
net/url_request/url_request_job_manager.cc:158: request, network_delegate);
Are you intentionally reversing the order the old vs new interceptors classes
are invoked?

From an earlier comment...
> The old URLRequest::Interceptor's MaybeIntercept method gets called prior to
the
> corresponding method of the new URLReqeustInterceptor.

As coded the newly added MaybeInterceptResponse MaybeInterceptRedirect are doing
things in the opposite order where the new gets called prior to the old.

Unless you are doing that knowingly and for a good reason, I think maintaining
the same relative order for all of the MaybeIntercept* methods would make more
sense.

[michaeln, 2014-11-06 22:11:21]

lgtm with the ordering changed (just chatted with ben about that)

[bengr, 2014-11-06 23:57:33]

Patchset #12 (id:350001) has been deleted

[bengr, 2014-11-06 23:57:48]

Patchset #11 (id:340001) has been deleted

[bengr, 2014-11-06 23:58:02]

https://codereview.chromium.org/686343002/diff/320001/android_webview/browser...
File android_webview/browser/net/aw_url_request_job_factory.h (right):

https://codereview.chromium.org/686343002/diff/320001/android_webview/browser...
android_webview/browser/net/aw_url_request_job_factory.h:37: virtual
net::URLRequestJob* MaybeInterceptRedirect(
On 2014/11/06 21:07:46, michaeln wrote:
> per new c++ 11 guidelines, just annotate with 'override' (generally use only
one
> of [virtual, override, final] )

Done.

https://codereview.chromium.org/686343002/diff/320001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/686343002/diff/320001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_unittest.cc:498: const GURL&
location) const override;
On 2014/11/06 21:07:46, michaeln wrote:
> maybe inline these one-liners to return nullptr

Acknowledged.

https://codereview.chromium.org/686343002/diff/320001/net/url_request/url_req...
File net/url_request/url_request_job_manager.cc (right):

https://codereview.chromium.org/686343002/diff/320001/net/url_request/url_req...
net/url_request/url_request_job_manager.cc:158: request, network_delegate);
On 2014/11/06 21:07:46, michaeln wrote:
> Are you intentionally reversing the order the old vs new interceptors classes
> are invoked?
> 
> From an earlier comment...
> > The old URLRequest::Interceptor's MaybeIntercept method gets called prior to
> the
> > corresponding method of the new URLReqeustInterceptor.
> 
> As coded the newly added MaybeInterceptResponse MaybeInterceptRedirect are
doing
> things in the opposite order where the new gets called prior to the old.
> 
> Unless you are doing that knowingly and for a good reason, I think maintaining
> the same relative order for all of the MaybeIntercept* methods would make more
> sense.
> 

Done.

https://codereview.chromium.org/686343002/diff/320001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/686343002/diff/320001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:7071: 
On 2014/11/06 18:49:37, mmenke wrote:
> nit:  Remove blank line.

Done.

https://codereview.chromium.org/686343002/diff/320001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:7094: }
On 2014/11/06 18:49:37, mmenke wrote:
> nit:  Don't use braces when both parts of an if take up one line.

Done.

https://codereview.chromium.org/686343002/diff/320001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:7126: EXPECT_EQ(200,
req->response_headers()->response_code());
On 2014/11/06 18:49:37, mmenke wrote:
> Should probably check if the response was a success before dereferencing
> response_headers().

Done.

https://codereview.chromium.org/686343002/diff/320001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:7158: EXPECT_EQ(200,
req->response_headers()->response_code());
On 2014/11/06 18:49:37, mmenke wrote:
> Should probably check if the response was a success before dereferencing
> response_headers().

Done.

[bengr, 2014-11-07 00:17:51]

The CQ bit was checked by bengr@chromium.org

[commit-bot: I haz the power, 2014-11-07 00:19:23]

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

[commit-bot: I haz the power, 2014-11-07 01:37:10]

Committed patchset #11 (id:370001)

[commit-bot: I haz the power, 2014-11-07 01:37:48]

Patchset 11 (id:??) landed as
https://crrev.com/1bf8e9456a103d086f53400bdc169c437a1590a4
Cr-Commit-Position: refs/heads/master@{#303154}