Bug 141162 - [Streams API] ReadableStream constructor start function should be able to error the stream
Summary: [Streams API] ReadableStream constructor start function should be able to err...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on: 141045 141160 142435
Blocks: 142478
  Show dependency treegraph
 
Reported: 2015-02-02 08:28 PST by youenn fablet
Modified: 2015-05-08 03:45 PDT (History)
6 users (show)

See Also:


Attachments
Patch (8.04 KB, patch)
2015-02-02 08:43 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (8.35 KB, patch)
2015-02-06 03:34 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing (13.28 KB, patch)
2015-03-06 10:00 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (15.92 KB, patch)
2015-03-09 10:57 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Cleaning things (17.10 KB, patch)
2015-03-11 21:19 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Adding reader support (18.34 KB, patch)
2015-03-27 08:10 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing for landing (33.42 KB, patch)
2015-05-06 03:36 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing (17.89 KB, patch)
2015-05-07 05:45 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (17.91 KB, patch)
2015-05-08 02:49 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (17.91 KB, patch)
2015-05-08 02:52 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2015-02-02 08:28:06 PST
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.
Comment 1 youenn fablet 2015-02-02 08:43:48 PST
Created attachment 245880 [details]
Patch
Comment 2 youenn fablet 2015-02-06 03:34:50 PST
Created attachment 246158 [details]
Patch
Comment 3 youenn fablet 2015-03-06 10:00:15 PST
Created attachment 248074 [details]
Rebasing
Comment 4 Benjamin Poulain 2015-03-08 21:09:51 PDT
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.
Comment 5 youenn fablet 2015-03-09 10:57:45 PDT
Created attachment 248257 [details]
Patch
Comment 6 youenn fablet 2015-03-11 21:19:01 PDT
Created attachment 248485 [details]
Cleaning things
Comment 7 youenn fablet 2015-03-27 08:10:04 PDT
Created attachment 249570 [details]
Adding reader support
Comment 8 youenn fablet 2015-05-06 03:36:32 PDT
Created attachment 252465 [details]
Rebasing for landing
Comment 9 youenn fablet 2015-05-07 05:45:10 PDT
Created attachment 252588 [details]
Rebasing
Comment 10 Darin Adler 2015-05-07 09:10:24 PDT
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())
Comment 11 youenn fablet 2015-05-08 02:49:35 PDT
Created attachment 252713 [details]
Patch for landing
Comment 12 youenn fablet 2015-05-08 02:52:38 PDT
Created attachment 252714 [details]
Patch for landing
Comment 13 youenn fablet 2015-05-08 02:56:04 PDT
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.
Comment 14 WebKit Commit Bot 2015-05-08 03:45:30 PDT
Comment on attachment 252714 [details]
Patch for landing

Clearing flags on attachment: 252714

Committed r183991: <http://trac.webkit.org/changeset/183991>
Comment 15 WebKit Commit Bot 2015-05-08 03:45:37 PDT
All reviewed patches have been landed.  Closing bug.