Bug 46532 - Add LowPass2FilterNode files
Summary: Add LowPass2FilterNode files
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
Depends on:
Reported: 2010-09-24 15:39 PDT by Chris Rogers
Modified: 2010-10-05 00:23 PDT (History)
8 users (show)

See Also:

Patch (7.83 KB, patch)
2010-09-24 15:40 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (7.23 KB, patch)
2010-10-04 14:49 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (7.21 KB, patch)
2010-10-04 15:23 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-09-24 15:39:09 PDT
Add LowPass2FilterNode files
Comment 1 Chris Rogers 2010-09-24 15:40:11 PDT
Created attachment 68774 [details]
Comment 2 Kenneth Russell 2010-10-04 14:28:23 PDT
Comment on attachment 68774 [details]

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

The code looks OK, but I'm marking this r- for the license update as discussed offline. Please also consider getting the Web Audio spec up to date with this change before landing it.

> WebCore/webaudio/LowPass2FilterNode.idl:34
> +    ] LowPass2FilterNode : AudioNode {

http://chromium.googlecode.com/svn/trunk/samples/audio/specification/specification.html#APIOverview-section says that LowPass2FilterNode "would be" a subclass of AudioNode; I think you should modify the spec to include the API definition for this class before committing it.
Comment 3 Chris Rogers 2010-10-04 14:49:31 PDT
Created attachment 69689 [details]
Comment 4 Chris Rogers 2010-10-04 14:50:27 PDT
Fixed the license.  Also I put a FIXME in the .idl file about moving to a BiquadNode implementation.
Comment 5 Kenneth Russell 2010-10-04 14:59:31 PDT
Comment on attachment 69689 [details]

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

Sorry I didn't catch the m_biquadProcessor issue earlier but please change it here as well as in HighPass2FilterNode.

> WebCore/webaudio/LowPass2FilterNode.cpp:38
> +    m_processor = lowpassFilter.release();

Per below, the weak m_biquadProcessor pointer is very confusing. Please change this to "m_processor = adoptPtr(...)" and add a helper function to fetch the processor as a BiquadProcessor.

> WebCore/webaudio/LowPass2FilterNode.h:48
> +    BiquadProcessor* m_biquadProcessor;

This weak pointer makes the lifetime of this object confusing. Make this a helper function "BiquadProcessor* biquadProcessor()" returning a static_cast of the result of processor().
Comment 6 Chris Rogers 2010-10-04 15:23:35 PDT
Created attachment 69698 [details]
Comment 7 Chris Rogers 2010-10-04 15:25:16 PDT
You're right, your suggestions make it much cleaner -- FIXED.
Comment 8 Kenneth Russell 2010-10-04 15:48:18 PDT
Comment on attachment 69698 [details]

Looks good to me.
Comment 9 WebKit Commit Bot 2010-10-05 00:23:33 PDT
Comment on attachment 69698 [details]

Clearing flags on attachment: 69698

Committed r69087: <http://trac.webkit.org/changeset/69087>
Comment 10 WebKit Commit Bot 2010-10-05 00:23:40 PDT
All reviewed patches have been landed.  Closing bug.