Generate a bit less code to handle failed arity checks.

Generate a bit less code to handle failed arity checks.

If a failed argument count check is in the context of an 'extended'
ExceptionState object, then throw the required TypeError using it
instead. This saves having to duplicate the information it already
contains, generating a bit less code overall.

R=mkwst@chromium.org
BUG=270033
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163527

[sof, 2013-12-04 22:41:59]

With the grand switchover to the extended ExceptionState now completed & landed,
a minor change covering the code generated for failed arity/argument count
checks.

[jochen (gone - plz use gerrit), 2013-12-05 08:58:09]

i defer to Mike

[Mike West, 2013-12-05 11:48:59]

The change broadly looks good. I'm not sure about the change in ExceptionState
semantics, however. Comments inline.

https://codereview.chromium.org/105693002/diff/1/Source/bindings/v8/Exception...
File Source/bindings/v8/ExceptionState.cpp (right):

https://codereview.chromium.org/105693002/diff/1/Source/bindings/v8/Exception...
Source/bindings/v8/ExceptionState.cpp:89: throwIfNeeded();
This is a change from the semantics we've set up for ExceptionState. Generally,
we pass the state somewhere, allow that somewhere to queue up an exception, and
then _actually_ throw the exception when we get back to bindings.

I'd prefer to keep that implicit contract, even if it means that throwing
exceptions _from_ bindings is a bit more verbose.

https://codereview.chromium.org/105693002/diff/1/Source/bindings/v8/Exception...
File Source/bindings/v8/ExceptionState.h (right):

https://codereview.chromium.org/105693002/diff/1/Source/bindings/v8/Exception...
Source/bindings/v8/ExceptionState.h:85: void notEnoughArguments(int expected,
int actual);
Hrm. 

1. This should have "throw" in the name.
2. Does this provide enough value to stand alone? I'm wondering where the line
is between "throwTypeError(ExceptionMessages::xxx(...))" and
"throwXXXError(...)". Are we going to end up with 10s of methods on
ExceptionState?

https://codereview.chromium.org/105693002/diff/1/Source/bindings/v8/custom/V8...
File Source/bindings/v8/custom/V8CryptoCustom.cpp (right):

https://codereview.chromium.org/105693002/diff/1/Source/bindings/v8/custom/V8...
Source/bindings/v8/custom/V8CryptoCustom.cpp:43: ExceptionState
exceptionState(ExceptionState::ExecutionContext, "getRandomValues", "Crypto",
info.Holder(), info.GetIsolate());
These custom bindings changes look reasonable, thank you!

[sof, 2013-12-05 12:01:32]

https://codereview.chromium.org/105693002/diff/1/Source/bindings/v8/Exception...
File Source/bindings/v8/ExceptionState.cpp (right):

https://codereview.chromium.org/105693002/diff/1/Source/bindings/v8/Exception...
Source/bindings/v8/ExceptionState.cpp:89: throwIfNeeded();
On 2013/12/05 11:48:59, Mike West wrote:
> This is a change from the semantics we've set up for ExceptionState.
Generally,
> we pass the state somewhere, allow that somewhere to queue up an exception,
and
> then _actually_ throw the exception when we get back to bindings.
> 
> I'd prefer to keep that implicit contract, even if it means that throwing
> exceptions _from_ bindings is a bit more verbose.

It seems redundant to generate that much code at each site doing an arity check.

Contract expectations can be accounted for by naming, I think. I didn't prefix
"throw" as it isn't throwing a non-existent NotEnoughArguments exception.
Perhaps "report" or some such would be clearer.

[Mike West, 2013-12-05 12:15:15]

https://codereview.chromium.org/105693002/diff/1/Source/bindings/v8/Exception...
File Source/bindings/v8/ExceptionState.cpp (right):

https://codereview.chromium.org/105693002/diff/1/Source/bindings/v8/Exception...
Source/bindings/v8/ExceptionState.cpp:89: throwIfNeeded();
On 2013/12/05 12:01:32, sof wrote:
> It seems redundant to generate that much code at each site doing an arity
check.

As long as we can assume that this is always called in the same context in which
the ExceptionState was created, I agree with you. If I pass 'exceptionState'
into a function, however, I don't want a situation where 'throwIfNeeded' might
be called both by the callee and the caller.
 
> Contract expectations can be accounted for by naming, I think. I didn't prefix
> "throw" as it isn't throwing a non-existent NotEnoughArguments exception.
> Perhaps "report" or some such would be clearer.

Sure. This comes back to one of the other questions. When do we want the
callsite to call 'throwDOMException' or 'throwTypeException' with an appropriate
message, and when do we want to wrap those to set the message implicitly?

[sof, 2013-12-09 22:53:20]

https://codereview.chromium.org/105693002/diff/1/Source/bindings/v8/Exception...
File Source/bindings/v8/ExceptionState.h (right):

https://codereview.chromium.org/105693002/diff/1/Source/bindings/v8/Exception...
Source/bindings/v8/ExceptionState.h:85: void notEnoughArguments(int expected,
int actual);
On 2013/12/05 11:48:59, Mike West wrote:
> Hrm. 
> 
> 1. This should have "throw" in the name.
> 2. Does this provide enough value to stand alone? I'm wondering where the line
> is between "throwTypeError(ExceptionMessages::xxx(...))" and
> "throwXXXError(...)". Are we going to end up with 10s of methods on
> ExceptionState?

(FTR, there are currently 484 applications of this in the generated bindings
code.)

If the concern is that having helpers like this will make the general use(r) of
ExceptionState too hard to work with, then the class system could naturally be
used to separate the two different uses of it.

Anyway, with it being suggested that this arity-check helper shouldn't also do
the 'real' throw by calling throwIfNeeded(), the case for introducing one here
does indeed fall away. I've taken it out.

[Mike West, 2013-12-10 09:46:46]

LGTM. Let's go with this for the moment, and see where we end up as we get
through more of the bindings code. :)

[Mike West, 2013-12-10 09:47:43]

LGTM. Let's go with this for the moment, and see where we end up as we get
through more of the bindings code. :)

[commit-bot: I haz the power, 2013-12-10 10:15:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/105693002/40001

[commit-bot: I haz the power, 2013-12-10 11:21:17]

Change committed as 163527