Implement request's redirect mode and RequestRedirect for Fetch

Implement request's redirect mode and RequestRedirect for Fetch

As in Spec: https://fetch.spec.whatwg.org/
BUG=471673

[shiva.jm, 2015-05-20 05:52:25]

shiva.jm@samsung.com changed reviewers:
+ a.cavalcanti@samsung.com, habib.virji@samsung.com

[shiva.jm, 2015-05-20 05:53:31]

Please have a look.

[shiva.jm, 2015-05-22 05:10:48]

shiva.jm@samsung.com changed reviewers:
+ hiroshige@chromium.org, horo@chromium.org, jsbell@chromium.org,
yhirano@chromium.org

[shiva.jm, 2015-05-22 05:11:58]

please have a look.

[shiva.jm, 2015-05-22 05:13:42]

shiva.jm@samsung.com changed reviewers:
+ pdr@chromium.org, tkent@chromium.org

[shiva.jm, 2015-05-22 05:13:43]

pdr@chromium.org: Please review changes in 

tkent@chromium.org: Please review changes in

[tkent, 2015-05-22 05:20:27]

On 2015/05/22 05:13:43, shiva.jm wrote:
> mailto:pdr@chromium.org: Please review changes in 
> 
> mailto:tkent@chromium.org: Please review changes in

in what?
This should be reviewed by Fetch API developers first.

[pdr., 2015-05-22 17:44:38]

pdr@chromium.org changed reviewers:
+ japhet@chromium.org, mkwst@chromium.org

[pdr., 2015-05-22 17:46:23]

+cc japhet and mkwst who are owners of fetch.

[yhirano, 2015-05-25 06:30:53]

I'm a bit confused: Is this CL implementing the redirect mode? Does setting it
to "error" cause any actual behavior change other than the property value? Is
there any tests about the behavior change?

https://codereview.chromium.org/1143083002/diff/1/LayoutTests/http/tests/fetc...
File LayoutTests/http/tests/fetch/script-tests/request.js (right):

https://codereview.chromium.org/1143083002/diff/1/LayoutTests/http/tests/fetc...
LayoutTests/http/tests/fetch/script-tests/request.js:627:
assert_equals(request1.redirect, redirect1 || 'follow',
What does this "redirect1 || 'follow'" mean?

https://codereview.chromium.org/1143083002/diff/1/LayoutTests/http/tests/fetc...
LayoutTests/http/tests/fetch/script-tests/request.js:630:
assert_equals(request1.redirect, redirect1 || 'follow',
ditto

https://codereview.chromium.org/1143083002/diff/1/Source/modules/fetch/Reques...
File Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1143083002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.cpp:144: // TODO(yhirano): "16. If |init|'s
redirect member is present and its is
Please delete this TODO as you implement it.

https://codereview.chromium.org/1143083002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.cpp:151: // TODO(yhirano): "17. Let |redirect| be
|init|'s redirect member if it is
ditto

https://codereview.chromium.org/1143083002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.cpp:153: // TODO(yhirano): "18 If |redirect| is
non-null, set |request|'s redirect
ditto

https://codereview.chromium.org/1143083002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.cpp:155: // TODO(yhirano): "19 If |request|'s
redirect mode is manual, set it to
ditto

[shiva.jm, 2015-05-25 12:01:57]

Please have a look, updated the review comments and tests.

Yes, its for implementing the redirect mode. I think there is no behavior change
here, its just setting the property value.

In HTTP Spec (https://fetch.spec.whatwg.org/#http-fetch), only if response
status is 301 - 308, redirect mode is considered.

https://codereview.chromium.org/1143083002/diff/1/LayoutTests/http/tests/fetc...
File LayoutTests/http/tests/fetch/script-tests/request.js (right):

https://codereview.chromium.org/1143083002/diff/1/LayoutTests/http/tests/fetc...
LayoutTests/http/tests/fetch/script-tests/request.js:627:
assert_equals(request1.redirect, redirect1 || 'follow',
On 2015/05/25 06:30:52, yhirano wrote:
> What does this "redirect1 || 'follow'" mean?

Done.

https://codereview.chromium.org/1143083002/diff/1/LayoutTests/http/tests/fetc...
LayoutTests/http/tests/fetch/script-tests/request.js:630:
assert_equals(request1.redirect, redirect1 || 'follow',
On 2015/05/25 06:30:52, yhirano wrote:
> ditto

Done.

https://codereview.chromium.org/1143083002/diff/1/Source/modules/fetch/Reques...
File Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1143083002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.cpp:144: // TODO(yhirano): "16. If |init|'s
redirect member is present and its is
On 2015/05/25 06:30:52, yhirano wrote:
> Please delete this TODO as you implement it.

Done.

https://codereview.chromium.org/1143083002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.cpp:151: // TODO(yhirano): "17. Let |redirect| be
|init|'s redirect member if it is
On 2015/05/25 06:30:53, yhirano wrote:
> ditto

Done.

https://codereview.chromium.org/1143083002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.cpp:153: // TODO(yhirano): "18 If |redirect| is
non-null, set |request|'s redirect
On 2015/05/25 06:30:52, yhirano wrote:
> ditto

Done.

https://codereview.chromium.org/1143083002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.cpp:155: // TODO(yhirano): "19 If |request|'s
redirect mode is manual, set it to
On 2015/05/25 06:30:52, yhirano wrote:
> ditto

Done.

[yhirano, 2015-05-26 03:23:45]

On 2015/05/25 12:01:57, shiva.jm wrote:
> Please have a look, updated the review comments and tests.
> 
> Yes, its for implementing the redirect mode. I think there is no behavior
change
> here, its just setting the property value.
> 
> In HTTP Spec (https://fetch.spec.whatwg.org/#http-fetch), only if response
> status is 301 - 308, redirect mode is considered.
> 

Then, please implement the behavior change. The property is for controlling the
fetch operation.
If you want to divide the change into multiple CLs, it's OK, but I generally
like adding the property as the last CL, because exposing a non-working property
may confuse developers.

[yhirano, 2015-08-19 10:45:05]

Note: horo@ implemented the feature.
https://codereview.chromium.org/1265133002/
https://codereview.chromium.org/1271733002/
https://codereview.chromium.org/1280733002/

[shiva.jm, 2015-08-19 16:02:18]

On 2015/08/19 10:45:05, yhirano wrote:
> Note: horo@ implemented the feature.
> https://codereview.chromium.org/1265133002/
> https://codereview.chromium.org/1271733002/
> https://codereview.chromium.org/1280733002/

Ok, thanks