Summary: | Add DelayProcessor files | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Rogers <crogers> | ||||||
Component: | New Bugs | Assignee: | 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
Chris Rogers
2010-10-11 16:48:09 PDT
Created attachment 70494 [details]
Patch
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 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 Created attachment 70637 [details]
Patch
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 on attachment 70637 [details] Patch Clearing flags on attachment: 70637 Committed r70603: <http://trac.webkit.org/changeset/70603> All reviewed patches have been landed. Closing bug. |