Bug 145110 – [Streams API] Migrate closed promise handling from ReadableStreamReader to ReadableStream

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Following on bug 144907, resolution of closed promise should be moved to ReadableStream.

[youenn fablet]

Created attachment 253294 [details]
Patch

[youenn fablet]

Comment on attachment 253294 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253294&action=review

> Source/WebCore/Modules/streams/ReadableStream.h:79
> +    typedef std::function<void(JSC::JSValue)> ClosedFailureCallback;

I should have probably made the parameter a const JSC::JSValue&.
Can be updated at landing time or in a future version of the patch.

[youenn fablet]

Created attachment 253297 [details]
Fixing unlocking of client + style

[Darin Adler]

Comment on attachment 253297 [details]
Fixing unlocking of client + style

View in context: https://bugs.webkit.org/attachment.cgi?id=253297&action=review

> Source/WebCore/Modules/streams/ReadableStream.cpp:61
> +void ReadableStream::cleanCallbacks()

I would say “clear” here, not “clean”.

> Source/WebCore/Modules/streams/ReadableStream.cpp:-66
> -    if (!m_reader)
> -        return;

What guarantees m_reader is non-null? I don’t think we want to append a null pointer to m_releasedReaders.

> Source/WebCore/Modules/streams/ReadableStream.cpp:77
> +    if (!m_closedSuccessCallback)
> +        return;
> +    m_closedSuccessCallback();
> +    cleanCallbacks();

What if m_closedFailureCallback is non-null, but m_closedSuccessCallback is null? We are going to keep it around here and not clear it. Is that really what we want? I think we want to clear the callbacks unconditionally.

> Source/WebCore/Modules/streams/ReadableStream.cpp:-77
> -    if (!m_reader)
> -        return;

What guarantees m_reader is non-null? I don’t think we want to append a null pointer to m_releasedReaders.

> Source/WebCore/Modules/streams/ReadableStream.cpp:90
> +    if (!m_closedFailureCallback)
> +        return;
> +    m_closedFailureCallback(error());
> +    cleanCallbacks();

What if m_closedFailureCallback is non-null, but m_closedSuccessCallback is null? We are going to keep it around here and not clear it. Is that really what we want? I think we want to clear the callbacks unconditionally.

> Source/WebCore/Modules/streams/ReadableStream.h:64
>      ReadableStreamReader& getReader();

This is not our normal naming scheme. We call this something like ensureReader, I think. We had a discussion of this a while back.

> Source/WebCore/Modules/streams/ReadableStream.h:66
> +    const ReadableStreamReader* reader() const { return m_reader.get(); }

This should be right next to getReader above since they are a closely related pair, not after the isLocked function. I also think it’s strange that "has a reader" and "is locked" are the same concept.

> Source/WebCore/Modules/streams/ReadableStream.h:79
> +    typedef std::function<void(const JSC::JSValue&)> ClosedFailureCallback;

This should just be JSC::JSValue. We don’t want to pass those by const&.

