Bug 47517 - Add DelayProcessor files
Summary: Add DelayProcessor files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-11 16:48 PDT by Chris Rogers
Modified: 2010-10-26 20:21 PDT (History)
8 users (show)

See Also:


Attachments
Patch (5.16 KB, patch)
2010-10-11 16:49 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (5.17 KB, patch)
2010-10-13 11:28 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.