Improve 'Selection' exception messages.

Improve 'Selection' exception messages.

Rather than throwing generic exceptions, we should give developers a bit
of context around what failed and why. This patch addresses that concern
for the Selection APIs.

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

[Mike West, 2013-08-09 13:51:45]

Hi Erik! #3 of #?.

-mike

[arv (Not doing code reviews), 2013-08-09 15:22:48]

Waiting on previous CL to resolve the naming guidelines.

[Mike West, 2013-08-12 09:11:14]

Updated to "Failed to execute 'XXX' on a 'Selection' object:...", in line with
the discussion on the Location patch.

WDYT?

[arv (Not doing code reviews), 2013-08-12 14:37:54]

On 2013/08/12 09:11:14, Mike West wrote:
> Updated to "Failed to execute 'XXX' on a 'Selection' object:...", in line with
> the discussion on the Location patch.
> 
> WDYT?

Failed to *call* sounds better to me.

[arv (Not doing code reviews), 2013-08-12 14:40:10]

This raises another question... I'm starting to think that we should not include
the method/accessor and interface name in the error message. It is just
repeating information that the debugger will give us.

[arv (Not doing code reviews), 2013-08-12 14:41:00]

This raises another question... I'm starting to think that we should not include
the method/accessor and interface name in the error message. It is just
repeating information that the debugger will give us.

Adding Adam and Christophe for opinions.

[Use mkwst_at_chromium.org plz., 2013-08-12 14:46:10]

On 2013/08/12 14:41:00, arv wrote:
> This raises another question... I'm starting to think that we should not
include
> the method/accessor and interface name in the error message. It is just
> repeating information that the debugger will give us.

If an exception is caught by a global `window.onerror` handler and shipped off
to a collection server for processing, the debugger isn't particularly useful.
That's a use-case I think we ought to care about here.

[do-not-use, 2013-08-12 14:46:50]

On 2013/08/12 14:41:00, arv wrote:
> This raises another question... I'm starting to think that we should not
include
> the method/accessor and interface name in the error message. It is just
> repeating information that the debugger will give us.
> 
> Adding Adam and Christophe for opinions.

I agree with Erik that providing the method name in the error message is not
really common / needed. IMHO, we can remove them.

[Use mkwst_at_chromium.org plz., 2013-08-12 14:50:30]

On 2013/08/12 14:37:54, arv wrote:
> On 2013/08/12 09:11:14, Mike West wrote:
> > Updated to "Failed to execute 'XXX' on a 'Selection' object:...", in line
with
> > the discussion on the Location patch.
> > 
> > WDYT?
> 
> Failed to *call* sounds better to me.

Well, Blink didn't really fail to call the method. The call is what generated
the exception. "Unsuccessfully executed XXX" is probably most accurate, but
sounds even worse. :)

[arv (Not doing code reviews), 2013-08-12 15:10:10]

The error occurred in XXX, so you are right called is not correct either.

Even for window.onerror we get the location and the stack of where the
error occurred. That gives the user enough information about which object
and method was called to cause the error.

On Mon, Aug 12, 2013 at 10:50 AM, <mkwst@google.com> wrote:

> On 2013/08/12 14:37:54, arv wrote:
>
>> On 2013/08/12 09:11:14, Mike West wrote:
>> > Updated to "Failed to execute 'XXX' on a 'Selection' object:...", in
>> line
>>
> with
>
>> > the discussion on the Location patch.
>> >
>> > WDYT?
>>
>
>  Failed to *call* sounds better to me.
>>
>
> Well, Blink didn't really fail to call the method. The call is what
> generated
> the exception. "Unsuccessfully executed XXX" is probably most accurate, but
> sounds even worse. :)
>
>
https://codereview.chromium.**org/22608004/<https://codereview.chromium.org/2...
>



-- 
erik

[Mike West, 2013-08-13 10:31:57]

On 2013/08/12 15:10:10, arv wrote:
> The error occurred in XXX, so you are right called is not correct either.
> 
> Even for window.onerror we get the location and the stack of where the
> error occurred. That gives the user enough information about which object
> and method was called to cause the error.

While I agree that the stack trace should give them enough information to track
down the error on their own, I think we should make developers' jobs as simple
as possible. The current workflow looks something like opening up the trace,
clicking a link, saying "Oh, crap. I minified my code.", clicking the
unminification button, clicking the stack trace link again, and then trying to
piece together exactly what the variables 'x' and 'b'  were at the time of the
call. We can help folks along by giving them hints about what `x[b](a, c)` is
actually doing.

[arv (Not doing code reviews), 2013-08-13 13:49:01]

OK. Let's continue down this path then. We can always change this later.


On Tue, Aug 13, 2013 at 6:31 AM, <mkwst@chromium.org> wrote:

