Bug 151588 – [Streams API] Implement pipeTo method in readable Stream

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Xabier Rodríguez Calvar]

[Streams API] Implement pipeTo method in readable Stream

[Xabier Rodríguez Calvar]

Created attachment 266129 [details]
Patch

[Build Bot]

Comment on attachment 266129 [details]
Patch

Attachment 266129 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/471661

New failing tests:
streams/reference-implementation/pipe-to.html

[Build Bot]

Created attachment 266131 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 266129 [details]
Patch

Attachment 266129 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/471665

New failing tests:
streams/reference-implementation/pipe-to.html

[Build Bot]

Created attachment 266132 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 266129 [details]
Patch

Attachment 266129 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/471662

New failing tests:
streams/reference-implementation/pipe-to.html

[Build Bot]

Created attachment 266133 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Xabier Rodríguez Calvar]

Created attachment 266136 [details]
Patch

[Xabier Rodríguez Calvar]

(In reply to comment #2)
> New failing tests:
> streams/reference-implementation/pipe-to.html

The issue is again bug 147933 so I flagged the test.

[youenn fablet]

Comment on attachment 266136 [details]
Patch

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

Just a quick review. Seems ok to me.

> Source/WebCore/Modules/streams/ReadableStream.js:106
> +    const preventClose = @isObject(options) ? Boolean(options.preventClose) : false;

Why not using "!!" instead of Boolean?

> Source/WebCore/Modules/streams/ReadableStream.js:119
> +        lastRead = reader.read();

Maybe add a comment that it is ok there to call read() directly since reader might not be a ReadableStreamReader?
Or at the top of pipeTo?

> Source/WebCore/Modules/streams/ReadableStream.js:123
> +            else if (dest.state === "writable") {

I guess dest is also generic?
Maybe beef up the above comment with that also?

[Darin Adler]

Comment on attachment 266136 [details]
Patch

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

Would be nice to expect some more success in the tests. A little unclear on why they are marked flaky. On Windows and Mac.

> Source/WebCore/Modules/streams/ReadableStream.js:102
> +function pipeTo(dest, options)

Do we have to abbreviate destination as dest?

>> Source/WebCore/Modules/streams/ReadableStream.js:106
>> +    const preventClose = @isObject(options) ? Boolean(options.preventClose) : false;
> 
> Why not using "!!" instead of Boolean?

Also seems like this is a strange way of writing &&.

> Source/WebCore/Modules/streams/ReadableStream.js:137
> +        } else

If we followed our C++ coding style here we would use braces around the multi line else even though it is a single statement.

[Xabier Rodríguez Calvar]

Created attachment 266137 [details]
Patch

[youenn fablet]

> I guess dest is also generic?
> Maybe beef up the above comment with that also?

As per https://github.com/whatwg/streams/issues/407, dest is not supposed to be generic.
The reference implementation is just not up-to-date.
We probably want to revisit a bit our plan here.

[Xabier Rodríguez Calvar]

(In reply to comment #13)
> As per https://github.com/whatwg/streams/issues/407, dest is not supposed to
> be generic.
> The reference implementation is just not up-to-date.
> We probably want to revisit a bit our plan here.

Now or later?

[Xabier Rodríguez Calvar]

(In reply to comment #11)
> Would be nice to expect some more success in the tests. A little unclear on
> why they are marked flaky. On Windows and Mac.

The problem is bug 147933 that prevents test from working on Mac and Win. In GTK+ they work like a charm.

> > Source/WebCore/Modules/streams/ReadableStream.js:102
> > +function pipeTo(dest, options)
> 
> Do we have to abbreviate destination as dest?

I read your mind as I changed it before you mentioned it.

> >> Source/WebCore/Modules/streams/ReadableStream.js:106
> >> +    const preventClose = @isObject(options) ? Boolean(options.preventClose) : false;
> > 
> > Why not using "!!" instead of Boolean?
> 
> Also seems like this is a strange way of writing &&.
> 
> > Source/WebCore/Modules/streams/ReadableStream.js:137
> > +        } else
> 
> If we followed our C++ coding style here we would use braces around the
> multi line else even though it is a single statement.

Changed both.

I am ready to land if Youenn is pleased.

[Xabier Rodríguez Calvar]

Created attachment 266138 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 266138 [details]
Patch for landing

Clearing flags on attachment: 266138

Committed r192765: <http://trac.webkit.org/changeset/192765>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.