Templaing ReadableJSStream might allow easier support for teeing streams as well as more easily supporting ReadableByteStream. The queue management in particular could be templatized.
Created attachment 256509 [details] Patch
Attachment 256509 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/streams/ReadableStream.h:145: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 256509 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256509&action=review Still really concerned about the use of JSC::Strong here. I am increasingly thinking this belongs in JavaScriptCore not WebCore, despite the comments about where Blink chose to put it. > Source/WebCore/Modules/streams/ReadableStream.h:141 > +// Subclasses should implement error storage, pulling and cancelling. Normally we’d say “derived classes” rather than “subclasses” to match C++ terminology. > Source/WebCore/Modules/streams/ReadableStream.h:142 > +template <typename ChunkType> We typically omit out the space between template and "<". > Source/WebCore/Modules/streams/ReadableStream.h:143 > +class ReadableEnqueuingStream : public ReadableStream { Could/should put "final" here. > Source/WebCore/bindings/js/ReadableJSStream.h:60 > +template <> Again, normally no space after "template". I also think this looks better on a single line, but not sure others agree. > Source/WebCore/bindings/js/ReadableJSStream.h:61 > +class ReadableEnqueuingStream<ReadableJSStreamValue> : public ReadableStream { Is it really helpful to have this be a class template specialization? Why not have this be a non-template class? Related question; perhaps this should derive from ReadableEnqueuingStream<ReadableJSStreamValue> rather than from ReadableStream, for more code sharing. Then we’d want that class to not be final, but this one would be. I guess the problem with that is that it’s not OK to call the enqueueChunk feature from the base, so maybe not good to inherit that way. Too bad, though. > Source/WebCore/bindings/js/ReadableJSStream.h:63 > + double desiredSize() const { return m_highWaterMark - m_totalQueueSize; } Extra space here after "const".
Thanks for the review. (In reply to comment #3) > Comment on attachment 256509 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256509&action=review > > Still really concerned about the use of JSC::Strong here. I am increasingly > thinking this belongs in JavaScriptCore not WebCore, despite the comments > about where Blink chose to put I agree, although I think we can find workarounds to remove JSC::Strong uses. In addition to straightforward GC, putting ReadableStream into JSC would allow us to get closer to the spec. We would probably use UnderlyingSource to tie WebCore (HTTP, MediaStream) with JSC streams. Part of the implementation would be done in JS as well. Having it in WebCore ensures that the pipe between specific WebCore producer and consumer can be made efficient (HTTP to MediaStream e.g.). I am thinking that this would still be possible to do it even if streams are in JSC, but further validating this would be good. It was also helping to have everything in WebCore so that the queuing is made efficient (no use of promises all the time). But early optimization is usually bad so probably not a strong point. > > > Source/WebCore/Modules/streams/ReadableStream.h:141 > > +// Subclasses should implement error storage, pulling and cancelling. > > Normally we’d say “derived classes” rather than “subclasses” to match C++ > terminology. OK > > > Source/WebCore/Modules/streams/ReadableStream.h:142 > > +template <typename ChunkType> > > We typically omit out the space between template and "<". OK > > > Source/WebCore/Modules/streams/ReadableStream.h:143 > > +class ReadableEnqueuingStream : public ReadableStream { > > Could/should put "final" here. > > > Source/WebCore/bindings/js/ReadableJSStream.h:60 > > +template <> > > Again, normally no space after "template". I also think this looks better on > a single line, but not sure others agree. OK > > > Source/WebCore/bindings/js/ReadableJSStream.h:61 > > +class ReadableEnqueuingStream<ReadableJSStreamValue> : public ReadableStream { > > Is it really helpful to have this be a class template specialization? Why > not have this be a non-template class? That could be done too. > > Related question; perhaps this should derive from > ReadableEnqueuingStream<ReadableJSStreamValue> rather than from > ReadableStream, for more code sharing. Then we’d want that class to not be > final, but this one would be. I guess the problem with that is that it’s not > OK to call the enqueueChunk feature from the base, so maybe not good to > inherit that way. Too bad, though. We could add an incrementSize() method and specialize it for ReadableJSStreamValue. > > > Source/WebCore/bindings/js/ReadableJSStream.h:63 > > + double desiredSize() const { return m_highWaterMark - m_totalQueueSize; } > > Extra space here after "const". OK
Created attachment 256679 [details] Patch for landing
(In reply to comment #5) > Created attachment 256679 [details] > Patch for landing I fixed the style issues and final. I will further check improvements at my return in two weeks.
Comment on attachment 256679 [details] Patch for landing Clearing flags on attachment: 256679 Committed r186740: <http://trac.webkit.org/changeset/186740>
All reviewed patches have been landed. Closing bug.