Add more informative messages for DOM exceptions.

Add more informative messages for DOM exceptions.

BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158561

[Raymond Toy (Google), 2013-09-26 22:59:02]

Please take a look.

[Mike West, 2013-09-27 06:44:38]

Thank you for taking a look at this! The messages you've put together here are a
significant improvement!

I have a few nit-picky suggestions below. Generally, I'd like to see the
developer's input included in the error message: "The provided value 'xxx' is
invalid; only 'y', 'z', and 'a' are supported." gives some immediate feedback
that "The value is not 'y', 'z', 'or 'a'." doesn't.

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
File Source/modules/webaudio/AudioBuffer.cpp (right):

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioBuffer.cpp:104: "getChannelData",
If this is called as `ab.getChannelData()`, then I'd prefer
'ExceptionMessages::failedToExecute'. 'failedToGet' implies direct property
access, which I don't think this maps to.

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioBuffer.cpp:106: "invalid channel index."));
How about "The provided index (" + String::number(channelIndex) + ") is
invalid."?

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
File Source/modules/webaudio/AudioContext.cpp (left):

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioContext.cpp:344: ASSERT(mediaElement);
Why drop the ASSERT?

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioContext.cpp:301: "invalid number of channels,
frames, or sample rate."));
For these kinds of errors, it would be nice to tell the developer which of the
numbers was invalid. That said, this is still strictly better than the previous
message. :)

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioContext.cpp:397:
ExceptionMessages::failedToConstruct("MediaStreamAudioSourceNode"));
"invalid MediaStream source"?

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioContext.cpp:547: "invalid number of outputs."));
"The number of outputs (" + String::number(numberOfOutputs) ") is invalid."?

It would be nice if you could give a pointer in the message as to what would be
valid: "We only support 17 outputs." or something. :)

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioContext.cpp:572: "invalid number of inputs."));
Same as for outputs.

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioContext.cpp:602: "invalid real or imag arrays for
wave table."));
One option would be to split this up into multiple exceptions: "Invalid real
array", "Invalid imaginary array", "The real array and imaginary array must be
the same length (8 vs 7)."

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
File Source/modules/webaudio/AudioNode.cpp (right):

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioNode.cpp:150: "invalid output index."));
"The provided output index (8) is larger than the number of outputs (3)."

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioNode.cpp:160: "invalid input index."));
Same as above.

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioNode.cpp:233: "invalid output index."));
Same as above.

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioNode.cpp:263: "invalid number of channels."));
Perhaps split this into "The channel count may not be 0." and "The provided
channel count (x) is greater than the maximum number of channels supported (y)."

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioNode.cpp:300: "must be \"max\", \"clamped-max\", or
\"explicit\"."));
I'd suggest including the developer's input here: "The provided mode '" + mode +
"' is invalid. Only 'x', 'y', and 'z' are supported."

I've also generally been using single-quotes to denote code in error messages,
but that's pure preference without any real rationale behind it.

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioNode.cpp:334: "must be \"speakers\" or
\"discrete\"."));
Same as above.

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Defau...
File Source/modules/webaudio/DefaultAudioDestinationNode.cpp (right):

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Defau...
Source/modules/webaudio/DefaultAudioDestinationNode.cpp:128: "invalid channel
count."));
Same as above.

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Delay...
File Source/modules/webaudio/DelayNode.cpp (right):

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Delay...
Source/modules/webaudio/DelayNode.cpp:48: "invalid max delay time."));
Same as above.

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Offli...
File Source/modules/webaudio/OfflineAudioContext.cpp (right):

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Offli...
Source/modules/webaudio/OfflineAudioContext.cpp:56: "invalid number of channels
or sample rate"));
Same as above.

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/WaveS...
File Source/modules/webaudio/WaveShaperNode.cpp (right):

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/WaveS...
Source/modules/webaudio/WaveShaperNode.cpp:80: "must be one of: \"none\",
\"2x\", \"4x\"."));
Same as above.

[Raymond Toy (Google), 2013-09-27 16:23:53]

On 2013/09/27 06:44:38, Mike West wrote:
> Thank you for taking a look at this! The messages you've put together here are
a
> significant improvement!

