Summary: | createChannelMerger and createChannelSplitter should throw INDEX_SIZE_ERR for invalid numberOfInputs value | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hyemi Shin <hyemi.sin> | ||||||||||||
Component: | Web Audio | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | buildbot, cdumez, commit-queue, rniwa | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
URL: | http://www.w3.org/TR/2013/WD-webaudio-20131010 | ||||||||||||||
Attachments: |
|
Description
Hyemi Shin
2015-11-05 03:14:32 PST
Created attachment 264848 [details]
Patch
Created attachment 264849 [details]
Patch
Comment on attachment 264849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264849&action=review Thanks for this patch, Hyemi, and sorry for the delay in getting a review. It looks good, but note that AudioContext::createPeriodicWave has the same problem. r=me if you could fix that too. > Source/WebCore/Modules/webaudio/AudioContext.cpp:611 > + ec = INDEX_SIZE_ERR; Hm, I see that this change is correct, because ChannelSplitterNode::create (and also ChannelMergerNode::create) will only ever fail if the numberOfInputs parameter is invalid. I'm a bit uncomfortable with relying on a null return value from create to signal that, since it's not at all clear from reading AudioContext.cpp why the ExceptionCode should be INDEX_SIZE_ERROR. I was going to ask you to pass the ExceptionCode down into the create function, but I see this pattern is established throughout AudioContext.cpp, so the code you have here is best. > LayoutTests/webaudio/audiochannelmerger-basic-expected.txt:6 > +PASS IndexSizeError was thrown for numberOfInputs > 32. Ah, I see 32 is valid; good catch. I would mention this fix separately in the changelog, since it is noteworthy. Created attachment 270576 [details]
Patch
I added the fix for createPeriodicWave also. Because I'm not good at english that much, my change-log could be hard to understand or not clear. Feel free to point out wrong sentence which needs to be changed. Comment on attachment 270576 [details] Patch Attachment 270576 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/777381 New failing tests: webaudio/audiochannelsplitter.html Created attachment 270577 [details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 270578 [details]
Patch
cq? Comment on attachment 270578 [details] Patch Clearing flags on attachment: 270578 Committed r196130: <http://trac.webkit.org/changeset/196130> All reviewed patches have been landed. Closing bug. |