Bug 171665 - [Readable Streams API] Enable creation of ReadableStreamBYOBReader
Summary: [Readable Streams API] Enable creation of ReadableStreamBYOBReader
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Romain Bellessort
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-04 09:17 PDT by Romain Bellessort
Modified: 2017-05-09 10:15 PDT (History)
4 users (show)

See Also:


Attachments
Patch (56.05 KB, patch)
2017-05-04 10:33 PDT, Romain Bellessort
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (935.89 KB, application/zip)
2017-05-04 11:33 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.10 MB, application/zip)
2017-05-04 11:38 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-elcapitan (1.53 MB, application/zip)
2017-05-04 12:03 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (7.92 MB, application/zip)
2017-05-04 12:50 PDT, Build Bot
no flags Details
Patch (70.60 KB, patch)
2017-05-05 03:37 PDT, Romain Bellessort
no flags Details | Formatted Diff | Diff
Patch (71.40 KB, patch)
2017-05-09 08:03 PDT, Romain Bellessort
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Romain Bellessort 2017-05-04 09:17:01 PDT
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.
Comment 1 Romain Bellessort 2017-05-04 10:33:07 PDT
Created attachment 309062 [details]
Patch
Comment 2 Build Bot 2017-05-04 11:33:39 PDT
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
Comment 3 Build Bot 2017-05-04 11:33:41 PDT
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
Comment 4 Build Bot 2017-05-04 11:38:20 PDT
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
Comment 5 Build Bot 2017-05-04 11:38:21 PDT
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
Comment 6 Build Bot 2017-05-04 12:03:32 PDT
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
Comment 7 Build Bot 2017-05-04 12:03:33 PDT
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
Comment 8 Build Bot 2017-05-04 12:50:39 PDT
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
Comment 9 Build Bot 2017-05-04 12:50:41 PDT
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
Comment 10 Romain Bellessort 2017-05-05 03:37:05 PDT
Created attachment 309155 [details]
Patch
Comment 11 Romain Bellessort 2017-05-05 06:28:07 PDT
Comment on attachment 309155 [details]
Patch

New patch updates some WPT test expectations that had not been updated in previous version of the patch.
Comment 12 youenn fablet 2017-05-05 08:13:10 PDT
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.
Comment 13 Romain Bellessort 2017-05-09 08:03:39 PDT
Created attachment 309494 [details]
Patch
Comment 14 Romain Bellessort 2017-05-09 09:03:16 PDT
(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.
Comment 15 youenn fablet 2017-05-09 09:42:58 PDT
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?
Comment 16 Romain Bellessort 2017-05-09 09:45:21 PDT
(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.
Comment 17 WebKit Commit Bot 2017-05-09 10:15:43 PDT
Comment on attachment 309494 [details]
Patch

Clearing flags on attachment: 309494

Committed r216513: <http://trac.webkit.org/changeset/216513>
Comment 18 WebKit Commit Bot 2017-05-09 10:15:45 PDT
All reviewed patches have been landed.  Closing bug.