Thank you for making this available! I always hated these generic messages when
the code knows exactly what went wrong.
> 
> I have a few nit-picky suggestions below. Generally, I'd like to see the
> developer's input included in the error message: "The provided value 'xxx' is
> invalid; only 'y', 'z', and 'a' are supported." gives some immediate feedback
> that "The value is not 'y', 'z', 'or 'a'." doesn't.

Yes, I'll try to make the messages even more informative, if possible.

> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
> File Source/modules/webaudio/AudioBuffer.cpp (right):
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
> Source/modules/webaudio/AudioBuffer.cpp:104: "getChannelData",
> If this is called as `ab.getChannelData()`, then I'd prefer
> 'ExceptionMessages::failedToExecute'. 'failedToGet' implies direct property
> access, which I don't think this maps to.
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
> Source/modules/webaudio/AudioBuffer.cpp:106: "invalid channel index."));
> How about "The provided index (" + String::number(channelIndex) + ") is
> invalid."?
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
> File Source/modules/webaudio/AudioContext.cpp (left):
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
> Source/modules/webaudio/AudioContext.cpp:344: ASSERT(mediaElement);
> Why drop the ASSERT?
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
> File Source/modules/webaudio/AudioContext.cpp (right):
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
> Source/modules/webaudio/AudioContext.cpp:301: "invalid number of channels,
> frames, or sample rate."));
> For these kinds of errors, it would be nice to tell the developer which of the
> numbers was invalid. That said, this is still strictly better than the
previous
> message. :)
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
> Source/modules/webaudio/AudioContext.cpp:397:
> ExceptionMessages::failedToConstruct("MediaStreamAudioSourceNode"));
> "invalid MediaStream source"?
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
> Source/modules/webaudio/AudioContext.cpp:547: "invalid number of outputs."));
> "The number of outputs (" + String::number(numberOfOutputs) ") is invalid."?
> 
> It would be nice if you could give a pointer in the message as to what would
be
> valid: "We only support 17 outputs." or something. :)
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
> Source/modules/webaudio/AudioContext.cpp:572: "invalid number of inputs."));
> Same as for outputs.
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
> Source/modules/webaudio/AudioContext.cpp:602: "invalid real or imag arrays for
> wave table."));
> One option would be to split this up into multiple exceptions: "Invalid real
> array", "Invalid imaginary array", "The real array and imaginary array must be
> the same length (8 vs 7)."
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
> File Source/modules/webaudio/AudioNode.cpp (right):
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
> Source/modules/webaudio/AudioNode.cpp:150: "invalid output index."));
> "The provided output index (8) is larger than the number of outputs (3)."
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
> Source/modules/webaudio/AudioNode.cpp:160: "invalid input index."));
> Same as above.
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
> Source/modules/webaudio/AudioNode.cpp:233: "invalid output index."));
> Same as above.
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
> Source/modules/webaudio/AudioNode.cpp:263: "invalid number of channels."));
> Perhaps split this into "The channel count may not be 0." and "The provided
> channel count (x) is greater than the maximum number of channels supported
(y)."
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
> Source/modules/webaudio/AudioNode.cpp:300: "must be \"max\", \"clamped-max\",
or
> \"explicit\"."));
> I'd suggest including the developer's input here: "The provided mode '" + mode
+
> "' is invalid. Only 'x', 'y', and 'z' are supported."
> 
> I've also generally been using single-quotes to denote code in error messages,
> but that's pure preference without any real rationale behind it.
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
> Source/modules/webaudio/AudioNode.cpp:334: "must be \"speakers\" or
> \"discrete\"."));
> Same as above.
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Defau...
> File Source/modules/webaudio/DefaultAudioDestinationNode.cpp (right):
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Defau...
> Source/modules/webaudio/DefaultAudioDestinationNode.cpp:128: "invalid channel
> count."));
> Same as above.
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Delay...
> File Source/modules/webaudio/DelayNode.cpp (right):
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Delay...
> Source/modules/webaudio/DelayNode.cpp:48: "invalid max delay time."));
> Same as above.
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Offli...
> File Source/modules/webaudio/OfflineAudioContext.cpp (right):
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Offli...
> Source/modules/webaudio/OfflineAudioContext.cpp:56: "invalid number of
channels
> or sample rate"));
> Same as above.
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/WaveS...
> File Source/modules/webaudio/WaveShaperNode.cpp (right):
> 
>
https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/WaveS...
> Source/modules/webaudio/WaveShaperNode.cpp:80: "must be one of: \"none\",
> \"2x\", \"4x\"."));
> Same as above.

