Throw exception for non-finite values in setValueCurve

Throw exception for non-finite values in setValueCurve

If the curve given to setValueCurveAtTime contains non-finite values,
throw an exception.  All other AudioParam methods including the value
setter or the automation methods do not allow a non-finite value to be
specified.  The same should be true for the curve values given to
setValueCurveAtTime.

BUG=612787
TEST=audioparam-setValueCurve-exceptions.html

Committed: https://crrev.com/9e1870e6504d085c7b624ae16f55d03e46dc8ef0
Cr-Commit-Position: refs/heads/master@{#396042}

[Raymond Toy, 2016-05-18 16:07:28]

rtoy@chromium.org changed reviewers:
+ hongchan@chromium.org

[Raymond Toy, 2016-05-18 16:07:28]

PTAL

[hongchan, 2016-05-19 21:10:58]

lgtm

[Raymond Toy, 2016-05-20 14:20:54]

PTAL

[hongchan, 2016-05-20 15:54:57]

On 2016/05/20 14:20:54, Raymond Toy wrote:
> PTAL

Question: can we just stop and throw if any illegal value is found in the array?
(before returning |this|)

[Raymond Toy, 2016-05-20 20:40:11]

On 2016/05/20 at 15:54:57, hongchan wrote:
> On 2016/05/20 14:20:54, Raymond Toy wrote:
> > PTAL
> 
> Question: can we just stop and throw if any illegal value is found in the
array? (before returning |this|)

Just returning null.

[hongchan, 2016-05-20 21:14:08]

The spec doesn't clarify the behavior on illegal values in the array. 

http://webaudio.github.io/web-audio-api/#widl-AudioParam-setValueCurveAtTime-...

Right? I think we should delay the implementation until we design the behavior
right.

https://codereview.chromium.org/1995583002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioParam.cpp (right):

https://codereview.chromium.org/1995583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParam.cpp:479: break;
So what will happen after this break? It still returns a valid AudioParam object
at the end. Right?

1) You have 4 items in the array.
2) The 2nd item is out of range. So it breaks here. (meaning no more
verification for 3rd, 4th items)
3) Handler gets scheduled.
4) Return the AudioParam object.

Is this what we want? 
- So we warn user but we don't necessarily stop the operation?
- What if 3rd item is NaN?

[Raymond Toy, 2016-05-20 21:30:26]

On 2016/05/20 at 21:14:08, hongchan wrote:
> The spec doesn't clarify the behavior on illegal values in the array. 
> 
>
http://webaudio.github.io/web-audio-api/#widl-AudioParam-setValueCurveAtTime-...
> 
> Right? I think we should delay the implementation until we design the behavior
right.

As you know from yesterday's teleconf, there was an issue filed on this and we
agreed that non-finite values in the curve must throw an exception.  It's not
yet in the spec, but there's a PR for it already.  I doubt that it is
controversial.
> 
>
https://codereview.chromium.org/1995583002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webaudio/AudioParam.cpp (right):
> 
>
https://codereview.chromium.org/1995583002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webaudio/AudioParam.cpp:479: break;
> So what will happen after this break? It still returns a valid AudioParam
object at the end. Right?
> 
> 1) You have 4 items in the array.
> 2) The 2nd item is out of range. So it breaks here. (meaning no more
verification for 3rd, 4th items)
> 3) Handler gets scheduled.
> 4) Return the AudioParam object.
> 
> Is this what we want? 
> - So we warn user but we don't necessarily stop the operation?
> - What if 3rd item is NaN?

[Raymond Toy, 2016-05-20 21:30:41]

https://codereview.chromium.org/1995583002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioParam.cpp (right):

