Bug 141162 – [Streams API] ReadableStream constructor start function should be able to error the stream

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

[youenn fablet]

As per https://streams.spec.whatwg.org/#rs-constructor, the start parameter function should be able to throw.
In that case, the error should be rethrown.

[youenn fablet]

Created attachment 245880 [details]
Patch

[youenn fablet]

Created attachment 246158 [details]
Patch

[youenn fablet]

Created attachment 248074 [details]
Rebasing

[Benjamin Poulain]

Comment on attachment 248074 [details]
Rebasing

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

Quick look:

> Source/WebCore/ChangeLog:3
> +        [Streams API] ReadableStream constructor  start function should be able to error the stream

Two spaces between "constructor" and "start".

> Source/WebCore/ChangeLog:12
> +        Calling error callback from the JS start function.
> +        Rejecting closed promise when readablestream is errored.
> +        Adding support to reject promise with any JSValue.
> +        Added isJS to ReadableStreamSource so that JSReadableStream can get access to more meaningful errors (generic JSValue) 
> +        instead of Strings without the JSValue to transit through ReadableStream.

In this part of the changelog, before detailing the changes, it is best to use paragraph explaining the global goal of the patch.

> Source/WebCore/Modules/streams/ReadableStreamSource.h:63
> +    virtual const String& errorDescription() = 0;
> +
> +    virtual bool isJS() { return false; }

Shouldn't those be const?

> Source/WebCore/bindings/js/ReadableStreamJSSource.h:61
> +    virtual bool isJS() { return true; }
> +    virtual const String& errorDescription();

Missing override here.

[youenn fablet]

Created attachment 248257 [details]
Patch

[youenn fablet]

Created attachment 248485 [details]
Cleaning things

[youenn fablet]

Created attachment 249570 [details]
Adding reader support

[youenn fablet]

Created attachment 252465 [details]
Rebasing for landing

[youenn fablet]

Created attachment 252588 [details]
Rebasing

[Darin Adler]

Comment on attachment 252588 [details]
Rebasing

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

> Source/WebCore/Modules/streams/ReadableStreamReader.cpp:126
> +    if (m_closedErrorCallback) {
> +        ClosedErrorCallback closedErrorCallback = WTF::move(m_closedErrorCallback);
> +        closedErrorCallback();
> +    }
> +    ASSERT(!m_closedErrorCallback);
> +    m_closedSuccessCallback = nullptr;

Even though this matches the old code. I’m not sure I like it written this way. Is it important to null out the callback before we call it? If so, then the WTF::move is OK, but if not we should do the more straightforward:

    if (m_closedErrorCallback)
        m_closedErrorCallback();
    m_closedErrorCallback = nullptr;
    m_closedSuccessCallback = nullptr;

The local variable, the move, and the assertion all seem like overkill that unnecessarily obscure the logic.

It would also be nice to share the code that nulls out both callbacks and releases the stream rather than including it twice.

> Source/WebCore/bindings/js/ReadableJSStream.cpp:144
> +    Ref<Reader> reader =  Reader::create(*this);

Extra space here.

> Source/WebCore/bindings/js/ReadableJSStream.cpp:147
> +    return reader.get();

Do we really need the ".get()" here? I’m surprised. In fact, I think that adds in reference count churn that we’d like to avoid.

> Source/WebCore/bindings/js/ReadableJSStream.cpp:176
> +    Reader* reader = static_cast<Reader*>(this->reader());
> +    if (reader)
> +        reader->storeError(exec, error);

Is this typecast really needed? Could we override the reader() function to know the more specific type? That’s a pattern we use elsewhere, such as in the render tree classes.

Would be nice to declare this inside the if rather than just before it:

    if (auto reader = this->reader())

[youenn fablet]

Created attachment 252713 [details]
Patch for landing

[youenn fablet]

Created attachment 252714 [details]
Patch for landing

[youenn fablet]

Thanks for the review.
See inline for some comments.

(In reply to comment #10)
> Comment on attachment 252588 [details]
> Rebasing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252588&action=review
> 
> > Source/WebCore/Modules/streams/ReadableStreamReader.cpp:126
> > +    if (m_closedErrorCallback) {
> > +        ClosedErrorCallback closedErrorCallback = WTF::move(m_closedErrorCallback);
> > +        closedErrorCallback();
> > +    }
> > +    ASSERT(!m_closedErrorCallback);
> > +    m_closedSuccessCallback = nullptr;
> 
> Even though this matches the old code. I’m not sure I like it written this
> way. Is it important to null out the callback before we call it? If so, then
> the WTF::move is OK, but if not we should do the more straightforward:
> 
>     if (m_closedErrorCallback)
>         m_closedErrorCallback();
>     m_closedErrorCallback = nullptr;
>     m_closedSuccessCallback = nullptr;
> 
> The local variable, the move, and the assertion all seem like overkill that
> unnecessarily obscure the logic.
> 
> It would also be nice to share the code that nulls out both callbacks and
> releases the stream rather than including it twice.

I like that. Let's do that as a follow-up patch.

> > Source/WebCore/bindings/js/ReadableJSStream.cpp:144
> > +    Ref<Reader> reader =  Reader::create(*this);
> 
> Extra space here.

Fixed.

> > Source/WebCore/bindings/js/ReadableJSStream.cpp:147
> > +    return reader.get();
> 
> Do we really need the ".get()" here? I’m surprised. In fact, I think that
> adds in reference count churn that we’d like to avoid.

Fixed using static_reference_cast instead.

> > Source/WebCore/bindings/js/ReadableJSStream.cpp:176
> > +    Reader* reader = static_cast<Reader*>(this->reader());
> > +    if (reader)
> > +        reader->storeError(exec, error);
> 
> Is this typecast really needed? Could we override the reader() function to
> know the more specific type? That’s a pattern we use elsewhere, such as in
> the render tree classes.

AFAICT, it will be the only case where we will need to do this cast.
Let's refactor it if more cases happen.

> Would be nice to declare this inside the if rather than just before it:
> 
>     if (auto reader = this->reader())

Fixed.

[WebKit Commit Bot]

Comment on attachment 252714 [details]
Patch for landing

Clearing flags on attachment: 252714

Committed r183991: <http://trac.webkit.org/changeset/183991>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.