Bug 141162

Created attachment 245880 [details] Patch

Created attachment 246158 [details] Patch

Created attachment 248074 [details] Rebasing

Comment on attachment 248074 [details] Rebasing View in context: https://bugs.webkit.org/attachment.cgi?id=248074&action=review Quick look: > Source/WebCore/ChangeLog:3 > + [Streams API] ReadableStream constructor start function should be able to error the stream Two spaces between "constructor" and "start". > Source/WebCore/ChangeLog:12 > + Calling error callback from the JS start function. > + Rejecting closed promise when readablestream is errored. > + Adding support to reject promise with any JSValue. > + Added isJS to ReadableStreamSource so that JSReadableStream can get access to more meaningful errors (generic JSValue) > + instead of Strings without the JSValue to transit through ReadableStream. In this part of the changelog, before detailing the changes, it is best to use paragraph explaining the global goal of the patch. > Source/WebCore/Modules/streams/ReadableStreamSource.h:63 > + virtual const String& errorDescription() = 0; > + > + virtual bool isJS() { return false; } Shouldn't those be const? > Source/WebCore/bindings/js/ReadableStreamJSSource.h:61 > + virtual bool isJS() { return true; } > + virtual const String& errorDescription(); Missing override here.

Created attachment 248257 [details] Patch

Created attachment 248485 [details] Cleaning things

Created attachment 249570 [details] Adding reader support

Created attachment 252465 [details] Rebasing for landing

Created attachment 252588 [details] Rebasing

Comment on attachment 252588 [details] Rebasing View in context: https://bugs.webkit.org/attachment.cgi?id=252588&action=review > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:126 > + if (m_closedErrorCallback) { > + ClosedErrorCallback closedErrorCallback = WTF::move(m_closedErrorCallback); > + closedErrorCallback(); > + } > + ASSERT(!m_closedErrorCallback); > + m_closedSuccessCallback = nullptr; Even though this matches the old code. I’m not sure I like it written this way. Is it important to null out the callback before we call it? If so, then the WTF::move is OK, but if not we should do the more straightforward: if (m_closedErrorCallback) m_closedErrorCallback(); m_closedErrorCallback = nullptr; m_closedSuccessCallback = nullptr; The local variable, the move, and the assertion all seem like overkill that unnecessarily obscure the logic. It would also be nice to share the code that nulls out both callbacks and releases the stream rather than including it twice. > Source/WebCore/bindings/js/ReadableJSStream.cpp:144 > + Ref<Reader> reader = Reader::create(*this); Extra space here. > Source/WebCore/bindings/js/ReadableJSStream.cpp:147 > + return reader.get(); Do we really need the ".get()" here? I’m surprised. In fact, I think that adds in reference count churn that we’d like to avoid. > Source/WebCore/bindings/js/ReadableJSStream.cpp:176 > + Reader* reader = static_cast<Reader*>(this->reader()); > + if (reader) > + reader->storeError(exec, error); Is this typecast really needed? Could we override the reader() function to know the more specific type? That’s a pattern we use elsewhere, such as in the render tree classes. Would be nice to declare this inside the if rather than just before it: if (auto reader = this->reader())

Created attachment 252713 [details] Patch for landing

Created attachment 252714 [details] Patch for landing

Thanks for the review. See inline for some comments. (In reply to comment #10) > Comment on attachment 252588 [details] > Rebasing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=252588&action=review > > > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:126 > > + if (m_closedErrorCallback) { > > + ClosedErrorCallback closedErrorCallback = WTF::move(m_closedErrorCallback); > > + closedErrorCallback(); > > + } > > + ASSERT(!m_closedErrorCallback); > > + m_closedSuccessCallback = nullptr; > > Even though this matches the old code. I’m not sure I like it written this > way. Is it important to null out the callback before we call it? If so, then > the WTF::move is OK, but if not we should do the more straightforward: > > if (m_closedErrorCallback) > m_closedErrorCallback(); > m_closedErrorCallback = nullptr; > m_closedSuccessCallback = nullptr; > > The local variable, the move, and the assertion all seem like overkill that > unnecessarily obscure the logic. > > It would also be nice to share the code that nulls out both callbacks and > releases the stream rather than including it twice. I like that. Let's do that as a follow-up patch. > > Source/WebCore/bindings/js/ReadableJSStream.cpp:144 > > + Ref<Reader> reader = Reader::create(*this); > > Extra space here. Fixed. > > Source/WebCore/bindings/js/ReadableJSStream.cpp:147 > > + return reader.get(); > > Do we really need the ".get()" here? I’m surprised. In fact, I think that > adds in reference count churn that we’d like to avoid. Fixed using static_reference_cast instead. > > Source/WebCore/bindings/js/ReadableJSStream.cpp:176 > > + Reader* reader = static_cast<Reader*>(this->reader()); > > + if (reader) > > + reader->storeError(exec, error); > > Is this typecast really needed? Could we override the reader() function to > know the more specific type? That’s a pattern we use elsewhere, such as in > the render tree classes. AFAICT, it will be the only case where we will need to do this cast. Let's refactor it if more cases happen. > Would be nice to declare this inside the if rather than just before it: > > if (auto reader = this->reader()) Fixed.

Comment on attachment 252714 [details] Patch for landing Clearing flags on attachment: 252714 Committed r183991: <http://trac.webkit.org/changeset/183991>

All reviewed patches have been landed. Closing bug.