AudioContext resume/close/suspend should reject promises with a DOM exception in lieu of throwing exceptions

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

WebKit current implementation of AudioContext resume, close and suspend may throw exceptions in case of error.
It would make more sense to reject the promise with the exception as value.
This would be more consistent with other promise based APIs.

[Jer Noble]

That will require a spec change. I encourage you to file a spec bug at https://github.com/WebAudio/web-audio-api.

[youenn fablet]

(In reply to comment #1)
> That will require a spec change. I encourage you to file a spec bug at
> https://github.com/WebAudio/web-audio-api.

I just checked and the issue is already filed and apparently on its way to be landed in the spec: https://github.com/WebAudio/web-audio-api/issues/441

[Jer Noble]

Ok then!

[youenn fablet]

Created attachment 253274 [details]
Patch

[Build Bot]

Comment on attachment 253274 [details]
Patch

Attachment 253274 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4657628286287872

New failing tests:
fast/dom/document-contentType-DOMParser.html
fast/dom/xmlserializer-serialize-to-string-exception.html
media/track/regions-webvtt/vtt-region-constructor.html
fast/dom/DOMURL/set-href-attribute-pathname.html
fast/canvas/canvas-imageData.html
fast/canvas/canvas-2d-imageData-create-nonfinite.html
fast/dom/DOMURL/set-href-attribute-hostname.html
fast/canvas/canvas-putImageData.html
fast/dom/DOMURL/set-href-attribute-host.html
fast/dom/DOMURL/url-constructor.html
fast/canvas/canvas-getImageData-invalid.html
fast/canvas/radialGradient-infinite-values.html
fast/dom/DOMURL/set-href-attribute-hash.html
fast/canvas/linearGradient-infinite-values.html

[Build Bot]

Created attachment 253277 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 253274 [details]
Patch

Attachment 253274 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5251477308178432

New failing tests:
fast/dom/document-contentType-DOMParser.html
fast/canvas/radialGradient-infinite-values.html
fast/dom/xmlserializer-serialize-to-string-exception.html
fast/canvas/linearGradient-infinite-values.html
media/track/regions-webvtt/vtt-region-constructor.html
storage/indexeddb/objectstore-cursor.html
fast/canvas/canvas-imageData.html
fast/canvas/canvas-2d-imageData-create-nonfinite.html
fast/dom/DOMURL/set-href-attribute-hostname.html
fast/canvas/canvas-putImageData.html
fast/dom/DOMURL/set-href-attribute-host.html
fast/canvas/canvas-getImageData-invalid.html
fast/dom/DOMURL/set-href-attribute-pathname.html
fast/dom/DOMURL/url-constructor.html
storage/indexeddb/intversion-bad-parameters.html
fast/dom/DOMURL/set-href-attribute-hash.html

[Build Bot]

Created attachment 253278 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[youenn fablet]

Created attachment 253284 [details]
Fixing TypeError message

[youenn fablet]

Comment on attachment 253284 [details]
Fixing TypeError message

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

> Source/WebCore/Modules/webaudio/AudioContext.cpp:-1115
> -        scriptExecutionContext()->postTask(successCallback);

Calling postTask here is unneeded as the promise will call the JS resolve callback through postTask anyway

> Source/WebCore/Modules/webaudio/AudioContext.cpp:1120
> +        failureCallback(0);

This is a way to reject with no value, using the same mechanism as other error promise rejection.
It seems better than to keep using a ExceptionCode that would be used to reject the promise in the custom binding code.

In the future, successCallback may also take a std::nullptr_t to make promise rejection/resolution consistent and potentially automated by code generator.

[Jer Noble]

Comment on attachment 253284 [details]
Fixing TypeError message

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

>> Source/WebCore/Modules/webaudio/AudioContext.cpp:-1115
>> -        scriptExecutionContext()->postTask(successCallback);
> 
> Calling postTask here is unneeded as the promise will call the JS resolve callback through postTask anyway

I don't believe that is true, unless the Promise code has drastically changed lately.

[youenn fablet]

(In reply to comment #11)
> Comment on attachment 253284 [details]
> Fixing TypeError message
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253284&action=review
> 
> >> Source/WebCore/Modules/webaudio/AudioContext.cpp:-1115
> >> -        scriptExecutionContext()->postTask(successCallback);
> > 
> > Calling postTask here is unneeded as the promise will call the JS resolve callback through postTask anyway
> 
> I don't believe that is true, unless the Promise code has drastically
> changed lately.

My comment was not very clear, here is a longer rephrasing.

The spec says that the promise should be immediately resolved if the audio context state is already suspended. The current implementation does not do that immediately but after a micro-task.

The JS function assigned to the promise through Promise.then() will be called asynchronously by the JS promise anyway. I believe that this is implemented by postTask but maybe I am wrong here.

Let's split that as a separate bug then.

[youenn fablet]

(In reply to comment #12)
> (In reply to comment #11)
> > Comment on attachment 253284 [details]
> > Fixing TypeError message
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=253284&action=review
> > 
> > >> Source/WebCore/Modules/webaudio/AudioContext.cpp:-1115
> > >> -        scriptExecutionContext()->postTask(successCallback);
> > > 
> > > Calling postTask here is unneeded as the promise will call the JS resolve callback through postTask anyway
> > 
> > I don't believe that is true, unless the Promise code has drastically
> > changed lately.
> 
> My comment was not very clear, here is a longer rephrasing.
> 
> The spec says that the promise should be immediately resolved if the audio
> context state is already suspended. The current implementation does not do
> that immediately but after a micro-task.
> 
> The JS function assigned to the promise through Promise.then() will be
> called asynchronously by the JS promise anyway. I believe that this is
> implemented by postTask but maybe I am wrong here.
> 
> Let's split that as a separate bug then.

Filed bug 145164, a 100 above this bug ;)

[Darin Adler]

Comment on attachment 253284 [details]
Fixing TypeError message

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

> Source/JavaScriptCore/runtime/Error.h:59
> -JS_EXPORT_PRIVATE JSObject* createTypeError(ExecState*, const String&);
> +JS_EXPORT_PRIVATE JSObject* createTypeError(ExecState*, const String& message = ASCIILiteral("Type error"));

This creates unnecessary code bloat as each call site generates the literal. It’s better to overload instead of using a default argument value.

> Source/WebCore/bindings/js/JSDOMBinding.h:253
>  // Convert a DOM implementation exception code into a JavaScript exception in the execution state.

This comment describes the second function below; it’s not good to have it immediately before a function that does something subtly different!

[youenn fablet]

Created attachment 253455 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 253455 [details]
Patch

Clearing flags on attachment: 253455

Committed r184651: <http://trac.webkit.org/changeset/184651>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.