Bug 66830 – AudioPannerNode::setPanningModel() does not update m_panningModel properly

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Chris Rogers]

AudioPannerNode::setPanningModel() correctly creates a new panner object of the correct type, but does not update m_panningModel properly.
So the .panningModel attribute will not show the new model.

[Raymond Toy]

Is the fix as simple as adding m_panningModel = model in AudioPannerNode::setPanningModel in AudioPannerNode.cpp?

[Chris Rogers]

(In reply to comment #1)
> Is the fix as simple as adding m_panningModel = model in AudioPannerNode::setPanningModel in AudioPannerNode.cpp?

It seems as simple as that :)

[Raymond Toy]

Created attachment 123615 [details]
Patch

[WebKit Review Bot]

Attachment 123615 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/weba..." exit_code: 1

Source/WebCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Raymond Toy]

Created attachment 123622 [details]
Patch

[Chris Rogers]

Comment on attachment 123622 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123622&action=review

> LayoutTests/webaudio/panner-set-model.html:77
> +          createTestAndRun(context);

This test seems more complicated than necessary.  Can't we just create a single AudioPannerNode and then set the panningModel attribute for each of the constant values, then check that the attribute returns the same value?

Then we can get rid of createTestAndRun() and all of the associated testing machinery around.

[Raymond Toy]

Comment on attachment 123622 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123622&action=review

>> LayoutTests/webaudio/panner-set-model.html:77
>> +          createTestAndRun(context);
> 
> This test seems more complicated than necessary.  Can't we just create a single AudioPannerNode and then set the panningModel attribute for each of the constant values, then check that the attribute returns the same value?
> 
> Then we can get rid of createTestAndRun() and all of the associated testing machinery around.

Simplified test as suggested.

[Raymond Toy]

Created attachment 124031 [details]
Patch

[Chris Rogers]

Comment on attachment 124031 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124031&action=review

> Source/WebCore/webaudio/AudioPannerNode.cpp:155
>      if (!m_panner.get() || model != m_panningModel) {

I just noticed that we should do bounds checking on "model" and reject illegal values

> LayoutTests/webaudio/panner-set-model.html:18
> +      var renderLengthSeconds = 0.001;

We don't need these two variables since we don't need to use an offline audio context.

> LayoutTests/webaudio/panner-set-model.html:30
> +          var context = new webkitAudioContext(2, sampleRate * renderLengthSeconds, sampleRate);

I think we can use a "regular" AudioContext here (no constructor arguments)

> LayoutTests/webaudio/panner-set-model.html:36
> +          // correctly.

We should add a comment describing the "magic" number 3, describing the existing panning constant values...

> LayoutTests/webaudio/panner-set-model.html:43
> +                   success = false;

nit: extra space before "success"

> LayoutTests/webaudio/panner-set-model.html:45
> +          }

We can add a test case where we try to set the panning model to an illegal value, and make sure it doesn't get set to that value.

> LayoutTests/webaudio/panner-set-model.html:54
> +                  

nit: extra blank line

[Raymond Toy]

Comment on attachment 124031 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124031&action=review

>> LayoutTests/webaudio/panner-set-model.html:36
>> +          // correctly.
> 
> We should add a comment describing the "magic" number 3, describing the existing panning constant values...

Do you want to say something more than that there are currently 3 panner models numbered 0 to 2?

>> LayoutTests/webaudio/panner-set-model.html:45
>> +          }
> 
> We can add a test case where we try to set the panning model to an illegal value, and make sure it doesn't get set to that value.

I tried setting the model to 99.  .panningModel is 99.   Maybe we shouldn't add this test now and file a bug report about it and fix it there, updating this test appropriately?

[Chris Rogers]

