Bug 91364 – It is invalid when both numberOfInputChannels and numberOfOutputChannels to be zero in JavaScriptAudioNode.

• Component: Web Audio
• Status: RESOLVED FIXED

[Li Yin]

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.

[Li Yin]

Created attachment 152496 [details]
Patch

[Kentaro Hara]

Looks reasonable. (I am not an AudioContext guy though.)

[Li Yin]

(In reply to comment #2)
> Looks reasonable. (I am not an AudioContext guy though.)

Thanks for your review.

[Raymond Toy]

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."

[Li Yin]

Created attachment 153633 [details]
Patch

[Li Yin]

(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.

[Kenneth Russell]

Raymond, is this patch ready to go in your opinion?

[Raymond Toy]

(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.

[Kenneth Russell]

Comment on attachment 153633 [details]
Patch

OK. I'm clearing the review bit in this case. Please request review again once this is determined.

[Li Yin]

(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.

[Raymond Toy]

(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.

[Kenneth Russell]

Comment on attachment 153633 [details]
Patch

Thanks Raymond. r=me

[WebKit Review Bot]

Comment on attachment 153633 [details]
Patch

Clearing flags on attachment: 153633

Committed r123662: <http://trac.webkit.org/changeset/123662>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.