> Source/WebCore/Modules/streams/ReadableStreamReader.cpp:39
> +    if (m_stream.isReadable() && m_stream.reader()  != this) {

Extra space here before the "!=".

> Source/WebCore/Modules/streams/ReadableStreamReader.h:52
>      ReadableStreamReader(ReadableStream& stream)
>          : m_stream(stream) { };

Semicolon here should be removed. Also, I suggest either putting this all on one line rather than two lines.

[youenn fablet]

Comment on attachment 253297 [details]
Fixing unlocking of client + style

Thanks for the review.
I will fix most points when landing.

As of getReader()/ensureReader(), there are pros and cons, since getReader maps to its JS counter-part and I am not sure native code will use readers.


View in context: https://bugs.webkit.org/attachment.cgi?id=253297&action=review

>> Source/WebCore/Modules/streams/ReadableStream.cpp:61
>> +void ReadableStream::cleanCallbacks()
> 
> I would say “clear” here, not “clean”.

OK

>> Source/WebCore/Modules/streams/ReadableStream.cpp:-66
>> -        return;
> 
> What guarantees m_reader is non-null? I don’t think we want to append a null pointer to m_releasedReaders.

OK

>> Source/WebCore/Modules/streams/ReadableStream.cpp:77
>> +    cleanCallbacks();
> 
> What if m_closedFailureCallback is non-null, but m_closedSuccessCallback is null? We are going to keep it around here and not clear it. Is that really what we want? I think we want to clear the callbacks unconditionally.

Agreed, I fixed that in bug 144790 latest patch.
But I will update it here before landing.

>> Source/WebCore/Modules/streams/ReadableStream.cpp:-77
>> -        return;
> 
> What guarantees m_reader is non-null? I don’t think we want to append a null pointer to m_releasedReaders.

OK

>> Source/WebCore/Modules/streams/ReadableStream.cpp:90
>> +    cleanCallbacks();
> 
> What if m_closedFailureCallback is non-null, but m_closedSuccessCallback is null? We are going to keep it around here and not clear it. Is that really what we want? I think we want to clear the callbacks unconditionally.

OK

>> Source/WebCore/Modules/streams/ReadableStream.h:64
>>      ReadableStreamReader& getReader();
> 
> This is not our normal naming scheme. We call this something like ensureReader, I think. We had a discussion of this a while back.

But it maps to the getReader() JS function.
I named it first createReader() because it always creates a reader.
Changed to getReader now that we return a &.

>> Source/WebCore/Modules/streams/ReadableStream.h:66
>> +    const ReadableStreamReader* reader() const { return m_reader.get(); }
> 
> This should be right next to getReader above since they are a closely related pair, not after the isLocked function. I also think it’s strange that "has a reader" and "is locked" are the same concept.

OK to move reader() close to getReader()

>> Source/WebCore/Modules/streams/ReadableStream.h:79
>> +    typedef std::function<void(const JSC::JSValue&)> ClosedFailureCallback;
> 
> This should just be JSC::JSValue. We don’t want to pass those by const&.

Out of curiosity, what is the rationale behind that?

>> Source/WebCore/Modules/streams/ReadableStreamReader.cpp:39
>> +    if (m_stream.isReadable() && m_stream.reader()  != this) {
> 
> Extra space here before the "!=".

OK

>> Source/WebCore/Modules/streams/ReadableStreamReader.h:52
>>          : m_stream(stream) { };
> 
> Semicolon here should be removed. Also, I suggest either putting this all on one line rather than two lines.

OK

[youenn fablet]

Created attachment 253389 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 253389 [details]
Patch for landing

Clearing flags on attachment: 253389

Committed r184585: <http://trac.webkit.org/changeset/184585>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 253297 [details]
Fixing unlocking of client + style

View in context: https://bugs.webkit.org/attachment.cgi?id=253297&action=review

>>> Source/WebCore/Modules/streams/ReadableStream.h:79
>>> +    typedef std::function<void(const JSC::JSValue&)> ClosedFailureCallback;
>> 
>> This should just be JSC::JSValue. We don’t want to pass those by const&.
> 
> Out of curiosity, what is the rationale behind that?

JSC::JSValue is a single 64-bit value and fits in a register on our typical 64-bit platforms. Needlessly passing it by const& just makes code with unnecessarily poorer performance. Analogous passing const int& instead of int.

Also, there’s always a small downside to passing anything as a const& instead of a value, since it has happened in practice that someone passed a data member, and then while the code was running the object it was passed in from was deallocated and overwritten. That can’t happen when passing by value so passing by value should be the default and passing a const& should be considered an optimization.

Even for objects that are a bit larger, if they have simple semantics it’s better to pass them by value instead of const& as long as it’s efficient to copy the object. A WebKit example of this is StringView and you’ll see we never pass a const StringView&.