[webkit-reviews] review denied: [Bug 48875] Add JavaScriptAudioNode files : [Attachment 72747] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Chris Rogers <crogers at google.com>'s
request for review:
Bug 48875: Add JavaScriptAudioNode files
https://bugs.webkit.org/show_bug.cgi?id=48875

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

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=72747&action=review

This looks good overall; I went through the bounds checking assertions
carefully and they all look correct. Because of the potential deletion issue
before the callback is called, I'm marking it r-.

> WebCore/webaudio/JavaScriptAudioNode.cpp:101
> +    m_outputBuffers.append(AudioBuffer::create(2, bufferSize(),
sampleRate));

This could be a loop to reduce duplicated code.

> WebCore/webaudio/JavaScriptAudioNode.cpp:200
> +	       callOnMainThread(fireProcessEventDispatch, this);

Some sort of guard is needed to ensure that this node isn't deallocated by the
time fireProcessEventDispatch is called. Perhaps you could increment one of the
node's reference counts here and decrement it in the callback.

> WebCore/webaudio/JavaScriptAudioNode.h:57
> +	   return adoptRef(new JavaScriptAudioNode(context, sampleRate,
bufferSize, numberOfInputs, numberOfOutputs));

Unless there's a strong reason to inline this, move the body to the .cpp.