Streams Implementation Update: async read

Streams Implementation Update: async read

This CL is one of Streams Implementation Update series. This CL does:
- Make ReadableStreamReader.read asynchronous.
- Remove .state, .ready and .isActive from ReadableStreamReader.
- Remove .closed from ReadableStream.

BUG=393911
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192220

[yhirano, 2015-03-17 06:52:30]

Patchset #2 (id:20001) has been deleted

[yhirano, 2015-03-17 06:56:47]

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

[yhirano, 2015-03-17 06:58:01]

This CL depends on https://codereview.chromium.org/1001233002/.
Currently response.body.getReader()read() returns Promise<ArrayBuffer>. It will
be fixed in the next CL.

[yhirano, 2015-03-17 07:20:11]

Patchset #3 (id:60001) has been deleted

[tyoshino (SeeGerritForStatus), 2015-03-18 07:10:50]

https://codereview.chromium.org/1004623007/diff/40001/Source/core/streams/Rea...
File Source/core/streams/ReadableStreamReader.cpp (right):

https://codereview.chromium.org/1004623007/diff/40001/Source/core/streams/Rea...
Source/core/streams/ReadableStreamReader.cpp:48: return closed(scriptState);
https://streams.spec.whatwg.org/branch-snapshots/async-read-take-3/

Now it's specified to return a new promise.

https://codereview.chromium.org/1004623007/diff/40001/Source/core/streams/Rea...
Source/core/streams/ReadableStreamReader.cpp:59: return closed(scriptState);
ditto

[tyoshino (SeeGerritForStatus), 2015-03-18 07:39:10]

https://codereview.chromium.org/1004623007/diff/40001/Source/core/streams/Rea...
File Source/core/streams/ReadableStream.cpp (right):

https://codereview.chromium.org/1004623007/diff/40001/Source/core/streams/Rea...
Source/core/streams/ReadableStream.cpp:83: resolveAllPendingReadsAsDone();
L83-L86 and L95-L99 are the same.

https://codereview.chromium.org/1004623007/diff/40001/Source/core/streams/Rea...
Source/core/streams/ReadableStream.cpp:96: m_state = Closed;
this should be done after resolving the promises? L110-121 no longer applies?

[yhirano, 2015-03-18 07:49:04]

Patchset #4 (id:100001) has been deleted

[yhirano, 2015-03-18 07:49:08]

Patchset #3 (id:80001) has been deleted

[yhirano, 2015-03-18 08:07:49]

https://codereview.chromium.org/1004623007/diff/40001/Source/core/streams/Rea...
File Source/core/streams/ReadableStream.cpp (right):

https://codereview.chromium.org/1004623007/diff/40001/Source/core/streams/Rea...
Source/core/streams/ReadableStream.cpp:83: resolveAllPendingReadsAsDone();
On 2015/03/18 07:39:10, tyoshino wrote:
> L83-L86 and L95-L99 are the same.

Done.

https://codereview.chromium.org/1004623007/diff/40001/Source/core/streams/Rea...
Source/core/streams/ReadableStream.cpp:96: m_state = Closed;
On 2015/03/18 07:39:10, tyoshino wrote:
> this should be done after resolving the promises? L110-121 no longer applies?

We don't have to care about it, because it is ScriptPromiseProperty related and
we don't have a ScriptPromiseProperty in ReadableStream now.

https://codereview.chromium.org/1004623007/diff/40001/Source/core/streams/Rea...
File Source/core/streams/ReadableStreamReader.cpp (right):

https://codereview.chromium.org/1004623007/diff/40001/Source/core/streams/Rea...
Source/core/streams/ReadableStreamReader.cpp:48: return closed(scriptState);
On 2015/03/18 07:10:50, tyoshino wrote:
> https://streams.spec.whatwg.org/branch-snapshots/async-read-take-3/
> 
> Now it's specified to return a new promise.

Done.

https://codereview.chromium.org/1004623007/diff/40001/Source/core/streams/Rea...
Source/core/streams/ReadableStreamReader.cpp:59: return closed(scriptState);
On 2015/03/18 07:10:50, tyoshino wrote:
> ditto

Done.

[domenic (use chromium.org), 2015-03-18 08:32:16]

d@domenic.me changed reviewers:
+ d@domenic.me

[domenic (use chromium.org), 2015-03-18 08:32:17]

Reviewed IDL, looks like an accurate subset of what we have in the spec :)

https://codereview.chromium.org/1004623007/diff/140001/Source/core/streams/Re...
File Source/core/streams/ReadableStream.idl (right):

https://codereview.chromium.org/1004623007/diff/140001/Source/core/streams/Re...
Source/core/streams/ReadableStream.idl:12: [CallWith=ScriptState] Promise<any>
cancel(any reason);
I think this could be Promise<void>, both here and in reader.

