Bug 145792 – [Streams API] ReadableJSStream should handle promises returned by JS source start callback

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

"start", "pull" and "cancel" may finish asynchronously by returning promises.
ReadableJSStream should handle these promises.
To start with, this should be done for the "start" callback.

[youenn fablet]

Created attachment 254636 [details]
Patch

[youenn fablet]

Created attachment 254641 [details]
Trying to fix Win build

[youenn fablet]

Comment on attachment 254641 [details]
Trying to fix Win build

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

> Source/WebCore/bindings/js/ReadableJSStream.cpp:78
> +    JSPromise* promise = jsDynamicCast<JSPromise*>(callFunction(state, function, m_source.get(), arguments));

By doing the cast here, we ensure that thenPromise is only called if JS callback returns a promise.
But the code inside thenPromise is nothing special to promise.
We could call thenPromise whenever callback returned value has a then property which is callable.

[WebKit Commit Bot]

Comment on attachment 254641 [details]
Trying to fix Win build

Rejecting attachment 254641 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 254641, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/6265508483563520

[youenn fablet]

Created attachment 254734 [details]
Fixing log

[WebKit Commit Bot]

Comment on attachment 254734 [details]
Fixing log

Clearing flags on attachment: 254734

Committed r185465: <http://trac.webkit.org/changeset/185465>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alex Christensen]

This broke at least the 32-bit mac build.
https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%2832-bit%20Build%29/builds/3693

[WebKit Commit Bot]

Re-opened since this is blocked by bug 145893

[youenn fablet]

Created attachment 254807 [details]
Cleaning NativeStdFunction definition

[youenn fablet]

Comment on attachment 254807 [details]
Cleaning NativeStdFunction definition

Let's try it again

[Darin Adler]

Comment on attachment 254807 [details]
Cleaning NativeStdFunction definition

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

> Source/JavaScriptCore/runtime/JSFunction.cpp:78
>      } else

WebKit coding style normally omits else after a return.

[Darin Adler]

Comment on attachment 254807 [details]
Cleaning NativeStdFunction definition

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

> Source/JavaScriptCore/runtime/JSFunction.cpp:92
> +struct JSStdFunction: public JSFunction {

In a struct we normally omit public since it’s the default. We also put a space before the colon here.

I’m not sure it’s right to make this a struct, though, since it’s derived from a a class. I think this should probably be a class instead.

[youenn fablet]

(In reply to comment #13)
> Comment on attachment 254807 [details]
> Cleaning NativeStdFunction definition
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254807&action=review
> 
> > Source/JavaScriptCore/runtime/JSFunction.cpp:92
> > +struct JSStdFunction: public JSFunction {
> 
> In a struct we normally omit public since it’s the default. We also put a
> space before the colon here.
> 
> I’m not sure it’s right to make this a struct, though, since it’s derived
> from a a class. I think this should probably be a class instead.

Thanks.
I am postponing cq until processing further feedback

[youenn fablet]

Created attachment 254815 [details]
Fixing JSStdFunction

[WebKit Commit Bot]

Comment on attachment 254815 [details]
Fixing JSStdFunction

Clearing flags on attachment: 254815

Committed r185537: <http://trac.webkit.org/changeset/185537>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.