From Spec: http://www.w3.org/TR/webaudio/#JavaScriptAudioNode-section numberOfInputChannels and numberOfOutputChannels determine the number of input and output channels. It is invalid for both numberOfInputChannels and numberOfOutputChannels to be zero.
Created attachment 152496 [details] Patch
Looks reasonable. (I am not an AudioContext guy though.)
(In reply to comment #2) > Looks reasonable. (I am not an AudioContext guy though.) Thanks for your review.
Comment on attachment 152496 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152496&action=review Just a few minor comments on the pass/fail messages. Otherwise it looks good. Should we test the case where inputs = 0 and outputs = 2 and inputs = 2 and outputs = 0? > LayoutTests/webaudio/javascriptaudionode.html:92 > + testFailed("Exception should be thrown for both of numberOfInputChannels and numberOfOutputChannels to be zero."); Slight grammatical fix: "Exception should be thrown when both numberOfInputChannels and numberOfOutputChannels are zero." > LayoutTests/webaudio/javascriptaudionode.html:94 > + testPassed("Exception was thrown for both of numberOfInputChannels and numberOfOutputChannels to be zero."); "Exception was thrown when both numberOfInputChannels and numberOfOutputChannels are zero." > LayoutTests/webaudio/javascriptaudionode.html:101 > + testFailed("Exception should not be thrown."); Be more explicit on failure to match the pass case? Something like "Exception should not be thrown when numberOfInputChannels = 1 and numberOfOutputChannels = 0." > LayoutTests/webaudio/javascriptaudionode.html:108 > + testFailed("Exception should not be thrown."); Be more explicit on failure to match the pass case? Something like "Exception should not be thrown when numberOfInputChannels = 0 and numberOfOutputChannels = 1."
Created attachment 153633 [details] Patch
(In reply to comment #4) > (From update of attachment 152496 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152496&action=review > > Just a few minor comments on the pass/fail messages. Otherwise it looks good. Should we test the case where inputs = 0 and outputs = 2 and inputs = 2 and outputs = 0? > > > LayoutTests/webaudio/javascriptaudionode.html:92 > > + testFailed("Exception should be thrown for both of numberOfInputChannels and numberOfOutputChannels to be zero."); > > Slight grammatical fix: > > "Exception should be thrown when both numberOfInputChannels and numberOfOutputChannels are zero." > > > LayoutTests/webaudio/javascriptaudionode.html:94 > > + testPassed("Exception was thrown for both of numberOfInputChannels and numberOfOutputChannels to be zero."); > > "Exception was thrown when both numberOfInputChannels and numberOfOutputChannels are zero." > > > LayoutTests/webaudio/javascriptaudionode.html:101 > > + testFailed("Exception should not be thrown."); > > Be more explicit on failure to match the pass case? Something like > > "Exception should not be thrown when numberOfInputChannels = 1 and numberOfOutputChannels = 0." > > > LayoutTests/webaudio/javascriptaudionode.html:108 > > + testFailed("Exception should not be thrown."); > > Be more explicit on failure to match the pass case? Something like > > "Exception should not be thrown when numberOfInputChannels = 0 and numberOfOutputChannels = 1." Done. Thanks for your comments.
Raymond, is this patch ready to go in your opinion?
(In reply to comment #7) > Raymond, is this patch ready to go in your opinion? Yes, pretty much. BUT, there has been some recent (side) discussion on the W3C audio list about whether JavaScriptNode's should allow 0 inputs. I can't seem to find the relevant messages right now, so perhaps we should hold off on this.
Comment on attachment 153633 [details] Patch OK. I'm clearing the review bit in this case. Please request review again once this is determined.
(In reply to comment #8) > (In reply to comment #7) > > Raymond, is this patch ready to go in your opinion? > > Yes, pretty much. > > BUT, there has been some recent (side) discussion on the W3C audio list about whether JavaScriptNode's should allow 0 inputs. I can't seem to find the relevant messages right now, so perhaps we should hold off on this. Hi Raymond, could you please add some detailed information about this discussion? I can't find it in recent mail list. Thanks.
(In reply to comment #10) > (In reply to comment #8) > > (In reply to comment #7) > > > Raymond, is this patch ready to go in your opinion? > > > > Yes, pretty much. > > > > BUT, there has been some recent (side) discussion on the W3C audio list about whether JavaScriptNode's should allow 0 inputs. I can't seem to find the relevant messages right now, so perhaps we should hold off on this. > > Hi Raymond, could you please add some detailed information about this discussion? I can't find it in recent mail list. > Thanks. I can't find the discussion either. But the spec clearly allows 0 inputs, and your patch allows that and clearly disallows the case when the number of inputs and outputs are both 0. This looks fine to me. Sorry for the trouble.
Comment on attachment 153633 [details] Patch Thanks Raymond. r=me
Comment on attachment 153633 [details] Patch Clearing flags on attachment: 153633 Committed r123662: <http://trac.webkit.org/changeset/123662>
All reviewed patches have been landed. Closing bug.