From Spec: http://www.w3.org/TR/webaudio/#AudioPannerNode-section The distance model can be set to LINEAR_DISTANCE, INVERSE_DISTANCE or EXPONENTIAL_DISTANCE. It will be better to raise exception, when it is set incorrectly, like panningModel.
Created attachment 151702 [details] Patch
Comment on attachment 151702 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151702&action=review Except for the two questions, this looks good. Thanks for the patch! > Source/WebCore/Modules/webaudio/AudioPannerNode.idl:41 > + attribute unsigned short panningModel Does the glue generation support unsigned short now? > Source/WebCore/Modules/webaudio/AudioPannerNode.idl:50 > + attribute unsigned short distanceModel Does the glue generation support unsigned short now?
(In reply to comment #1) > Created an attachment (id=151702) [details] > Patch (In reply to comment #2) > (From update of attachment 151702 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151702&action=review > > Except for the two questions, this looks good. Thanks for the patch! > > > Source/WebCore/Modules/webaudio/AudioPannerNode.idl:41 > > + attribute unsigned short panningModel > > Does the glue generation support unsigned short now? > > > Source/WebCore/Modules/webaudio/AudioPannerNode.idl:50 > > + attribute unsigned short distanceModel > > Does the glue generation support unsigned short now? I think so, look this: http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/webaudio/BiquadFilterNode.idl#L40
Comment on attachment 151702 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151702&action=review This patch is trying to raise an exception for the code that has not so far raised an exception. Isn't there any compatibility concern? > Source/WebCore/ChangeLog:8 > + The distance model can be only set to LINEAR_DISTANCE, INVERSE_DISTANCE or EXPONENTIAL_DISTANCE. Please add the link to the spec to this ChangeLog. >>> Source/WebCore/Modules/webaudio/AudioPannerNode.idl:41 >>> + attribute unsigned short panningModel >> >> Does the glue generation support unsigned short now? > > I think so, look this: http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/webaudio/BiquadFilterNode.idl#L40 Yes, unsigned short is already supported. > LayoutTests/webaudio/audiopannernode-basic.html:111 > + if (success) { > + testPassed("panningModel and distanceModel tests passed."); > + } else { > + testFailed("panningModel and distanceModel tests failed."); > + } > + Nit: I am not sure if the 'success' check is needed. You can remove all 'success'-related code (if you want).
Created attachment 153721 [details] Patch
(In reply to comment #4) > This patch is trying to raise an exception for the code that has not so far raised an exception. Isn't there any compatibility concern? > Currently, there is not related clarification in WebAudio specification. In WebKit implementation, if panningModel attribute can raise exception, I think distanceModel should do this as well. > > Source/WebCore/ChangeLog:8 > > + The distance model can be only set to LINEAR_DISTANCE, INVERSE_DISTANCE or EXPONENTIAL_DISTANCE. > > Please add the link to the spec to this ChangeLog. Done. > > >>> Source/WebCore/Modules/webaudio/AudioPannerNode.idl:41 > >>> + attribute unsigned short panningModel > >> > >> Does the glue generation support unsigned short now? > > > > I think so, look this: http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/webaudio/BiquadFilterNode.idl#L40 > > Yes, unsigned short is already supported. > > > LayoutTests/webaudio/audiopannernode-basic.html:111 > > + if (success) { > > + testPassed("panningModel and distanceModel tests passed."); > > + } else { > > + testFailed("panningModel and distanceModel tests failed."); > > + } > > + > > Nit: I am not sure if the 'success' check is needed. You can remove all 'success'-related code (if you want). Done, yes, we can delete these lines. Thanks for your review.
(In reply to comment #4) > This patch is trying to raise an exception for the code that has not so far raised an exception. Isn't there any compatibility concern? From the new spec, some exceptions are added for API, they can define and describe the api better, the compatibility should not be a problem.
(In reply to comment #7) > (In reply to comment #4) > > This patch is trying to raise an exception for the code that has not so far raised an exception. Isn't there any compatibility concern? > > From the new spec, some exceptions are added for API, they can define and describe the api better, the compatibility should not be a problem. rtoy: If you think this is OK, I'm happy to r+ the patch.
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #4) > > > This patch is trying to raise an exception for the code that has not so far raised an exception. Isn't there any compatibility concern? > > > > From the new spec, some exceptions are added for API, they can define and describe the api better, the compatibility should not be a problem. > > rtoy: If you think this is OK, I'm happy to r+ the patch. According to Comment #2, it seems that Raymond has agreed the patch :)
Hi Chris, Could you please give some comments? Thanks in advance.
Comment on attachment 153721 [details] Patch Li, thanks for the patch!
Comment on attachment 153721 [details] Patch Clearing flags on attachment: 153721 Committed r124237: <http://trac.webkit.org/changeset/124237>
All reviewed patches have been landed. Closing bug.