Bug 118405 – Update Exception handling in WebAudio

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

[Praveen Jadhav]

AudioBufferSourceNode, OscillatorNode and AnalyserNode doesn't propagate any exceptions specified in WebAudio specification.

[Praveen Jadhav]

Created attachment 206122 [details]
Patch

Patch updated to handle exception in WebAudio.

[Chris Dumez]

Comment on attachment 206122 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        AudioBufferSourceNode, OscillatorNode and AnalyserNode don't propagate any exceptions specified in WebAudio spec.

Throwing new exceptions is risky business when it comes to backward compatibility.

> Source/WebCore/Modules/webaudio/AudioContext.cpp:555
> +        ec = INDEX_SIZE_ERR;

Where is this exception name specified?

> Source/WebCore/Modules/webaudio/AudioContext.cpp:601
> +    if (!real || !imag || (real->length() != imag->length() || (real->length() > 4096))) {

Would be nice to have a global static const variable for that magic number. Alsom , the spec says that the value should be greater than 0 but it does not seem to be covered, right?

> Source/WebCore/Modules/webaudio/AudioContext.cpp:602
> +        ec = INDEX_SIZE_ERR;

Does the spec actually specify it needs to be a IndexSizeError and not a SyntaxError? I cannot find it.

> LayoutTests/webaudio/analyser-exception.html:34
> +    shouldThrow("analyser.minDecibels = -20");

shouldThrow() can take a second argument with the exception name to validate that as well. If you expect a specific exception, you probably want to add that (although the spec does not seem to indicate exception names?)

[Sam Weinig]

Comment on attachment 206122 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        AudioBufferSourceNode, OscillatorNode and AnalyserNode don't propagate any exceptions specified in WebAudio spec.
> 
> Throwing new exceptions is risky business when it comes to backward compatibility.

While I usually am a champion for this point, given how little WebAudio content there is, I am not too worried here.

>> LayoutTests/webaudio/analyser-exception.html:34
>> +    shouldThrow("analyser.minDecibels = -20");
> 
> shouldThrow() can take a second argument with the exception name to validate that as well. If you expect a specific exception, you probably want to add that (although the spec does not seem to indicate exception names?)

I agree with Christophe, you should specify what exception you expect.  If the spec isn't specifying it clearly, we should get it updated.

[Praveen Jadhav]

Created attachment 206215 [details]
Patch

Patch updated as per the review comments. Description for start and stop methods in AudioBufferSourceNode says 'an exception will be thrown' but doesn't specify the exact DOM exception. INVALID_STATE_ERR seems to be appropriate error in this case.

@crogers: WDYT?

[Chris Rogers]

Comment on attachment 206215 [details]
Patch

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

Looks good if comment is added

> LayoutTests/webaudio/audiobuffersource-exception.html:34
> +

It would be nice to add brief comments describing why each case should throw an exception.

[Praveen Jadhav]

Created attachment 206296 [details]
Patch

Comments updated in Layouttest files.

[WebKit Commit Bot]

Comment on attachment 206296 [details]
Patch

Clearing flags on attachment: 206296

Committed r152489: <http://trac.webkit.org/changeset/152489>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.