[webkit-reviews] review requested: [Bug 85681] Remove RefTypeDisabled : [Attachment 141797] WIP

[bugzilla-daemon at webkit.org]

Raymond Toy <rtoy at chromium.org> has asked  for review:
Bug 85681: Remove RefTypeDisabled
https://bugs.webkit.org/show_bug.cgi?id=85681

Attachment 141797: WIP
https://bugs.webkit.org/attachment.cgi?id=141797&action=review

------- Additional Comments from Raymond Toy <rtoy at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=141797&action=review


Updated according to review.  Chris, please review the changes in
AudioNodeInput::disable() and AudioNode::disableOutputsIfNecessary(). 
AudioNodeInput::disable() is better, but AudioNode::disableOutputsIfNecessary()
is not as nice as I would want, but I'm not sure how else to do it.

>> Source/WebCore/Modules/webaudio/AudioNode.cpp:270
>> +	fprintf(stderr, "%p: %d: AudioNode::enableOutputs %d %d\n", this,
nodeType(), m_normalRefCount, m_connectionRefCount);
> 
> we're using print (instead of fprintf) let's stay consistent -- in another
patch you can change everything over to fprintf, but let's not mix that up in
this one

Removed.  I forgot to delete this debugging print.

>> Source/WebCore/Modules/webaudio/AudioNode.cpp:302
>> +	if (m_isDisabled && m_connectionRefCount > 0 && refType ==
RefTypeConnection)
> 
> Can we just move the if() logic from line 302 into line 272 and call
enable... directly?

Done.  But need to add refType as a parameter to work with finishDeref.

>> Source/WebCore/Modules/webaudio/AudioNode.cpp:340
>> +void AudioNode::disableOutputsIfNotDisabled()
> 
> I'd move the code for this method up to just after the "enable" version of
this method

Done.

>> Source/WebCore/Modules/webaudio/AudioNode.h:158
>> +	void disableOutputsIfNotDisabled();
> 
> nit: I'm too crazy about these names - how about:
> enableOutputsIfNecessary
> disableOutputsIfNecessary

New names applied.