Throw TypeError instead of InvalidAccessError for invalid times

Throw RangeError instead of InvalidAccessError for invalid times

Automation methods are supposed to throw a RangeError if the start time
or time constant are invalid.  Currently they throw
InvalidAccessError.

See spec issue: https://github.com/WebAudio/web-audio-api/issues/1182

BUG=685657, 726793
TEST=dom-exceptions.html,
AudioParam/audioparam-exceptional-values.html,
AudioParam/audioparam-method-channing.html

Review-Url: https://codereview.chromium.org/2656993002
Cr-Commit-Position: refs/heads/master@{#477812}
Committed: https://chromium.googlesource.com/chromium/src/+/36682d2fd06777d9dd371416fff959aefa36c14a

[Raymond Toy, 2017-01-26 17:22:41]

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

[Raymond Toy, 2017-01-26 17:22:41]

PTAL

[haraken, 2017-01-26 18:01:53]

V8TypeError is a JS error defined in Web IDL to handle exceptions that happen in
the binding layer. You're expected to use a DOM exception for exceptions that
happen in DOM operations.

What does the WebAudio spec say about it?

[Raymond Toy, 2017-01-26 18:11:58]

On 2017/01/26 18:01:53, haraken wrote:
> V8TypeError is a JS error defined in Web IDL to handle exceptions that happen
in
> the binding layer. You're expected to use a DOM exception for exceptions that
> happen in DOM operations.
> 
> What does the WebAudio spec say about it?

The spec for setValueAtTime
(https://webaudio.github.io/web-audio-api/#widl-AudioParam-setValueAtTime-Audi...)
for example, says setting the startTime to a non-positive or non-finite value
throws a TypeError.

The spec could be wrong.

[hongchan, 2017-01-26 22:38:43]

On 2017/01/26 18:11:58, Raymond Toy wrote:
> On 2017/01/26 18:01:53, haraken wrote:
> > V8TypeError is a JS error defined in Web IDL to handle exceptions that
happen
> in
> > the binding layer. You're expected to use a DOM exception for exceptions
that
> > happen in DOM operations.
> > 
> > What does the WebAudio spec say about it?
> 
> The spec for setValueAtTime
>
(https://webaudio.github.io/web-audio-api/#widl-AudioParam-setValueAtTime-Audi...)
> for example, says setting the startTime to a non-positive or non-finite value
> throws a TypeError.
> 
> The spec could be wrong.

Should we hold this CL until we clarify that?

[hongchan, 2017-03-24 15:41:19]

On 2017/01/26 22:38:43, hongchan wrote:
> On 2017/01/26 18:11:58, Raymond Toy wrote:
> > On 2017/01/26 18:01:53, haraken wrote:
> > > V8TypeError is a JS error defined in Web IDL to handle exceptions that
> happen
> > in
> > > the binding layer. You're expected to use a DOM exception for exceptions
> that
> > > happen in DOM operations.
> > > 
> > > What does the WebAudio spec say about it?
> > 
> > The spec for setValueAtTime
> >
>
(https://webaudio.github.io/web-audio-api/#widl-AudioParam-setValueAtTime-Audi...)
> > for example, says setting the startTime to a non-positive or non-finite
value
> > throws a TypeError.
> > 
> > The spec could be wrong.
> 
> Should we hold this CL until we clarify that?

Just for the reference:
https://github.com/WebAudio/web-audio-api/issues/1166

[Raymond Toy, 2017-05-26 16:19:03]

Description was changed from

==========
Throw TypeError instead of InvalidAccessError for invalid times

Automation methods are supposed to throw a TypeError if the start time
or time constant are invalid.  Currently they throw
InvalidAccessError.

BUG=685657
TEST=dom-exceptions.html,
AudioParam/audioparam-exceptional-values.html,
AudioParam/audioparam-method-channing.html
==========

to

==========
Throw RangeError instead of InvalidAccessError for invalid times

Automation methods are supposed to throw a RangeError if the start time
or time constant are invalid.  Currently they throw
InvalidAccessError.

BUG=685657
TEST=dom-exceptions.html,
AudioParam/audioparam-exceptional-values.html,
AudioParam/audioparam-method-channing.html
==========

[Raymond Toy, 2017-05-26 16:20:49]

On 2017/03/24 15:41:19, hongchan wrote:
> On 2017/01/26 22:38:43, hongchan wrote:
> > On 2017/01/26 18:11:58, Raymond Toy wrote:
> > > On 2017/01/26 18:01:53, haraken wrote:
> > > > V8TypeError is a JS error defined in Web IDL to handle exceptions that
> > happen
> > > in
> > > > the binding layer. You're expected to use a DOM exception for exceptions
> > that
> > > > happen in DOM operations.
> > > > 
> > > > What does the WebAudio spec say about it?
> > > 
> > > The spec for setValueAtTime
> > >
> >
>
(https://webaudio.github.io/web-audio-api/#widl-AudioParam-setValueAtTime-Audi...)
> > > for example, says setting the startTime to a non-positive or non-finite
> value
> > > throws a TypeError.
> > > 
> > > The spec could be wrong.
> > 
> > Should we hold this CL until we clarify that?
> 
> Just for the reference:
> https://github.com/WebAudio/web-audio-api/issues/1166

This issue has been resolved and the corresponding pull request has landed.
We should throw a RangeError for these cases.

PTAL.

[hongchan, 2017-05-26 20:48:13]

lgtm

[Raymond Toy, 2017-05-26 20:48:18]

On 2017/05/26 16:20:49, Raymond Toy wrote:
> On 2017/03/24 15:41:19, hongchan wrote:
> > On 2017/01/26 22:38:43, hongchan wrote:
> > > On 2017/01/26 18:11:58, Raymond Toy wrote:
> > > > On 2017/01/26 18:01:53, haraken wrote:
> > > > > V8TypeError is a JS error defined in Web IDL to handle exceptions that
> > > happen
> > > > in
> > > > > the binding layer. You're expected to use a DOM exception for
exceptions
> > > that
> > > > > happen in DOM operations.
> > > > > 
> > > > > What does the WebAudio spec say about it?
> > > > 
> > > > The spec for setValueAtTime
> > > >
> > >
> >
>
(https://webaudio.github.io/web-audio-api/#widl-AudioParam-setValueAtTime-Audi...)
> > > > for example, says setting the startTime to a non-positive or non-finite
> > value
> > > > throws a TypeError.
> > > > 
> > > > The spec could be wrong.
> > > 
> > > Should we hold this CL until we clarify that?
> > 
> > Just for the reference:
> > https://github.com/WebAudio/web-audio-api/issues/1166
> 
> This issue has been resolved and the corresponding pull request has landed.
> We should throw a RangeError for these cases.
> 
> PTAL.

Please hold off.  I think I should use ThrowRangeError method to signal the
error, and there needs to be some additional tests added.  The tests we have
check that errors are thrown but doesn't check the actual error type.

[Raymond Toy, 2017-05-30 17:56:37]

PTAL.  ThrowRangeError is used now and tests updated to check for the exception
type.

[Raymond Toy, 2017-05-30 18:15:10]

Description was changed from

==========
Throw RangeError instead of InvalidAccessError for invalid times

Automation methods are supposed to throw a RangeError if the start time
or time constant are invalid.  Currently they throw
InvalidAccessError.

BUG=685657
TEST=dom-exceptions.html,
AudioParam/audioparam-exceptional-values.html,
AudioParam/audioparam-method-channing.html
==========

to

==========
Throw RangeError instead of InvalidAccessError for invalid times

Automation methods are supposed to throw a RangeError if the start time
or time constant are invalid.  Currently they throw
InvalidAccessError.

BUG=685657, 726793
TEST=dom-exceptions.html,
AudioParam/audioparam-exceptional-values.html,
AudioParam/audioparam-method-channing.html
==========

[Raymond Toy, 2017-06-06 16:27:19]

Ping

[hongchan, 2017-06-06 16:34:21]

On 2017/06/06 16:27:19, Raymond Toy wrote:
> Ping

Sorry that I missed this one - lgtm again with nit.

[hongchan, 2017-06-06 16:34:29]

https://codereview.chromium.org/2656993002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/webaudio/AudioParam/audioparam-exceptional-values.html
(right):

https://codereview.chromium.org/2656993002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webaudio/AudioParam/audioparam-exceptional-values.html:53:
funcArg: (value) => [value, 1, 1]
Line 48-53 needs to be wrapped properly.

https://codereview.chromium.org/2656993002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webaudio/AudioParam/audioparam-exceptional-values.html:193:
// method.
Not a showstopper, but the comment is usually located above the function name.
No?

https://codereview.chromium.org/2656993002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webaudio/AudioParam/audioparam-exceptional-values.html:206:
// Specialized printer for automation arguments so that messages make
Ditto.

[Raymond Toy, 2017-06-06 17:55:33]

https://codereview.chromium.org/2656993002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/webaudio/AudioParam/audioparam-exceptional-values.html
(right):

https://codereview.chromium.org/2656993002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webaudio/AudioParam/audioparam-exceptional-values.html:53:
funcArg: (value) => [value, 1, 1]
On 2017/06/06 16:34:29, hongchan wrote:
> Line 48-53 needs to be wrapped properly.

clang-format all the things!

https://codereview.chromium.org/2656993002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webaudio/AudioParam/audioparam-exceptional-values.html:193:
// method.
On 2017/06/06 16:34:29, hongchan wrote:
> Not a showstopper, but the comment is usually located above the function name.
> No?

Done.

https://codereview.chromium.org/2656993002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webaudio/AudioParam/audioparam-exceptional-values.html:206:
// Specialized printer for automation arguments so that messages make
On 2017/06/06 16:34:29, hongchan wrote:
> Ditto.

Done.

[Raymond Toy, 2017-06-07 20:08:10]

Description was changed from

==========
Throw RangeError instead of InvalidAccessError for invalid times

Automation methods are supposed to throw a RangeError if the start time
or time constant are invalid.  Currently they throw
InvalidAccessError.

BUG=685657, 726793
TEST=dom-exceptions.html,
AudioParam/audioparam-exceptional-values.html,
AudioParam/audioparam-method-channing.html
==========

to

==========
Throw RangeError instead of InvalidAccessError for invalid times

Automation methods are supposed to throw a RangeError if the start time
or time constant are invalid.  Currently they throw
InvalidAccessError.

See spec issue: https://github.com/WebAudio/web-audio-api/issues/1182

BUG=685657, 726793
TEST=dom-exceptions.html,
AudioParam/audioparam-exceptional-values.html,
AudioParam/audioparam-method-channing.html
==========

[Raymond Toy, 2017-06-07 20:09:11]

The CQ bit was checked by rtoy@chromium.org

[Raymond Toy, 2017-06-07 20:09:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
hongchan@chromium.org
Link to the patchset: https://codereview.chromium.org/2656993002/#ps120001
(title: "Address review comments and clang-format it")

[commit-bot: I haz the power, 2017-06-07 20:09:38]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 21:10:21]

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

[commit-bot: I haz the power, 2017-06-07 21:10:23]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Raymond Toy, 2017-06-07 22:44:02]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2017-06-07 22:44:41]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 23:55:20]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496875442378960,
"parent_rev": "7d2c2483af8243eab43b70e24cc30bd6f61b3714", "commit_rev":
"36682d2fd06777d9dd371416fff959aefa36c14a"}

[commit-bot: I haz the power, 2017-06-07 23:55:33]

Description was changed from

==========
Throw RangeError instead of InvalidAccessError for invalid times

Automation methods are supposed to throw a RangeError if the start time
or time constant are invalid.  Currently they throw
InvalidAccessError.

See spec issue: https://github.com/WebAudio/web-audio-api/issues/1182

BUG=685657, 726793
TEST=dom-exceptions.html,
AudioParam/audioparam-exceptional-values.html,
AudioParam/audioparam-method-channing.html
==========

to

==========
Throw RangeError instead of InvalidAccessError for invalid times

Automation methods are supposed to throw a RangeError if the start time
or time constant are invalid.  Currently they throw
InvalidAccessError.

See spec issue: https://github.com/WebAudio/web-audio-api/issues/1182

BUG=685657, 726793
TEST=dom-exceptions.html,
AudioParam/audioparam-exceptional-values.html,
AudioParam/audioparam-method-channing.html

Review-Url: https://codereview.chromium.org/2656993002
Cr-Commit-Position: refs/heads/master@{#477812}
Committed:
https://chromium.googlesource.com/chromium/src/+/36682d2fd06777d9dd371416fff9...
==========

[commit-bot: I haz the power, 2017-06-07 23:55:36]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/36682d2fd06777d9dd371416fff9...