Bug 147978

Created attachment 258892 [details] Patch Brought changes in the tests done at the reference. I left some of the custom ones cause I think they are not explicitely covered but I do think they are implicitly. Youenn, any opinion about leaving or removing them? I also fixed the implementation according to the latest changes in the spec.

(In reply to comment #1) > Created attachment 258892 [details] > Patch > > Brought changes in the tests done at the reference. I left some of the > custom ones cause I think they are not explicitely covered but I do think > they are implicitly. Youenn, any opinion about leaving or removing them? I > also fixed the implementation according to the latest changes in the spec. It is a bit difficult to precisely check all the changes. Could you split the patch in two: 1. Inverting the order of read/closed promise resolution (with updated related tests) 2. Updating other tests It might not be necessary to move the read promises from ReadableStream to ReadableStreamReader. The current design is to make controller and reader just dummy objects whose sole purpose is to expose specific parts of ReadableStream.

(In reply to comment #2) > (In reply to comment #1) > > Created attachment 258892 [details] > > Patch > > > > Brought changes in the tests done at the reference. I left some of the > > custom ones cause I think they are not explicitely covered but I do think > > they are implicitly. Youenn, any opinion about leaving or removing them? I > > also fixed the implementation according to the latest changes in the spec. > > It is a bit difficult to precisely check all the changes. > Could you split the patch in two: > 1. Inverting the order of read/closed promise resolution (with updated > related tests) > 2. Updating other tests I can try that. > It might not be necessary to move the read promises from ReadableStream to > ReadableStreamReader. > The current design is to make controller and reader just dummy objects whose > sole purpose is to expose specific parts of ReadableStream. The new spec says that you have to be able to resolve the promises belonging to a specific reader, so you have to track which promises belong to which reader.

A stream may have only one reader at a time. This allows storing pending requests in the stream itself. A minor point is that the reader class (several instances per stream potentially) is made smaller.

Created attachment 258998 [details] Patch As Youenn suggested, I am splitting the patch in two. In the end it is true that we don't need to move the read pending requests to the reader. Basically I renamed the releaseReader into closeReader according to the spec and did the necessary changes in promise resolution order and object release.

(In reply to comment #5) > Created attachment 258998 [details] > Patch > > As Youenn suggested, I am splitting the patch in two. In the end it is true > that we don't need to move the read pending requests to the reader. > Basically I renamed the releaseReader into closeReader according to the spec > and did the necessary changes in promise resolution order and object release. That looks mostly good to me. I would make the following "fine-tuning" tweaks to changeStateToErrored: 1. m_state could be set earlier than what the patch is doing (no behavior impact but closer to spec). 2. after setting m_state to Errored, we could exit early if m_reader is null. I would leave releaseReader renaming to another patch, as it might be good to rename at the same time ReadableStream::close into finishClosing. At the same time of renaming these methods, I would inline clearCallbacks. This would get us closer to the spec.

Created attachment 259140 [details] Patch Set m_state to Errored earlier, bailout if no m_reader and keep releaseReader as method name as suggested by Youenn.

(In reply to comment #6) > 1. m_state could be set earlier than what the patch is doing (no behavior > impact but closer to spec). True, I mixed the reader and stream state. That's why I was setting it later. I changed everything you requested and if reviewers agree, I think we are ready for a r+.

Sounds good to me. Maybe rename the bug and update change logs accordingly, at cq time or updated patch if additional changes are required.

(In reply to comment #9) > Sounds good to me. > Maybe rename the bug and update change logs accordingly, at cq time or > updated patch if additional changes are required. I can do it, but I don't think it's even needed, as it is an update according to the spec, the only point is that we don't follow the same naming conventions, which is accessory.

Comment on attachment 259140 [details] Patch Clearing flags on attachment: 259140 Committed r188580: <http://trac.webkit.org/changeset/188580>

All reviewed patches have been landed. Closing bug.

(In reply to comment #10) > (In reply to comment #9) > > Sounds good to me. > > Maybe rename the bug and update change logs accordingly, at cq time or > > updated patch if additional changes are required. > > I can do it, but I don't think it's even needed, as it is an update > according to the spec, the only point is that we don't follow the same > naming conventions, which is accessory. Bug title is a minor point but still useful. It seems to me the title should help future developers grasp quickly the scope of the patch. The current bug title seems to convey what you are trying to do (sync the impl with the spec), not what the patch is. Something like "[Streams API] ReadableStreamReader read promises should be resolved before closed promise" might have been clearer. Also, at some point, we may need again to "update implementation with the latest spec". The same applies a bit to bug 148075 as well. How about something along the lines of "[Streams API] ReadableStream close methods should better match specification naming"?