> On 2013/08/12 15:10:10, arv wrote:
>
>> The error occurred in XXX, so you are right called is not correct either.
>>
>
>  Even for window.onerror we get the location and the stack of where the
>> error occurred. That gives the user enough information about which object
>> and method was called to cause the error.
>>
>
> While I agree that the stack trace should give them enough information to
> track
> down the error on their own, I think we should make developers' jobs as
> simple
> as possible. The current workflow looks something like opening up the
> trace,
> clicking a link, saying "Oh, crap. I minified my code.", clicking the
> unminification button, clicking the stack trace link again, and then
> trying to
> piece together exactly what the variables 'x' and 'b'  were at the time of
> the
> call. We can help folks along by giving them hints about what `x[b](a, c)`
> is
> actually doing.
>
>
https://codereview.chromium.**org/22608004/<https://codereview.chromium.org/2...
>



-- 
erik

[arv (Not doing code reviews), 2013-08-13 13:57:30]

https://codereview.chromium.org/22608004/diff/11001/LayoutTests/fast/dom/Sele...
File LayoutTests/fast/dom/Selection/script-tests/collapseToX-empty-selection.js
(right):

https://codereview.chromium.org/22608004/diff/11001/LayoutTests/fast/dom/Sele...
LayoutTests/fast/dom/Selection/script-tests/collapseToX-empty-selection.js:7:
shouldThrow("sel.collapseToStart()");
Why don't you want to test the message any more?

https://codereview.chromium.org/22608004/diff/11001/Source/core/page/DOMSelec...
File Source/core/page/DOMSelection.cpp (right):

https://codereview.chromium.org/22608004/diff/11001/Source/core/page/DOMSelec...
Source/core/page/DOMSelection.cpp:221: es.throwDOMException(InvalidStateError,
"Failed to execute 'collapseToEnd' on a 'Selection' object: there is no
selection.");
I feel like we are repeating ourselves a lot. Maybe use String::format?

es.throwDOMException(InvalidStateError, String::format(failedToExecutePattern,
"collapseToEnd", "Selection", "there is no selection"));

where pattern is defined somewhere where it can be reused

failedToExecutePattern = "Failed to execute '%s' on a '%s' object: %s."

[Mike West, 2013-08-13 14:00:16]

Thanks, I'll update the patch.

https://codereview.chromium.org/22608004/diff/11001/LayoutTests/fast/dom/Sele...
File LayoutTests/fast/dom/Selection/script-tests/collapseToX-empty-selection.js
(right):

https://codereview.chromium.org/22608004/diff/11001/LayoutTests/fast/dom/Sele...
LayoutTests/fast/dom/Selection/script-tests/collapseToX-empty-selection.js:7:
shouldThrow("sel.collapseToStart()");
On 2013/08/13 13:57:31, arv wrote:
> Why don't you want to test the message any more?

Laziness?

I realized that `shouldThrow` will output the message to the expectations file
if I leave the second parameter off entirely. That's a lot less work than
updating all the HTML files every time we iterated on some text. :)

https://codereview.chromium.org/22608004/diff/11001/Source/core/page/DOMSelec...
File Source/core/page/DOMSelection.cpp (right):

https://codereview.chromium.org/22608004/diff/11001/Source/core/page/DOMSelec...
Source/core/page/DOMSelection.cpp:221: es.throwDOMException(InvalidStateError,
"Failed to execute 'collapseToEnd' on a 'Selection' object: there is no
selection.");
On 2013/08/13 13:57:31, arv wrote:
> I feel like we are repeating ourselves a lot. Maybe use String::format?
> 
> es.throwDOMException(InvalidStateError, String::format(failedToExecutePattern,
> "collapseToEnd", "Selection", "there is no selection"));
> 
> where pattern is defined somewhere where it can be reused
> 
> failedToExecutePattern = "Failed to execute '%s' on a '%s' object: %s."

I can do that. I have vague performance reluctance to use String::format, but
given the rarity with which exceptions should be thrown, I guess it's nothing to
really worry about.

[arv (Not doing code reviews), 2013-08-13 14:07:48]

https://codereview.chromium.org/22608004/diff/11001/LayoutTests/fast/dom/Sele...
File LayoutTests/fast/dom/Selection/script-tests/collapseToX-empty-selection.js
(right):

https://codereview.chromium.org/22608004/diff/11001/LayoutTests/fast/dom/Sele...
LayoutTests/fast/dom/Selection/script-tests/collapseToX-empty-selection.js:7:
shouldThrow("sel.collapseToStart()");
On 2013/08/13 14:00:17, Mike West wrote:
> On 2013/08/13 13:57:31, arv wrote:
> > Why don't you want to test the message any more?
> 
> Laziness?
> 
> I realized that `shouldThrow` will output the message to the expectations file
> if I leave the second parameter off entirely. That's a lot less work than
> updating all the HTML files every time we iterated on some text. :)

