Bug 141160 – [Streams API] Support the start function parameter in ReadableStream constructor

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

As per https://streams.spec.whatwg.org/#rs-constructor, the object passed to the ReadableStream constructor may have a start parameter.
This start parameter must be a method that is called when constructing the ReadableStream object.

[youenn fablet]

Created attachment 245877 [details]
Patch

[Carlos Garcia Campos]

hmm, I see part of this patch was in patch attached to bug #138967, if that patch is now obsolete, please mark it as obsolete :-)

[youenn fablet]

Created attachment 246098 [details]
Patch

[youenn fablet]

Created attachment 246100 [details]
Adding missing include for mac builds

[Carlos Garcia Campos]

Comment on attachment 246100 [details]
Adding missing include for mac builds

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

> Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:54
> -    auto successCallback = [wrapper](RefPtr<ReadableStream> stream) mutable {
> -        wrapper.resolve(stream.get());
> +    auto successCallback = [wrapper]() mutable {
> +        wrapper.resolve(jsUndefined());

Was this wrong?

> Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:65
> -    auto successCallback = [wrapper](RefPtr<ReadableStream> stream) mutable {
> -        wrapper.resolve(stream.get());
> +    auto successCallback = [wrapper]() mutable {
> +        wrapper.resolve(jsUndefined());

Ditto.

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:62
> +    Identifier propertyIdentifier(exec, identifier);
> +    PropertyName propertyName = propertyIdentifier;

This could probably be PropertyName propertyName = Identifier(exec, identifier);

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:75
> +inline static JSValue getPropertyFromObject(ExecState* exec, JSObject* object, const char* identifier)

static inline

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:89
> +    JSValue value = call(exec, jsFunction, callType, callData, thisValue.isUndefined() ? m_readableStream : thisValue, arguments, &exception);

How can this be called with m_readableStream == nullptr? Isn't start function a member of the underlaying source and not of readable stream?

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:121
>          return;
>      }

I know this is not part of this patch, but it looks weird to me that the underlaying source raises exceptions for construct parameters of the ReadableStream object. Also I wonder if the internal error is supposed to be used also for these kind of errors during constructor. If I understood the spec correctly, the internal error is set when the error function moves the stream to errored state. I think this arguments parsing should be done in constructJSReadableStream() returning directly throwVMError with a TypeError in this cases. That also prevents that a ReadableStreamJSSource is created just to return an error. If the parsing is done before, the constructor might receive the JSObject s parameter.

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:127
> +    JSValue argumentValue = exec->argument(0);
> +    JSObject* argument = argumentValue.getObject();
> +
> +    JSValue startFunction = getPropertyFromObject(exec, argument, "start");
> +    if (!startFunction.isUndefined() && !startFunction.isFunction()) {

You don't need argumentValue and argument, you can use exec->argument(0).getObject() directly.

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:129
> +        setInternalError(exec, ASCIILiteral("ReadableStream constructor object start property should be functions."));
> +        return;

Same here, I wonder if this should prevent the stream from being created, or if this should raise an exception when start is called. And the same for the other functions when they are implemented. If we want to handle that before the stream is created, I think we could do that in the create() method, and pass start, pull and cancel functions as parameters of the constructor, or don't create the object and throw exceptions in case of errors. I also wonder why the underlaying object needs to be refcounted, It's created at the ReadableStream construction and the only instance is always owned only by the ReadableStream.

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:189
> +    m_readableStream = readableStream;
> +
> +    JSValue enqueueFunction = createReadableStreamEnqueueFunction(exec);
> +    JSValue closeFunction = createReadableStreamCloseFunction(exec);
> +    JSValue errorFunction = createReadableStreamErrorFunction(exec);
> +
> +    m_enqueueFunction.set(exec->vm(), enqueueFunction);
> +    m_closeFunction.set(exec->vm(), closeFunction);
> +    m_errorFunction.set(exec->vm(), errorFunction);
> +
> +    JSValue streamValue = m_readableStream;
> +    setSlotToObject(exec, m_enqueueFunction.get(), "ReadableStream", streamValue);
> +    setSlotToObject(exec, m_closeFunction.get(), "ReadableStream", streamValue);
> +    setSlotToObject(exec, m_errorFunction.get(), "ReadableStream", streamValue);

I'm not sure I understand this. enqueue, close and error functions are members of ReadableStream, not the underlaying source, right? why are they created here then? Shouldn't they be created by ReadableStream and passed as arguments to start? Couldn't we call ReadableStream::start() instead of source->start()? Then the ReadableStream would call source->start() passing the 3 functions as arguments. If the source has a start function it will be called. If we can do that, maybe the underlaying source doesn't need know about the ReadableStream and we don't need the setStream, nor getJSReadableStream.

[Xabier Rodríguez Calvar]

(In reply to comment #5)
> > Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:54
> > -    auto successCallback = [wrapper](RefPtr<ReadableStream> stream) mutable {
> > -        wrapper.resolve(stream.get());
> > +    auto successCallback = [wrapper]() mutable {
> > +        wrapper.resolve(jsUndefined());
> 
> Was this wrong?

Yes. The spec says that promises should be resolved with undefined and we were resolving them wrong before.

> > Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:65
> > -    auto successCallback = [wrapper](RefPtr<ReadableStream> stream) mutable {
> > -        wrapper.resolve(stream.get());
> > +    auto successCallback = [wrapper]() mutable {
> > +        wrapper.resolve(jsUndefined());
> 
> Ditto.

Ditto.

> > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:62
> > +    Identifier propertyIdentifier(exec, identifier);
> > +    PropertyName propertyName = propertyIdentifier;
> 
> This could probably be PropertyName propertyName = Identifier(exec,
> identifier);

No, it can't. We had it like that before and the Identified object was going out of scope after the assignment and getting the slot could end up in a crash. Checked it with gdb, that's why I rewrote it like that.

> > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:75
> > +inline static JSValue getPropertyFromObject(ExecState* exec, JSObject* object, const char* identifier)
> 
> static inline

Roger.

> > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:89
> > +    JSValue value = call(exec, jsFunction, callType, callData, thisValue.isUndefined() ? m_readableStream : thisValue, arguments, &exception);
> 
> How can this be called with m_readableStream == nullptr? Isn't start
> function a member of the underlaying source and not of readable stream?

This doesn't mean that. We are there checking the passed thisValue and if that thisValue is undefined, then we pass m_readableStream as this object when invoking the function.

> > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:121
> >          return;
> >      }
> 
> I know this is not part of this patch, but it looks weird to me that the
> underlaying source raises exceptions for construct parameters of the
> ReadableStream object. Also I wonder if the internal error is supposed to be
> used also for these kind of errors during constructor. If I understood the
> spec correctly, the internal error is set when the error function moves the
> stream to errored state. I think this arguments parsing should be done in
> constructJSReadableStream() returning directly throwVMError with a TypeError
> in this cases. That also prevents that a ReadableStreamJSSource is created
> just to return an error. If the parsing is done before, the constructor
> might receive the JSObject s parameter.

This is a pending subject that we have. IMHO, we should continue improving the way we manage errors among the three worlds, JS, ReadableStream and the source.

> > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:127
> > +    JSValue argumentValue = exec->argument(0);
> > +    JSObject* argument = argumentValue.getObject();
> > +
> > +    JSValue startFunction = getPropertyFromObject(exec, argument, "start");
> > +    if (!startFunction.isUndefined() && !startFunction.isFunction()) {
> 
> You don't need argumentValue and argument, you can use
> exec->argument(0).getObject() directly.

Roger.

> > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:129
> > +        setInternalError(exec, ASCIILiteral("ReadableStream constructor object start property should be functions."));
> > +        return;
> 
> Same here, I wonder if this should prevent the stream from being created, or
> if this should raise an exception when start is called. And the same for the
> other functions when they are implemented. If we want to handle that before
> the stream is created, I think we could do that in the create() method, and
> pass start, pull and cancel functions as parameters of the constructor, or
> don't create the object and throw exceptions in case of errors.

Sorry, but I didn't understand you comment. What we are doing now is bailing out with an exception, meaning that the JSRS is not created, when the source finds any of those problems. And I don't think we can do that any other way because both objects are entangled, meaning that the spec says that we have to do somethings in some order, but we try to keep separated as much as possible the JSC world from the pure WebCore world. That's why we have to do things in a certain order.

> I also
> wonder why the underlaying object needs to be refcounted, It's created at
> the ReadableStream construction and the only instance is always owned only
> by the ReadableStream.

I am trying to know now if there is any case were the source can live by itself. I could give this a try, but it might happen that I should get to the RefCounted in case of native sources, like the XHR one, that we have developed but didn't push to bugzilla yet.

> > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:189
> > +    m_readableStream = readableStream;
> > +
> > +    JSValue enqueueFunction = createReadableStreamEnqueueFunction(exec);
> > +    JSValue closeFunction = createReadableStreamCloseFunction(exec);
> > +    JSValue errorFunction = createReadableStreamErrorFunction(exec);
> > +
> > +    m_enqueueFunction.set(exec->vm(), enqueueFunction);
> > +    m_closeFunction.set(exec->vm(), closeFunction);
> > +    m_errorFunction.set(exec->vm(), errorFunction);
> > +
> > +    JSValue streamValue = m_readableStream;
> > +    setSlotToObject(exec, m_enqueueFunction.get(), "ReadableStream", streamValue);
> > +    setSlotToObject(exec, m_closeFunction.get(), "ReadableStream", streamValue);
> > +    setSlotToObject(exec, m_errorFunction.get(), "ReadableStream", streamValue);
> 
> I'm not sure I understand this. enqueue, close and error functions are
> members of ReadableStream, not the underlaying source, right? why are they
> created here then? Shouldn't they be created by ReadableStream and passed as
> arguments to start? Couldn't we call ReadableStream::start() instead of
> source->start()? Then the ReadableStream would call source->start() passing
> the 3 functions as arguments. If the source has a start function it will be
> called. If we can do that, maybe the underlaying source doesn't need know
> about the ReadableStream and we don't need the setStream, nor
> getJSReadableStream.

Doing that would mean adding a lot of JSC code to the ReadableStream class, which we try to make as JSC unaware as possible.

It might be difficult to see it now, but this source is not the only one, we have right now working the XHR adaptation, which has a completely different source and it doesn't have anything to do with JS one, neither it needs all that JS adaptation, just to create a JSReadableStream with a custom XHR source providing the streams interface.

We decided to create this architecture and make the ReadableStreamJSSource because it makes sense from the architecture POV as the ReadableStream performs the common actions needed for JS and native sources and we create different sources for different things, like the XHR and the JS one, that is invoked only when we create a ReadableStream manually, but using the JS contructor. In the case of XHR, for example, you don't need any of this, just create the XMLHttpRequest object, set the response to "stream" type and get it with .response() (these api calls might not be accurate).

[Carlos Garcia Campos]

(In reply to comment #6)
> (In reply to comment #5)
> > > Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:54
> > > -    auto successCallback = [wrapper](RefPtr<ReadableStream> stream) mutable {
> > > -        wrapper.resolve(stream.get());
> > > +    auto successCallback = [wrapper]() mutable {
> > > +        wrapper.resolve(jsUndefined());
> > 
> > Was this wrong?
> 
> Yes. The spec says that promises should be resolved with undefined and we
> were resolving them wrong before.

Ok, just curious, the changelog doesn't explain why this change, and it looked unrelated to the start implementation.

> > > Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:65
> > > -    auto successCallback = [wrapper](RefPtr<ReadableStream> stream) mutable {
> > > -        wrapper.resolve(stream.get());
> > > +    auto successCallback = [wrapper]() mutable {
> > > +        wrapper.resolve(jsUndefined());
> > 
> > Ditto.
> 
> Ditto.

Ditto :-)

> > > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:62
> > > +    Identifier propertyIdentifier(exec, identifier);
> > > +    PropertyName propertyName = propertyIdentifier;
> > 
> > This could probably be PropertyName propertyName = Identifier(exec,
> > identifier);
> 
> No, it can't. We had it like that before and the Identified object was going
> out of scope after the assignment and getting the slot could end up in a
> crash. Checked it with gdb, that's why I rewrote it like that.

Oh, ok.

> > > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:75
> > > +inline static JSValue getPropertyFromObject(ExecState* exec, JSObject* object, const char* identifier)
> > 
> > static inline
> 
> Roger.
> 
> > > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:89
> > > +    JSValue value = call(exec, jsFunction, callType, callData, thisValue.isUndefined() ? m_readableStream : thisValue, arguments, &exception);
> > 
> > How can this be called with m_readableStream == nullptr? Isn't start
> > function a member of the underlaying source and not of readable stream?
> 
> This doesn't mean that. We are there checking the passed thisValue and if
> that thisValue is undefined, then we pass m_readableStream as this object
> when invoking the function.

Right, I understood it wrongly, but still, callFunction is never called with a thisValue, or am I wrong again?

> > > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:121
> > >          return;
> > >      }
> > 
> > I know this is not part of this patch, but it looks weird to me that the
> > underlaying source raises exceptions for construct parameters of the
> > ReadableStream object. Also I wonder if the internal error is supposed to be
> > used also for these kind of errors during constructor. If I understood the
> > spec correctly, the internal error is set when the error function moves the
> > stream to errored state. I think this arguments parsing should be done in
> > constructJSReadableStream() returning directly throwVMError with a TypeError
> > in this cases. That also prevents that a ReadableStreamJSSource is created
> > just to return an error. If the parsing is done before, the constructor
> > might receive the JSObject s parameter.
> 
> This is a pending subject that we have. IMHO, we should continue improving
> the way we manage errors among the three worlds, JS, ReadableStream and the
> source.

Just looked weird to me saving the type errors that happen while constructing the JS objects in an m_error, instead of just throwing the exceptions.

> > > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:127
> > > +    JSValue argumentValue = exec->argument(0);
> > > +    JSObject* argument = argumentValue.getObject();
> > > +
> > > +    JSValue startFunction = getPropertyFromObject(exec, argument, "start");
> > > +    if (!startFunction.isUndefined() && !startFunction.isFunction()) {
> > 
> > You don't need argumentValue and argument, you can use
> > exec->argument(0).getObject() directly.
> 
> Roger.
> 
> > > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:129
> > > +        setInternalError(exec, ASCIILiteral("ReadableStream constructor object start property should be functions."));
> > > +        return;
> > 
> > Same here, I wonder if this should prevent the stream from being created, or
> > if this should raise an exception when start is called. And the same for the
> > other functions when they are implemented. If we want to handle that before
> > the stream is created, I think we could do that in the create() method, and
> > pass start, pull and cancel functions as parameters of the constructor, or
> > don't create the object and throw exceptions in case of errors.
> 
> Sorry, but I didn't understand you comment. What we are doing now is bailing
> out with an exception, meaning that the JSRS is not created, when the source
> finds any of those problems. And I don't think we can do that any other way
> because both objects are entangled, meaning that the spec says that we have
> to do somethings in some order, but we try to keep separated as much as
> possible the JSC world from the pure WebCore world. That's why we have to do
> things in a certain order.

I'm not talking about changing the order things happen, just moving the code where it logically belongs, but you know the spec better than me.

> > I also
> > wonder why the underlaying object needs to be refcounted, It's created at
> > the ReadableStream construction and the only instance is always owned only
> > by the ReadableStream.
> 
> I am trying to know now if there is any case were the source can live by
> itself. I could give this a try, but it might happen that I should get to
> the RefCounted in case of native sources, like the XHR one, that we have
> developed but didn't push to bugzilla yet.

What's the XHR source? could you point me to the spec and code you have for that?

> > > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:189
> > > +    m_readableStream = readableStream;
> > > +
> > > +    JSValue enqueueFunction = createReadableStreamEnqueueFunction(exec);
> > > +    JSValue closeFunction = createReadableStreamCloseFunction(exec);
> > > +    JSValue errorFunction = createReadableStreamErrorFunction(exec);
> > > +
> > > +    m_enqueueFunction.set(exec->vm(), enqueueFunction);
> > > +    m_closeFunction.set(exec->vm(), closeFunction);
> > > +    m_errorFunction.set(exec->vm(), errorFunction);
> > > +
> > > +    JSValue streamValue = m_readableStream;
> > > +    setSlotToObject(exec, m_enqueueFunction.get(), "ReadableStream", streamValue);
> > > +    setSlotToObject(exec, m_closeFunction.get(), "ReadableStream", streamValue);
> > > +    setSlotToObject(exec, m_errorFunction.get(), "ReadableStream", streamValue);
> > 
> > I'm not sure I understand this. enqueue, close and error functions are
> > members of ReadableStream, not the underlaying source, right? why are they
> > created here then? Shouldn't they be created by ReadableStream and passed as
> > arguments to start? Couldn't we call ReadableStream::start() instead of
> > source->start()? Then the ReadableStream would call source->start() passing
> > the 3 functions as arguments. If the source has a start function it will be
> > called. If we can do that, maybe the underlaying source doesn't need know
> > about the ReadableStream and we don't need the setStream, nor
> > getJSReadableStream.
> 
> Doing that would mean adding a lot of JSC code to the ReadableStream class,
> which we try to make as JSC unaware as possible.

I'm not talking about ReadableStream class, but about JSReadableStream

> It might be difficult to see it now, but this source is not the only one, we
> have right now working the XHR adaptation, which has a completely different
> source and it doesn't have anything to do with JS one, neither it needs all
> that JS adaptation, just to create a JSReadableStream with a custom XHR
> source providing the streams interface.
> 
> We decided to create this architecture and make the ReadableStreamJSSource
> because it makes sense from the architecture POV as the ReadableStream
> performs the common actions needed for JS and native sources and we create
> different sources for different things, like the XHR and the JS one, that is
> invoked only when we create a ReadableStream manually, but using the JS
> contructor. In the case of XHR, for example, you don't need any of this,
> just create the XMLHttpRequest object, set the response to "stream" type and
> get it with .response() (these api calls might not be accurate).

I didn't know there were more kind of underlaying sources, I'll check the spec again.

[youenn fablet]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > > Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:54
> > > > -    auto successCallback = [wrapper](RefPtr<ReadableStream> stream) mutable {
> > > > -        wrapper.resolve(stream.get());
> > > > +    auto successCallback = [wrapper]() mutable {
> > > > +        wrapper.resolve(jsUndefined());
> > > 
> > > Was this wrong?
> > 
> > Yes. The spec says that promises should be resolved with undefined and we
> > were resolving them wrong before.
> 
> Ok, just curious, the changelog doesn't explain why this change, and it
> looked unrelated to the start implementation.


Yep, it should go in the changelog.

> > It might be difficult to see it now, but this source is not the only one, we
> > have right now working the XHR adaptation, which has a completely different
> > source and it doesn't have anything to do with JS one, neither it needs all
> > that JS adaptation, just to create a JSReadableStream with a custom XHR
> > source providing the streams interface.
> > 
> > We decided to create this architecture and make the ReadableStreamJSSource
> > because it makes sense from the architecture POV as the ReadableStream
> > performs the common actions needed for JS and native sources and we create
> > different sources for different things, like the XHR and the JS one, that is
> > invoked only when we create a ReadableStream manually, but using the JS
> > contructor. In the case of XHR, for example, you don't need any of this,
> > just create the XMLHttpRequest object, set the response to "stream" type and
> > get it with .response() (these api calls might not be accurate).
> 
> I didn't know there were more kind of underlaying sources, I'll check the
> spec again.

At the time we developed the XHR source, it was supposed for streams to be integrated into XHR. The spec that was used has now been deprecated, although it is already implemented in other browsers.
It seems the overall direction is to add streams support for the replacement of XHR, the fetch API.

In any case, streams can either have JS sources/consumers or native sources/consumers: XHR/Fetch, media stream...
That is why we are trying hard to keep ReadableStream as JSC unaware as possible.
Similar strategies have been used for other promise-related APIs in WebKit by the way.

[Benjamin Poulain]

Comment on attachment 246100 [details]
Adding missing include for mac builds

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

I only had time for a quick review. Here is some comment:

> Source/WebCore/Modules/streams/ReadableStream.cpp:89
> +    callOnMainThread(std::bind(WTF::move(m_closedCallback)));

I would ASSERT(!m_closedCallback) after this since keeping the promise alive would be pretty bad.

> Source/WebCore/Modules/streams/ReadableStream.cpp:96
> +    if (m_state == State::Closed)
> +        resolveClosedCallback();

I don't get why you resolve immediately on closed().

> Source/WebCore/Modules/streams/ReadableStream.cpp:104
> +        m_state = State::Closed;
> +        resolveReadyCallback();
> +        resolveClosedCallback();

Shouldn't this be:
    resolveReadyCallback();
    m_state = State::Closed;
    resolveClosedCallback();

> Source/WebCore/Modules/streams/ReadableStream.cpp:111
> +    if (!m_readyCallback)

Add ASSERT(m_state == State::Waiting)?

> Source/WebCore/Modules/streams/ReadableStream.cpp:113
> +    callOnMainThread(std::bind(WTF::move(m_readyCallback)));

I would ASSERT(!m_readyCallback) after this since keeping the promise alive would be pretty bad.

> Source/WebCore/Modules/streams/ReadableStream.cpp:120
> +    if (m_state != State::Waiting)
> +        resolveReadyCallback();

I don't get why you resolve immediately on ready().

>> Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:54
>> +        wrapper.resolve(jsUndefined());
> 
> Was this wrong?

Good catch.

It is a bit weird to use undefined but that's what the spec says...

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:113
> +    m_startFunction.set(exec->vm(), jsUndefined());

I don't get this.

> Source/WebCore/bindings/js/ReadableStreamJSSource.h:67
> +    JSReadableStream* m_readableStream;

m_readableStream = nullptr;

And you can remove the initialization from the constructor.

> Source/WebCore/bindings/js/ReadableStreamJSSource.h:75
> +    // Functions given as constructor parameters.
> +    JSC::Strong<JSC::Unknown> m_startFunction;
> +
> +    // Functions passed as parameters when calling the functions passed in the constructor.
> +    JSC::Strong<JSC::Unknown> m_enqueueFunction;
> +    JSC::Strong<JSC::Unknown> m_closeFunction;
> +    JSC::Strong<JSC::Unknown> m_errorFunction;

Why aren't those typed?

> LayoutTests/ChangeLog:15
> +        * streams/readablestream-start-expected.txt: Added.
> +        * streams/readablestream-start.html: Added.

Do you have coverage for reusing the premise object?

You create a new promise every time, which is observable. I am actually unconvinced it is correct.

For example:
    var promise1 = readableStream.ready;
    promise1.then(foobar);
    readableStream.ready === promise1; // Shouldn't this be true? Is it?

[Carlos Garcia Campos]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > > Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:54
> > > > > -    auto successCallback = [wrapper](RefPtr<ReadableStream> stream) mutable {
> > > > > -        wrapper.resolve(stream.get());
> > > > > +    auto successCallback = [wrapper]() mutable {
> > > > > +        wrapper.resolve(jsUndefined());
> > > > 
> > > > Was this wrong?
> > > 
> > > Yes. The spec says that promises should be resolved with undefined and we
> > > were resolving them wrong before.
> > 
> > Ok, just curious, the changelog doesn't explain why this change, and it
> > looked unrelated to the start implementation.
> 
> 
> Yep, it should go in the changelog.

Or in a different patch if it's unrelated to this bug :-)

> > > It might be difficult to see it now, but this source is not the only one, we
> > > have right now working the XHR adaptation, which has a completely different
> > > source and it doesn't have anything to do with JS one, neither it needs all
> > > that JS adaptation, just to create a JSReadableStream with a custom XHR
> > > source providing the streams interface.
> > > 
> > > We decided to create this architecture and make the ReadableStreamJSSource
> > > because it makes sense from the architecture POV as the ReadableStream
> > > performs the common actions needed for JS and native sources and we create
> > > different sources for different things, like the XHR and the JS one, that is
> > > invoked only when we create a ReadableStream manually, but using the JS
> > > contructor. In the case of XHR, for example, you don't need any of this,
> > > just create the XMLHttpRequest object, set the response to "stream" type and
> > > get it with .response() (these api calls might not be accurate).
> > 
> > I didn't know there were more kind of underlaying sources, I'll check the
> > spec again.
> 
> At the time we developed the XHR source, it was supposed for streams to be
> integrated into XHR. The spec that was used has now been deprecated,
> although it is already implemented in other browsers.
> It seems the overall direction is to add streams support for the replacement
> of XHR, the fetch API.
> 
> In any case, streams can either have JS sources/consumers or native
> sources/consumers: XHR/Fetch, media stream...
> That is why we are trying hard to keep ReadableStream as JSC unaware as
> possible.
> Similar strategies have been used for other promise-related APIs in WebKit
> by the way.

And I agree, but I meant JSReadableStream, not ReadableStream

[Xabier Rodríguez Calvar]

(In reply to comment #10)
> > > > > > Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:54
> > > > > > -    auto successCallback = [wrapper](RefPtr<ReadableStream> stream) mutable {
> > > > > > -        wrapper.resolve(stream.get());
> > > > > > +    auto successCallback = [wrapper]() mutable {
> > > > > > +        wrapper.resolve(jsUndefined());
> > > > > 
> > > > > Was this wrong?
> > > > 
> > > > Yes. The spec says that promises should be resolved with undefined and we
> > > > were resolving them wrong before.
> > > 
> > > Ok, just curious, the changelog doesn't explain why this change, and it
> > > looked unrelated to the start implementation.
> > 
> > 
> > Yep, it should go in the changelog.
> 
> Or in a different patch if it's unrelated to this bug :-)

I agree on having it on a different patch. Makes sense.

> > > > It might be difficult to see it now, but this source is not the only one, we
> > > > have right now working the XHR adaptation, which has a completely different
> > > > source and it doesn't have anything to do with JS one, neither it needs all
> > > > that JS adaptation, just to create a JSReadableStream with a custom XHR
> > > > source providing the streams interface.
> > > > 
> > > > We decided to create this architecture and make the ReadableStreamJSSource
> > > > because it makes sense from the architecture POV as the ReadableStream
> > > > performs the common actions needed for JS and native sources and we create
> > > > different sources for different things, like the XHR and the JS one, that is
> > > > invoked only when we create a ReadableStream manually, but using the JS
> > > > contructor. In the case of XHR, for example, you don't need any of this,
> > > > just create the XMLHttpRequest object, set the response to "stream" type and
> > > > get it with .response() (these api calls might not be accurate).
> > > 
> > > I didn't know there were more kind of underlaying sources, I'll check the
> > > spec again.
> > 
> > At the time we developed the XHR source, it was supposed for streams to be
> > integrated into XHR. The spec that was used has now been deprecated,
> > although it is already implemented in other browsers.
> > It seems the overall direction is to add streams support for the replacement
> > of XHR, the fetch API.
> > 
> > In any case, streams can either have JS sources/consumers or native
> > sources/consumers: XHR/Fetch, media stream...
> > That is why we are trying hard to keep ReadableStream as JSC unaware as
> > possible.
> > Similar strategies have been used for other promise-related APIs in WebKit
> > by the way.
> 
> And I agree, but I meant JSReadableStream, not ReadableStream

I might not be clear explaining the whole architecture. We have

* JSReadableStream which is the autogenerated class from the IDL bridging from JS to WebCore. There we adapt only the methods declared in the IDL file, including the constructor. A JS created stream might use all methods including the constructor (actually the constructor is compulsory in this case) and a native stream might use all methods but it won't use the constructor as the object will be constructed somewhere else, say XMLHttpRequest.response().

* ReadableStream implements the common parts of the algorithm for native and JS sources, it delegates some parts of the code on the ReadableStreamSource abstract class.

* ReadableStreamJSSource, inherits ReadableStreamSource and implements the specific parts of the spec that have to do only with JS, say invoking JS methods, preparing exceptions to be launched, etc. In this class we have to adapt data and operations between ReadableStream and JSReadableStream, so that ReadableStream stays as JSC unaware as possible and we can put things directly to JSReadableStream. That's why sometimes JSReadableStream deals directly with the source, because of the data we don't want to store in ReadableStream.

I hope to have been a bit clearer now and please do not hesitate to put in doubt any decission or ask any questions. That's what we are here for :)

[Xabier Rodríguez Calvar]

(In reply to comment #9)
> > Source/WebCore/Modules/streams/ReadableStream.cpp:89
> > +    callOnMainThread(std::bind(WTF::move(m_closedCallback)));
> 
> I would ASSERT(!m_closedCallback) after this since keeping the promise alive
> would be pretty bad.

Roger.

> > Source/WebCore/Modules/streams/ReadableStream.cpp:96
> > +    if (m_state == State::Closed)
> > +        resolveClosedCallback();
> 
> I don't get why you resolve immediately on closed().

Imagine a code like:

rs.close();
rs.closed().then(whatever);

We might have already transitioned to "closed" before the promise was requested and if we don't do this, the promise might end up unresolved. Btw, by immediatelly resolving we understand here that we call the resolve function, but it is deferred to the main thread.

> > Source/WebCore/Modules/streams/ReadableStream.cpp:104
> > +        m_state = State::Closed;
> > +        resolveReadyCallback();
> > +        resolveClosedCallback();
> 
> Shouldn't this be:
>     resolveReadyCallback();
>     m_state = State::Closed;
>     resolveClosedCallback();

The spec says that when closing we have to fulfill both ready and closed. We want that the state is already marked as closed while we resolve the ready callback because the client has to get the proper state. That's why semantically, IMHO, it is better this way.

Why did I write "semantically"? Because it doesn't matter as promises are resolved asynchronously so the state will be the right one when the promise is actually going to be fulfilled.
 
> > Source/WebCore/Modules/streams/ReadableStream.cpp:111
> > +    if (!m_readyCallback)
> 
> Add ASSERT(m_state == State::Waiting)?

I don't think we can, because I am almost positive that we have to resolve any pending ready promises in other states too, for example when we move to errored.

> > Source/WebCore/Modules/streams/ReadableStream.cpp:113
> > +    callOnMainThread(std::bind(WTF::move(m_readyCallback)));
> 
> I would ASSERT(!m_readyCallback) after this since keeping the promise alive
> would be pretty bad.

Roger.

> > Source/WebCore/Modules/streams/ReadableStream.cpp:120
> > +    if (m_state != State::Waiting)
> > +        resolveReadyCallback();
> 
> I don't get why you resolve immediately on ready().

Same case as for closed.
 
> >> Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:54
> >> +        wrapper.resolve(jsUndefined());
> > 
> > Was this wrong?
> 
> Good catch.
> 
> It is a bit weird to use undefined but that's what the spec says...

No need to tell me :) actually there is a reference test checking these things O_O

> > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:113
> > +    m_startFunction.set(exec->vm(), jsUndefined());
> 
> I don't get this.

We initialize all these functions to undefined and after that we overwrite then with the corresponding value given in the constructor if any, but it is good to have them like that to check later if the value is undefined, that means that it wasn't set by the constructor and we have to go with the spec default behavior.

> > Source/WebCore/bindings/js/ReadableStreamJSSource.h:67
> > +    JSReadableStream* m_readableStream;
> 
> m_readableStream = nullptr;
> 
> And you can remove the initialization from the constructor.

Roger.

> > Source/WebCore/bindings/js/ReadableStreamJSSource.h:75
> > +    // Functions given as constructor parameters.
> > +    JSC::Strong<JSC::Unknown> m_startFunction;
> > +
> > +    // Functions passed as parameters when calling the functions passed in the constructor.
> > +    JSC::Strong<JSC::Unknown> m_enqueueFunction;
> > +    JSC::Strong<JSC::Unknown> m_closeFunction;
> > +    JSC::Strong<JSC::Unknown> m_errorFunction;
> 
> Why aren't those typed?

Roger.

> > LayoutTests/ChangeLog:15
> > +        * streams/readablestream-start-expected.txt: Added.
> > +        * streams/readablestream-start.html: Added.
> 
> Do you have coverage for reusing the premise object?
> 
> You create a new promise every time, which is observable. I am actually
> unconvinced it is correct.
> 
> For example:
>     var promise1 = readableStream.ready;
>     promise1.then(foobar);
>     readableStream.ready === promise1; // Shouldn't this be true? Is it?

Very interesting comment. The spec speaks about storing the promises in slots and, they are returned when requested and replaced when they are resolved. So I conclude your assumption is right. We'll look at this.

[Xabier Rodríguez Calvar]

(In reply to comment #7)
> > > > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:89
> > > > +    JSValue value = call(exec, jsFunction, callType, callData, thisValue.isUndefined() ? m_readableStream : thisValue, arguments, &exception);
> > > 
> > > How can this be called with m_readableStream == nullptr? Isn't start
> > > function a member of the underlaying source and not of readable stream?
> > 
> > This doesn't mean that. We are there checking the passed thisValue and if
> > that thisValue is undefined, then we pass m_readableStream as this object
> > when invoking the function.
> 
> Right, I understood it wrongly, but still, callFunction is never called with
> a thisValue, or am I wrong again?

It is, I don't remember more places, but we have at least the strategy methods:

unsigned ReadableStreamJSSource::chunkSize(ExecState* exec, JSValue chunk);
bool ReadableStreamJSSource::shouldApplyBackpressure(unsigned queueSize);

Actually I had to do this when I wrote one of the test using a different strategy that was created as an object with its attributes that were never reached with the former implementation we were passing undefined there.

> > > > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:127
> > > > +    JSValue argumentValue = exec->argument(0);
> > > > +    JSObject* argument = argumentValue.getObject();
> > > > +
> > > > +    JSValue startFunction = getPropertyFromObject(exec, argument, "start");
> > > > +    if (!startFunction.isUndefined() && !startFunction.isFunction()) {
> > > 
> > > You don't need argumentValue and argument, you can use
> > > exec->argument(0).getObject() directly.
> > 
> > Roger.
> > 
> > > > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:129
> > > > +        setInternalError(exec, ASCIILiteral("ReadableStream constructor object start property should be functions."));
> > > > +        return;
> > > 
> > > Same here, I wonder if this should prevent the stream from being created, or
> > > if this should raise an exception when start is called. And the same for the
> > > other functions when they are implemented. If we want to handle that before
> > > the stream is created, I think we could do that in the create() method, and
> > > pass start, pull and cancel functions as parameters of the constructor, or
> > > don't create the object and throw exceptions in case of errors.
> > 
> > Sorry, but I didn't understand you comment. What we are doing now is bailing
> > out with an exception, meaning that the JSRS is not created, when the source
> > finds any of those problems. And I don't think we can do that any other way
> > because both objects are entangled, meaning that the spec says that we have
> > to do somethings in some order, but we try to keep separated as much as
> > possible the JSC world from the pure WebCore world. That's why we have to do
> > things in a certain order.
> 
> I'm not talking about changing the order things happen, just moving the code
> where it logically belongs, but you know the spec better than me.

From my POV it is a trade off between architecture and spec and it is not easy :)

> > > > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:189
> > > > +    m_readableStream = readableStream;
> > > > +
> > > > +    JSValue enqueueFunction = createReadableStreamEnqueueFunction(exec);
> > > > +    JSValue closeFunction = createReadableStreamCloseFunction(exec);
> > > > +    JSValue errorFunction = createReadableStreamErrorFunction(exec);
> > > > +
> > > > +    m_enqueueFunction.set(exec->vm(), enqueueFunction);
> > > > +    m_closeFunction.set(exec->vm(), closeFunction);
> > > > +    m_errorFunction.set(exec->vm(), errorFunction);
> > > > +
> > > > +    JSValue streamValue = m_readableStream;
> > > > +    setSlotToObject(exec, m_enqueueFunction.get(), "ReadableStream", streamValue);
> > > > +    setSlotToObject(exec, m_closeFunction.get(), "ReadableStream", streamValue);
> > > > +    setSlotToObject(exec, m_errorFunction.get(), "ReadableStream", streamValue);
> > > 
> > > I'm not sure I understand this. enqueue, close and error functions are
> > > members of ReadableStream, not the underlaying source, right? why are they
> > > created here then? Shouldn't they be created by ReadableStream and passed as
> > > arguments to start? Couldn't we call ReadableStream::start() instead of
> > > source->start()? Then the ReadableStream would call source->start() passing
> > > the 3 functions as arguments. If the source has a start function it will be
> > > called. If we can do that, maybe the underlaying source doesn't need know
> > > about the ReadableStream and we don't need the setStream, nor
> > > getJSReadableStream.
> > 
> > Doing that would mean adding a lot of JSC code to the ReadableStream class,
> > which we try to make as JSC unaware as possible.
> 
> I'm not talking about ReadableStream class, but about JSReadableStream

enqueue, close and error are one of those things that only make sense in the JS world with JS based streams and that's why given our arch, we create them in the ReadableStreamJSSource to adapt code from the ReadableStream. They don't make sense in a native source, for example.

[youenn fablet]

> > > LayoutTests/ChangeLog:15
> > > +        * streams/readablestream-start-expected.txt: Added.
> > > +        * streams/readablestream-start.html: Added.
> > 
> > Do you have coverage for reusing the premise object?
> > 
> > You create a new promise every time, which is observable. I am actually
> > unconvinced it is correct.
> > 
> > For example:
> >     var promise1 = readableStream.ready;
> >     promise1.then(foobar);
> >     readableStream.ready === promise1; // Shouldn't this be true? Is it?
> 
> Very interesting comment. The spec speaks about storing the promises in
> slots and, they are returned when requested and replaced when they are
> resolved. So I conclude your assumption is right. We'll look at this.

Good point, I created bug 141650 to capture that. It seems to be best handled separately, especially if we handle that at JSReadableStream level.

[youenn fablet]

(In reply to comment #11)
> (In reply to comment #10)
> > > > > > > Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:54
> > > > > > > -    auto successCallback = [wrapper](RefPtr<ReadableStream> stream) mutable {
> > > > > > > -        wrapper.resolve(stream.get());
> > > > > > > +    auto successCallback = [wrapper]() mutable {
> > > > > > > +        wrapper.resolve(jsUndefined());
> > > > > > 
> > > > > > Was this wrong?
> > > > > 
> > > > > Yes. The spec says that promises should be resolved with undefined and we
> > > > > were resolving them wrong before.
> > > > 
> > > > Ok, just curious, the changelog doesn't explain why this change, and it
> > > > looked unrelated to the start implementation.
> > > 
> > > 
> > > Yep, it should go in the changelog.
> > 
> > Or in a different patch if it's unrelated to this bug :-)
> 
> I agree on having it on a different patch. Makes sense.

Well, we are not able to test that behaviour without adding some functionality that triggers promise resolution.
We can split this in two bugs, but we would end up changing code twice.

I would prefer keeping the fix here, as we would have the functionality right from the beginning, and the tests will be marked passed.
But I am ok if you prefer to have it as two patches to get a easier/better review.

[youenn fablet]

Created attachment 247116 [details]
Updated according review and bug 141650

[youenn fablet]

Created attachment 247231 [details]
Rebasing

[youenn fablet]

Created attachment 247331 [details]
Improving style and comments

[youenn fablet]

Created attachment 248073 [details]
Rebasing a

[youenn fablet]

Created attachment 248148 [details]
Removing promise resolution

[youenn fablet]

Created attachment 248253 [details]
Patch

[Benjamin Poulain]

Comment on attachment 248253 [details]
Patch

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

Quick review. Nothing bad so far, some comments:

> Source/WebCore/ChangeLog:9
> +        This function takes three functions (close , enqueue and error) as

" , " -> ", "

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:34
>  #include "ReadableStreamJSSource.h"
> +#include "ScriptExecutionContext.h"

Let's fix the headers:
    #include "ReadableStreamJSSource.h"
should be just after
    #include "config.h"

All the other includes should be together.

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:130
> +    // FIXME: Implement this method.

No need for this FIXME, notImplemented() has the same information.

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:154
> +    // FIXME: Implement this method.

No need for this.

> Source/WebCore/bindings/js/ReadableStreamJSSource.h:64
> +    JSC::JSValue callFunction(JSC::ExecState*, JSC::JSValue /* jsFunction */, JSC::JSValue /* thisValue */, const JSC::ArgList&);

There should be no need to comment the argument names here.

> Source/WebCore/bindings/js/ReadableStreamJSSource.h:77
> +    JSC::Strong<JSC::JSFunction> m_enqueueFunction;
> +    JSC::Strong<JSC::JSFunction> m_closeFunction;
> +    JSC::Strong<JSC::JSFunction> m_errorFunction;

I am quite confused why you hold strongly onto those.

It looks to me like they could be created on the fly when calling the function and never needed again.

Can you explain?

> LayoutTests/ChangeLog:9
> +        Added a test to check that ReadableStream constructor accepts an empty object as parameter.
> +        Added tests to check that start JS function is called, can close a stream and can throw errors.

IMHO you should have two more test:
-Test what is the "this" argument in the start function.
-Check the types of enqueue, close, error

> LayoutTests/streams/readablestream-start.html:23
> +    mashedError = new Error('potato');

hehe

> LayoutTests/streams/readablestream-start.html:28
> +            function(enqueue, close, error) {
> +                throw mashedError;

Good test!

[youenn fablet]

Comment on attachment 248253 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        This function takes three functions (close , enqueue and error) as
> 
> " , " -> ", "

OK

>> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:34
>> +#include "ScriptExecutionContext.h"
> 
> Let's fix the headers:
>     #include "ReadableStreamJSSource.h"
> should be just after
>     #include "config.h"
> 
> All the other includes should be together.

OK

>> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:130
>> +    // FIXME: Implement this method.
> 
> No need for this FIXME, notImplemented() has the same information.

OK

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:135
> +static JSFunction* createReadableStreamEnqueueFunction(ExecState* exec)

Will add "inline" to all three create functions.

>> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:154
>> +    // FIXME: Implement this method.
> 
> No need for this.

OK

>> Source/WebCore/bindings/js/ReadableStreamJSSource.h:64
>> +    JSC::JSValue callFunction(JSC::ExecState*, JSC::JSValue /* jsFunction */, JSC::JSValue /* thisValue */, const JSC::ArgList&);
> 
> There should be no need to comment the argument names here.

This is purely informal, I will remove them.

>> Source/WebCore/bindings/js/ReadableStreamJSSource.h:77
>> +    JSC::Strong<JSC::JSFunction> m_errorFunction;
> 
> I am quite confused why you hold strongly onto those.
> 
> It looks to me like they could be created on the fly when calling the function and never needed again.
> 
> Can you explain?

Enqueue and close are passed as parameters to the pull function, which may be called several times.
We could create error on the fly given how we plan to handle pull promise resolution.
I will add a FIXME so that we can revisit that case later on.

>> LayoutTests/ChangeLog:9
>> +        Added tests to check that start JS function is called, can close a stream and can throw errors.
> 
> IMHO you should have two more test:
> -Test what is the "this" argument in the start function.
> -Check the types of enqueue, close, error

The types of enqueue, close and error are already checked.
I will add an assert for "this" and improve the change log.

[youenn fablet]

> >> Source/WebCore/bindings/js/ReadableStreamJSSource.h:64
> >> +    JSC::JSValue callFunction(JSC::ExecState*, JSC::JSValue /* jsFunction */, JSC::JSValue /* thisValue */, const JSC::ArgList&);
> > 
> > There should be no need to comment the argument names here.
> 
> This is purely informal, I will remove them.
s/informal/informative/

[youenn fablet]

Created attachment 248381 [details]
Fix according review

[Benjamin Poulain]

(In reply to comment #23)
> >> Source/WebCore/bindings/js/ReadableStreamJSSource.h:64
> >> +    JSC::JSValue callFunction(JSC::ExecState*, JSC::JSValue /* jsFunction */, JSC::JSValue /* thisValue */, const JSC::ArgList&);
> > 
> > There should be no need to comment the argument names here.
> 
> This is purely informal, I will remove them.

Note that having the argument names is good here because it is not clear what the arguments are from their type.

My comment was referring to the "/* */", those should be unnecessary, the name will just be ignored by the compiler.

> >> Source/WebCore/bindings/js/ReadableStreamJSSource.h:77
> >> +    JSC::Strong<JSC::JSFunction> m_errorFunction;
> > 
> > I am quite confused why you hold strongly onto those.
> > 
> > It looks to me like they could be created on the fly when calling the function and never needed again.
> > 
> > Can you explain?
> 
> Enqueue and close are passed as parameters to the pull function, which may
> be called several times.
> We could create error on the fly given how we plan to handle pull promise
> resolution.
> I will add a FIXME so that we can revisit that case later on.

Who call them several times? JavaScript?

If JavaScript holds onto them, they will have a strong reference on the heap or the JS Stack. There should be no need to keep a strong reference in your object.

[youenn fablet]

(In reply to comment #26)
> (In reply to comment #23)
> > >> Source/WebCore/bindings/js/ReadableStreamJSSource.h:64
> > >> +    JSC::JSValue callFunction(JSC::ExecState*, JSC::JSValue /* jsFunction */, JSC::JSValue /* thisValue */, const JSC::ArgList&);
> > > 
> > > There should be no need to comment the argument names here.
> > 
> > This is purely informal, I will remove them.
> 
> Note that having the argument names is good here because it is not clear
> what the arguments are from their type.
> 
> My comment was referring to the "/* */", those should be unnecessary, the
> name will just be ignored by the compiler.

I see. I used to "/* */" because of some check-webkit-style warnings I got in the past.
I prefer then re-adding the names, this may be done at cq time I guess.

> > >> Source/WebCore/bindings/js/ReadableStreamJSSource.h:77
> > >> +    JSC::Strong<JSC::JSFunction> m_errorFunction;
> > > 
> > > I am quite confused why you hold strongly onto those.
> > > 
> > > It looks to me like they could be created on the fly when calling the function and never needed again.
> > > 
> > > Can you explain?
> > 
> > Enqueue and close are passed as parameters to the pull function, which may
> > be called several times.
> > We could create error on the fly given how we plan to handle pull promise
> > resolution.
> > I will add a FIXME so that we can revisit that case later on.
> 
> Who call them several times? JavaScript?

Pull is called by the readable stream.
For instance, JavaScript may call ReadableStream.read. 
The stream then returns the first value in the queue.
Then the stream calls UnderlyingSource.pull if data in the queue is low.

> If JavaScript holds onto them, they will have a strong reference on the heap
> or the JS Stack. There should be no need to keep a strong reference in your
> object.

The spec currently mandates this approach but this could be done differently.
I would tend to go forward with the current approach and in parallel discuss this with streams spec editors.

[youenn fablet]

> > If JavaScript holds onto them, they will have a strong reference on the heap
> > or the JS Stack. There should be no need to keep a strong reference in your
> > object.
> 
> The spec currently mandates this approach but this could be done differently.
> I would tend to go forward with the current approach and in parallel discuss
> this with streams spec editors.

I opened https://github.com/whatwg/streams/issues/292.

[Benjamin Poulain]

Comment on attachment 248381 [details]
Fix according review

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

Partial review. I'll have to look into the scope of the JSLockHolder.

I think there is a bug with the callback function names.

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:96
> +    if (callType == CallTypeNone)

I am confused by this, when would the call type not be CallTypeHost?

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:136
> +    return JSFunction::create(exec->vm(), exec->callee()->globalObject(), 1, ASCIILiteral("CreateReadableStreamEnqueueFunction"), enqueueReadableStreamFunction);

The spec says the functions are anonymous. Isn't the 4th argument the name?
I think you are supposed to pass String() for the name here.

I guess you do not have tests for that, time to add one :)

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:148
> +    return JSFunction::create(exec->vm(), exec->callee()->globalObject(), 0, ASCIILiteral("CreateReadableStreamCloseFunction"), closeReadableStreamFunction);

Ditto.

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:159
> +    return JSFunction::create(exec->vm(), exec->callee()->globalObject(), 1, ASCIILiteral("CreateReadableStreamErrorFunction"), errorReadableStreamFunction);

Ditto.

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:172
> +    setInternalSlotToObject(exec, m_enqueueFunction.get(), readableStreamSlotName(), m_readableStream);
> +    setInternalSlotToObject(exec, m_closeFunction.get(), readableStreamSlotName(), m_readableStream);
> +    setInternalSlotToObject(exec, m_errorFunction.get(), readableStreamSlotName(), m_readableStream);

Shame we have to do this, I wish NativeFunction was a std::function.
We would have to teach the JIT how to invoke closure. :(

> Source/WebCore/bindings/js/ReadableStreamJSSource.h:77
> +    // FIXME: Decide whether creating the error function on the fly when calling the start source function.

Unless we have a reason to keep this object alive, let's change this right now.

It is likely the implementation of start() will not hold onto the error function. Let's not hold a JSCell alive for a function we don't need.

> LayoutTests/streams/readablestream-start.html:16
> +            assert_array_equals(Object.getOwnPropertyNames(close), ['name', 'length']);

Could you generalize this to the other two arguments if that's not already covered elsewhere?
You should test length and name on every one of them too.

[youenn fablet]

Comment on attachment 248381 [details]
Fix according review

As of JSLockHolder scope, we could add it after the startFunction.isFunction test.
We add it in the start method and not within the callFunction method as it is needed 
before creating JS functions which will happen for resolving start promise and the error function wrapper as well.

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

>> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:96
>> +    if (callType == CallTypeNone)
> 
> I am confused by this, when would the call type not be CallTypeHost?

Right, it is no longer needed since we check that jsFunction is a function.

>> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:136
>> +    return JSFunction::create(exec->vm(), exec->callee()->globalObject(), 1, ASCIILiteral("CreateReadableStreamEnqueueFunction"), enqueueReadableStreamFunction);
> 
> The spec says the functions are anonymous. Isn't the 4th argument the name?
> I think you are supposed to pass String() for the name here.
> 
> I guess you do not have tests for that, time to add one :)

Ah!!!
The function length should also be checked in the same test I guess.

>> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:148
>> +    return JSFunction::create(exec->vm(), exec->callee()->globalObject(), 0, ASCIILiteral("CreateReadableStreamCloseFunction"), closeReadableStreamFunction);
> 
> Ditto.

OK

>> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:159
>> +    return JSFunction::create(exec->vm(), exec->callee()->globalObject(), 1, ASCIILiteral("CreateReadableStreamErrorFunction"), errorReadableStreamFunction);
> 
> Ditto.

OK

>> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:172
>> +    setInternalSlotToObject(exec, m_errorFunction.get(), readableStreamSlotName(), m_readableStream);
> 
> Shame we have to do this, I wish NativeFunction was a std::function.
> We would have to teach the JIT how to invoke closure. :(

Yep, a FIXME may be good...

>> Source/WebCore/bindings/js/ReadableStreamJSSource.h:77
>> +    // FIXME: Decide whether creating the error function on the fly when calling the start source function.
> 
> Unless we have a reason to keep this object alive, let's change this right now.
> 
> It is likely the implementation of start() will not hold onto the error function. Let's not hold a JSCell alive for a function we don't need.

OK

>> LayoutTests/streams/readablestream-start.html:16
>> +            assert_array_equals(Object.getOwnPropertyNames(close), ['name', 'length']);
> 
> Could you generalize this to the other two arguments if that's not already covered elsewhere?
> You should test length and name on every one of them too.

OK

[Benjamin Poulain]

Comment on attachment 248381 [details]
Fix according review

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

Ok, let's r- from the previous comments and missing test cases. I have not spotted anything else but I'd like a second look.

Poke me when it is ready :)

>>> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:172
>>> +    setInternalSlotToObject(exec, m_errorFunction.get(), readableStreamSlotName(), m_readableStream);
>> 
>> Shame we have to do this, I wish NativeFunction was a std::function.
>> We would have to teach the JIT how to invoke closure. :(
> 
> Yep, a FIXME may be good...

I think the FIXME would not been seen since the part to change is in JSC.

If you want to start hacking on JSC, that could be a nice little project to prototype :)

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:192
> +            throwVMError(exec, createTypeError(exec, ASCIILiteral("ReadableStream constructor object start property should be a function.")));

Is this standard? The spec says InvokeOrNoop on start.

[youenn fablet]

Created attachment 249569 [details]
Adding reader support

[youenn fablet]

Created attachment 250054 [details]
Removing close

[youenn fablet]

Comment on attachment 250054 [details]
Removing close

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

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:156
> +    if (!m_source) {

We could probably initialize m_source as an empty object if there is no given JS object to ReadableStreamJSSource constructor.
That would remove that test and similar tests for pull and cancel.
Will add a FIXME at cq time if there is no further change envisioned.

> Source/WebCore/bindings/js/ReadableStreamJSSource.h:69
> +    JSC::Strong<JSC::JSFunction> m_errorFunction;

We store m_errorFunction as it will be reused as start/pull promise rejection callback.

[Benjamin Poulain]

Comment on attachment 250054 [details]
Removing close

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

Some minor comments.

Reading through the spec of ReadableStream's constructor: isn't one of the big use case a start function that return a promise?
I am a bit confused by the spec to be honest :). Do you know where is the definition of "Resolve [arbitrary object] as a promise"?

> Source/WebCore/Modules/streams/ReadableStream.cpp:63
> +    // FIXME: Implement pulling data

notImplemented();

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:142
> +    setInternalSlotToObject(exec, m_enqueueFunction.get(), readableStreamSlotName(), m_readableStream);
> +    setInternalSlotToObject(exec, m_closeFunction.get(), readableStreamSlotName(), m_readableStream);
> +    setInternalSlotToObject(exec, m_errorFunction.get(), readableStreamSlotName(), m_readableStream);

You need tests making sure none of them are observable.

> LayoutTests/streams/readablestream-start.html:33
> +    var rs = new ReadableStream(source);

Do you already have tests passing a object without start method as the source?
-undefined as the source?
-null as the source?
-no source?

It would also be good to have a test with the start function somewhere on the prototype chain of the source object.

[youenn fablet]

Comment on attachment 250054 [details]
Removing close

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


>Reading through the spec of ReadableStream's constructor: isn't one of the big use case a start function that return a promise?

Probably, although pull can also return a promise.

>I am a bit confused by the spec to be honest :). Do you know where is the definition of "Resolve [arbitrary object] as a promise"?

I think it is under standardization.
See https://github.com/domenic/promises-unwrapping

>> Source/WebCore/Modules/streams/ReadableStream.cpp:63
>> +    // FIXME: Implement pulling data
> 
> notImplemented();

OK

>> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:142
>> +    setInternalSlotToObject(exec, m_errorFunction.get(), readableStreamSlotName(), m_readableStream);
> 
> You need tests making sure none of them are observable.

It is part of the first test in streams/readablestream-start.html

>> LayoutTests/streams/readablestream-start.html:33
>> +    var rs = new ReadableStream(source);
> 
> Do you already have tests passing a object without start method as the source?
> -undefined as the source?
> -null as the source?
> -no source?
> 
> It would also be good to have a test with the start function somewhere on the prototype chain of the source object.

There are tests for 1 and 4, but not 2 and 3. I will add them.
Ok also for adding a test on start function being on the prototype chain.

[youenn fablet]

Created attachment 250131 [details]
Adding test, better init of m_source

[youenn fablet]

Created attachment 250132 [details]
Adding test, better init of m_source

[youenn fablet]

Created attachment 250201 [details]
Introducing controller

[youenn fablet]

Comment on attachment 250201 [details]
Introducing controller

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

> Source/WebCore/ChangeLog:9
> +        This function takes a controller object that has three JS functions as members: close, enqueue and error.

Controller is described in https://streams.spec.whatwg.org/#rs-controller-class
It is probably not worth introducing it as a JS interface.

[Benjamin Poulain]

Comment on attachment 250201 [details]
Introducing controller

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

Let's move forward. I am more unconvinced by the spec that the patch :)

> Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:136
> +    setPropertyToObject(exec, controller, "enqueue", enqueueFunction);
> +    setPropertyToObject(exec, controller, "close", closeFunction);
> +    setPropertyToObject(exec, controller, "error", errorFunction);

Should those really be on the object itself and not the prototype?
The spec is crazy vague about the definition of ReadableStreamController.

Can you please ask the working group to have a normative definition of the ReadableStreamController?

[youenn fablet]

(In reply to comment #41)
> Comment on attachment 250201 [details]
> Introducing controller
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250201&action=review
> 
> Let's move forward. I am more unconvinced by the spec that the patch :)
> 
> > Source/WebCore/bindings/js/ReadableStreamJSSource.cpp:136
> > +    setPropertyToObject(exec, controller, "enqueue", enqueueFunction);
> > +    setPropertyToObject(exec, controller, "close", closeFunction);
> > +    setPropertyToObject(exec, controller, "error", errorFunction);
> 
> Should those really be on the object itself and not the prototype?
> The spec is crazy vague about the definition of ReadableStreamController.

Yes, they should be on the prototype.
This should be improved later on. 

> Can you please ask the working group to have a normative definition of the
> ReadableStreamController?

ok

[WebKit Commit Bot]

Comment on attachment 250201 [details]
Introducing controller

Clearing flags on attachment: 250201

Committed r182591: <http://trac.webkit.org/changeset/182591>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.