https://codereview.chromium.org/1995583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParam.cpp:479: break;
On 2016/05/20 21:14:07, hoch wrote:
> So what will happen after this break? It still returns a valid AudioParam
object
> at the end. Right?
> 
> 1) You have 4 items in the array.
> 2) The 2nd item is out of range. So it breaks here. (meaning no more
> verification for 3rd, 4th items)
> 3) Handler gets scheduled.
> 4) Return the AudioParam object.
> 
> Is this what we want? 
> - So we warn user but we don't necessarily stop the operation?
> - What if 3rd item is NaN?

No.  I was intending to scan the array just once.  More logic needed to do that.

[Raymond Toy, 2016-05-24 22:35:32]

Patchset #6 (id:100001) has been deleted

[hongchan, 2016-05-24 23:01:42]

On 2016/05/20 21:30:41, Raymond Toy wrote:
>
https://codereview.chromium.org/1995583002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webaudio/AudioParam.cpp (right):
> 
>
https://codereview.chromium.org/1995583002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webaudio/AudioParam.cpp:479: break;
> On 2016/05/20 21:14:07, hoch wrote:
> > So what will happen after this break? It still returns a valid AudioParam
> object
> > at the end. Right?
> > 
> > 1) You have 4 items in the array.
> > 2) The 2nd item is out of range. So it breaks here. (meaning no more
> > verification for 3rd, 4th items)
> > 3) Handler gets scheduled.
> > 4) Return the AudioParam object.
> > 
> > Is this what we want? 
> > - So we warn user but we don't necessarily stop the operation?
> > - What if 3rd item is NaN?
> 
> No.  I was intending to scan the array just once.  More logic needed to do
that.

PS5 looks good. Should we check the new behavior with a layout test?

[Raymond Toy, 2016-05-24 23:03:43]

On 2016/05/24 23:01:42, hoch wrote:
> On 2016/05/20 21:30:41, Raymond Toy wrote:
> >
>
https://codereview.chromium.org/1995583002/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/modules/webaudio/AudioParam.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1995583002/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/webaudio/AudioParam.cpp:479: break;
> > On 2016/05/20 21:14:07, hoch wrote:
> > > So what will happen after this break? It still returns a valid AudioParam
> > object
> > > at the end. Right?
> > > 
> > > 1) You have 4 items in the array.
> > > 2) The 2nd item is out of range. So it breaks here. (meaning no more
> > > verification for 3rd, 4th items)
> > > 3) Handler gets scheduled.
> > > 4) Return the AudioParam object.
> > > 
> > > Is this what we want? 
> > > - So we warn user but we don't necessarily stop the operation?
> > > - What if 3rd item is NaN?
> > 
> > No.  I was intending to scan the array just once.  More logic needed to do
> that.
> 
> PS5 looks good. Should we check the new behavior with a layout test?

You mean more tests than are were already added in
audioparam-setValueCurve-exceptions.html?

[hongchan, 2016-05-24 23:28:35]

On 2016/05/24 23:03:43, Raymond Toy wrote:
> On 2016/05/24 23:01:42, hoch wrote:
> > On 2016/05/20 21:30:41, Raymond Toy wrote:
> > >
> >
>
https://codereview.chromium.org/1995583002/diff/60001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/modules/webaudio/AudioParam.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1995583002/diff/60001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/modules/webaudio/AudioParam.cpp:479: break;
> > > On 2016/05/20 21:14:07, hoch wrote:
> > > > So what will happen after this break? It still returns a valid
AudioParam
> > > object
> > > > at the end. Right?
> > > > 
> > > > 1) You have 4 items in the array.
> > > > 2) The 2nd item is out of range. So it breaks here. (meaning no more
> > > > verification for 3rd, 4th items)
> > > > 3) Handler gets scheduled.
> > > > 4) Return the AudioParam object.
> > > > 
> > > > Is this what we want? 
> > > > - So we warn user but we don't necessarily stop the operation?
> > > > - What if 3rd item is NaN?
> > > 
> > > No.  I was intending to scan the array just once.  More logic needed to do
> > that.
> > 
> > PS5 looks good. Should we check the new behavior with a layout test?
> 
> You mean more tests than are were already added in
> audioparam-setValueCurve-exceptions.html?