I thought as much.

The problem is that we will end up with PASS even if the wrong exception is
thrown. It will require updating the expectation but as long as it says PASS I
doubt people will realize that they might have broken something.

(I really hate how we write our tests and this is one of the reasons.)

https://codereview.chromium.org/22608004/diff/11001/Source/core/page/DOMSelec...
File Source/core/page/DOMSelection.cpp (right):

https://codereview.chromium.org/22608004/diff/11001/Source/core/page/DOMSelec...
Source/core/page/DOMSelection.cpp:221: es.throwDOMException(InvalidStateError,
"Failed to execute 'collapseToEnd' on a 'Selection' object: there is no
selection.");
On 2013/08/13 14:00:17, Mike West wrote:
> On 2013/08/13 13:57:31, arv wrote:
> > I feel like we are repeating ourselves a lot. Maybe use String::format?
> > 
> > es.throwDOMException(InvalidStateError,
String::format(failedToExecutePattern,
> > "collapseToEnd", "Selection", "there is no selection"));
> > 
> > where pattern is defined somewhere where it can be reused
> > 
> > failedToExecutePattern = "Failed to execute '%s' on a '%s' object: %s."
> 
> I can do that. I have vague performance reluctance to use String::format, but
> given the rarity with which exceptions should be thrown, I guess it's nothing
to
> really worry about.

Yeah, I don't think we need to optimize for the exception case ;-)

[Mike West, 2013-08-13 14:09:20]

https://codereview.chromium.org/22608004/diff/11001/LayoutTests/fast/dom/Sele...
File LayoutTests/fast/dom/Selection/script-tests/collapseToX-empty-selection.js
(right):

https://codereview.chromium.org/22608004/diff/11001/LayoutTests/fast/dom/Sele...
LayoutTests/fast/dom/Selection/script-tests/collapseToX-empty-selection.js:7:
shouldThrow("sel.collapseToStart()");
On 2013/08/13 14:07:49, arv wrote:
> On 2013/08/13 14:00:17, Mike West wrote:
> > On 2013/08/13 13:57:31, arv wrote:
> > > Why don't you want to test the message any more?
> > 
> > Laziness?
> > 
> > I realized that `shouldThrow` will output the message to the expectations
file
> > if I leave the second parameter off entirely. That's a lot less work than
> > updating all the HTML files every time we iterated on some text. :)
> 
> I thought as much.
> 
> The problem is that we will end up with PASS even if the wrong exception is
> thrown. It will require updating the expectation but as long as it says PASS I
> doubt people will realize that they might have broken something.
> 
> (I really hate how we write our tests and this is one of the reasons.)

That's a fair point. Hrm.

Ok. I'll attempt to overcome my laziness. :/

[Mike West, 2013-08-13 14:54:43]

The latest patchset delazifies the tests, and adds
'ExceptionState::failedToExecute(...)', which seemed like a reasonable place to
put it. WDYT?

I'd imagine adding 'failedToSet' and 'failedToGet' in future patches to cover
some other case I've seen.

-mike

[arv (Not doing code reviews), 2013-08-13 17:57:33]

I don't think this belongs in ExceptionState. How about introducing a new file
with a namespace/static methods; ExceptionMessages?

[Use mkwst_at_chromium.org plz., 2013-08-13 21:01:05]

On 2013/08/13 17:57:33, arv wrote:
> I don't think this belongs in ExceptionState. How about introducing a new file
> with a namespace/static methods; ExceptionMessages?

Hrm. It seems like we'd always end up including those two files together.

I guess it's cleaner to split out the methods, however. I'll add them.

[Mike West, 2013-08-14 07:24:35]

On 2013/08/13 21:01:05, mkwst wrote:
> On 2013/08/13 17:57:33, arv wrote:
> > I don't think this belongs in ExceptionState. How about introducing a new
file
> > with a namespace/static methods; ExceptionMessages?
> 
> Hrm. It seems like we'd always end up including those two files together.
> 
> I guess it's cleaner to split out the methods, however. I'll add them.

The latest patchset extracts these bits into ExceptionMessages. Mind taking
another look?

[arv (Not doing code reviews), 2013-08-14 16:19:37]

LGTM

[Mike West, 2013-08-14 16:28:30]

On 2013/08/14 16:19:37, arv wrote:
> LGTM

Thanks for working through this with me.

[commit-bot: I haz the power, 2013-08-14 16:28:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/22608004/37001

[commit-bot: I haz the power, 2013-08-15 03:38:15]

Change committed as 156131