Bug 147978 – [Streams API] Update implementation with the latest spec

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Xabier Rodríguez Calvar]

ç[Streams API] Update implementation and tests with the latest spec

[Xabier Rodríguez Calvar]

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.

[youenn fablet]

(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.

[Xabier Rodríguez Calvar]

(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.

[youenn fablet]

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.

[Xabier Rodríguez Calvar]

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.

[youenn fablet]

(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.

[Xabier Rodríguez Calvar]

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.

[Xabier Rodríguez Calvar]

(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+.

[youenn fablet]

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

[Xabier Rodríguez Calvar]

(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.

[WebKit Commit Bot]

Comment on attachment 259140 [details]
Patch

Clearing flags on attachment: 259140

Committed r188580: <http://trac.webkit.org/changeset/188580>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

(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"?