Bug 151588 - [Streams API] Implement pipeTo method in readable Stream
Summary: [Streams API] Implement pipeTo method in readable Stream
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xabier Rodríguez Calvar
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-11-24 04:11 PST by Xabier Rodríguez Calvar
Modified: 2015-11-24 10:47 PST (History)
7 users (show)

See Also:


Attachments
Patch (36.71 KB, patch)
2015-11-24 04:31 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-yosemite (769.18 KB, application/zip)
2015-11-24 05:16 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (782.04 KB, application/zip)
2015-11-24 05:19 PST, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-yosemite (781.36 KB, application/zip)
2015-11-24 05:24 PST, Build Bot
no flags Details
Patch (38.62 KB, patch)
2015-11-24 07:55 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (38.89 KB, patch)
2015-11-24 09:17 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch for landing (38.87 KB, patch)
2015-11-24 09:50 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xabier Rodríguez Calvar 2015-11-24 04:11:09 PST
[Streams API] Implement pipeTo method in readable Stream
Comment 1 Xabier Rodríguez Calvar 2015-11-24 04:31:28 PST
Created attachment 266129 [details]
Patch
Comment 2 Build Bot 2015-11-24 05:16:51 PST
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
Comment 3 Build Bot 2015-11-24 05:16:53 PST
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
Comment 4 Build Bot 2015-11-24 05:19:25 PST
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
Comment 5 Build Bot 2015-11-24 05:19:27 PST
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
Comment 6 Build Bot 2015-11-24 05:24:31 PST
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
Comment 7 Build Bot 2015-11-24 05:24:33 PST
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
Comment 8 Xabier Rodríguez Calvar 2015-11-24 07:55:40 PST
Created attachment 266136 [details]
Patch
Comment 9 Xabier Rodríguez Calvar 2015-11-24 07:56:39 PST
(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.
Comment 10 youenn fablet 2015-11-24 08:04:23 PST
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?
Comment 11 Darin Adler 2015-11-24 09:06:05 PST
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.
Comment 12 Xabier Rodríguez Calvar 2015-11-24 09:17:38 PST
Created attachment 266137 [details]
Patch
Comment 13 youenn fablet 2015-11-24 09:18:37 PST
> 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.
Comment 14 Xabier Rodríguez Calvar 2015-11-24 09:35:16 PST
(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?
Comment 15 Xabier Rodríguez Calvar 2015-11-24 09:48:37 PST
(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.
Comment 16 Xabier Rodríguez Calvar 2015-11-24 09:50:00 PST
Created attachment 266138 [details]
Patch for landing
Comment 17 WebKit Commit Bot 2015-11-24 10:47:27 PST
Comment on attachment 266138 [details]
Patch for landing

Clearing flags on attachment: 266138

Committed r192765: <http://trac.webkit.org/changeset/192765>
Comment 18 WebKit Commit Bot 2015-11-24 10:47:31 PST
All reviewed patches have been landed.  Closing bug.