Bug 143363 – [Streams API] ReadableStream constructor start function should be able to close the stream

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

[youenn fablet]

Support for the close function passed as parameter to "start" and "pull" source functions should be implemented.
THis should end up putting the stream and reader in closed state and fulfilling the reader closed promise.

[youenn fablet]

Created attachment 250060 [details]
Patch

[youenn fablet]

Created attachment 250434 [details]
Patch

[Benjamin Poulain]

Comment on attachment 250434 [details]
Patch

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

Very quick review. Something is fishy. If that's right, please add tests for the controller.

Can you please add the type exception on errored? It is not testable yet, but it may be harder to forget if there is already if branch for it.

> Source/WebCore/Modules/streams/ReadableStream.cpp:61
> +void ReadableStream::changeStateToClosed()

How will you raise the TypeError when closeRequested == true or state == "errored"?

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:127
> +    getReadableJSStream(exec).changeStateToClosed();
> +    return JSValue::encode(jsUndefined());

Hum, reading the spec side by side with the code...I don't think that is quite correct.

Say I keep the function close in a variable close. I could do:
    close.call("foobar");
and that would still close the original stream.

The specs says to get the internal slot on the this object. If the this object is not a controller:
    If IsReadableStreamController(this) is false, throw a TypeError exception.

Note that this also let you do close.call(otherController) to close the controller of one object with the close function from an other object.

If I am reading this correctly, you need to have the internal slot on the controller object, and get it out through the this object.

[youenn fablet]

(In reply to comment #3)
> Comment on attachment 250434 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250434&action=review
> 
> Very quick review. Something is fishy. If that's right, please add tests for
> the controller.
> 
> Can you please add the type exception on errored? It is not testable yet,
> but it may be harder to forget if there is already if branch for it.
> 
> > Source/WebCore/Modules/streams/ReadableStream.cpp:61
> > +void ReadableStream::changeStateToClosed()
> 
> How will you raise the TypeError when closeRequested == true or state ==
> "errored"?
> 
> > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:127
> > +    getReadableJSStream(exec).changeStateToClosed();
> > +    return JSValue::encode(jsUndefined());
> 
> Hum, reading the spec side by side with the code...I don't think that is
> quite correct.
> 
> Say I keep the function close in a variable close. I could do:
>     close.call("foobar");
> and that would still close the original stream.
> 
> The specs says to get the internal slot on the this object. If the this
> object is not a controller:
>     If IsReadableStreamController(this) is false, throw a TypeError
> exception.
> 
> Note that this also let you do close.call(otherController) to close the
> controller of one object with the close function from an other object.
> 
> If I am reading this correctly, you need to have the internal slot on the
> controller object, and get it out through the this object.

You are reading correctly.
I was expecting to first do that patch and then upgrade the controller implementation so that it matches the spec and ensure close is still working.
But I can do it the other way as well.

For the controller, I am not yet sure whether going the IDL way or making a full custom implementation of it.

[Benjamin Poulain]

(In reply to comment #4)
> > If I am reading this correctly, you need to have the internal slot on the
> > controller object, and get it out through the this object.
> 
> You are reading correctly.
> I was expecting to first do that patch and then upgrade the controller
> implementation so that it matches the spec and ensure close is still working.
> But I can do it the other way as well.
> 
> For the controller, I am not yet sure whether going the IDL way or making a
> full custom implementation of it.

A fix in a follow up is okay with me. I r+ed this one.

Poke me when you have the follow up.

[youenn fablet]

Created attachment 250702 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 250702 [details]
Patch for landing

Clearing flags on attachment: 250702

Committed r182794: <http://trac.webkit.org/changeset/182794>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 143714

[Xabier Rodríguez Calvar]

Created attachment 251421 [details]
Patch

Rebased against trunk.

[WebKit Commit Bot]

Comment on attachment 251421 [details]
Patch

Clearing flags on attachment: 251421

Committed r183395: <http://trac.webkit.org/changeset/183395>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.