Bug 142435 – [Streams API] Closing a ReadableStream should resolve the ready and closed promises

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[youenn fablet]

As per the spec, When close() is called, the stream should be put in closed state.
At the same time, the ready and closed promises should be resolved.

[youenn fablet]

Created attachment 248149 [details]
Patch

[Benjamin Poulain]

Comment on attachment 248149 [details]
Patch

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

Please poke me on IRC when this is up for review.

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

I am quite confused by this callOnMainThread. Are stream multithread?

If you intended to resolve the promise asynchronously, it should be scheduled on the next Microtask, I don't think callOnMainThread() does that.

Can you explain this?

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

I can't get used to it :)

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

Same question for callOnMainThread()

> Source/WebCore/bindings/js/JSDOMPromise.h:101
> +inline void DeferredWrapper::resolve<JSC::JSValue>(const JSC::JSValue& value)

I would add a resolve() without argument.

> LayoutTests/streams/readablestream-start.html:52
> +test1 = async_test('ReadableStream close should fulfill ready and closed promises');

I would like tests for every aspect of the patch, including:
-Open a stream, close it. Add a callback for "closed" after a timer.
-Open a stream. Add ready callback in response to start.
-Open a stream. Add closed callback in response to start.
-Verify that neither callback are called synchronously on close.
-etc.

This is a new feature, we need shitload of tests.
The difference between a great feature and a shitty feature is flawless execution and great performance. Having overkill test coverage will give you the flawless execution.

[youenn fablet]

Created attachment 248254 [details]
Switching to postTask

[youenn fablet]

Comment on attachment 248149 [details]
Patch

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

>> Source/WebCore/Modules/streams/ReadableStream.cpp:88
>> +    callOnMainThread(std::bind(WTF::move(m_closedSuccessCallback)));
> 
> I am quite confused by this callOnMainThread. Are stream multithread?
> 
> If you intended to resolve the promise asynchronously, it should be scheduled on the next Microtask, I don't think callOnMainThread() does that.
> 
> Can you explain this?

Right, switching to ScriptExecutionContext::postTask is more appropriate.

>> Source/WebCore/bindings/js/JSDOMPromise.h:101
>> +inline void DeferredWrapper::resolve<JSC::JSValue>(const JSC::JSValue& value)
> 
> I would add a resolve() without argument.

We will add later on a reject(JSValue) which is actually needed as an error may be anything.
This function mirrors that future one.

> LayoutTests/streams/readablestream-start.html:53
> +test1.step(function()

Agreed.
I added some more tests and will continue adding more.

[youenn fablet]

Created attachment 248482 [details]
Cleaning things

[youenn fablet]

Wontfix as spec changed and no longer has a ready promise.