Implement https://streams.spec.whatwg.org/#rs-controller-enqueue, including resolution of read promise.
Created attachment 253527 [details] Patch
Created attachment 254162 [details] Patch
Comment on attachment 254162 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254162&action=review > Source/WebCore/Modules/streams/ReadableStream.cpp:172 > + m_readRequests.first().successCallback(value); > + m_readRequests.remove(0); The logical operation to use here is takeFirst. Vector doesn’t have a convenience for takeFirst because using takeFirst is a clue that you should not be using a Vector, but rather a Deque; Vector has fast takeLast, but slow takeFirst. Actually calling "remove(0)" is an anti-pattern. > Source/WebCore/bindings/js/JSReadableStreamControllerCustom.cpp:59 > + if (stream.isErrored()) > + return exec->vm().throwException(exec, stream.error()); > + if (stream.isCloseRequested()) > + return exec->vm().throwException(exec, createTypeError(exec, ASCIILiteral("Calling enqueue on a stream which is closing"))); Do we have test coverage for both of these? I couldn’t find it. It is not good that we need to do a custom binding here. You aren’t supposed to need custom bindings just to raise an exception. It’s not good that the rules about when it’s OK to call this function are in the binding, rather than in the DOM class. We want the binding code to be solely about binding, not about the semantics of the class, if at all possible. > Source/WebCore/bindings/js/ReadableJSStream.cpp:162 > + JSValue chunk = m_chunkQueue.first().get(); > + m_chunkQueue.remove(0); > + return chunk; Same comment about takeFirst as above. This should be a Deque, not a Vector.
Comment on attachment 254162 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254162&action=review >> Source/WebCore/Modules/streams/ReadableStream.cpp:172 >> + m_readRequests.remove(0); > > The logical operation to use here is takeFirst. Vector doesn’t have a convenience for takeFirst because using takeFirst is a clue that you should not be using a Vector, but rather a Deque; Vector has fast takeLast, but slow takeFirst. Actually calling "remove(0)" is an anti-pattern. Thanks for pointing me to Deque, I will apply to value queue when landing this patch. For the callback queue, I may need to add Deque::append similarly to Vector::append. If so, I will do that as a follow-up patch. Any reason to not use STL dedicated containers btw? >> Source/WebCore/bindings/js/JSReadableStreamControllerCustom.cpp:59 >> + return exec->vm().throwException(exec, createTypeError(exec, ASCIILiteral("Calling enqueue on a stream which is closing"))); > > Do we have test coverage for both of these? I couldn’t find it. > > It is not good that we need to do a custom binding here. You aren’t supposed to need custom bindings just to raise an exception. It’s not good that the rules about when it’s OK to call this function are in the binding, rather than in the DOM class. We want the binding code to be solely about binding, not about the semantics of the class, if at all possible. There is code coverage for both in streams/reference-implementation/readable-stream.html Tests are named as: PASS ReadableStream: enqueue should throw when the stream is closed PASS ReadableStream: enqueue should throw the stored error when the stream is errored As of where to put the exception throwing, it is not straightforward to do that within the DOM class. Binding code is expected to pass exceptions/errors as ExceptionCode. But we may want to throw a generic JSValue for instance if stream is errored. Also ReadableStreamContoller is a special DOM class. It is only expected to be used with ReadableJSStream and we could have emulated it as a pure JS class. Note also that the separation of code between ReadableStreamController binding code and ReadableJSStream tries to follow the organisation of the spec. >> Source/WebCore/bindings/js/ReadableJSStream.cpp:162 >> + return chunk; > > Same comment about takeFirst as above. This should be a Deque, not a Vector. OK
Created attachment 254261 [details] Patch for landing, including using Deque for stack of JSValue
Comment on attachment 254261 [details] Patch for landing, including using Deque for stack of JSValue Clearing flags on attachment: 254261 Committed r185197: <http://trac.webkit.org/changeset/185197>
All reviewed patches have been landed. Closing bug.
(In reply to comment #4) > Any reason to not use STL dedicated containers btw? It’s hard to answer that question; it's a really broad one because there are a lot of containers. One issue is that std::deque would use the system malloc, not the faster bmalloc used by WTF::Deque. Another is that std::vector is missing many of the features of WTF::Vector. Both class templates might have quite different memory characteristics than their WTF counterparts in terms of overhead and things like growth/shrinking strategies. Something we’d definitely have to discuss with others in the project. Not an easy choice.