Remove RefInfo class
Created attachment 144943 [details] Patch
Comment on attachment 144943 [details] Patch Attachment 144943 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12868002
Comment on attachment 144943 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144943&action=review > Source/WebCore/Modules/webaudio/AudioContext.cpp:556 > +} I don't understand why this method is needed? Why can't you simply make AudioScheduledSourceNode::finish() be virtual, then override in AudioBufferSourceNode and call AudioBufferSourceNode::clearPannerNode() there? > Source/WebCore/Modules/webaudio/AudioContext.cpp:673 > void AudioContext::addDeferredFinishDeref(AudioNode* node, AudioNode::RefType refType) Why do we still pass in the refType argument if it's no longer needed?
Comment on attachment 144943 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144943&action=review >> Source/WebCore/Modules/webaudio/AudioContext.cpp:556 >> +} > > I don't understand why this method is needed? > > Why can't you simply make AudioScheduledSourceNode::finish() be virtual, then override in AudioBufferSourceNode and call AudioBufferSourceNode::clearPannerNode() there? Unfinished source nodes still need to clear the panner. The virtual finish() method would work for other cases, though. > Source/WebCore/Modules/webaudio/AudioContext.cpp:673 > void AudioContext::addDeferredFinishDeref(AudioNode* node, AudioNode::RefType refType) My mistake.
(In reply to comment #4) > (From update of attachment 144943 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144943&action=review > > >> Source/WebCore/Modules/webaudio/AudioContext.cpp:556 > >> +} > > > > I don't understand why this method is needed? > > > > Why can't you simply make AudioScheduledSourceNode::finish() be virtual, then override in AudioBufferSourceNode and call AudioBufferSourceNode::clearPannerNode() there? > > Unfinished source nodes still need to clear the panner. The virtual finish() method would work for other cases, though. I don't understand -- isn't it exactly the right place to call clearPannerNode() in the finish() method? Why go to the trouble of doing it later on and adding more complex code in AudioContext? > > > Source/WebCore/Modules/webaudio/AudioContext.cpp:673 > > void AudioContext::addDeferredFinishDeref(AudioNode* node, AudioNode::RefType refType) > > My mistake.
Comment on attachment 144943 [details] Patch Attachment 144943 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12866148 New failing tests: webaudio/oscillator-square.html webaudio/mixing.html webaudio/javascriptaudionode-upmix2-8channel-input.html webaudio/javascriptaudionode.html webaudio/panner-equalpower-stereo.html webaudio/oscillator-sawtooth.html
Created attachment 145019 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 145148 [details] Patch
Comment on attachment 145148 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145148&action=review > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:470 > + } It's bad practice to simply copy/paste identical code from AudioScheduledSourceNode::finish(). It's not only less concise, but is also fragile since a bug fix or change in one part of the code could easily be missed by not changing the other part. Instead, simply call clearPannerNode() followed by: AudioScheduledSourceNode::finish() > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.h:88 > + // Called when we have no more sound to play or the noteOff() time has been reached. Best not to copy/paste this comment from AudioScheduledSourceNode. Instead it's better to simply comment this as: // AudioScheduledSourceNode. > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.h:89 > + virtual void finish(); Please use OVERRIDE keyword here. > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.h:-127 > - RefPtr<AudioPannerNode> m_pannerNode; Please add a short comment here describing why we don't use RefPtr -- something like: // We manually manage ref-counting, since we use RefTypeConnection.
Created attachment 145327 [details] Patch
Comment on attachment 145148 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145148&action=review >> Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:470 >> + } > > It's bad practice to simply copy/paste identical code from AudioScheduledSourceNode::finish(). It's not only less concise, but is also fragile since a bug fix or change in one part of the code could easily be missed by not changing the other part. > > Instead, simply call clearPannerNode() followed by: > > AudioScheduledSourceNode::finish() Fixed. >> Source/WebCore/Modules/webaudio/AudioBufferSourceNode.h:88 >> + // Called when we have no more sound to play or the noteOff() time has been reached. > > Best not to copy/paste this comment from AudioScheduledSourceNode. Instead it's better to simply comment this as: > > // AudioScheduledSourceNode. Fixed. >> Source/WebCore/Modules/webaudio/AudioBufferSourceNode.h:89 >> + virtual void finish(); > > Please use OVERRIDE keyword here. Fixed. >> Source/WebCore/Modules/webaudio/AudioBufferSourceNode.h:-127 >> - RefPtr<AudioPannerNode> m_pannerNode; > > Please add a short comment here describing why we don't use RefPtr -- something like: > > // We manually manage ref-counting, since we use RefTypeConnection. Added comment.
Comment on attachment 145327 [details] Patch Thanks
Comment on attachment 145327 [details] Patch Thank you.
Comment on attachment 145327 [details] Patch Rejecting attachment 145327 [details] from commit-queue. rtoy@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
Comment on attachment 145327 [details] Patch Oops. I meant commit-queue?, not +.
Comment on attachment 145327 [details] Patch Clearing flags on attachment: 145327 Committed r119302: <http://trac.webkit.org/changeset/119302>
All reviewed patches have been landed. Closing bug.