Bug 87904 – Remove RefInfo class

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Raymond Toy]

Remove RefInfo class

[Raymond Toy]

Created attachment 144943 [details]
Patch

[Build Bot]

Comment on attachment 144943 [details]
Patch

Attachment 144943 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12868002

[Chris Rogers]

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?

[Raymond Toy]

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.

[Chris Rogers]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Raymond Toy]

Created attachment 145148 [details]
Patch

[Chris Rogers]

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.

[Raymond Toy]

Created attachment 145327 [details]
Patch

[Raymond Toy]

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.

[Chris Rogers]

Comment on attachment 145327 [details]
Patch

Thanks

[Raymond Toy]

Comment on attachment 145327 [details]
Patch

Thank you.

[WebKit Review Bot]

Comment on attachment 145327 [details]
Patch

Rejecting attachment 145327 [details] from commit-queue.

[e-mail] 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.

[Raymond Toy]

Comment on attachment 145327 [details]
Patch

Oops. I meant commit-queue?, not +.

[WebKit Review Bot]

Comment on attachment 145327 [details]
Patch

Clearing flags on attachment: 145327

Committed r119302: <http://trac.webkit.org/changeset/119302>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.