[1/3 blink] Support redirect option of Request and "opaqueredirect" response type.

[1/3 blink] Support redirect option of Request and "opaqueredirect" response type.

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

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

BUG=510650, 517837
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200291

[horo, 2015-08-06 16:47:46]

Patchset #1 (id:1) has been deleted

[horo, 2015-08-06 16:47:52]

Patchset #1 (id:20001) has been deleted

[horo, 2015-08-06 16:47:58]

Patchset #1 (id:40001) has been deleted

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

Patchset #1 (id:60001) has been deleted

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

Patchset #1 (id:80001) has been deleted

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

Patchset #1 (id:100001) has been deleted

[horo, 2015-08-06 17:29:05]

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

[horo, 2015-08-06 17:29:05]

yhirano@
Could you please review this?

[horo, 2015-08-06 17:30:38]

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

[horo, 2015-08-06 17:30:39]

falken@
Could you please review this?

[falken, 2015-08-07 05:32:58]

I'd like to understand the change first (the spec is difficult to read).

Previously, fetch(url) followed redirects. So respondWith(fetch(url)) on a
navigation to to url which redirects resulted in the the omnibox showing "url"
and the page context being the response from the redirected location. In the
case the location is a different origin, it'd be a network.

The new change adds RequestRedirect on Request. When RequestRedirect is
"manual", and you fetch(url) to a url that redirects, you get an opaqueredirect
response instead of following the redirect. When you respondWith() an
opaqueredirect, the browser follows redirects in the same way as
respondWith(Response.redirect(target)): the omnibox will be the redirected
location.

Furthermore, for navigations, RequestRedirect is manual.

Is that accurate?

[horo, 2015-08-07 07:01:24]

On 2015/08/07 05:32:58, falken wrote:
> I'd like to understand the change first (the spec is difficult to read).
> 
> Previously, fetch(url) followed redirects. So respondWith(fetch(url)) on a
> navigation to to url which redirects resulted in the the omnibox showing "url"
> and the page context being the response from the redirected location. In the
> case the location is a different origin, it'd be a network.
> 
> The new change adds RequestRedirect on Request. When RequestRedirect is
> "manual", and you fetch(url) to a url that redirects, you get an
opaqueredirect
> response instead of following the redirect. When you respondWith() an
> opaqueredirect, the browser follows redirects in the same way as
> respondWith(Response.redirect(target)): the omnibox will be the redirected
> location.
> 
> Furthermore, for navigations, RequestRedirect is manual.
> 
> Is that accurate?

Yes.
The redirect mode of navigation request is "follow" now and it will be "manual".
https://github.com/whatwg/fetch/issues/101#issuecomment-128269549

But these 3 CLs just introduce redirect mode.
I will create another CL to change the redirect mode of navigation request.

[falken, 2015-08-07 07:39:12]

lgtm

