[Streams API] In RS during enqueuing error should be reported only if readable

[Xabier Rodríguez Calvar]

[Streams API] In RS during enqueuing error should be reported only if readable

[Xabier Rodríguez Calvar]

Created attachment 267787 [details]
Patch

[youenn fablet]

Looks ok.
I wonder why the patch does not contain any test modification.
What about the tests added in https://github.com/whatwg/streams/commit/2e7f87e45806d58114592c923aed299798d10161?

[Xabier Rodríguez Calvar]

(In reply to comment #2)
> Looks ok.
> I wonder why the patch does not contain any test modification.
> What about the tests added in
> https://github.com/whatwg/streams/commit/
> 2e7f87e45806d58114592c923aed299798d10161?

You're right, I saw them at that moment and I forget about them later.

[Xabier Rodríguez Calvar]

Created attachment 267836 [details]
Patch

Added tests

[youenn fablet]

Comment on attachment 267836 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/streams-api/readable-streams/bad-strategies.js:46
> +}, 'Readable stream: strategy.size errors the stream and then throws');

Are we not already passing this test without ReadableStreamInternals.js tests?

> LayoutTests/imported/w3c/web-platform-tests/streams-api/readable-streams/bad-strategies.js:71
> +    assert_equals(error.name, 'RangeError', 'enqueue should throw a RangeError');

Are we not passing this test without the patch?
The checks of this test may not be sufficient enough.
It seems to me we should check that rs.getReader().closed be equal to theError.

[Xabier Rodríguez Calvar]

(In reply to comment #5)
> > LayoutTests/imported/w3c/web-platform-tests/streams-api/readable-streams/bad-strategies.js:46
> > +}, 'Readable stream: strategy.size errors the stream and then throws');
> 
> Are we not already passing this test without ReadableStreamInternals.js
> tests?

> > LayoutTests/imported/w3c/web-platform-tests/streams-api/readable-streams/bad-strategies.js:71
> > +    assert_equals(error.name, 'RangeError', 'enqueue should throw a RangeError');

Both fail in debug because of the assert, but not in release.

[youenn fablet]

Comment on attachment 267836 [details]
Patch

r=me.
It would be good to improve the second test: making it a promise test, expecting the error to be theError and not a RangeError, either here or as a pull request to whatwg streams github.

[Xabier Rodríguez Calvar]

(In reply to comment #7)
> Comment on attachment 267836 [details]
> Patch
> 
> r=me.

Landing.

> It would be good to improve the second test: making it a promise test,
> expecting the error to be theError and not a RangeError, either here or as a
> pull request to whatwg streams github.

I'll do it GitHub.

[Xabier Rodríguez Calvar]

(In reply to comment #8)
> > It would be good to improve the second test: making it a promise test,
> > expecting the error to be theError and not a RangeError, either here or as a
> > pull request to whatwg streams github.
> 
> I'll do it GitHub.

https://github.com/whatwg/streams/pull/420

[WebKit Commit Bot]

Comment on attachment 267836 [details]
Patch

Clearing flags on attachment: 267836

Committed r194391: <http://trac.webkit.org/changeset/194391>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.