Bug 109674 – start and stop attributes in AudioBufferSourceNode should include "raises (exception)" in IDL files

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Web Audio

[Praveen Jadhav]

WebAudio Specifications describes that start and stop should throw exception (http://www.w3.org/TR/webaudio/#AudioBufferSourceNode). But the exception handling mechanism is implemented in the code.

[Praveen Jadhav]

Created attachment 188284 [details]
Patch

Please review the patch. If code changes are fine, I will update LayoutTest cases for these scenarios.

[Kentaro Hara]

Comment on attachment 188284 [details]
Patch

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

The change looks reasonable. I'm looking forward to seeing the test:)

> Source/WebCore/ChangeLog:9
> +        Exception handling support is implemented for start and stop
> +        attributes in AudioBufferSourceNode.idl and OscillatorNode.idl.

Please add the spec link.

[Early Warning System Bot]

Comment on attachment 188284 [details]
Patch

Attachment 188284 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16542493

[WebKit Review Bot]

Comment on attachment 188284 [details]
Patch

Attachment 188284 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16580258

[Early Warning System Bot]

Comment on attachment 188284 [details]
Patch

Attachment 188284 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16540588

[EFL EWS Bot]

Comment on attachment 188284 [details]
Patch

Attachment 188284 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16538507

[WebKit Review Bot]

Comment on attachment 188284 [details]
Patch

Attachment 188284 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16581215

[Kentaro Hara]

Comment on attachment 188284 [details]
Patch

marking r- for now due to failures and missing tests.

[Praveen Jadhav]

Created attachment 188470 [details]
Patch

Patch updated with layout test cases.

[Praveen Jadhav]

These changes may require modifications in WebAudio specifications as the IDL doesn't specify including "raises(DOMException)" for start and stop functions in AudioBufferSourceNode.

[Kentaro Hara]

Comment on attachment 188470 [details]
Patch

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

> LayoutTests/webaudio/audiobuffersource-note.html:12
> +<div id="description"></div>
> +<div id="console"></div>

Nit: They are not needed.

> LayoutTests/webaudio/audiobuffersource-note.html:19
> +        testRunner.dumpAsText();

Nit: This is not needed.

> LayoutTests/webaudio/audiobuffersource-note.html:23
> +        testRunner.waitUntilDone();
> +    }
> +
> +    window.jsTestIsAsync = true;

This test needs not to be async.

> LayoutTests/webaudio/audiobuffersource-note.html:37
> +    try {
> +        bufferSource.start(0);
> +        buffersource.stop(0.5);
> +        buffersource.stop(0.8);
> +        testFailed("Exception should be thrown if stop is called twice.");
> +    } catch(e) {
> +        testPassed("Exception been thrown as stop is called twice.");
> +    }

You can use shouldThrow() in js-test-pre.js.

> LayoutTests/webaudio/audiobuffersource-note.html:45
> +    try {
> +        buffersource.start(0);
> +        buffersource.start(0);
> +        testFailed("Exception should be thrown if start is called twice.");
> +    } catch(e) {
> +        testPassed("Exception been thrown as start is called twice.");
> +    }

Ditto.

> LayoutTests/webaudio/audiobuffersource-note.html:52
> +    try {
> +        buffersource.stop(0);
> +        testFailed("Exception should be thrown if stop is called before start.");
> +    } catch(e) {
> +        testPassed("Exception been thrown as stop is called before start.");
> +    }

Ditto.

> LayoutTests/webaudio/audiobuffersource-note.html:54
> +    finishJSTest();

Remove this. This test needs not to be async.

> LayoutTests/webaudio/audiobuffersource-note.html:57
> +runTest();

Nit: You can directly write test cases here instead of calling runTest().

[Kentaro Hara]

(In reply to comment #10)
> These changes may require modifications in WebAudio specifications as the IDL doesn't specify including "raises(DOMException)" for start and stop functions in AudioBufferSourceNode.

Actually "raises(DOMException)" was removed from the Web IDL spec. So the WebAudio spec doesn't need to add it to their IDLs. However, some description about what DOM exception should be thrown when has to be added to the WebAudio spec.

crogers: WDYT?

[Praveen Jadhav]

(In reply to comment #12)
> (In reply to comment #10)
> > These changes may require modifications in WebAudio specifications as the IDL doesn't specify including "raises(DOMException)" for start and stop functions in AudioBufferSourceNode.
> 
> Actually "raises(DOMException)" was removed from the Web IDL spec. So the WebAudio spec doesn't need to add it to their IDLs. However, some description about what DOM exception should be thrown when has to be added to the WebAudio spec.
> 
> crogers: WDYT?

I was just going through https://bugs.webkit.org/show_bug.cgi?id=93142. Seems like the description in spec requires some more clarity as to whether exception should be thrown or not.
If "raises(DOMException" is removed from Web IDL spec, does it mean only in the spec or even in the implementation as well. If its the second case, then the patch updated here will be invalid.

How should this situation be handled?

[Chris Rogers]

Comment on attachment 188470 [details]
Patch

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

> Source/WebCore/Modules/webaudio/AudioScheduledSourceNode.cpp:150
> +    if (!(m_playbackState == SCHEDULED_STATE || m_playbackState == PLAYING_STATE)) {

I think it's better to be less restrictive here and instead check:

if (m_playbackState == UNSCHEDULED_STATE)

Because in some cases it's possible for the source to reach the FINISHED_STATE by itself and we don't want an exception thrown.

[Kentaro Hara]

(In reply to comment #13)
> I was just going through https://bugs.webkit.org/show_bug.cgi?id=93142. Seems like the description in spec requires some more clarity as to whether exception should be thrown or not.
> If "raises(DOMException" is removed from Web IDL spec, does it mean only in the spec or even in the implementation as well. If its the second case, then the patch updated here will be invalid.

It means 'raises(DOMException)' should be removed from both the spec and the implementation. The spec should be conformant with the Web IDL spec.

c.f. This is a spec of Indexed DB. It might be helpful to see how exceptions are speced: http://www.w3.org/TR/IndexedDB/#exceptions

[Joshua Bell]

(In reply to comment #15)
> c.f. This is a spec of Indexed DB. It might be helpful to see how exceptions are speced: http://www.w3.org/TR/IndexedDB/#exceptions

FWIW, the IDB spec isn't a perfect example; the WebIDL fragments need updating and it doesn't have consistent prose defining the conditions for which exceptions should be thrown. (There are tracking bugs to update the spec.)

But basically: WebIDL no longer requires that attributes or methods be annotated to indicate that they may throw. But WebKitIDL *does*, so that the ExceptionCode parameter is autogenerated in the binding code.

In the spec, the description of each attribute or method should enumerate (algorithmically, if possible) the conditions which are tested for and which exception should be thrown in each case. The WebIDL may implicitly require other exceptions to be thrown when accessing a method (e.g. if you pass an incorrect type), but the implementation of those should be handled by the code generators, and no WebKitIDL annotations are necessary in general.

[Praveen Jadhav]

Bug 102461 already raised for this issue. Hence marking this as duplicate.

Mr. Li Yin, 
Would you consider including DOMString instead of raises(DOMException) in the IDL file? You may refer to Changeset http://trac.webkit.org/changeset/138631 on how it is implemented.

*** This bug has been marked as a duplicate of bug 102461 ***