[yhirano, 2015-03-18 08:36:28]

Thank you for the review!

https://codereview.chromium.org/1004623007/diff/140001/Source/core/streams/Re...
File Source/core/streams/ReadableStream.idl (right):

https://codereview.chromium.org/1004623007/diff/140001/Source/core/streams/Re...
Source/core/streams/ReadableStream.idl:12: [CallWith=ScriptState] Promise<any>
cancel(any reason);
On 2015/03/18 08:32:17, domenic wrote:
> I think this could be Promise<void>, both here and in reader.

Done.

[yhirano, 2015-03-18 14:03:24]

https://codereview.chromium.org/1004623007/diff/40001/Source/core/streams/Rea...
File Source/core/streams/ReadableStreamReader.cpp (right):

https://codereview.chromium.org/1004623007/diff/40001/Source/core/streams/Rea...
Source/core/streams/ReadableStreamReader.cpp:48: return closed(scriptState);
On 2015/03/18 08:07:49, yhirano wrote:
> On 2015/03/18 07:10:50, tyoshino wrote:
> > https://streams.spec.whatwg.org/branch-snapshots/async-read-take-3/
> > 
> > Now it's specified to return a new promise.
> 
> Done.

Oh, sorry, I forgot that ScriptPromise.cast(promise) returns promise itself. I
will fix it tomorrow.

[yhirano, 2015-03-19 01:36:13]

https://codereview.chromium.org/1004623007/diff/40001/Source/core/streams/Rea...
File Source/core/streams/ReadableStreamReader.cpp (right):

https://codereview.chromium.org/1004623007/diff/40001/Source/core/streams/Rea...
Source/core/streams/ReadableStreamReader.cpp:48: return closed(scriptState);
On 2015/03/18 14:03:23, yhirano wrote:
> On 2015/03/18 08:07:49, yhirano wrote:
> > On 2015/03/18 07:10:50, tyoshino wrote:
> > > https://streams.spec.whatwg.org/branch-snapshots/async-read-take-3/
> > > 
> > > Now it's specified to return a new promise.
> > 
> > Done.
> 
> Oh, sorry, I forgot that ScriptPromise.cast(promise) returns promise itself. I
> will fix it tomorrow.

Done.

[tyoshino (SeeGerritForStatus), 2015-03-19 07:54:52]

https://codereview.chromium.org/1004623007/diff/220001/Source/core/streams/Re...
File Source/core/streams/ReadableStreamReader.cpp (right):

https://codereview.chromium.org/1004623007/diff/220001/Source/core/streams/Re...
Source/core/streams/ReadableStreamReader.cpp:46: return
m_stream->cancelInternal(scriptState, reason);
don't you only want to skip lock status check? need state check?

https://codereview.chromium.org/1004623007/diff/220001/Source/core/streams/Re...
File Source/core/streams/ReadableStreamTest.cpp (right):

https://codereview.chromium.org/1004623007/diff/220001/Source/core/streams/Re...
Source/core/streams/ReadableStreamTest.cpp:441: String onFulfilled2,
onRejected2;
unused?

[yhirano, 2015-03-19 08:39:02]

https://codereview.chromium.org/1004623007/diff/220001/Source/core/streams/Re...
File Source/core/streams/ReadableStreamReader.cpp (right):

https://codereview.chromium.org/1004623007/diff/220001/Source/core/streams/Re...
Source/core/streams/ReadableStreamReader.cpp:46: return
m_stream->cancelInternal(scriptState, reason);
On 2015/03/19 07:54:52, tyoshino wrote:
> don't you only want to skip lock status check? need state check?

Stream's status must be Readable when locked.

https://codereview.chromium.org/1004623007/diff/220001/Source/core/streams/Re...
File Source/core/streams/ReadableStreamTest.cpp (right):

https://codereview.chromium.org/1004623007/diff/220001/Source/core/streams/Re...
Source/core/streams/ReadableStreamTest.cpp:441: String onFulfilled2,
onRejected2;
On 2015/03/19 07:54:52, tyoshino wrote:
> unused?

Thanks, done.

[tyoshino (SeeGerritForStatus), 2015-03-19 09:48:30]

https://codereview.chromium.org/1004623007/diff/220001/LayoutTests/http/tests...
File LayoutTests/http/tests/fetch/script-tests/fetch-body-mixin.js (right):

