Bug 160508 – [Streams API] Align getReader() with spec

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Romain Bellessort]

Following replacement of ReadableStreamReader by ReadableStreamDefaultReader, align getReader() with spec

[Romain Bellessort]

Created attachment 285232 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Romain Bellessort]

Created attachment 285238 [details]
Patch

[Romain Bellessort]

Created attachment 285243 [details]
Patch

[Romain Bellessort]

Fixed failing tests and replaced TODO by FIXME with proper formatting.

[Xabier Rodríguez Calvar]

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.

[Romain Bellessort]

Created attachment 285333 [details]
Patch

[Romain Bellessort]

(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.

[Xabier Rodríguez Calvar]

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.

[Romain Bellessort]

Created attachment 287507 [details]
Patch

[Romain Bellessort]

(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 :)

[Xabier Rodríguez Calvar]

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")

;

[Romain Bellessort]

Created attachment 287509 [details]
Patch

[Romain Bellessort]

(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")
> 
> ;

[WebKit Commit Bot]

Comment on attachment 287509 [details]
Patch

Clearing flags on attachment: 287509

Committed r205248: <http://trac.webkit.org/changeset/205248>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.