Bug 145164 – AudioContext suspend/resume/close should resolve promises immediately when state is already suspended/active/closed

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

This is a split from https://bugs.webkit.org/show_bug.cgi?id=145064.
AudioContext suspend/resume/close methods return promises.
In some cases, they can immediately resolve/reject promises.
The current implementation is doing the resolve/reject of promises asynchronously using postTask.

This may cause some inconsistencies in the order which promises callbacks are actually executed.

[youenn fablet]

Created attachment 253376 [details]
Patch

[youenn fablet]

Comment on attachment 253376 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Test: webaudio/audiocontext-promise.html

Tried this test on Google Chrome and it actually fails.
It seems that pure JS promise callbacks are resolved first in Chrome.

I did a test mixing AudioContext and ReadableStreamReader promises on WebKit.
As expected, the execution order does not seem right, AudioContext callbacks being always called after ReadableStream callbacks.

We should be consistent in our various promise-based APIs, at least align ReadableStream and AudioContext.

[Jer Noble]

The spec is actually ambiguous on a couple of these cases. And even the meaning of "immediately" is ambiguous.  We should push the WG to adopt unambiguous language, e.g. resume(): "If the context is already running, return a resolved promise().", "If the context is closed, return a rejected promise().", etc.

[Jer Noble]

I filed a spec bug: <https://github.com/WebAudio/web-audio-api/issues/535>

[youenn fablet]

(In reply to comment #4)
> I filed a spec bug: <https://github.com/WebAudio/web-audio-api/issues/535>

Great!
Let's continue the discussion there.

[Darin Adler]

My review on the other bug does not mean I approve this change. It’s Jer and Sam who should be discussing this change to how promises work with audio in a WebKit context. I don’t know enough about it.

[youenn fablet]

It seems the discussion in https://github.com/WebAudio/web-audio-api/issues/535 is converging towards making the change.

Time to review the patch?

[Jer Noble]

Comment on attachment 253376 [details]
Patch

Yes indeed. r=me.

[WebKit Commit Bot]

Comment on attachment 253376 [details]
Patch

Clearing flags on attachment: 253376

Committed r184641: <http://trac.webkit.org/changeset/184641>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.