[Raymond Toy (Google), 2013-09-27 16:24:10]

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
File Source/modules/webaudio/AudioContext.cpp (left):

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioContext.cpp:344: ASSERT(mediaElement);
On 2013/09/27 06:44:39, Mike West wrote:
> Why drop the ASSERT?

When running the webaudio layout tests in debug mode, this ASSERT asserts and
crashes the test. Since we check mediaElement in the next line, we would not be
missing anything, while still allowing us to test this case in the layout test.

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioContext.cpp:301: "invalid number of channels,
frames, or sample rate."));
On 2013/09/27 06:44:39, Mike West wrote:
> For these kinds of errors, it would be nice to tell the developer which of the
> numbers was invalid. That said, this is still strictly better than the
previous
> message. :)

Agreed. I'll try to make this (and all others) more informative.  For this
particular one, it might be difficult to produce a more informative message
without access to the limits used in AudioBuffer::create.  (I'll need to check).

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
File Source/modules/webaudio/AudioNode.cpp (right):

https://codereview.chromium.org/24877002/diff/1/Source/modules/webaudio/Audio...
Source/modules/webaudio/AudioNode.cpp:300: "must be \"max\", \"clamped-max\", or
\"explicit\"."));
On 2013/09/27 06:44:39, Mike West wrote:
> I'd suggest including the developer's input here: "The provided mode '" + mode
+
> "' is invalid. Only 'x', 'y', and 'z' are supported."

Agreed.
> 
> I've also generally been using single-quotes to denote code in error messages,
> but that's pure preference without any real rationale behind it.

I originally had single quotes, but kind of wanted to distinguish how you print
messages vs the values.  Currently we print something like

Failed to set `channelCountMode' of `AudioNode': must be "max", "clamped-max",
"explicit".

Not sure if this distinction is necessary. Single quotes are fine with me.

[Raymond Toy (Google), 2013-09-27 19:59:12]

Even more informative messages added.

[Ken Russell (switch to Gerrit), 2013-09-27 23:15:38]

Almost all of the side-by-side diffs say "error: old chunk mismatch". Could you
please re-upload the CL?

https://codereview.chromium.org/24877002/diff/24001/LayoutTests/webaudio/dom-...
File LayoutTests/webaudio/dom-exceptions.html (right):

https://codereview.chromium.org/24877002/diff/24001/LayoutTests/webaudio/dom-...
LayoutTests/webaudio/dom-exceptions.html:31: // Test createion of various
objects
Typo: createion

[Raymond Toy (Google), 2013-09-28 02:27:08]

On 2013/09/27 23:15:38, Ken Russell wrote:
> Almost all of the side-by-side diffs say "error: old chunk mismatch". Could
you
> please re-upload the CL?
> 
>
https://codereview.chromium.org/24877002/diff/24001/LayoutTests/webaudio/dom-...
> File LayoutTests/webaudio/dom-exceptions.html (right):
> 
>
https://codereview.chromium.org/24877002/diff/24001/LayoutTests/webaudio/dom-...
> LayoutTests/webaudio/dom-exceptions.html:31: // Test createion of various
> objects
> Typo: createion

Sorry about that. Not sure what happened. This new patch looks better.

[Ken Russell (switch to Gerrit), 2013-09-28 02:39:40]

Thanks. These look nice. LGTM

[Mike West, 2013-09-28 17:09:18]

LGTM2. Thank you!

[Raymond Toy (Google), 2013-09-30 16:06:22]

On 2013/09/28 17:09:18, Mike West wrote:
> LGTM2. Thank you!

Thanks, everyone!

[commit-bot: I haz the power, 2013-09-30 16:06:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtoy@google.com/24877002/37001

[commit-bot: I haz the power, 2013-10-01 00:31:37]

Change committed as 158561