https://codereview.chromium.org/1004623007/diff/220001/LayoutTests/http/tests...
LayoutTests/http/tests/fetch/script-tests/fetch-body-mixin.js:131: return
Promise.resolve().then(function() {
is this necessary?

https://codereview.chromium.org/1004623007/diff/220001/LayoutTests/http/tests...
LayoutTests/http/tests/fetch/script-tests/fetch-body-mixin.js:152: return
response.text().then(unreached_rejection(test), function() {
hmm. how about adding unreached_fulfillment? it may emit confusing message in
the future as it's supposed to be used for rejection.

https://codereview.chromium.org/1004623007/diff/220001/LayoutTests/http/tests...
File LayoutTests/http/tests/fetch/script-tests/stream-reader.js (left):

https://codereview.chromium.org/1004623007/diff/220001/LayoutTests/http/tests...
LayoutTests/http/tests/fetch/script-tests/stream-reader.js:86: }, 'read contents
with ExclusiveStreamReader');
need to replace the reader name later.

https://codereview.chromium.org/1004623007/diff/220001/LayoutTests/http/tests...
File LayoutTests/http/tests/fetch/script-tests/stream-reader.js (right):

https://codereview.chromium.org/1004623007/diff/220001/LayoutTests/http/tests...
LayoutTests/http/tests/fetch/script-tests/stream-reader.js:10: return
reader.closed.then(function() {return chunks;});
is this "reader.closed.then(function() {" necessary?

https://codereview.chromium.org/1004623007/diff/220001/Source/core/streams/Re...
File Source/core/streams/ReadableStreamReader.cpp (right):

https://codereview.chromium.org/1004623007/diff/220001/Source/core/streams/Re...
Source/core/streams/ReadableStreamReader.cpp:46: return
m_stream->cancelInternal(scriptState, reason);
On 2015/03/19 08:39:02, yhirano wrote:
> On 2015/03/19 07:54:52, tyoshino wrote:
> > don't you only want to skip lock status check? need state check?
> 
> Stream's status must be Readable when locked.

Oops, right.

[yhirano, 2015-03-19 11:00:30]

https://codereview.chromium.org/1004623007/diff/220001/LayoutTests/http/tests...
File LayoutTests/http/tests/fetch/script-tests/fetch-body-mixin.js (right):

https://codereview.chromium.org/1004623007/diff/220001/LayoutTests/http/tests...
LayoutTests/http/tests/fetch/script-tests/fetch-body-mixin.js:131: return
Promise.resolve().then(function() {
On 2015/03/19 09:48:30, tyoshino wrote:
> is this necessary?

Just to catch exceptions.

https://codereview.chromium.org/1004623007/diff/220001/LayoutTests/http/tests...
LayoutTests/http/tests/fetch/script-tests/fetch-body-mixin.js:152: return
response.text().then(unreached_rejection(test), function() {
On 2015/03/19 09:48:30, tyoshino wrote:
> hmm. how about adding unreached_fulfillment? it may emit confusing message in
> the future as it's supposed to be used for rejection.

Done.

https://codereview.chromium.org/1004623007/diff/220001/LayoutTests/http/tests...
File LayoutTests/http/tests/fetch/script-tests/stream-reader.js (left):

https://codereview.chromium.org/1004623007/diff/220001/LayoutTests/http/tests...
LayoutTests/http/tests/fetch/script-tests/stream-reader.js:86: }, 'read contents
with ExclusiveStreamReader');
On 2015/03/19 09:48:30, tyoshino wrote:
> need to replace the reader name later.

This should have been fixed in the last CL... I would fix it now.

https://codereview.chromium.org/1004623007/diff/220001/LayoutTests/http/tests...
File LayoutTests/http/tests/fetch/script-tests/stream-reader.js (right):

https://codereview.chromium.org/1004623007/diff/220001/LayoutTests/http/tests...
LayoutTests/http/tests/fetch/script-tests/stream-reader.js:10: return
reader.closed.then(function() {return chunks;});
On 2015/03/19 09:48:30, tyoshino wrote:
> is this "reader.closed.then(function() {" necessary?

You're right. Done.

[tyoshino (SeeGerritForStatus), 2015-03-19 12:18:09]

lgtm

https://codereview.chromium.org/1004623007/diff/220001/LayoutTests/http/tests...
File LayoutTests/http/tests/fetch/script-tests/fetch-body-mixin.js (right):

https://codereview.chromium.org/1004623007/diff/220001/LayoutTests/http/tests...
LayoutTests/http/tests/fetch/script-tests/fetch-body-mixin.js:131: return
Promise.resolve().then(function() {
On 2015/03/19 11:00:30, yhirano wrote:
> On 2015/03/19 09:48:30, tyoshino wrote:
> > is this necessary?
> 
> Just to catch exceptions.

Ah, yeah.

[yhirano, 2015-03-19 12:46:05]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2015-03-19 12:46:49]

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

[commit-bot: I haz the power, 2015-03-19 13:05:18]

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

[commit-bot: I haz the power, 2015-03-19 13:05:19]

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

[yhirano, 2015-03-20 00:36:56]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2015-03-20 00:36:57]

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

[commit-bot: I haz the power, 2015-03-20 00:37:37]

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

[commit-bot: I haz the power, 2015-03-20 02:08:04]

Committed patchset #11 (id:280001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192220