AudioBufferSourceNode, OscillatorNode and AnalyserNode doesn't propagate any exceptions specified in WebAudio specification.
Created attachment 206122 [details] Patch Patch updated to handle exception in WebAudio.
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?)
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.
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?
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.
Created attachment 206296 [details] Patch Comments updated in Layouttest files.
Comment on attachment 206296 [details] Patch Clearing flags on attachment: 206296 Committed r152489: <http://trac.webkit.org/changeset/152489>
All reviewed patches have been landed. Closing bug.