Bug 164319 – [Readable Streams API] Implement ByteStreamController error()

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Romain Bellessort]

Implement error() method of ReadableByteStreamController

[Romain Bellessort]

Created attachment 293661 [details]
Patch

[youenn fablet]

Comment on attachment 293661 [details]
Patch

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

> Source/WebCore/Modules/streams/ReadableByteStreamController.js:41
> +        @makeThisTypeError("ReadableByteStreamController", "error");

Should we add a test for this one as well?
Maybe it can be covered by WebIDL based tests (see wpt for more info).

> Source/WebCore/Modules/streams/ReadableByteStreamController.js:43
> +    const stream = this.@controlledReadableStream;

No need to define stream since it is used only once.

> Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:87
> +    return @isObject(controller) && !!controller.@underlyingByteSource;

This is the same test mechanism as isReadableStreamController. We might not need to restate the same comment here, just forward to ReadableStreamInternals.js.
Also maybe we should just do something like controller instanceof @ReadableStreamDefaultController, even though this departs from the spec.
It should be equivalent at least currently since all steams/controllers are built using these constructors.

> Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:94
> +    const stream = controller.@controlledReadableStream;

stream is used only once in an assert, just better to not define it.

> Source/WebCore/Modules/streams/ReadableStream.js:63
> +            @throwTypeError("ReadableByteStreamController could not be created: " + e.message);

Isn't there a risk that the try/catch error will change the error from range error to type error?
If so, shouldn't we do something like:
let readableByteStreamControllerConstructor;
try {
   readableByteStreamControllerConstructor = @ReadableByteStreamController;
} catch (e) {
 //not implemented error
}
this.@readableStreamController = new readableByteStreamControllerConstructor(this, underlyingSource, strategy.highWaterMark);

[Romain Bellessort]

Created attachment 293670 [details]
Patch

[Romain Bellessort]

Created attachment 293674 [details]
Patch

[Romain Bellessort]

(In reply to comment #2)
> Comment on attachment 293661 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=293661&action=review
> 
> > Source/WebCore/Modules/streams/ReadableByteStreamController.js:41
> > +        @makeThisTypeError("ReadableByteStreamController", "error");
> 
> Should we add a test for this one as well?
> Maybe it can be covered by WebIDL based tests (see wpt for more info).

I'm not sure how this could be tested. On the other hand, I was planning to add a test for the following test (line 43, check whether stream is in readable state) after implementing close().

> > Source/WebCore/Modules/streams/ReadableByteStreamController.js:43
> > +    const stream = this.@controlledReadableStream;
> 
> No need to define stream since it is used only once.

I removed it (and did the same in ReadableStreamDefaultController.js).

> > Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:87
> > +    return @isObject(controller) && !!controller.@underlyingByteSource;
> 
> This is the same test mechanism as isReadableStreamController. We might not
> need to restate the same comment here, just forward to
> ReadableStreamInternals.js.
> Also maybe we should just do something like controller instanceof
> @ReadableStreamDefaultController, even though this departs from the spec.
> It should be equivalent at least currently since all steams/controllers are
> built using these constructors.

I tried using "instanceof", but it results in a TypeError (instanceof called on an object with an invalid prototype property). Therefore I kept the same code, but modified the comment to point to isReadableStreamDefaultController.

> > Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:94
> > +    const stream = controller.@controlledReadableStream;
> 
> stream is used only once in an assert, just better to not define it.

Removed.

> > Source/WebCore/Modules/streams/ReadableStream.js:63
> > +            @throwTypeError("ReadableByteStreamController could not be created: " + e.message);
> 
> Isn't there a risk that the try/catch error will change the error from range
> error to type error?
> If so, shouldn't we do something like:
> let readableByteStreamControllerConstructor;
> try {
>    readableByteStreamControllerConstructor = @ReadableByteStreamController;
> } catch (e) {
>  //not implemented error
> }
> this.@readableStreamController = new
> readableByteStreamControllerConstructor(this, underlyingSource,
> strategy.highWaterMark);

Right, this is cleaner. In the new patch, I implemented something similar except that I define readableByteStreamControllerConstructor only inside the try statement (and still use “new @ReadableByteStreamController()” after the try/catch). In the end, it will be possible to remove the try/catch without modifying the rest of the code.

[youenn fablet]

(In reply to comment #5)
> (In reply to comment #2)
> > Comment on attachment 293661 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=293661&action=review
> > 
> > > Source/WebCore/Modules/streams/ReadableByteStreamController.js:41
> > > +        @makeThisTypeError("ReadableByteStreamController", "error");
> > 
> > Should we add a test for this one as well?
> > Maybe it can be covered by WebIDL based tests (see wpt for more info).
> 
> I'm not sure how this could be tested. On the other hand, I was planning to
> add a test for the following test (line 43, check whether stream is in
> readable state) after implementing close().

You can try to use https://developer.mozilla.org/fr/docs/Web/JavaScript/Reference/Objets_globaux/Function/apply with a this object of a different type and print the exception message or check at least that it is a TypeError.

Please add something like LayoutTests/imported/w3c/web-platform-tests/streams/readable-streams/readable-stream-reader.js or LayoutTests/imported/w3c/web-platform-tests/fetch/api/request/request-idl.html (WebIDL based)

[Romain Bellessort]

Created attachment 293875 [details]
Patch

[Romain Bellessort]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #2)
> > > Comment on attachment 293661 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=293661&action=review
> > > 
> > > > Source/WebCore/Modules/streams/ReadableByteStreamController.js:41
> > > > +        @makeThisTypeError("ReadableByteStreamController", "error");
> > > 
> > > Should we add a test for this one as well?
> > > Maybe it can be covered by WebIDL based tests (see wpt for more info).
> > 
> > I'm not sure how this could be tested. On the other hand, I was planning to
> > add a test for the following test (line 43, check whether stream is in
> > readable state) after implementing close().
> 
> You can try to use
> https://developer.mozilla.org/fr/docs/Web/JavaScript/Reference/
> Objets_globaux/Function/apply with a this object of a different type and
> print the exception message or check at least that it is a TypeError.

Thanks, I added such a test.

> Please add something like
> LayoutTests/imported/w3c/web-platform-tests/streams/readable-streams/
> readable-stream-reader.js or
> LayoutTests/imported/w3c/web-platform-tests/fetch/api/request/request-idl.
> html (WebIDL based)

I tried both approaches but had troubles with WebIDL-based tests. So I added corresponding tests in readable-byte-stream-controller.js. All behave as expected.

[WebKit Commit Bot]

Comment on attachment 293875 [details]
Patch

Clearing flags on attachment: 293875

Committed r208382: <http://trac.webkit.org/changeset/208382>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

Reverted r208382 for reason:

This change appears to have caused 3 SerializedCryptoKeyWrapTest API tests to fail on macOS.

Committed r208422: <http://trac.webkit.org/changeset/208422>

[Ryan Haddad]

(In reply to comment #11)
> Reverted r208382 for reason:
> 
> This change appears to have caused 3 SerializedCryptoKeyWrapTest API tests
> to fail on macOS.
> 
> Committed r208422: <http://trac.webkit.org/changeset/208422>

Link to failures:

https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/1231

[Ryan Haddad]

The API tests are still failing after the rollout. 

https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20%28Tests%29/builds/19305

I will roll the patch back in later today unless someone else beats me to it.

[Ryan Haddad]

Reverted r208422 for reason:

Roll r208382 back in since it was not responsible for the API test failures seen on macOS.

Committed r208434: <http://trac.webkit.org/changeset/208434>