[Fetch API] Reflect spec changes of bodyUsed property

[Fetch API] Reflect spec changes of bodyUsed property

This CL changes the meaning of |bodyUsed| property on Request and Response.
This CL also changes ReadableStreamReader's behavior when released. See
the links in the discussion thread for the background.

Discussion: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/puUJocJ_5F4
BUG=472931
Committed: https://crrev.com/f8417d536a3bb95b93c2a5f2d5abf5a76be17c96
Cr-Commit-Position: refs/heads/master@{#361798}

[yhirano, 2015-10-29 08:43:54]

Patchset #2 (id:20001) has been deleted

[yhirano, 2015-10-29 08:51:49]

Description was changed from

==========
bodyused implementation

BUG=
==========

to

==========
[Fetch API] Reflect spec changes of bodyUsed property

This CL changes the meaning of |bodyUsed| property on Request and Response.
This change also changes ReadableStreamReader's behavior when
released.

See the links in the discussion thread for the background.
  
Discussion:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/puUJocJ_5F4
BUG=472931
==========

[yhirano, 2015-10-29 08:52:21]

Description was changed from

==========
[Fetch API] Reflect spec changes of bodyUsed property

This CL changes the meaning of |bodyUsed| property on Request and Response.
This change also changes ReadableStreamReader's behavior when
released.

See the links in the discussion thread for the background.
  
Discussion:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/puUJocJ_5F4
BUG=472931
==========

to

==========
[Fetch API] Reflect spec changes of bodyUsed property

This CL changes the meaning of |bodyUsed| property on Request and Response.
This CL also changes ReadableStreamReader's behavior when released. See
the links in the discussion thread for the background.
  
Discussion:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/puUJocJ_5F4
BUG=472931
==========

[yhirano, 2015-10-29 08:53:59]

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

[tyoshino (SeeGerritForStatus), 2015-10-30 04:50:34]

https://codereview.chromium.org/1418813004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/streams/ReadableStreamReader.cpp (right):

https://codereview.chromium.org/1418813004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/streams/ReadableStreamReader.cpp:56:
resolver->resolve(closed(scriptState).v8Value());
closed may be still pending if this reader has been released without
close/error?

need similar change as read(), or to be done in the next CL?

https://codereview.chromium.org/1418813004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/Body.cpp (right):

https://codereview.chromium.org/1418813004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/Body.cpp:203:
UseCounter::count(executionContext(), UseCounter::FetchBodyStream);
This starts counting the calls from bodyUsed and isBodyLocked

[yhirano, 2015-10-30 06:00:57]

https://codereview.chromium.org/1418813004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/streams/ReadableStreamReader.cpp (right):

https://codereview.chromium.org/1418813004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/streams/ReadableStreamReader.cpp:56:
resolver->resolve(closed(scriptState).v8Value());
On 2015/10/30 04:50:34, tyoshino wrote:
> closed may be still pending if this reader has been released without
> close/error?
> 
> need similar change as read(), or to be done in the next CL?

m_closed is rejected when this reader is released. That being said, creating and
returning a rejected promise is easier to understand. Fixed.

https://codereview.chromium.org/1418813004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/Body.cpp (right):

https://codereview.chromium.org/1418813004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/Body.cpp:203:
UseCounter::count(executionContext(), UseCounter::FetchBodyStream);
On 2015/10/30 04:50:34, tyoshino wrote:
> This starts counting the calls from bodyUsed and isBodyLocked

Done.

[yhirano, 2015-11-17 04:38:53]

ping

[tyoshino (SeeGerritForStatus), 2015-11-18 09:55:36]

lgtm

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

https://codereview.chromium.org/1418813004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js:327:
'Request should not be flagged as used if it does not have' +
indent

https://codereview.chromium.org/1418813004/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/response.js
(left):

https://codereview.chromium.org/1418813004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/response.js:240:
assert_equals(text, 'hello');
shouldn't test above here kept? already covered somewhere else?

https://codereview.chromium.org/1418813004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/response.js:361:
return Promise.all([res.text(), clone.text()]).then(function(r) {
keep check on clone.text()?

https://codereview.chromium.org/1418813004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/response.js:377: },
'Cancel and Clone on Response');
ditto

https://codereview.chromium.org/1418813004/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/stream-reader.js
(right):

https://codereview.chromium.org/1418813004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/stream-reader.js:98:

how about moving these to response.js as it's not using stream?

https://codereview.chromium.org/1418813004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/Response.cpp (right):

https://codereview.chromium.org/1418813004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/Response.cpp:395: return
(internalBodyBuffer() && internalBodyBuffer()->stream()->isDisturbed()) ||
(bodyBuffer() && bodyBuffer()->stream()->isDisturbed());
just need to check internalBodyBuffer?

[yhirano, 2015-11-19 08:41:29]

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

https://codereview.chromium.org/1418813004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js:327:
'Request should not be flagged as used if it does not have' +
On 2015/11/18 09:55:36, tyoshino wrote:
> indent

Done.

https://codereview.chromium.org/1418813004/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/response.js
(left):

https://codereview.chromium.org/1418813004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/response.js:240:
assert_equals(text, 'hello');
On 2015/11/18 09:55:36, tyoshino wrote:
> shouldn't test above here kept? already covered somewhere else?

Technically yes, in stream-reader.js.

https://codereview.chromium.org/1418813004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/response.js:361:
return Promise.all([res.text(), clone.text()]).then(function(r) {
On 2015/11/18 09:55:36, tyoshino wrote:
> keep check on clone.text()?

Tested at 'Cancelling stream should not affect cloned one.' in stream-reader.js

https://codereview.chromium.org/1418813004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/response.js:377: },
'Cancel and Clone on Response');
On 2015/11/18 09:55:36, tyoshino wrote:
> ditto

Added 'Used => clone' and 'Locked => clone'. Found a bug and fixed. Thanks!

https://codereview.chromium.org/1418813004/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/stream-reader.js
(right):

https://codereview.chromium.org/1418813004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/stream-reader.js:98:

On 2015/11/18 09:55:36, tyoshino wrote:
> how about moving these to response.js as it's not using stream?

request.js and response.js don't contain tests using fetch() function.

https://codereview.chromium.org/1418813004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/Response.cpp (right):

https://codereview.chromium.org/1418813004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/Response.cpp:395: return
(internalBodyBuffer() && internalBodyBuffer()->stream()->isDisturbed()) ||
(bodyBuffer() && bodyBuffer()->stream()->isDisturbed());
On 2015/11/18 09:55:36, tyoshino wrote:
> just need to check internalBodyBuffer?

Done.

[hiroshige, 2015-11-19 12:50:37]

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js
(right):

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js:340:
'Request should not be flagged as used if it does not have' +
nit: ' ' at end of the string (current: 'does not havebody').

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/cachestorage/Cache.cpp (right):

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/cachestorage/Cache.cpp:499: // availability.
The spec says we must not check the request's body's availability and thus we
follow the spec here, and the previous implementation was not spec-conformant,
right?
If so, stating the spec (not just "we" = Blink) doesn't check request's body in
the comment might be clearer.

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/Body.h (right):

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/Body.h:43: bool isBodyLocked();
In the Fetch spec, |locked| is only defined for Response (not for Request) and
used in Response's clone() and consume body.
However, in this CL, isBodyLocked() is also used for Request: Request's consume
body, Request::clone().
Is this OK?
(I assumes this is because we cannot observe intermediate state of Request body
while reading and thus isBodyLocked() always returns false where it is used for
Request, and perhaps there's a future plan to introduce |locked| to Request. If
so, leaving a brief comment might be helpful.)

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/Body.h:57: bool m_opaque;
Seems not used and thus we can delete m_opaque.

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BodyStreamBufferTest.cpp (right):

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BodyStreamBufferTest.cpp:176:
MockFetchDataLoaderClient* client1 = MockFetchDataLoaderClient::create();
nit: |client1| can be |client| because now we have only one client here.

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BodyStreamBufferTest.cpp:228:
MockFetchDataLoaderClient* client1 = MockFetchDataLoaderClient::create();
ditto.

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/Request.cpp:544: if (bodyUsed() ||
isBodyLocked()) {
nit: we write "isBodyLocked() || bodyUsed()" in some locations and "bodyUsed()
|| isBodyLocked()" in others; Using the same order makes the code easier to
search and read.

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerResponseError.h
(right):

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerResponseError.h:23:
WebServiceWorkerResponseErrorBodyLocked,
histograms.xml should be also updated?
https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...

[yhirano, 2015-11-19 13:19:03]

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js
(right):

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js:340:
'Request should not be flagged as used if it does not have' +
On 2015/11/19 12:50:36, hiroshige (slow) wrote:
> nit: ' ' at end of the string (current: 'does not havebody').

Done.

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/cachestorage/Cache.cpp (right):

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/cachestorage/Cache.cpp:499: // availability.
On 2015/11/19 12:50:36, hiroshige (slow) wrote:
> The spec says we must not check the request's body's availability and thus we
> follow the spec here, and the previous implementation was not spec-conformant,
> right?
> If so, stating the spec (not just "we" = Blink) doesn't check request's body
in
> the comment might be clearer.

Done.

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/Body.h (right):

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/Body.h:43: bool isBodyLocked();
On 2015/11/19 12:50:36, hiroshige (slow) wrote:
> In the Fetch spec, |locked| is only defined for Response (not for Request) and
> used in Response's clone() and consume body.
> However, in this CL, isBodyLocked() is also used for Request: Request's
consume
> body, Request::clone().
> Is this OK?
> (I assumes this is because we cannot observe intermediate state of Request
body
> while reading and thus isBodyLocked() always returns false where it is used
for
> Request, and perhaps there's a future plan to introduce |locked| to Request.
If
> so, leaving a brief comment might be helpful.)

It's OK, I'm introducing it right now[1] and it doesn't change the behavior.

1: https://github.com/yutakahirano/fetch-with-streams/pull/62

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/Body.h:57: bool m_opaque;
On 2015/11/19 12:50:36, hiroshige (slow) wrote:
> Seems not used and thus we can delete m_opaque.

Sorry, it's a rebase error. Fixed.

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BodyStreamBufferTest.cpp (right):

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BodyStreamBufferTest.cpp:176:
MockFetchDataLoaderClient* client1 = MockFetchDataLoaderClient::create();
On 2015/11/19 12:50:37, hiroshige (slow) wrote:
> nit: |client1| can be |client| because now we have only one client here.

Done.

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BodyStreamBufferTest.cpp:228:
MockFetchDataLoaderClient* client1 = MockFetchDataLoaderClient::create();
On 2015/11/19 12:50:37, hiroshige (slow) wrote:
> ditto.

Done.

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/Request.cpp:544: if (bodyUsed() ||
isBodyLocked()) {
On 2015/11/19 12:50:37, hiroshige (slow) wrote:
> nit: we write "isBodyLocked() || bodyUsed()" in some locations and "bodyUsed()
> || isBodyLocked()" in others; Using the same order makes the code easier to
> search and read.

Done.

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerResponseError.h
(right):

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerResponseError.h:23:
WebServiceWorkerResponseErrorBodyLocked,
On 2015/11/19 12:50:37, hiroshige (slow) wrote:
> histograms.xml should be also updated?
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...

Done.

[yhirano, 2015-11-19 13:19:34]

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

[yhirano, 2015-11-19 13:19:34]

+horo@ for modules/serviceworkers and modules/cachestorage.

[yhirano, 2015-11-19 14:17:15]

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/cachestorage/Cache.cpp (right):

https://codereview.chromium.org/1418813004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/cachestorage/Cache.cpp:499: // availability.
On 2015/11/19 13:19:03, yhirano wrote:
> On 2015/11/19 12:50:36, hiroshige (slow) wrote:
> > The spec says we must not check the request's body's availability and thus
we
> > follow the spec here, and the previous implementation was not
spec-conformant,
> > right?
> > If so, stating the spec (not just "we" = Blink) doesn't check request's body
> in
> > the comment might be clearer.
> 
> Done.

Actually, a Request having 'GET' method must not have body and hence this check
always passes. Replaced the comment with an assertion.

[yhirano, 2015-11-19 14:28:30]

On 2015/11/19 13:19:34, yhirano wrote:
> +horo@ for modules/serviceworkers and modules/cachestorage.

Also for
third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerResponseError.h.

[hiroshige, 2015-11-23 18:59:35]

lgtm.

https://codereview.chromium.org/1418813004/diff/200001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1418813004/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:73325: +  <int value="9"
label="ErrorBodyLocked"/>
Oh, I found WebServiceWorkerResponseErrorResponseTypeOpaqueRedirect is also
missing, so
9 for WebServiceWorkerResponseErrorResponseTypeOpaqueRedirect and
10 for WebServiceWorkerResponseErrorBodyLocked.

[yhirano, 2015-11-25 04:43:01]

https://codereview.chromium.org/1418813004/diff/200001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1418813004/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:73325: +  <int value="9"
label="ErrorBodyLocked"/>
On 2015/11/23 18:59:35, hiroshige (slow) wrote:
> Oh, I found WebServiceWorkerResponseErrorResponseTypeOpaqueRedirect is also
> missing, so
> 9 for WebServiceWorkerResponseErrorResponseTypeOpaqueRedirect and
> 10 for WebServiceWorkerResponseErrorBodyLocked.

Thanks, fixed.

[yhirano, 2015-11-25 04:43:08]

horo: ping

[horo, 2015-11-25 05:16:08]

lgtm

[yhirano, 2015-11-25 06:32:37]

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

[yhirano, 2015-11-25 06:32:38]

+mpearson@ for histograms.xml

[Mark P, 2015-11-25 19:00:51]

histograms.xml lgtm

[yhirano, 2015-11-26 01:38:18]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2015-11-26 01:38:19]

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

[commit-bot: I haz the power, 2015-11-26 01:39:15]

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

[commit-bot: I haz the power, 2015-11-26 02:38:18]

Description was changed from

==========
[Fetch API] Reflect spec changes of bodyUsed property

This CL changes the meaning of |bodyUsed| property on Request and Response.
This CL also changes ReadableStreamReader's behavior when released. See
the links in the discussion thread for the background.
  
Discussion:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/puUJocJ_5F4
BUG=472931
==========

to

==========
[Fetch API] Reflect spec changes of bodyUsed property

This CL changes the meaning of |bodyUsed| property on Request and Response.
This CL also changes ReadableStreamReader's behavior when released. See
the links in the discussion thread for the background.
  
Discussion:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/puUJocJ_5F4
BUG=472931
==========

[commit-bot: I haz the power, 2015-11-26 02:38:20]

Committed patchset #13 (id:260001)

[commit-bot: I haz the power, 2015-11-26 02:39:15]

Description was changed from

==========
[Fetch API] Reflect spec changes of bodyUsed property

This CL changes the meaning of |bodyUsed| property on Request and Response.
This CL also changes ReadableStreamReader's behavior when released. See
the links in the discussion thread for the background.
  
Discussion:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/puUJocJ_5F4
BUG=472931
==========

to

==========
[Fetch API] Reflect spec changes of bodyUsed property

This CL changes the meaning of |bodyUsed| property on Request and Response.
This CL also changes ReadableStreamReader's behavior when released. See
the links in the discussion thread for the background.

Discussion:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/puUJocJ_5F4
BUG=472931

Committed: https://crrev.com/f8417d536a3bb95b93c2a5f2d5abf5a76be17c96
Cr-Commit-Position: refs/heads/master@{#361798}
==========

[commit-bot: I haz the power, 2015-11-26 02:39:16]

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