Bug 171665 – [Readable Streams API] Enable creation of ReadableStreamBYOBReader

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Romain Bellessort]

Streams API defines that in addition to ReadableStreamDefaulReader, ReadableStreamBYOBReader may also be used. The first step toward this should be to enable the creation of a ReadableStreamBYOBReader.

[Romain Bellessort]

Created attachment 309062 [details]
Patch

[Build Bot]

Comment on attachment 309062 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/streams/readable-byte-streams/general.dedicatedworker.html

[Build Bot]

Created attachment 309068 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

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

[Build Bot]

Comment on attachment 309062 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/streams/readable-byte-streams/general.dedicatedworker.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 309062 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/streams/readable-byte-streams/general.dedicatedworker.html

[Build Bot]

Created attachment 309078 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

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

[Build Bot]

Comment on attachment 309062 [details]
Patch

Attachment 309062 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3673709

New failing tests:
imported/w3c/web-platform-tests/streams/readable-byte-streams/general.html
imported/w3c/web-platform-tests/streams/readable-byte-streams/general.dedicatedworker.html

[Build Bot]

Created attachment 309092 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Romain Bellessort]

Created attachment 309155 [details]
Patch

[Romain Bellessort]

Comment on attachment 309155 [details]
Patch

New patch updates some WPT test expectations that had not been updated in previous version of the patch.

[youenn fablet]

Comment on attachment 309155 [details]
Patch

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

> Source/WebCore/Modules/streams/ReadableStream.js:100
> +    if (mode === 'byob')

Looking at the spec, this looks fine.
Is there a difference between these two lines and "if (mode == 'byob')".

> Source/WebCore/Modules/streams/ReadableStreamBYOBReader.idl:16
> + *     from this software without specific prior written permission.

This is the old license template. The current one is a two steps list license.
Might want to use https://trac.webkit.org/browser/trunk/Source/WebKit/LICENSE instead.

> Source/WebCore/bindings/js/JSReadableStreamPrivateConstructors.cpp:73
> +EncodedJSValue JSC_HOST_CALL constructJSReadableStreamBYOBReader(ExecState& exec)

This function is very similar to constructJSReadableStreamDefaultReader.
Can we share code?

> Source/WebCore/bindings/js/JSReadableStreamPrivateConstructors.cpp:77
> +    JSDOMGlobalObject& globalObject = *static_cast<JSDOMGlobalObject*>(exec.lexicalGlobalObject());

Might be good to modernize a bit constructJSReadableStreamDefaultReader and constructJSReadableStreamBYOBReader.
Can you use auto for these lines?
Also, JSC::jsCast is used in the current code base for global object casting.

[Romain Bellessort]

Created attachment 309494 [details]
Patch

[Romain Bellessort]

(In reply to youenn fablet from comment #12)
> Comment on attachment 309155 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309155&action=review
> 
> > Source/WebCore/Modules/streams/ReadableStream.js:100
> > +    if (mode === 'byob')
> 
> Looking at the spec, this looks fine.
> Is there a difference between these two lines and "if (mode == 'byob')".
I don't think there's a difference, so I changed for your proposed version (with a comment to clearly indicate why a double equals is used).

> > Source/WebCore/Modules/streams/ReadableStreamBYOBReader.idl:16
> > + *     from this software without specific prior written permission.
> 
> This is the old license template. The current one is a two steps list
> license.
> Might want to use
> https://trac.webkit.org/browser/trunk/Source/WebKit/LICENSE instead.
Thanks, I've fixed this in the new patch.

> > Source/WebCore/bindings/js/JSReadableStreamPrivateConstructors.cpp:73
> > +EncodedJSValue JSC_HOST_CALL constructJSReadableStreamBYOBReader(ExecState& exec)
> 
> This function is very similar to constructJSReadableStreamDefaultReader.
> Can we share code?
Code is now shared through a template. If another approach seems better, please tell me.

> > Source/WebCore/bindings/js/JSReadableStreamPrivateConstructors.cpp:77
> > +    JSDOMGlobalObject& globalObject = *static_cast<JSDOMGlobalObject*>(exec.lexicalGlobalObject());
> 
> Might be good to modernize a bit constructJSReadableStreamDefaultReader and
> constructJSReadableStreamBYOBReader.
> Can you use auto for these lines?
> Also, JSC::jsCast is used in the current code base for global object casting.
Fixed, thanks.

[youenn fablet]

Comment on attachment 309494 [details]
Patch

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

> Source/WebCore/Modules/streams/ReadableStreamBYOBReader.idl:3
> + * Copyright (C) 2015 Igalia S.L.

Igalia?

[Romain Bellessort]

(In reply to youenn fablet from comment #15)
> Comment on attachment 309494 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309494&action=review
> 
> > Source/WebCore/Modules/streams/ReadableStreamBYOBReader.idl:3
> > + * Copyright (C) 2015 Igalia S.L.
> 
> Igalia?
I kept Igalia as I did not start this file from scratch but form a copy of ReadableStreamDefaultReader.idl, which was coauthored by Igalia.

[WebKit Commit Bot]

Comment on attachment 309494 [details]
Patch

Clearing flags on attachment: 309494

Committed r216513: <http://trac.webkit.org/changeset/216513>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.