Bug 47517 – Add DelayProcessor files

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

[Chris Rogers]

Add DelayProcessor files

[Chris Rogers]

Created attachment 70494 [details]
Patch

[chris fleizach]

Comment on attachment 70494 [details]
Patch

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

> WebCore/webaudio/DelayProcessor.h:34
> +namespace WebCore {

don't you want an #if ENABLE(WEB_AUDIO) in the header file too?

> WebCore/webaudio/DelayProcessor.h:45
> +    AudioParam* delayTime() { return m_delayTime.get(); }

this should be const

> WebCore/webaudio/DelayProcessor.h:48
> +    RefPtr<AudioParam> m_delayTime;

why this is protected instead of private

[Chris Rogers]

Comment on attachment 70494 [details]
Patch

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

Hi Chris, thanks for the review.  I'm uploading a new patch with the fixes.

>> WebCore/webaudio/DelayProcessor.h:34
>> +namespace WebCore {
> 
> don't you want an #if ENABLE(WEB_AUDIO) in the header file too?

I think it's only supposed to go into the .cpp file.  This is how the other feature enables work, like 3D_CANVAS in WebCore/html/canvas

>> WebCore/webaudio/DelayProcessor.h:45
>> +    AudioParam* delayTime() { return m_delayTime.get(); }
> 
> this should be const

FIXED

>> WebCore/webaudio/DelayProcessor.h:48
>> +    RefPtr<AudioParam> m_delayTime;
> 
> why this is protected instead of private

FIXED

[Chris Rogers]

Created attachment 70637 [details]
Patch

[Kenneth Russell]

Comment on attachment 70637 [details]
Patch

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

Looks okay; r+'ing, but with one comment. Feel free to upload a revised patch.

> WebCore/webaudio/DelayProcessor.cpp:44
> +        uninitialize();

Looking at this afresh, this block of code looks like something that should be in the base class's destructor. Also, given that uninitialize() checks the isInitialized() flag it doesn't seem necessary to have the "if" test.

[WebKit Commit Bot]

Comment on attachment 70637 [details]
Patch

Clearing flags on attachment: 70637

Committed r70603: <http://trac.webkit.org/changeset/70603>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.