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.
Is the fix as simple as adding m_panningModel = model in AudioPannerNode::setPanningModel in AudioPannerNode.cpp?
(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 :)
Created attachment 123615 [details] Patch
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.
Created attachment 123622 [details] Patch
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.
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.
Created attachment 124031 [details] Patch
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
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?
(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...
Created attachment 124203 [details] Patch
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.
Created attachment 124234 [details] Patch
(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.
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.
Created attachment 124240 [details] Patch
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"
Created attachment 124352 [details] Patch
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.
Looks good.
Comment on attachment 124352 [details] Patch rs=me
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
Created attachment 124380 [details] Patch
Update patch to current sources. Only changed the changelogs.
Comment on attachment 124380 [details] Patch r=me again
Comment on attachment 124380 [details] Patch Clearing flags on attachment: 124380 Committed r106174: <http://trac.webkit.org/changeset/106174>
All reviewed patches have been landed. Closing bug.