Bug 48875 – Add JavaScriptAudioNode files

• Component: New Bugs
• Status: RESOLVED FIXED

[Chris Rogers]

Add JavaScriptAudioNode files

[Chris Rogers]

Created attachment 72747 [details]
Patch

[Chris Rogers]

Implements JavaScriptAudioNode as described in the web audio specification:
http://chromium.googlecode.com/svn/trunk/samples/audio/specification/specification.html#JavaScriptAudioNode-section

[Kenneth Russell]

Comment on attachment 72747 [details]
Patch

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.

[Chris Rogers]

Created attachment 73440 [details]
Patch

[Chris Rogers]

Comment on attachment 72747 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72747&action=review

>> WebCore/webaudio/JavaScriptAudioNode.cpp:101
>> +    m_outputBuffers.append(AudioBuffer::create(2, bufferSize(), sampleRate));
> 
> This could be a loop to reduce duplicated code.

FIXED

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

I added a call to ref() and corresponding call to deref() to avoid a possible problem here.

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

FIXED

[Kenneth Russell]

Comment on attachment 73440 [details]
Patch

Looks good to me.

[WebKit Commit Bot]

Comment on attachment 73440 [details]
Patch

Clearing flags on attachment: 73440

Committed r71719: <http://trac.webkit.org/changeset/71719>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.