Summary: | Update Exception handling in WebAudio | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Praveen Jadhav <praveen.j> | ||||||||
Component: | Web Audio | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cdumez, commit-queue, crogers, dev_sachin, eric.carlson, esprehn+autocc, glenn, jer.noble | ||||||||
Priority: | P2 | Keywords: | WebExposed | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Praveen Jadhav
2013-07-04 22:29:24 PDT
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. |