Bug 90937 - Improve BiquadFilterNode test case
Summary: Improve BiquadFilterNode test case
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Li Yin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-10 21:47 PDT by Li Yin
Modified: 2012-07-22 19:12 PDT (History)
6 users (show)

See Also:


Attachments
Patch (3.55 KB, patch)
2012-07-10 21:51 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (3.94 KB, patch)
2012-07-20 18:56 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (4.00 KB, patch)
2012-07-22 18:10 PDT, Li Yin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Li Yin 2012-07-10 21:47:34 PDT
Improve BiquadFilterNode test to cover constant value and number of inputs and outputs.
Comment 1 Li Yin 2012-07-10 21:51:10 PDT
Created attachment 151592 [details]
Patch
Comment 2 Raymond Toy 2012-07-19 10:31:02 PDT
Comment on attachment 151592 [details]
Patch

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

Except for the couple of nits, this looks fine.

> LayoutTests/webaudio/biquadfilternode-basic.html:31
> +    shouldBeTrue("filter.numberOfOutputs === 1");

I think it reads better if the text said something like

"Number of inputs to Biquad filter = 1"
"Number of outputs of Biquad filter = 1"

> LayoutTests/webaudio/biquadfilternode-basic.html:38
> +    var typeArray = [filter.LOWPASS,

Rename typeArray to filterTypeArray?
Comment 3 Li Yin 2012-07-20 18:56:03 PDT
Created attachment 153639 [details]
Patch
Comment 4 Li Yin 2012-07-20 19:02:36 PDT
(In reply to comment #2)
> (From update of attachment 151592 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151592&action=review
> 
> Except for the couple of nits, this looks fine.
> 
> > LayoutTests/webaudio/biquadfilternode-basic.html:31
> > +    shouldBeTrue("filter.numberOfOutputs === 1");
> 
> I think it reads better if the text said something like
> 
> "Number of inputs to Biquad filter = 1"
> "Number of outputs of Biquad filter = 1"
> 

Most of other models like using shouleBe(), such as FileAPI, it looks more simple, and also decreases the code lines.
Anyway, fixed.

> > LayoutTests/webaudio/biquadfilternode-basic.html:38
> > +    var typeArray = [filter.LOWPASS,
> 
> Rename typeArray to filterTypeArray?

Done.

Thanks for your comments.
Comment 5 Kentaro Hara 2012-07-21 04:09:09 PDT
Comment on attachment 153639 [details]
Patch

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

LGTM

> LayoutTests/webaudio/biquadfilternode-basic.html:-38
> -    // FIXME: use last defined filter constant (ALLPASS) instead of hard-coded 7 once it's defined in the IDL.

Let's add the link to the spec (http://www.w3.org/TR/webaudio/#BiquadFilterNode) to ChangeLog.
Comment 6 Li Yin 2012-07-22 18:10:21 PDT
Created attachment 153714 [details]
Patch
Comment 7 Kentaro Hara 2012-07-22 18:13:50 PDT
Comment on attachment 153714 [details]
Patch

LGTM. rtoy@ is LGTMing too. Let me r+ it.
Comment 8 WebKit Review Bot 2012-07-22 19:12:12 PDT
Comment on attachment 153714 [details]
Patch

Clearing flags on attachment: 153714

Committed r123306: <http://trac.webkit.org/changeset/123306>
Comment 9 WebKit Review Bot 2012-07-22 19:12:16 PDT
All reviewed patches have been landed.  Closing bug.