[webkit-reviews] review granted: [Bug 215267] Refresh WritableStream up to spec : [Attachment 406290] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted youenn fablet
<youennf at gmail.com>'s request for review:
Bug 215267: Refresh WritableStream up to spec
https://bugs.webkit.org/show_bug.cgi?id=215267

Attachment 406290: Patch

https://bugs.webkit.org/attachment.cgi?id=406290&action=review




--- Comment #14 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 406290
  --> https://bugs.webkit.org/attachment.cgi?id=406290
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406290&action=review

r=me

> Source/WebCore/Modules/streams/WritableStream.js:59
> +    // FIXME: underlyingSink, underlyingSink

Not sure what this FIXME means

> Source/WebCore/Modules/streams/WritableStreamInternals.js:170
> +    for (let index = 0, length = requests.length; index < length; ++index)
> +	   requests[index]. at reject.@call(@undefined, storedError);

Is it intentional that we'll get an exception if writeRequests shrinks during
this iteration?

>
LayoutTests/streams/reference-implementation/readable-stream-templated-expected
.txt:22
> -PASS ReadableStream (two chunks enqueued, then closed): piping with no
options and no destination errors 
> -PASS ReadableStream (two chunks enqueued, then closed): piping with {
preventClose: false } and no destination errors 
> -PASS ReadableStream (two chunks enqueued, then closed): piping with {
preventClose: true } and no destination errors 
> -PASS ReadableStream (two chunks enqueued, then closed): piping with {
preventClose: false } and a destination with that errors synchronously 
> -PASS ReadableStream (two chunks enqueued, then closed): piping with {
preventClose: true } and a destination with that errors synchronously 
> -PASS ReadableStream (two chunks enqueued, then closed): piping with {
preventClose: true } and a destination that errors on the last chunk 
> +FAIL ReadableStream (two chunks enqueued, then closed): piping with no
options and no destination errors |this| is not a Promise
> +FAIL ReadableStream (two chunks enqueued, then closed): piping with {
preventClose: false } and no destination errors |this| is not a Promise
> +FAIL ReadableStream (two chunks enqueued, then closed): piping with {
preventClose: true } and no destination errors |this| is not a Promise
> +FAIL ReadableStream (two chunks enqueued, then closed): piping with {
preventClose: false } and a destination with that errors synchronously |this|
is not a Promise
> +FAIL ReadableStream (two chunks enqueued, then closed): piping with {
preventClose: true } and a destination with that errors synchronously |this| is
not a Promise
> +FAIL ReadableStream (two chunks enqueued, then closed): piping with {
preventClose: true } and a destination that errors on the last chunk |this| is
not a Promise

What's up with these newly introduced test failures? Not catastrophic, since
this is off by default -- but seems like a significant bug. Can we make 'this'
a Promise? Same comment for the other test failures.