[Streams API] Implement pipeTo method in readable Stream
Created attachment 266129 [details] Patch
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
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 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
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 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
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
Created attachment 266136 [details] Patch
(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 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 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.
Created attachment 266137 [details] Patch
> 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.
(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?
(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.
Created attachment 266138 [details] Patch for landing
Comment on attachment 266138 [details] Patch for landing Clearing flags on attachment: 266138 Committed r192765: <http://trac.webkit.org/changeset/192765>
All reviewed patches have been landed. Closing bug.