Let's make a Feature Dashboard and OWP Launch bug for this. I don't think it
needs intent to ship but it'd be good for tracking purposes and letting people
know the change is happening.

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/F...
File Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/F...
Source/modules/fetch/FetchManager.cpp:142: FetchResponseData* taintedResponse =
responseData;
Should this be initialized to nullptr? It looks like it necessarily is assigned
something in the if-else.

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/F...
File Source/modules/fetch/FetchResponseData.h (right):

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/F...
Source/modules/fetch/FetchResponseData.h:27: enum Type { BasicType,
this formatting looks wrong

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/R...
File Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/R...
Source/modules/fetch/Request.cpp:154: // FIXME: "21. If |init|'s cache member is
present, set |request|'s cache
these FIXME should now be TODO(horo) or TODO(yhirano) [it doesn't commit you do
fixing, just says you're knowledgable about this TODO]:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#TODO_Comments

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/R...
Source/modules/fetch/Request.cpp:160: // if (init.redirect == "follw") {
nit: follw. i'd prefer to just not land commented out code but it's no biggie as
it'll be fixed soon

[horo, 2015-08-07 09:19:33]

Thank you.

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/F...
File Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/F...
Source/modules/fetch/FetchManager.cpp:142: FetchResponseData* taintedResponse =
responseData;
On 2015/08/07 07:39:12, falken wrote:
> Should this be initialized to nullptr? It looks like it necessarily is
assigned
> something in the if-else.

Done.
I changed these code to fix the layout test failures
(http/tests/fetch/*/fetch-access-control-redirect*.html).

When the location header doesn't exist, we have to treat the response as a
normal response and execute tainting.

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/F...
File Source/modules/fetch/FetchResponseData.h (right):

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/F...
Source/modules/fetch/FetchResponseData.h:27: enum Type { BasicType,
On 2015/08/07 07:39:12, falken wrote:
> this formatting looks wrong

Done.

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/R...
File Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/R...
Source/modules/fetch/Request.cpp:154: // FIXME: "21. If |init|'s cache member is
present, set |request|'s cache
On 2015/08/07 07:39:12, falken wrote:
> these FIXME should now be TODO(horo) or TODO(yhirano) [it doesn't commit you
do
> fixing, just says you're knowledgable about this TODO]:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#TODO_Comments

Done.

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/R...
Source/modules/fetch/Request.cpp:160: // if (init.redirect == "follw") {
On 2015/08/07 07:39:12, falken wrote:
> nit: follw. i'd prefer to just not land commented out code but it's no biggie
as
> it'll be fixed soon

Done.

[horo, 2015-08-07 10:02:12]

On 2015/08/07 07:39:12, falken wrote:
> lgtm
> 
> Let's make a Feature Dashboard and OWP Launch bug for this. I don't think it
> needs intent to ship but it'd be good for tracking purposes and letting people
> know the change is happening.

Done
https://www.chromestatus.com/feature/4614142321229824
https://crbug.com/517837

[horo, 2015-08-07 12:08:58]

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

[horo, 2015-08-07 12:08:58]

chrishtr@
Could you please review Source/platform/exported/*, Source/platform/network/*,
public/platform/*?

[yhirano, 2015-08-07 14:22:02]

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/F...
File Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/F...
Source/modules/fetch/FetchManager.cpp:144: if
(IsRedirectStatusCode(m_responseHttpStatusCode)) {
ASSERT(redirect mode is "manual")?

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/F...
File Source/modules/fetch/FetchResponseData.h (right):

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/F...
Source/modules/fetch/FetchResponseData.h:25: // "A response has an associated
type which is one of basic, CORS, default,
Please update the comment.

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/R...
File Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/R...
Source/modules/fetch/Request.cpp:78: // TODO(yhirano):
Please wrap the comment in 80 cols.

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/service...
File Source/modules/serviceworkers/RespondWithObserver.cpp (right):

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/service...
Source/modules/serviceworkers/RespondWithObserver.cpp:221: if
(isNavigationRequest(m_frameType) && responseType ==
FetchResponseData::OpaqueRedirectType) {
https://fetch.spec.whatwg.org/#http-fetch says

If one of the following conditions is true, return a network error:

response's type is "error".
request's mode is not "no-cors" and response's type is "opaque".
request is not a navigation request and response's type is "opaqueredirect".


Should this check be (!isNavigationRequest(m_frameType) && responseType ==
FetchResponseData::OpaqueRedirectType) ?

[horo, 2015-08-07 15:39:54]

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/F...
File Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/F...
Source/modules/fetch/FetchManager.cpp:144: if
(IsRedirectStatusCode(m_responseHttpStatusCode)) {
On 2015/08/07 14:22:02, yhirano wrote:
> ASSERT(redirect mode is "manual")?

Done.

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/F...
File Source/modules/fetch/FetchResponseData.h (right):

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/F...
Source/modules/fetch/FetchResponseData.h:25: // "A response has an associated
type which is one of basic, CORS, default,
On 2015/08/07 14:22:02, yhirano wrote:
> Please update the comment.

Done.

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/R...
File Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/fetch/R...
Source/modules/fetch/Request.cpp:78: // TODO(yhirano):
On 2015/08/07 14:22:02, yhirano wrote:
> Please wrap the comment in 80 cols.

Done.

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/service...
File Source/modules/serviceworkers/RespondWithObserver.cpp (right):

https://codereview.chromium.org/1265133002/diff/180001/Source/modules/service...
Source/modules/serviceworkers/RespondWithObserver.cpp:221: if
(isNavigationRequest(m_frameType) && responseType ==
FetchResponseData::OpaqueRedirectType) {
On 2015/08/07 14:22:02, yhirano wrote:
> https://fetch.spec.whatwg.org/#http-fetch says
> 
> If one of the following conditions is true, return a network error:
> 
> response's type is "error".
> request's mode is not "no-cors" and response's type is "opaque".
> request is not a navigation request and response's type is "opaqueredirect".
> 
> 
> Should this check be (!isNavigationRequest(m_frameType) && responseType ==
> FetchResponseData::OpaqueRedirectType) ?

Ah, yes.  Fixed.

[yhirano, 2015-08-10 04:37:08]

lgtm

[chrishtr, 2015-08-10 17:04:36]

lgtm

[horo, 2015-08-10 22:52:04]

The CQ bit was checked by horo@chromium.org

[horo, 2015-08-10 22:52:05]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org
Link to the patchset: https://codereview.chromium.org/1265133002/#ps240001
(title: "Fixed comments")

[commit-bot: I haz the power, 2015-08-10 22:52:15]

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

[commit-bot: I haz the power, 2015-08-10 22:54:44]

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

[commit-bot: I haz the power, 2015-08-10 22:54:45]

Try jobs failed on following builders:
  android_blink_compile_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)

[horo, 2015-08-11 01:54:15]

The CQ bit was checked by horo@chromium.org

[horo, 2015-08-11 01:54:15]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
chrishtr@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/1265133002/#ps260001
(title: "rebase")

[commit-bot: I haz the power, 2015-08-11 01:54:24]

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

[commit-bot: I haz the power, 2015-08-11 03:24:21]

Committed patchset #8 (id:260001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200291