Bug 47517

Summary: Add DelayProcessor files
Product: WebKit Reporter: Chris Rogers <crogers>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, commit-queue, dglazkov, eric.carlson, jamesr, jer.noble, kbr, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch none

Description Chris Rogers 2010-10-11 16:48:09 PDT
Add DelayProcessor files
Comment 1 Chris Rogers 2010-10-11 16:49:26 PDT
Created attachment 70494 [details]
Patch
Comment 2 chris fleizach 2010-10-13 00:53:15 PDT
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
Comment 3 Chris Rogers 2010-10-13 11:26:39 PDT
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
Comment 4 Chris Rogers 2010-10-13 11:28:50 PDT
Created attachment 70637 [details]
Patch
Comment 5 Kenneth Russell 2010-10-26 18:41:07 PDT
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.
Comment 6 WebKit Commit Bot 2010-10-26 20:21:53 PDT
Comment on attachment 70637 [details]
Patch

Clearing flags on attachment: 70637

Committed r70603: <http://trac.webkit.org/changeset/70603>
Comment 7 WebKit Commit Bot 2010-10-26 20:21:59 PDT
All reviewed patches have been landed.  Closing bug.