Bug 142435

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.