(In reply to comment #10)
> (From update of attachment 124031 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124031&action=review
> 
> >> LayoutTests/webaudio/panner-set-model.html:36
> >> +          // correctly.
> > 
> > We should add a comment describing the "magic" number 3, describing the existing panning constant values...
> 
> Do you want to say something more than that there are currently 3 panner models numbered 0 to 2?

Ideally, we wouldn't even have a loop and would create a testing function and do something like this:

testSet(panner.EQUALPOWER);
testSet(panner.HRTF);
testSet(panner.SOUNDFIELD);



> 
> >> LayoutTests/webaudio/panner-set-model.html:45
> >> +          }
> > 
> > We can add a test case where we try to set the panning model to an illegal value, and make sure it doesn't get set to that value.
> 
> I tried setting the model to 99.  .panningModel is 99.   Maybe we shouldn't add this test now and file a bug report about it and fix it there, updating this test appropriately?

In another comment about the C++ implementation, I mention we should be doing bounds checking...

[Raymond Toy]

Created attachment 124203 [details]
Patch

[Chris Rogers]

Comment on attachment 124203 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124203&action=review

> Source/WebCore/webaudio/AudioPannerNode.cpp:155
>      if (!m_panner.get() || model != m_panningModel) {

Please do bounds checking on "model" and silently fail if model is not valid

> LayoutTests/webaudio/panner-set-model.html:55
> +

Please add one additional test setting to an illegal value and validating that the value did *not* change.

[Raymond Toy]

Created attachment 124234 [details]
Patch

[Raymond Toy]

(In reply to comment #13)
> (From update of attachment 124203 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124203&action=review
> 
> > Source/WebCore/webaudio/AudioPannerNode.cpp:155
> >      if (!m_panner.get() || model != m_panningModel) {
> 
> Please do bounds checking on "model" and silently fail if model is not valid

Done.  Not sure if this is how you want me to test for a valid model, though.
> 
> > LayoutTests/webaudio/panner-set-model.html:55
> > +
> 
> Please add one additional test setting to an illegal value and validating that the value did *not* change.

Done.

[WebKit Review Bot]

Attachment 124234 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/weba..." exit_code: 1

Source/WebCore/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Raymond Toy]

Created attachment 124240 [details]
Patch

[Chris Rogers]

Comment on attachment 124240 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124240&action=review

Looks great if the comments are addressed.

> Source/WebCore/webaudio/AudioPannerNode.cpp:166
> +        // Silently fail, without doing anything.

I wouldn't use the word "fail" since it implies a defect in the implementation.  How about:

// Ignore illegal panner values.

Or, even better yet, no comment at all since it's quite obvious from the code itself.  WebKit style is to try to avoid obvious comments.

One thing we could consider is throwing an exception here, so another option is:

// FIXME: consider throwing exception for illegal model value.

> LayoutTests/webaudio/panner-set-model.html:62
> +              testFailed("Panner set to invalid model, but the panningModel changed fomr 2 to " + panner.panningModel);

typo: "fomr" -> "from"

[Raymond Toy]

Created attachment 124352 [details]
Patch

[Raymond Toy]

Comment on attachment 124240 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124240&action=review

>> Source/WebCore/webaudio/AudioPannerNode.cpp:166
>> +        // Silently fail, without doing anything.
> 
> I wouldn't use the word "fail" since it implies a defect in the implementation.  How about:
> 
> // Ignore illegal panner values.
> 
> Or, even better yet, no comment at all since it's quite obvious from the code itself.  WebKit style is to try to avoid obvious comments.
> 
> One thing we could consider is throwing an exception here, so another option is:
> 
> // FIXME: consider throwing exception for illegal model value.

Added FIXME comment.  Will file a bug on this so we don't forget.

>> LayoutTests/webaudio/panner-set-model.html:62
>> +              testFailed("Panner set to invalid model, but the panningModel changed fomr 2 to " + panner.panningModel);
> 
> typo: "fomr" -> "from"

Fixed.

[Chris Rogers]

Looks good.

[Kenneth Russell]

Comment on attachment 124352 [details]
Patch

rs=me

[WebKit Review Bot]

Comment on attachment 124352 [details]
Patch

Rejecting attachment 124352 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 10507 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
46>At revision 10507.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/11360166

[Raymond Toy]

Created attachment 124380 [details]
Patch

[Raymond Toy]

Update patch to current sources.  Only changed the changelogs.

[Kenneth Russell]

Comment on attachment 124380 [details]
Patch

r=me again

[WebKit Review Bot]

Comment on attachment 124380 [details]
Patch

Clearing flags on attachment: 124380

Committed r106174: <http://trac.webkit.org/changeset/106174>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.