Bug 180470 – [Readable Streams API] Throw RangeError if a size is provided when creating a readable byte stream

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

[Romain Bellessort]

When creating a ReadableStream with type 'bytes', no strategy size should be provided. As per updated spec (see https://github.com/whatwg/streams/pull/856), a RangeError should be thrown if size is not undefined.

[Romain Bellessort]

Created attachment 328564 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 328564 [details]
Patch

Code is ok, test is redudant as it commit da06ef1c8668bd121a461405361fc7f4615b6cd6 of WPT shows at https://github.com/w3c/web-platform-tests.git. So you want to import that test instead of creating a new one.

[Romain Bellessort]

(In reply to Xabier Rodríguez Calvar from comment #2)
> Comment on attachment 328564 [details]
> Patch
> 
> Code is ok, test is redudant as it commit
> da06ef1c8668bd121a461405361fc7f4615b6cd6 of WPT shows at
> https://github.com/w3c/web-platform-tests.git. So you want to import that
> test instead of creating a new one.

WPT test targets the same behavior, but it requires other changes from the latest spec (e.g. CountQueuingStrategy and ByteLengthQueuingStrategy) which are not implemented in current WebKit implementation. Maybe I could add a comment to the test I added to indicate that it should be removed once the implementation supports CountQueuingStrategy and ByteLengthQueuingStrategy?

[youenn fablet]

> WPT test targets the same behavior, but it requires other changes from the
> latest spec (e.g. CountQueuingStrategy and ByteLengthQueuingStrategy) which
> are not implemented in current WebKit implementation. Maybe I could add a
> comment to the test I added to indicate that it should be removed once the
> implementation supports CountQueuingStrategy and ByteLengthQueuingStrategy?

Does this mean that reimporting streams API will regress somehow our coverage?
We resent regularly so this will happen anyhow.

I like the simplicity of the test you are adding.
We could land that patch and upstream the test you are adding to WPT. Once upstreamed, we would need to remove the WebKit-specific version.

[Romain Bellessort]

(In reply to youenn fablet from comment #4)
> > WPT test targets the same behavior, but it requires other changes from the
> > latest spec (e.g. CountQueuingStrategy and ByteLengthQueuingStrategy) which
> > are not implemented in current WebKit implementation. Maybe I could add a
> > comment to the test I added to indicate that it should be removed once the
> > implementation supports CountQueuingStrategy and ByteLengthQueuingStrategy?
> 
> Does this mean that reimporting streams API will regress somehow our
> coverage?
> We resent regularly so this will happen anyhow.
> 
> I like the simplicity of the test you are adding.
> We could land that patch and upstream the test you are adding to WPT. Once
> upstreamed, we would need to remove the WebKit-specific version.

I haven't checked how many tests will be impacted, but yes, coverage will regress. Corresponding WPT test already comprises a very similar test (see https://github.com/w3c/web-platform-tests/commit/da06ef1c8668bd121a461405361fc7f4615b6cd6 , line 1968), but it also adds other tests as the queuing strategy may now be an instance of CountQueuingStrategy or ByteLengthQueuingStrategy. The corresponding test will fail as all tests are gathered in the same test case. An option may be to split this test case so that only CountQueuingStrategy and ByteLengthQueuingStrategy tests fail.

[Romain Bellessort]

In fact I was wrong, CountQueuingStrategy and ByteLengthQueuingStrategy are already implemented, so the test will pass, sorry for the misunderstanding. 

I will prepare a new patch with the imported test. Is it ok if I just copy the new test in the corresponding file, or should I replace the whole file?

[Romain Bellessort]

Created attachment 329097 [details]
Patch

[Romain Bellessort]

This patch imports only the corresponding test from WPT. If this is not the right approach please tell me.

[Xabier Rodríguez Calvar]

Comment on attachment 329097 [details]
Patch

Lgtm, but let's wait for Youenn's opinion before landing.

[EWS Watchlist]

Comment on attachment 329097 [details]
Patch

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

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

[EWS Watchlist]

Created attachment 329099 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

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

[EWS Watchlist]

Comment on attachment 329097 [details]
Patch

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

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

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 329097 [details]
Patch

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

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

[EWS Watchlist]

Created attachment 329102 [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

[EWS Watchlist]

Comment on attachment 329097 [details]
Patch

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

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

[EWS Watchlist]

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

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

[Romain Bellessort]

Created attachment 329109 [details]
Patch

[Romain Bellessort]

I had forgotten to update some expectations. The new patch fixes that.

[youenn fablet]

Comment on attachment 329109 [details]
Patch

LGTM too

[WebKit Commit Bot]

Comment on attachment 329109 [details]
Patch

Clearing flags on attachment: 329109

Committed r225784: <https://trac.webkit.org/changeset/225784>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35995043>