Following replacement of ReadableStreamReader by ReadableStreamDefaultReader, align getReader() with spec
Created attachment 285232 [details] Patch
Comment on attachment 285232 [details] Patch Attachment 285232 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1805027 New failing tests: imported/w3c/web-platform-tests/streams/readable-streams/templated.https.html imported/w3c/web-platform-tests/streams/readable-streams/general.https.html
Created attachment 285235 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 285232 [details] Patch Attachment 285232 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1805029 New failing tests: imported/w3c/web-platform-tests/streams/readable-streams/templated.https.html imported/w3c/web-platform-tests/streams/readable-streams/general.https.html
Created attachment 285236 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.5
Comment on attachment 285232 [details] Patch Attachment 285232 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1805030 New failing tests: imported/w3c/web-platform-tests/streams/readable-streams/templated.https.html imported/w3c/web-platform-tests/streams/readable-streams/general.https.html
Created attachment 285237 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 285238 [details] Patch
Created attachment 285243 [details] Patch
Fixed failing tests and replaced TODO by FIXME with proper formatting.
Comment on attachment 285243 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285243&action=review > Source/WebCore/Modules/streams/ReadableStream.js:82 > +function getReader(options) My recommendation is to try to us parameter destructuring as it is specified at the spec. It would mean to have it like function getReader({ mode } = {}) . Apparently this should be supported already, though I am not completely sure if that works in builtins (I remember some case of some ES6 feature that was supported but for some reason didn't work at the builtins). Let's hope this fixes the problem with the parameter of the test later. > Source/WebCore/Modules/streams/ReadableStream.js:93 > + // FIXME: update once ReadableByteStreamContoller and ReadableStreamBYOBReader are implemented. // FIXME: Update... > LayoutTests/imported/w3c/web-platform-tests/streams/readable-streams/general.js:65 > - assert_equals(rs.getReader.length, 0, 'getReader should have no parameters'); > + assert_equals(rs.getReader.length, 1, 'getReader should have no parameters'); This is something we can't do. The w3c test specifies a 0 here. If this is a bug in the test, that might perfectly be, let's update the expectations, create a follow up bug in WK, talk to the specs people about this, fix the test if needed and then port it back. > LayoutTests/streams/readable-stream-getReader.html:9 > + var rs = new ReadableStream(); Please, use const and let instead of var throughout the code. > LayoutTests/streams/readable-stream-getReader.html:21 > + // Any value different from undefined and 'byob' should throw a TypeError Period at the end.
Created attachment 285333 [details] Patch
(In reply to comment #11) > Comment on attachment 285243 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=285243&action=review > > > Source/WebCore/Modules/streams/ReadableStream.js:82 > > +function getReader(options) > > My recommendation is to try to us parameter destructuring as it is specified > at the spec. It would mean to have it like function getReader({ mode } = {}) > . Apparently this should be supported already, though I am not completely > sure if that works in builtins (I remember some case of some ES6 feature > that was supported but for some reason didn't work at the builtins). Let's > hope this fixes the problem with the parameter of the test later. I tried that initially, and tried again after your comment, but I couldn't build with destructuring. Therefore, the updated patch still relies on an "options" parameter. > > LayoutTests/imported/w3c/web-platform-tests/streams/readable-streams/general.js:65 > > - assert_equals(rs.getReader.length, 0, 'getReader should have no parameters'); > > + assert_equals(rs.getReader.length, 1, 'getReader should have no parameters'); > > This is something we can't do. The w3c test specifies a 0 here. If this is a > bug in the test, that might perfectly be, let's update the expectations, > create a follow up bug in WK, talk to the specs people about this, fix the > test if needed and then port it back. I see. Updated patch therefore no more modifies the test itself, but only the expectation.
Comment on attachment 285333 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285333&action=review I think patch is mostly ok now, only a couple of cosmetic issues that can be fixed in landing time or in follow ups. > Source/WebCore/Modules/streams/ReadableStream.js:88 > + if (options === @undefined) > + options = { }; > + You can do this after checking if "this" @isReadableStream. > LayoutTests/imported/w3c/web-platform-tests/streams/readable-streams/templated.https-expected.txt:4 > -FAIL calling getReader with invalid arguments should throw appropriate errors assert_throws: empty string mode should throw function "() => rs.getReader({ mode: '' })" did not throw > +PASS calling getReader with invalid arguments should throw appropriate errors This is ok as it is now but in addition to this I would create a follow up bug in WK and check if this is a spec error. Talk to Domenic, he's nice :) > LayoutTests/streams/readable-stream-getReader.html:6 > + You don't need two empty lines. > LayoutTests/streams/readable-stream-getReader.html:17 > + }, "getReader({mode: 'byob'}) should return a TypeError as BYOB reader is not yet implemented"); Fix indentation > LayoutTests/streams/readable-stream-getReader.html:21 > + // Any value different from undefined and 'byob' should throw a TypeError. For some reason the review tool does not show me after line 21 so I have a couple of comments: Line 23: fix indentation as for 17. Line 24: this is an extra line I think we don't need.
Created attachment 287507 [details] Patch
(In reply to comment #14) > Comment on attachment 285333 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=285333&action=review > > I think patch is mostly ok now, only a couple of cosmetic issues that can be > fixed in landing time or in follow ups. Thanks, I fixed those issues in new patch. > > LayoutTests/imported/w3c/web-platform-tests/streams/readable-streams/templated.https-expected.txt:4 > > -FAIL calling getReader with invalid arguments should throw appropriate errors assert_throws: empty string mode should throw function "() => rs.getReader({ mode: '' })" did not throw > > +PASS calling getReader with invalid arguments should throw appropriate errors > > This is ok as it is now but in addition to this I would create a follow up > bug in WK and check if this is a spec error. Talk to Domenic, he's nice :) OK I'll do that then :)
Comment on attachment 287507 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287507&action=review Sorry that I missed these two nits :( . I guess you can't land if you are not committer so you'll have to upload it again :( > Source/WebCore/Modules/streams/ReadableStream.js:94 > + throw new @TypeError("ReadableStreamBYOBReader is not implemented") Add ; > Source/WebCore/Modules/streams/ReadableStream.js:100 > + throw new @RangeError("Invalid mode is specified") ;
Created attachment 287509 [details] Patch
(In reply to comment #17) > Comment on attachment 287507 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=287507&action=review > > Sorry that I missed these two nits :( . I guess you can't land if you are > not committer so you'll have to upload it again :( My bad, I should have added them. I hope everything is ok now :) > > Source/WebCore/Modules/streams/ReadableStream.js:94 > > + throw new @TypeError("ReadableStreamBYOBReader is not implemented") > > Add ; > > > Source/WebCore/Modules/streams/ReadableStream.js:100 > > + throw new @RangeError("Invalid mode is specified") > > ;
Comment on attachment 287509 [details] Patch Clearing flags on attachment: 287509 Committed r205248: <http://trac.webkit.org/changeset/205248>
All reviewed patches have been landed. Closing bug.