[Streams API] Finish pulling must always be done asynchronously as it is the expected promise behavior (according to the spec)

[Xabier Rodríguez Calvar]

[Streams API] Fix readable-stream pull test

[Xabier Rodríguez Calvar]

Created attachment 255745 [details]
Patch

Add a delay to simulate the promise resolution

[youenn fablet]

Comment on attachment 255745 [details]
Patch

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

> Source/WebCore/Modules/streams/ReadableStream.cpp:142
> +            finishPulling();

Sounds good to me. I wonder whether this is necessary for native sources though.

The same principle might be useful for doCancel as well if that is observable.

Not sure the comment is needed.

[Darin Adler]

Comment on attachment 255745 [details]
Patch

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

> Source/WebCore/Modules/streams/ReadableStream.cpp:139
> +        // We need to post the task to simulate the delay in promise resolution.

I agree that this comment isn’t really all that helpful.

> Source/WebCore/Modules/streams/ReadableStream.cpp:140
> +        RefPtr<ReadableStream> protectedStream(this);

Wait, this is not right. To have this RefPtr do any good we need to capture protectedStream, not this.

[Xabier Rodríguez Calvar]

Created attachment 255760 [details]
Patch

Honored comments in bugzilla

[Xabier Rodríguez Calvar]

(In reply to comment #3)
> > Source/WebCore/Modules/streams/ReadableStream.cpp:139
> > +        // We need to post the task to simulate the delay in promise resolution.
> 
> I agree that this comment isn’t really all that helpful.

I think the comment is helpful, otherwise when reading the code people might think that there is no good reason to do that.

> > Source/WebCore/Modules/streams/ReadableStream.cpp:140
> > +        RefPtr<ReadableStream> protectedStream(this);
> 
> Wait, this is not right. To have this RefPtr do any good we need to capture
> protectedStream, not this.

Good catch, done.

[youenn fablet]

Related to the comment, would changing the name of the bug (and the change log) be sufficient as some lightweight documentation means?
Something like: "[Streams API] Finish pulling must always be done asynchronously"?

[Xabier Rodríguez Calvar]

(In reply to comment #6)
> Related to the comment, would changing the name of the bug (and the change
> log) be sufficient as some lightweight documentation means?
> Something like: "[Streams API] Finish pulling must always be done
> asynchronously"?

That can work, I could do that in the landing latch if Darin gives the r+

[Xabier Rodríguez Calvar]

Created attachment 255816 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 255816 [details]
Patch for landing

Clearing flags on attachment: 255816

Committed r186113: <http://trac.webkit.org/changeset/186113>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.