[Fetch API] Follow a body setting spec change on Request constructor.

[Fetch API] Follow a body setting spec change on Request constructor.

This change includes some refactoring, such as spec text update and
nullptr adoption.

BUG=457119, 462490, 462493, 476867
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193936

[yhirano, 2015-04-14 08:41:40]

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

[hiroshige, 2015-04-14 10:00:49]

Could you set BUG=457119, 462490, 462493 in the description?

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

https://codereview.chromium.org/1088823002/diff/80001/LayoutTests/http/tests/...
LayoutTests/http/tests/fetch/script-tests/request.js:298: 'Request should be
flagged as used if it has been consumed.');
Could you add a comment or assert message that this tests "Set input's body to
null." of Step 30 of https://fetch.spec.whatwg.org/#dom-request ?

https://codereview.chromium.org/1088823002/diff/80001/LayoutTests/http/tests/...
LayoutTests/http/tests/fetch/script-tests/request.js:307: // but the current
spec allows it.
The spec was fixed.
https://github.com/whatwg/fetch/commit/5be0642d1ad5b93ed8467ea9acc28f5786981599
Created: crbug.com/476867

https://codereview.chromium.org/1088823002/diff/80001/LayoutTests/http/tests/...
LayoutTests/http/tests/fetch/script-tests/request.js:319: 'A get request may not
have body.');
CONNECT is rejected because it is forbidden method:
Step 20 in https://fetch.spec.whatwg.org/#dom-request
not because 'A get request may not have body.'.

[yhirano, 2015-04-15 01:49:55]

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

https://codereview.chromium.org/1088823002/diff/80001/LayoutTests/http/tests/...
LayoutTests/http/tests/fetch/script-tests/request.js:298: 'Request should be
flagged as used if it has been consumed.');
On 2015/04/14 10:00:49, hiroshige wrote:
> Could you add a comment or assert message that this tests "Set input's body to
> null." of Step 30 of https://fetch.spec.whatwg.org/#dom-request ?

Done.

https://codereview.chromium.org/1088823002/diff/80001/LayoutTests/http/tests/...
LayoutTests/http/tests/fetch/script-tests/request.js:307: // but the current
spec allows it.
On 2015/04/14 10:00:49, hiroshige wrote:
> The spec was fixed.
>
https://github.com/whatwg/fetch/commit/5be0642d1ad5b93ed8467ea9acc28f5786981599
> Created: crbug.com/476867

Done.

https://codereview.chromium.org/1088823002/diff/80001/LayoutTests/http/tests/...
LayoutTests/http/tests/fetch/script-tests/request.js:319: 'A get request may not
have body.');
On 2015/04/14 10:00:49, hiroshige wrote:
> CONNECT is rejected because it is forbidden method:
> Step 20 in https://fetch.spec.whatwg.org/#dom-request
> not because 'A get request may not have body.'.

Done.

[yhirano, 2015-04-15 02:20:00]

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

https://codereview.chromium.org/1088823002/diff/80001/LayoutTests/http/tests/...
LayoutTests/http/tests/fetch/script-tests/request.js:298: 'Request should be
flagged as used if it has been consumed.');
On 2015/04/15 01:49:55, yhirano wrote:
> On 2015/04/14 10:00:49, hiroshige wrote:
> > Could you add a comment or assert message that this tests "Set input's body
to
> > null." of Step 30 of https://fetch.spec.whatwg.org/#dom-request ?
> 
> Done.

Note: I don't want to add the number (30) here because it is very fragile. See
L328-L330, for example.

[hiroshige, 2015-04-17 07:02:34]

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

https://codereview.chromium.org/1088823002/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/fetch/script-tests/request.js:317: 'A request with a
forbidden method may not have body.');
'Request() with a forbidden method must throw'?
A Request with a forbidden method is rejected even if |req| doesn't have body.

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

https://codereview.chromium.org/1088823002/diff/100001/Source/modules/fetch/R...
Source/modules/fetch/Request.cpp:59: // setting 'body passed' flag consume
entire body (e.g. text()).
Does this mean the following?
inputRequest->bodyUsed() is false and inputRequest->isBodyConsumed(), which
means that someone consumed body data without setting 'body passed' flag.
Currently the methods that consume entire body (e.g. text()) can do this.
So we can assume the entire body is already consumed and
Thus we can set an empty blob to the new request instead of creating a draining
stream.

https://codereview.chromium.org/1088823002/diff/100001/Source/modules/fetch/R...
Source/modules/fetch/Request.cpp:64:
blobData->setContentType(inputRequest->blobDataHandle()->type());
Is there any test for the Blob type set here?

[yhirano, 2015-04-17 07:31:37]

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

https://codereview.chromium.org/1088823002/diff/100001/LayoutTests/http/tests...
LayoutTests/http/tests/fetch/script-tests/request.js:317: 'A request with a
forbidden method may not have body.');
On 2015/04/17 07:02:34, hiroshige wrote:
> 'Request() with a forbidden method must throw'?
> A Request with a forbidden method is rejected even if |req| doesn't have body.

Thanks, done.

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

https://codereview.chromium.org/1088823002/diff/100001/Source/modules/fetch/R...
Source/modules/fetch/Request.cpp:59: // setting 'body passed' flag consume
entire body (e.g. text()).
On 2015/04/17 07:02:34, hiroshige wrote:
> Does this mean the following?
> inputRequest->bodyUsed() is false and inputRequest->isBodyConsumed(), which
> means that someone consumed body data without setting 'body passed' flag.
> Currently the methods that consume entire body (e.g. text()) can do this.
> So we can assume the entire body is already consumed and
> Thus we can set an empty blob to the new request instead of creating a
draining
> stream.

Yeah.
> Currently the methods that consume entire body (e.g. text()) can do this.
Currently only methods that consume entire body (e.g. text()) can do this.

https://codereview.chromium.org/1088823002/diff/100001/Source/modules/fetch/R...
Source/modules/fetch/Request.cpp:64:
blobData->setContentType(inputRequest->blobDataHandle()->type());
On 2015/04/17 07:02:34, hiroshige wrote:
> Is there any test for the Blob type set here?

No, then I tried to add one and found it is impossible because of another bug. I
added TODO in the test file.

[hiroshige, 2015-04-17 09:29:06]

lgtm.

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

https://codereview.chromium.org/1088823002/diff/120001/LayoutTests/http/tests...
LayoutTests/http/tests/fetch/script-tests/request.js:529: //          return
req.blob();
Shouldn't this be "return (new Request(req)).blob();"?

[yhirano, 2015-04-17 09:37:04]

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

https://codereview.chromium.org/1088823002/diff/120001/LayoutTests/http/tests...
LayoutTests/http/tests/fetch/script-tests/request.js:529: //          return
req.blob();
On 2015/04/17 09:29:06, hiroshige wrote:
> Shouldn't this be "return (new Request(req)).blob();"?

Done.

[yhirano, 2015-04-17 09:37:09]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2015-04-17 09:37:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org
Link to the patchset: https://codereview.chromium.org/1088823002/#ps140001
(title: " ")

[commit-bot: I haz the power, 2015-04-17 09:37:20]

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

[commit-bot: I haz the power, 2015-04-17 10:44:29]

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