For example:

biquad.frequency.setValueCurveAtTime(Float32Array.from([0, 1, 96000, NaN]);
This should fail before the warning, because NaN throws the exception.

I think the test case should cover the example I provided in #7.

WDYT?

[Raymond Toy, 2016-05-24 23:30:49]

On 2016/05/24 23:28:35, hoch wrote:
> On 2016/05/24 23:03:43, Raymond Toy wrote:
> > On 2016/05/24 23:01:42, hoch wrote:
> > > On 2016/05/20 21:30:41, Raymond Toy wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1995583002/diff/60001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/modules/webaudio/AudioParam.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1995583002/diff/60001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/modules/webaudio/AudioParam.cpp:479: break;
> > > > On 2016/05/20 21:14:07, hoch wrote:
> > > > > So what will happen after this break? It still returns a valid
> AudioParam
> > > > object
> > > > > at the end. Right?
> > > > > 
> > > > > 1) You have 4 items in the array.
> > > > > 2) The 2nd item is out of range. So it breaks here. (meaning no more
> > > > > verification for 3rd, 4th items)
> > > > > 3) Handler gets scheduled.
> > > > > 4) Return the AudioParam object.
> > > > > 
> > > > > Is this what we want? 
> > > > > - So we warn user but we don't necessarily stop the operation?
> > > > > - What if 3rd item is NaN?
> > > > 
> > > > No.  I was intending to scan the array just once.  More logic needed to
do
> > > that.
> > > 
> > > PS5 looks good. Should we check the new behavior with a layout test?
> > 
> > You mean more tests than are were already added in
> > audioparam-setValueCurve-exceptions.html?
> 
> For example:
> 
> biquad.frequency.setValueCurveAtTime(Float32Array.from([0, 1, 96000, NaN]);
> This should fail before the warning, because NaN throws the exception.
> 
> I think the test case should cover the example I provided in #7.
> 
> WDYT?

Yeah, I can add this.  Can't tell what order things happen in, but certainly
catches the issue you found (where the curve would still be used).

[Raymond Toy, 2016-05-25 16:44:09]

New test added. PTAL.

[hongchan, 2016-05-25 16:46:17]

lgtm

[Raymond Toy, 2016-05-25 16:47:50]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2016-05-25 16:48:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1995583002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1995583002/140001

[commit-bot: I haz the power, 2016-05-25 20:26:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-25 20:26:57]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Raymond Toy, 2016-05-25 21:46:03]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2016-05-25 21:47:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1995583002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1995583002/140001

[commit-bot: I haz the power, 2016-05-25 23:45:09]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-05-25 23:47:10]

Description was changed from

==========
Throw exception for non-finite values in setValueCurve

If the curve given to setValueCurveAtTime contains non-finite values,
throw an exception.  All other AudioParam methods including the value
setter or the automation methods do not allow a non-finite value to be
specified.  The same should be true for the curve values given to
setValueCurveAtTime.

BUG=612787
TEST=audioparam-setValueCurve-exceptions.html
==========

to

==========
Throw exception for non-finite values in setValueCurve

If the curve given to setValueCurveAtTime contains non-finite values,
throw an exception.  All other AudioParam methods including the value
setter or the automation methods do not allow a non-finite value to be
specified.  The same should be true for the curve values given to
setValueCurveAtTime.

BUG=612787
TEST=audioparam-setValueCurve-exceptions.html

Committed: https://crrev.com/9e1870e6504d085c7b624ae16f55d03e46dc8ef0
Cr-Commit-Position: refs/heads/master@{#396042}
==========

[commit-bot: I haz the power, 2016-05-25 23:47:11]

Patchset 7 (id:??) landed as
https://crrev.com/9e1870e6504d085c7b624ae16f55d03e46dc8ef0
Cr-Commit-Position: refs/heads/master@{#396042}