[2/3 chromium] Support redirect option of Request and "opaqueredirect" response type.

[2/3 chromium] Support redirect option of Request and "opaqueredirect" response type.

https://crbug.com/510650#c4 describes the details of the data flow.

1/3 blink: https://codereview.chromium.org/1265133002/
2/3 chromium: This
3/3 blink: https://codereview.chromium.org/1280733002/

BUG=510650, 517837
Committed: https://crrev.com/bdfdedcd90d11424dafe65e54b034126ea34255a
Cr-Commit-Position: refs/heads/master@{#343596}

[horo, 2015-08-06 16:49:01]

Patchset #1 (id:1) has been deleted

[horo, 2015-08-06 16:49:23]

Patchset #1 (id:20001) has been deleted

[horo, 2015-08-06 16:49:28]

Patchset #1 (id:40001) has been deleted

[horo, 2015-08-06 16:49:48]

Patchset #1 (id:60001) has been deleted

[horo, 2015-08-06 16:49:55]

Patchset #1 (id:80001) has been deleted

[horo, 2015-08-06 16:50:01]

Patchset #1 (id:100001) has been deleted

[horo, 2015-08-06 17:00:34]

Patchset #1 (id:120001) has been deleted

[horo, 2015-08-06 17:00:40]

Patchset #1 (id:140001) has been deleted

[horo, 2015-08-06 17:01:52]

Patchset #2 (id:180001) has been deleted

[horo, 2015-08-06 17:31:19]

horo@chromium.org changed reviewers:
+ falken@chromium.org, yhirano@chromium.org

[horo, 2015-08-06 17:31:20]

yhirano@, falken@
Could you please review this patch?

[falken, 2015-08-07 07:43:46]

lgtm

https://codereview.chromium.org/1271733002/diff/200001/content/browser/loader...
File content/browser/loader/resource_request_info_impl.cc (right):

https://codereview.chromium.org/1271733002/diff/200001/content/browser/loader...
content/browser/loader/resource_request_info_impl.cc:59:
FETCH_REDIRECT_MODE_FOLLOW,
i'd follow the // fetch_redirect_mode convention here too

[horo, 2015-08-07 09:42:22]

Thank you!

https://codereview.chromium.org/1271733002/diff/200001/content/browser/loader...
File content/browser/loader/resource_request_info_impl.cc (right):

https://codereview.chromium.org/1271733002/diff/200001/content/browser/loader...
content/browser/loader/resource_request_info_impl.cc:59:
FETCH_REDIRECT_MODE_FOLLOW,
On 2015/08/07 07:43:46, falken wrote:
> i'd follow the // fetch_redirect_mode convention here too

Done.

[yhirano, 2015-08-10 04:57:52]

https://codereview.chromium.org/1271733002/diff/240001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1271733002/diff/240001/content/browser/loader...
content/browser/loader/resource_loader.cc:288: ResponseCompleted();
You need to read the response body.

https://codereview.chromium.org/1271733002/diff/240001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/1271733002/diff/240001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:135: return
static_cast<blink::WebURLRequest::FetchRedirectMode>(redirect_mode);
If you rely on the fact that these two enums share same integral values, please
add comments in both definitions.

[horo, 2015-08-10 06:45:18]

https://codereview.chromium.org/1271733002/diff/240001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1271733002/diff/240001/content/browser/loader...
content/browser/loader/resource_loader.cc:288: ResponseCompleted();
On 2015/08/10 04:57:52, yhirano wrote:
> You need to read the response body.

We don't need to read the response body because there is no way to read the body
of opaque-redirect filtered response's internal response.

Added comment.

https://codereview.chromium.org/1271733002/diff/240001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/1271733002/diff/240001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:135: return
static_cast<blink::WebURLRequest::FetchRedirectMode>(redirect_mode);
On 2015/08/10 04:57:52, yhirano wrote:
> If you rely on the fact that these two enums share same integral values,
please
> add comments in both definitions.

We have STATIC_ASSERT_MATCHING_ENUMS() in web_url_loader_impl.cc.
I think it is enough.

[yhirano, 2015-08-10 07:03:07]

https://codereview.chromium.org/1271733002/diff/240001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1271733002/diff/240001/content/browser/loader...
content/browser/loader/resource_loader.cc:288: ResponseCompleted();
On 2015/08/10 06:45:18, horo wrote:
> On 2015/08/10 04:57:52, yhirano wrote:
> > You need to read the response body.
> 
> We don't need to read the response body because there is no way to read the
body
> of opaque-redirect filtered response's internal response.
> 
> Added comment.

A fetch API user cannot read the internal response body, but other APIs may read
it.

 "The fetch algorithm returns such a view to ensure APIs do not accidentally
leak information. If the information is required, e.g., to feed image data to a
decoder, the associated internal response can be used, which is only
"accessible" to internal specification algorithms."

https://codereview.chromium.org/1271733002/diff/240001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/1271733002/diff/240001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:135: return
static_cast<blink::WebURLRequest::FetchRedirectMode>(redirect_mode);
On 2015/08/10 06:45:18, horo wrote:
> On 2015/08/10 04:57:52, yhirano wrote:
> > If you rely on the fact that these two enums share same integral values,
> please
> > add comments in both definitions.
> 
> We have STATIC_ASSERT_MATCHING_ENUMS() in web_url_loader_impl.cc.
> I think it is enough.

Acknowledged.

[horo, 2015-08-10 07:19:45]

https://codereview.chromium.org/1271733002/diff/240001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1271733002/diff/240001/content/browser/loader...
content/browser/loader/resource_loader.cc:288: ResponseCompleted();
On 2015/08/10 07:03:07, yhirano wrote:
> On 2015/08/10 06:45:18, horo wrote:
> > On 2015/08/10 04:57:52, yhirano wrote:
> > > You need to read the response body.
> > 
> > We don't need to read the response body because there is no way to read the
> body
> > of opaque-redirect filtered response's internal response.
> > 
> > Added comment.
> 
> A fetch API user cannot read the internal response body, but other APIs may
read
> it.
> 
>  "The fetch algorithm returns such a view to ensure APIs do not accidentally
> leak information. If the information is required, e.g., to feed image data to
a
> decoder, the associated internal response can be used, which is only
> "accessible" to internal specification algorithms."


Which API can read the body?

Opaque-redirect filtered responses can be usable only when calling
FetchEvent.respondWith() for navigation requests.
I think navigation requests don't care the body of redirect responses.

[yhirano, 2015-08-10 07:41:20]

https://codereview.chromium.org/1271733002/diff/240001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1271733002/diff/240001/content/browser/loader...
content/browser/loader/resource_loader.cc:288: ResponseCompleted();
On 2015/08/10 07:19:45, horo wrote:
> On 2015/08/10 07:03:07, yhirano wrote:
> > On 2015/08/10 06:45:18, horo wrote:
> > > On 2015/08/10 04:57:52, yhirano wrote:
> > > > You need to read the response body.
> > > 
> > > We don't need to read the response body because there is no way to read
the
> > body
> > > of opaque-redirect filtered response's internal response.
> > > 
> > > Added comment.
> > 
> > A fetch API user cannot read the internal response body, but other APIs may
> read
> > it.
> > 
> >  "The fetch algorithm returns such a view to ensure APIs do not accidentally
> > leak information. If the information is required, e.g., to feed image data
to
> a
> > decoder, the associated internal response can be used, which is only
> > "accessible" to internal specification algorithms."
> 
> 
> Which API can read the body?
> 
> Opaque-redirect filtered responses can be usable only when calling
> FetchEvent.respondWith() for navigation requests.
> I think navigation requests don't care the body of redirect responses.

I don't know a specific example, but the fetch spec says ANY API can use the
internal response. Even if it is not used by anyone at this moment, someone may
use it in the future.

[horo, 2015-08-10 08:11:59]

https://codereview.chromium.org/1271733002/diff/240001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1271733002/diff/240001/content/browser/loader...
content/browser/loader/resource_loader.cc:288: ResponseCompleted();
On 2015/08/10 07:41:20, yhirano wrote:
> On 2015/08/10 07:19:45, horo wrote:
> > On 2015/08/10 07:03:07, yhirano wrote:
> > > On 2015/08/10 06:45:18, horo wrote:
> > > > On 2015/08/10 04:57:52, yhirano wrote:
> > > > > You need to read the response body.
> > > > 
> > > > We don't need to read the response body because there is no way to read
> the
> > > body
> > > > of opaque-redirect filtered response's internal response.
> > > > 
> > > > Added comment.
> > > 
> > > A fetch API user cannot read the internal response body, but other APIs
may
> > read
> > > it.
> > > 
> > >  "The fetch algorithm returns such a view to ensure APIs do not
accidentally
> > > leak information. If the information is required, e.g., to feed image data
> to
> > a
> > > decoder, the associated internal response can be used, which is only
> > > "accessible" to internal specification algorithms."
> > 
> > 
> > Which API can read the body?
> > 
> > Opaque-redirect filtered responses can be usable only when calling
> > FetchEvent.respondWith() for navigation requests.
> > I think navigation requests don't care the body of redirect responses.
> 
> I don't know a specific example, but the fetch spec says ANY API can use the
> internal response. Even if it is not used by anyone at this moment, someone
may
> use it in the future.

I see. I added TODO.

[yhirano, 2015-08-10 08:27:26]

https://codereview.chromium.org/1271733002/diff/280001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1271733002/diff/280001/content/browser/loader...
content/browser/loader/resource_loader.cc:287: CompleteResponseStarted();
You might want to notify the upload progress. (or TODO?)

https://codereview.chromium.org/1271733002/diff/280001/content/browser/loader...
content/browser/loader/resource_loader.cc:292: ResponseCompleted();
What happens when deferred?

[horo, 2015-08-10 08:57:10]

https://codereview.chromium.org/1271733002/diff/280001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1271733002/diff/280001/content/browser/loader...
content/browser/loader/resource_loader.cc:287: CompleteResponseStarted();
On 2015/08/10 08:27:25, yhirano wrote:
> You might want to notify the upload progress. (or TODO?)

Done.
Added comment.

https://codereview.chromium.org/1271733002/diff/280001/content/browser/loader...
content/browser/loader/resource_loader.cc:292: ResponseCompleted();
On 2015/08/10 08:27:26, yhirano wrote:
> What happens when deferred?

Who can set |defer|?
I think ResourceLoader::OnReceivedRedirect() is called directly from
URLRequest::NotifyReceivedRedirect() and which is called from
URLRequestJob::NotifyHeadersComplete().
So no one can set |defer|.

Is my understanding incorrect?

[yhirano, 2015-08-10 09:02:51]

https://codereview.chromium.org/1271733002/diff/280001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1271733002/diff/280001/content/browser/loader...
content/browser/loader/resource_loader.cc:292: ResponseCompleted();
On 2015/08/10 08:57:10, horo wrote:
> On 2015/08/10 08:27:26, yhirano wrote:
> > What happens when deferred?
> 
> Who can set |defer|?
> I think ResourceLoader::OnReceivedRedirect() is called directly from
> URLRequest::NotifyReceivedRedirect() and which is called from
> URLRequestJob::NotifyHeadersComplete().
> So no one can set |defer|.
> 
> Is my understanding incorrect?

|deferred_stage_| may be set in |CompleteResponseStarted|. I'm not sure what we
should do then.

[horo, 2015-08-10 09:42:29]

https://codereview.chromium.org/1271733002/diff/280001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1271733002/diff/280001/content/browser/loader...
content/browser/loader/resource_loader.cc:292: ResponseCompleted();
On 2015/08/10 09:02:51, yhirano wrote:
> On 2015/08/10 08:57:10, horo wrote:
> > On 2015/08/10 08:27:26, yhirano wrote:
> > > What happens when deferred?
> > 
> > Who can set |defer|?
> > I think ResourceLoader::OnReceivedRedirect() is called directly from
> > URLRequest::NotifyReceivedRedirect() and which is called from
> > URLRequestJob::NotifyHeadersComplete().
> > So no one can set |defer|.
> > 
> > Is my understanding incorrect?
> 
> |deferred_stage_| may be set in |CompleteResponseStarted|. I'm not sure what
we
> should do then.

Ah, I see.
I think we should change deferred_stage_ to DEFERRED_RESPONSE_COMPLETE here not
to read the body.

[yhirano, 2015-08-10 09:46:09]

lgtm

[horo, 2015-08-10 10:20:58]

horo@chromium.org changed reviewers:
+ inferno@chromium.org, jkarlin@chromium.org, kinuko@chromium.org,
mmenke@chromium.org

[horo, 2015-08-10 10:20:59]

jkarlin@
Could you please review content/browser/cache_storage/*?

inferno@
Could you please review content/common/resource_messages.h and
content/common/service_worker/service_worker_messages.h?

mmenke@
Could you please review content/browser/loader/*?

kinuko@
Could you please review content/child/*?

[jkarlin, 2015-08-10 10:48:59]

cache_storage/ lgtm

[kinuko, 2015-08-10 13:28:12]

content/child/* lgtm

[mmenke, 2015-08-10 14:12:03]

Redirects can have bodies, which we normally ignore - we don't even store them
in the cache.  For "opaqueredirect", should we be serving up redirect bodies?

[horo, 2015-08-10 14:26:19]

On 2015/08/10 14:12:03, mmenke wrote:
> Redirects can have bodies, which we normally ignore - we don't even store them
> in the cache.  For "opaqueredirect", should we be serving up redirect bodies?

"opaqueredirect" responses can be used only when Service Worker responds to
navigation requests (= page loading and worker script loading).
And navigation requests ignore the bodies of redirect responses.
So we can ignore the body for "opaqueredirect" responses.

[mmenke, 2015-08-10 15:52:03]

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

https://codereview.chromium.org/1271733002/diff/320001/content/browser/loader...
content/browser/loader/resource_loader.cc:283: if (info->fetch_redirect_mode()
== FETCH_REDIRECT_MODE_ERROR) {
We're going to need unit tests for both these paths.  I'd suggest RDH unittests,
since those are more integrationy, and cover how the ResourceLoader, RDH, and
AsyncResourceHandler all interact.

https://codereview.chromium.org/1271733002/diff/320001/content/browser/loader...
content/browser/loader/resource_loader.cc:288: // have to call
ReportUploadProgress().
This should actually probably be done unconditionally in this method - we don't
report multiple upload attempts as requests are redirected, so this method
should probably have the same behavior as OnResponseStarted.

https://codereview.chromium.org/1271733002/diff/320001/content/browser/loader...
content/browser/loader/resource_loader.cc:288: // have to call
ReportUploadProgress().
nit:  Move "will" should before "have".

https://codereview.chromium.org/1271733002/diff/320001/content/browser/loader...
content/browser/loader/resource_loader.cc:289: CompleteResponseStarted();
Suggest a blank line after CompleteResponseStarted().

https://codereview.chromium.org/1271733002/diff/320001/content/browser/loader...
content/browser/loader/resource_loader.cc:291: // the body of opaque-redirect
filtered response's internal response.
nit:  Don't use "we" in comments.

https://codereview.chromium.org/1271733002/diff/320001/content/browser/loader...
content/browser/loader/resource_loader.cc:292: // TODO(horo): When we will
support any API which expose the internal body,
nit:  Move "will" should before "have".

https://codereview.chromium.org/1271733002/diff/320001/content/browser/loader...
content/browser/loader/resource_loader.cc:292: // TODO(horo): When we will
support any API which expose the internal body,
Is this "when" or "if"?

https://codereview.chromium.org/1271733002/diff/320001/content/browser/loader...
content/browser/loader/resource_loader.cc:293: // we have to read the body
before calling ResponseCompleted().
Add warning that Cache changes will be needed before this is possible, or such
requests will have to bypass the cache.

https://codereview.chromium.org/1271733002/diff/320001/content/browser/loader...
content/browser/loader/resource_loader.cc:294: if (is_deferred()) {
I'm not following how this is possible.

https://codereview.chromium.org/1271733002/diff/320001/content/browser/loader...
content/browser/loader/resource_loader.cc:296: // we reset |deferred_stage_| to
call ResponseCompleted when resumed.
nit:  Don't use "we" in comments.

https://codereview.chromium.org/1271733002/diff/320001/content/browser/loader...
content/browser/loader/resource_loader.cc:297: DCHECK(DEFERRED_READ ==
deferred_stage_);
I'm really not following how this is possible.  We only defer reads once we
actually want to read the response body.  We only want to do that after we've
received the final set of headers, after following all redirects.  At this
point, we still have a redirect to follow.

https://codereview.chromium.org/1271733002/diff/320001/content/browser/loader...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/1271733002/diff/320001/content/browser/loader...
content/browser/loader/resource_request_info_impl.h:69: bool is_async);
I love this class's constructor.  :(

[horo, 2015-08-11 05:58:21]

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

https://codereview.chromium.org/1271733002/diff/320001/content/browser/loader...
content/browser/loader/resource_loader.cc:283: if (info->fetch_redirect_mode()
== FETCH_REDIRECT_MODE_ERROR) {
On 2015/08/10 15:52:02, mmenke wrote:
> We're going to need unit tests for both these paths.  I'd suggest RDH
unittests,
> since those are more integrationy, and cover how the ResourceLoader, RDH, and
> AsyncResourceHandler all interact.

Added ResourceDispatcherHostTest.FetchRedirectModeError and
ResourceDispatcherHostTest.FetchRedirectModeManual in
resource_dispatcher_host_unittest.cc

https://codereview.chromium.org/1271733002/diff/320001/content/browser/loader...
content/browser/loader/resource_loader.cc:288: // have to call
ReportUploadProgress().
On 2015/08/10 15:52:02, mmenke wrote:
> This should actually probably be done unconditionally in this method - we
don't
> report multiple upload attempts as requests are redirected, so this method
> should probably have the same behavior as OnResponseStarted.

FETCH_REDIRECT_MODE_MANUAL is supported only by Fetch API.
Fetch API doesn't set is_upload_progress_enabled flag.
So We don't need to call ReportUploadProgress().

Added "DCHECK(!GetRequestInfo()->is_upload_progress_enabled());"

https://codereview.chromium.org/1271733002/diff/320001/content/browser/loader...
content/browser/loader/resource_loader.cc:288: // have to call
ReportUploadProgress().
On 2015/08/10 15:52:02, mmenke wrote:
> nit:  Move "will" should before "have".

Done.

https://codereview.chromium.org/1271733002/diff/320001/content/browser/loader...
content/browser/loader/resource_loader.cc:289: CompleteResponseStarted();
On 2015/08/10 15:52:02, mmenke wrote:
> Suggest a blank line after CompleteResponseStarted().

Done.

https://codereview.chromium.org/1271733002/diff/320001/content/browser/loader...
content/browser/loader/resource_loader.cc:291: // the body of opaque-redirect
filtered response's internal response.
On 2015/08/10 15:52:02, mmenke wrote:
> nit:  Don't use "we" in comments.

Done.

https://codereview.chromium.org/1271733002/diff/320001/content/browser/loader...
content/browser/loader/resource_loader.cc:292: // TODO(horo): When we will
support any API which expose the internal body,
On 2015/08/10 15:52:02, mmenke wrote:
> Is this "when" or "if"?

Done.

https://codereview.chromium.org/1271733002/diff/320001/content/browser/loader...
content/browser/loader/resource_loader.cc:292: // TODO(horo): When we will
support any API which expose the internal body,
On 2015/08/10 15:52:02, mmenke wrote:
> nit:  Move "will" should before "have".

Done.

https://codereview.chromium.org/1271733002/diff/320001/content/browser/loader...
content/browser/loader/resource_loader.cc:293: // we have to read the body
before calling ResponseCompleted().
On 2015/08/10 15:52:02, mmenke wrote:
> Add warning that Cache changes will be needed before this is possible, or such
> requests will have to bypass the cache.

Done.

https://codereview.chromium.org/1271733002/diff/320001/content/browser/loader...
content/browser/loader/resource_loader.cc:294: if (is_deferred()) {
On 2015/08/10 15:52:02, mmenke wrote:
> I'm not following how this is possible.

Removed and added "DCHECK(!is_deferred())"

https://codereview.chromium.org/1271733002/diff/320001/content/browser/loader...
content/browser/loader/resource_loader.cc:296: // we reset |deferred_stage_| to
call ResponseCompleted when resumed.
On 2015/08/10 15:52:02, mmenke wrote:
> nit:  Don't use "we" in comments.

Removed.

[mmenke, 2015-08-11 16:06:14]

https://codereview.chromium.org/1271733002/diff/340001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1271733002/diff/340001/content/browser/loader...
content/browser/loader/resource_loader.cc:287: // TODO(horo): If we will support
upload progress events for Fetch API, we
nit:  will -> "are going to"  (Or just remove "will").  "will" isn't generally
used with future conditionals.

https://codereview.chromium.org/1271733002/diff/340001/content/browser/loader...
content/browser/loader/resource_loader.cc:292: DCHECK(!is_deferred());
Oh...I completely misunderstood this deferred stuff.  It's for side effects from
CompleteResponseStarted, not the initial state when this method is invoked.

Sorry about that.  Your old code was correct, but makes me nervous.  I want to
do some digging, and find out if there's a better way to do it than through a
magic state transition.

https://codereview.chromium.org/1271733002/diff/340001/content/browser/loader...
content/browser/loader/resource_loader.cc:297: // TODO(horo): If we will support
any API which expose the internal body,
nit:  will -> "are going to"  (Or just remove "will")

[horo, 2015-08-11 16:43:55]

https://codereview.chromium.org/1271733002/diff/340001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1271733002/diff/340001/content/browser/loader...
content/browser/loader/resource_loader.cc:287: // TODO(horo): If we will support
upload progress events for Fetch API, we
On 2015/08/11 16:06:14, mmenke wrote:
> nit:  will -> "are going to"  (Or just remove "will").  "will" isn't generally
> used with future conditionals.

Done.

https://codereview.chromium.org/1271733002/diff/340001/content/browser/loader...
content/browser/loader/resource_loader.cc:292: DCHECK(!is_deferred());
On 2015/08/11 16:06:14, mmenke wrote:
> Oh...I completely misunderstood this deferred stuff.  It's for side effects
from
> CompleteResponseStarted, not the initial state when this method is invoked.
> 
> Sorry about that.  Your old code was correct, but makes me nervous.  I want to
> do some digging, and find out if there's a better way to do it than through a
> magic state transition.

I added is_redirect_response argument to CompleteResponseStarted.
What do you think about it?

https://codereview.chromium.org/1271733002/diff/340001/content/browser/loader...
content/browser/loader/resource_loader.cc:297: // TODO(horo): If we will support
any API which expose the internal body,
On 2015/08/11 16:06:14, mmenke wrote:
> nit:  will -> "are going to"  (Or just remove "will")

Done.

[mmenke, 2015-08-11 18:48:56]

https://codereview.chromium.org/1271733002/diff/360001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1271733002/diff/360001/content/browser/loader...
content/browser/loader/resource_loader.cc:639: is_redirect_response ?
DEFERRED_RESPONSE_COMPLETE : DEFERRED_READ;
In this new path, we skip both OnWillRead and OnReadComplete(0), which
ResourceHandlers normally see for all successes.  I'm very concerned that this
new path will subtly break some ResourceHandler in a hard to debug manner.  And
I'm even more concerned that I'll end up being the one stuck with investigating
it, 2 months down the line when it suddenly turns up on stable as a mysterious
network crash.  Or 2 years down the line, when someone starts using
redirect_mode for something else entirely, using a different ResourceHandler
stack.

I think we could safely remove the 0-byte OnReadComplete(0) after the final read
everywhere, but then there's still the OnWillRead call we're skipping.

[mmenke, 2015-08-11 19:38:37]

Here's an idea:  Why does this even need to be implemented in the
ResourceLoader?  We already send the other process a notification of the
redirect, and wait for a response before following it.  Why can't it just cancel
the request when it learns of the redirect and take it from there?

That way, we touch a lot less code, and the logic will all live in
ServiceWorker.

[horo, 2015-08-12 05:35:30]

On 2015/08/11 19:38:37, mmenke wrote:
> Here's an idea:  Why does this even need to be implemented in the
> ResourceLoader?  We already send the other process a notification of the
> redirect, and wait for a response before following it.  Why can't it just
cancel
> the request when it learns of the redirect and take it from there?
> 
> That way, we touch a lot less code, and the logic will all live in
> ServiceWorker.

Ah, yes. That's a good idea.
I removed redirect mode related code from content::ResourceLoader (Patch Set
11).
I will add the logic for redirect mode in blink::DocumentThreadableLoader.

[horo, 2015-08-13 16:28:05]

mmenke@ inferno@
Ping?

[mmenke, 2015-08-13 16:42:26]

On 2015/08/13 16:28:05, horo wrote:
> mmenke@ inferno@
> Ping?

Oh...I thought you'd changed this to do everything in the renderer process,
since it already has all relevant information.  Why does this still need to go
through the browser process?

[horo, 2015-08-13 22:43:12]

On 2015/08/13 16:42:26, mmenke wrote:
> On 2015/08/13 16:28:05, horo wrote:
> > mmenke@ inferno@
> > Ping?
> 
> Oh...I thought you'd changed this to do everything in the renderer process,
> since it already has all relevant information.  Why does this still need to go
> through the browser process?

When Fetch API is called in the page, the redirect option must be passed from
the page to the service worker.

Page: fetch(new Request("test", {redirect: 'manual'}))
-> Service Worker: FetchEvent.request.redirect must be 'manual'.

[horo, 2015-08-13 23:03:21]

On 2015/08/13 22:43:12, horo wrote:
> On 2015/08/13 16:42:26, mmenke wrote:
> > On 2015/08/13 16:28:05, horo wrote:
> > > mmenke@ inferno@
> > > Ping?
> > 
> > Oh...I thought you'd changed this to do everything in the renderer process,
> > since it already has all relevant information.  Why does this still need to
go
> > through the browser process?
> 
> When Fetch API is called in the page, the redirect option must be passed from
> the page to the service worker.
> 
> Page: fetch(new Request("test", {redirect: 'manual'}))
> -> Service Worker: FetchEvent.request.redirect must be 'manual'.

Ah sorry, we don't need ResourceRequestInfoImp.fetch_redirect_mode().
Uploaded patch set 12.

[inferno, 2015-08-14 04:47:51]

inferno@chromium.org changed reviewers:
+ tsepez@chromium.org
- inferno@chromium.org

[mmenke, 2015-08-14 16:11:42]

On 2015/08/13 22:43:12, horo wrote:
> On 2015/08/13 16:42:26, mmenke wrote:
> > On 2015/08/13 16:28:05, horo wrote:
> > > mmenke@ inferno@
> > > Ping?
> > 
> > Oh...I thought you'd changed this to do everything in the renderer process,
> > since it already has all relevant information.  Why does this still need to
go
> > through the browser process?
> 
> When Fetch API is called in the page, the redirect option must be passed from
> the page to the service worker.
> 
> Page: fetch(new Request("test", {redirect: 'manual'}))
> -> Service Worker: FetchEvent.request.redirect must be 'manual'.

Ahh, I thought this was for an ServiceWorker-only API.  This LGTM, though I
admittedly don't know most of the files this touches.

[mmenke, 2015-08-14 16:12:05]

Forgot my really important nit!

https://codereview.chromium.org/1271733002/diff/400001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1271733002/diff/400001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1537: false,     //
should_replace_current_entry,
nit:  Remove comma

[horo, 2015-08-14 18:46:12]

Thank you.

https://codereview.chromium.org/1271733002/diff/400001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1271733002/diff/400001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1537: false,     //
should_replace_current_entry,
On 2015/08/14 16:12:05, mmenke wrote:
> nit:  Remove comma

Done.

[horo, 2015-08-14 18:48:21]

horo@chromium.org changed reviewers:
+ dcheng@chromium.org

[horo, 2015-08-14 18:48:22]

dcheng@
Could you please review content/common/resource_messages.h and
content/common/service_worker/service_worker_messages.h?

[dcheng, 2015-08-14 19:03:04]

https://codereview.chromium.org/1271733002/diff/420001/content/common/service...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/1271733002/diff/420001/content/common/service...
content/common/service_worker/service_worker_types.h:95: enum FetchRedirectMode
{
How about using enum class here?

[Tom Sepez, 2015-08-14 19:29:44]

Messages LGTM.

[horo, 2015-08-15 00:26:32]

https://codereview.chromium.org/1271733002/diff/420001/content/common/service...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/1271733002/diff/420001/content/common/service...
content/common/service_worker/service_worker_types.h:95: enum FetchRedirectMode
{
On 2015/08/14 19:03:04, dcheng wrote:
> How about using enum class here?

Sounds good.
Done.

[dcheng, 2015-08-15 05:23:58]

lgtm

[horo, 2015-08-15 13:28:54]

The CQ bit was checked by horo@chromium.org

[horo, 2015-08-15 13:28:55]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
jkarlin@chromium.org, kinuko@chromium.org, yhirano@chromium.org,
mmenke@chromium.org, tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1271733002/#ps440001
(title: "Use enum class")

[commit-bot: I haz the power, 2015-08-15 13:29:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1271733002/440001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1271733002/440001

[commit-bot: I haz the power, 2015-08-15 14:00:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-15 14:00:22]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[horo, 2015-08-15 15:53:13]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2015-08-15 15:53:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1271733002/440001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1271733002/440001

[commit-bot: I haz the power, 2015-08-15 16:30:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-15 16:30:35]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[horo, 2015-08-16 00:20:14]

The CQ bit was checked by horo@chromium.org

[horo, 2015-08-16 00:20:14]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
jkarlin@chromium.org, dcheng@chromium.org, kinuko@chromium.org,
mmenke@chromium.org, tsepez@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/1271733002/#ps460001
(title: "Add _MODE to avoid compile error at Windows.")

[commit-bot: I haz the power, 2015-08-16 00:20:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1271733002/460001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1271733002/460001

[commit-bot: I haz the power, 2015-08-16 01:49:20]

Committed patchset #15 (id:460001)

[commit-bot: I haz the power, 2015-08-16 01:49:56]

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

[nevertapsn1012, 2015-10-21 17:38:58]

nevertapsn1012@gmail.com changed reviewers:
+ nevertapsn1012@gmail.com

[nevertapsn1012, 2015-10-21 17:38:59]

lgtm