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.
Created attachment 248149 [details] Patch
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.
Created attachment 248254 [details] Switching to postTask
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.
Created attachment 248482 [details] Cleaning things
Wontfix as spec changed and no longer has a ready promise.