Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prohibit setting a strategy size for byte streams #856

Merged
merged 2 commits into from
Nov 17, 2017

Conversation

ricea
Copy link
Collaborator

@ricea ricea commented Nov 16, 2017

Readable byte streams always measure chunks in bytes, so specifying a size()
function in the strategy never did anything. This is confusing, so throw an
exception when size() is specified along with type: 'bytes'.

Update editorial descriptions to match the new behaviour.

Tests are in web-platform-tests/wpt#8279.

Fixes #729.


Preview | Diff

Readable byte streams always measure chunks in bytes, so specifying a size()
function in the strategy never did anything. This is confusing, so throw an
exception when size() is specified along with type: 'bytes'.

Update editorial descriptions to match the new behaviour.

Fixes whatwg#792.
@ricea ricea requested a review from domenic November 16, 2017 14:50
@ricea
Copy link
Collaborator Author

ricea commented Nov 16, 2017

@domenic The algorithmic changes are trivial, but the editorial parts are quite tricky.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Remember to do the WPT dance :)

<code>size()</code> method. <!-- TODO: https://github.com/whatwg/streams/issues/427 -->
Concretely, a queuing strategy is given by any JavaScript object with a <code>highWaterMark</code> property. For byte
streams the <code>highWaterMark</code> always has units of bytes. For other streams the default unit is <a>chunks</a>,
but a <code>size()</code> function can be included in the strategy object which returns the size for a given chunk. This
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good correction, from method to function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't actually intentional: I delete the text and rewrote it :-)

@ricea ricea merged commit ce56450 into whatwg:master Nov 17, 2017
@ricea ricea deleted the byte-streams-no-size-function branch November 17, 2017 04:20
hubot pushed a commit to WebKit/WebKit-http that referenced this pull request Dec 12, 2017
…eating a readable byte stream

https://bugs.webkit.org/show_bug.cgi?id=180470

Reviewed by Youenn Fablet.

LayoutTests/imported/w3c:

One new test imported from WPT to check that RangeError is thrown if a size is provided.

* web-platform-tests/streams/readable-byte-streams/general-expected.txt: Updated expectations.
* web-platform-tests/streams/readable-byte-streams/general.dedicatedworker-expected.txt: Updated expectations.
* web-platform-tests/streams/readable-byte-streams/general.js: Imported one test case from WPT.
* web-platform-tests/streams/readable-byte-streams/general.serviceworker.https-expected.txt: Updated expectations.

Source/WebCore:

Throw a RangeError if a ReadableStream is created with type 'bytes' and with a
non-undefined strategy size, as per latest spec:
- whatwg/streams#856
- https://streams.spec.whatwg.org/#rs-constructor (step 4.c)

One new test imported from WPT to check that RangeError is thrown.

* Modules/streams/ReadableStream.js:
(initializeReadableStream): Check strategy size and throw RangeError if needed.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@225784 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants