Bug 90952 – AudioPannerNode should raise exception when distanceModel is set incorrectly

• Component: Web Audio
• Status: RESOLVED FIXED

[Li Yin]

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.

[Li Yin]

Created attachment 151702 [details]
Patch

[Raymond Toy]

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?

[Li Yin]

(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

[Kentaro Hara]

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

[Li Yin]

Created attachment 153721 [details]
Patch

[Li Yin]

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

[Li Yin]

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

[Kentaro Hara]

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

[Li Yin]

(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 :)

[Li Yin]

Hi Chris,
  Could you please give some comments? Thanks in advance.

[Chris Rogers]

Comment on attachment 153721 [details]
Patch

Li, thanks for the patch!

[WebKit Review Bot]

Comment on attachment 153721 [details]
Patch

Clearing flags on attachment: 153721

Committed r124237: <http://trac.webkit.org/changeset/124237>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.