Summary: | [Streams API] Implement a barebone ReadableStreamReader interface | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||||||||||
Component: | WebCore Misc. | Assignee: | youenn fablet <youennf> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | benjamin, buildbot, calvaris, commit-queue, rniwa | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Attachments: |
|
Description
youenn fablet
2015-03-19 06:55:55 PDT
Created attachment 249102 [details]
Patch
Comment on attachment 249102 [details] Patch Attachment 249102 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5740958960320512 New failing tests: js/dom/global-constructors-attributes.html Created attachment 249103 [details]
Archive of layout-test-results from ews102 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 249102 [details] Patch Attachment 249102 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4727133540712448 New failing tests: js/dom/global-constructors-attributes.html Created attachment 249104 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 249105 [details]
Trying to fix expectations
Created attachment 249123 [details]
Removing unneeded state
Comment on attachment 249123 [details] Removing unneeded state View in context: https://bugs.webkit.org/attachment.cgi?id=249123&action=review > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:49 Should be moved to ReadableStreamReader constructor to ease ReadableStreamReader subclassing > Source/WebCore/Modules/streams/ReadableStreamReader.h:69 > + ReadableStream* m_stream; Should be initialized to nullptr > Source/WebCore/bindings/js/JSReadableStreamReaderCustom.cpp:118 > + RefPtr<ReadableStreamReader> readableStreamReader = ReadableStreamReader::create(stream->impl()); Should be replaced by stream->impl().createReader() so that stream decides which Reader subclass is used. Created attachment 249566 [details]
Moving suspendIfNeeded within constructor
Comment on attachment 249566 [details] Moving suspendIfNeeded within constructor View in context: https://bugs.webkit.org/attachment.cgi?id=249566&action=review Quick review. The patch does not seem to match the spec to me. Am I misreading something? > Source/WebCore/Modules/streams/ReadableStream.h:62 > + void setReader(ReadableStreamReader* reader) { m_reader = reader; } Maybe the name should also carry the information that this is a set-once operation. Something like: lockStreamToReader(ReadableStreamReader) lock(ReadableStreamReader) If I am not mistaken, the "reader" argument must be non-null. If that's the case, it should be passed by reference to the method. > Source/WebCore/Modules/streams/ReadableStream.h:64 > + // This method should be overriden by subclasses to create more specific stream readers. > + virtual Ref<ReadableStreamReader> createReader(); I would just make this virtual when needed. No point in paying this cost now. > Source/WebCore/Modules/streams/ReadableStream.h:77 > + RefPtr<ReadableStreamReader> m_reader; Shouldn't the ref() be in the other direction. You can get to ReaderStream from ReadableStreamReader, but you can't get an existing ReadableStreamReader from ReaderStream. > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:66 > +void ReadableStreamReader::setStream(ReadableStream& stream) Any reason this is not done by the constructor? > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:85 > + // FIXME: We should try and do better here. > + return false; We recently discovered that ActiveDOMObject impact on the page cache is larger than expected. We will have to be careful with this before enabling Streams by default. > Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:63 > + if (impl().reader()) > + return toJS(exec, globalObject(), impl().reader()); > + return toJS(exec, globalObject(), impl().createReader()); Does this match the spec? In the case when calling getReader() when there is already an existing reader. How I read it is: -https://streams.spec.whatwg.org/#rs-get-reader is AcquireReadableStreamReader(this) -https://streams.spec.whatwg.org/#acquire-readable-stream-reader creates a new object every time. -The constructor of ReadableStreamReader would then fail since the Stream is locked https://streams.spec.whatwg.org/#reader-constructor What am I missing? (In reply to comment #10) > Comment on attachment 249566 [details] > Moving suspendIfNeeded within constructor > > View in context: > https://bugs.webkit.org/attachment.cgi?id=249566&action=review > > Quick review. The patch does not seem to match the spec to me. Am I > misreading something? Thanks for the review. Patch does not align to the spec and will be corrected shortly. > > Source/WebCore/Modules/streams/ReadableStream.h:62 > > + void setReader(ReadableStreamReader* reader) { m_reader = reader; } > > Maybe the name should also carry the information that this is a set-once > operation. Something like: > lockStreamToReader(ReadableStreamReader) > lock(ReadableStreamReader) > > If I am not mistaken, the "reader" argument must be non-null. If that's the > case, it should be passed by reference to the method. It was planned to use setReader with nullptr to release the stream. But it will be more readable to use more meaningful method names. lock and release sounds ok. > > Source/WebCore/Modules/streams/ReadableStream.h:64 > > + // This method should be overriden by subclasses to create more specific stream readers. > > + virtual Ref<ReadableStreamReader> createReader(); > > I would just make this virtual when needed. No point in paying this cost now. OK. A future patch should make it virtual pure. > > Source/WebCore/Modules/streams/ReadableStream.h:77 > > + RefPtr<ReadableStreamReader> m_reader; > > Shouldn't the ref() be in the other direction. > > You can get to ReaderStream from ReadableStreamReader, but you can't get an > existing ReadableStreamReader from ReaderStream. Right. > > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:66 > > +void ReadableStreamReader::setStream(ReadableStream& stream) > > Any reason this is not done by the constructor? > > > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:85 > > + // FIXME: We should try and do better here. > > + return false; > > We recently discovered that ActiveDOMObject impact on the page cache is > larger than expected. > > We will have to be careful with this before enabling Streams by default. > Do you have a pointer to better understand this and the granularity we should try to reach? > > Source/WebCore/bindings/js/JSReadableStreamCustom.cpp:63 > > + if (impl().reader()) > > + return toJS(exec, globalObject(), impl().reader()); > > + return toJS(exec, globalObject(), impl().createReader()); > > Does this match the spec? > > In the case when calling getReader() when there is already an existing > reader. > > How I read it is: > -https://streams.spec.whatwg.org/#rs-get-reader is > AcquireReadableStreamReader(this) > -https://streams.spec.whatwg.org/#acquire-readable-stream-reader creates a > new object every time. > -The constructor of ReadableStreamReader would then fail since the Stream is > locked https://streams.spec.whatwg.org/#reader-constructor > > What am I missing? Nothing, you are just plain right! Created attachment 249730 [details]
Updated according review
Comment on attachment 249730 [details] Updated according review View in context: https://bugs.webkit.org/attachment.cgi?id=249730&action=review Everything looks good to me. Some comments: > Source/WebCore/Modules/streams/ReadableStream.h:62 > + void lock(ReadableStreamReader& reader) { m_reader = &reader; } Can this ASSERT_WITH_MESSAGE(!reader())? If I am not mistaken, it is never valid to replace the Reader on a locked stream. > Source/WebCore/Modules/streams/ReadableStream.h:63 > + Ref<ReadableStreamReader> createReader(); On the WebCore side, you have two ways to instantiate the Reader: ReadableStream::createReader() and ReadableStreamReader::create(). Unless you plan to use this as a factory, it would be better to get rid of ReadableStream::createReader(). > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:64 > +} In the destructor, you should unlock the ReadableStream. Since they are both ref-counted, some other object may hold a reference onto the ReadableStream, keeping it alive. The link ReadableStream->ReadableStreamReader must be cut here or we risk a dangling pointer. (In reply to comment #11) > > > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:66 > > > +void ReadableStreamReader::setStream(ReadableStream& stream) > > > > Any reason this is not done by the constructor? > > > > > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:85 > > > + // FIXME: We should try and do better here. > > > + return false; > > > > We recently discovered that ActiveDOMObject impact on the page cache is > > larger than expected. > > > > We will have to be careful with this before enabling Streams by default. > > > > Do you have a pointer to better understand this and the granularity we > should try to reach? I think Closed and Errored state should have no problem being suspended? Ready would be good but that would depend on the source. We should not keep a socket or a file open in the page cache. (In reply to comment #13) > Comment on attachment 249730 [details] > Updated according review > > View in context: > https://bugs.webkit.org/attachment.cgi?id=249730&action=review > > Everything looks good to me. Some comments: > > > Source/WebCore/Modules/streams/ReadableStream.h:62 > > + void lock(ReadableStreamReader& reader) { m_reader = &reader; } > > Can this ASSERT_WITH_MESSAGE(!reader())? > > If I am not mistaken, it is never valid to replace the Reader on a locked > stream. OK Let's add this assert in reader constructor. > > > Source/WebCore/Modules/streams/ReadableStream.h:63 > > + Ref<ReadableStreamReader> createReader(); > > On the WebCore side, you have two ways to instantiate the Reader: > ReadableStream::createReader() and ReadableStreamReader::create(). > > Unless you plan to use this as a factory, it would be better to get rid of > ReadableStream::createReader(). Next patch will remove ReadableStreamReader::create and make ReadableStream::createReader virtual pure. > > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:64 > > +} > > In the destructor, you should unlock the ReadableStream. > > Since they are both ref-counted, some other object may hold a reference onto > the ReadableStream, keeping it alive. The link > ReadableStream->ReadableStreamReader must be cut here or we risk a dangling > pointer. I thought about this and forgot to properly fix it, thanks! Later on, we will need to ensure that ReadableStreamReader does not get collected if it is locking a stream. (In reply to comment #15) > (In reply to comment #13) > > Comment on attachment 249730 [details] > > Updated according review > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=249730&action=review > > > > Everything looks good to me. Some comments: > > > > > Source/WebCore/Modules/streams/ReadableStream.h:62 > > > + void lock(ReadableStreamReader& reader) { m_reader = &reader; } > > > > Can this ASSERT_WITH_MESSAGE(!reader())? > > > > If I am not mistaken, it is never valid to replace the Reader on a locked > > stream. > > OK > Let's add this assert in reader constructor. > > > > > > Source/WebCore/Modules/streams/ReadableStream.h:63 > > > + Ref<ReadableStreamReader> createReader(); > > > > On the WebCore side, you have two ways to instantiate the Reader: > > ReadableStream::createReader() and ReadableStreamReader::create(). > > > > Unless you plan to use this as a factory, it would be better to get rid of > > ReadableStream::createReader(). > > Next patch will remove ReadableStreamReader::create and make > ReadableStream::createReader virtual pure. Next patch meaning bug 143130 Created attachment 249806 [details]
Patch for landing
Comment on attachment 249806 [details] Patch for landing Clearing flags on attachment: 249806 Committed r182180: <http://trac.webkit.org/changeset/182180> All reviewed patches have been landed. Closing bug. |