IDL: Simplify [TypeChecking=Interface] code generation

IDL: Simplify [TypeChecking=Interface] code generation

Instead of doing a separate hasInstance() check before converting, simply
use toImplWithTypeCheck() to convert and check after if the result was
null, as it is if not hasInstance().

The primary benefit of this is that we no longer need to optimize away the
extra hasInstance() check in toImplWithTypeCheck() by using an "unsafe"
conversion in the [TypeChecking=Interface] case. This simplifies the code
generator without losing the optimization.

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

[Jens Widell, 2014-12-18 13:47:04]

jl@opera.com changed reviewers:
+ haraken@chromium.org

[Jens Widell, 2014-12-18 13:47:05]

PTAL

https://codereview.chromium.org/808373002/diff/1/Source/bindings/tests/result...
File Source/bindings/tests/results/core/V8TestInterface.cpp (right):

https://codereview.chromium.org/808373002/diff/1/Source/bindings/tests/result...
Source/bindings/tests/results/core/V8TestInterface.cpp:1154: window =
toDOMWindow(info.GetIsolate(), info[0]);
This is a real change; toDOMWindow() does not just use V8Window::hasInstance()
to check the type. The new code is better, I think.

This doesn't affect any real methods. Window arguments aren't that common, and
apparently none of them have [TypeChecking=Interface].

https://codereview.chromium.org/808373002/diff/1/Source/bindings/tests/result...
File Source/bindings/tests/results/core/V8TestObject.cpp (left):

https://codereview.chromium.org/808373002/diff/1/Source/bindings/tests/result...
Source/bindings/tests/results/core/V8TestObject.cpp:10236: if (info.Length() > 1
&& !V8Node::hasInstance(info[1], info.GetIsolate())) {
I dropped the "info.Length() > N" check on purpose, since it's essentially
redundant.

For non-optional arguments, the basic arity check at the beginning of the
function covers it.

For optional arguments without default value, the code handling the argument
value is never reached if the argument is missing.

For optional arguments with default value, the code isn't reached either if the
argument is missing, since there's an outer if-statement whose else-branch sets
the default instead.

But for optional non-nullable arguments with [Default=Undefined], we now throw
if the argument is missing (instead of passing on a null pointer), since
undefined isn't a valid value for a type-checked interface argument. There are
no such method arguments in real IDL files today, and any such argument should
really just be nullable with a "= null" default value.

This line is the only place it makes a difference. (And this method is a test
case added to verify that the optimization I now remove was working correctly,
so could probably be dropped.)

[haraken, 2014-12-18 14:50:27]

LGTM, nice change.

https://codereview.chromium.org/808373002/diff/1/Source/bindings/tests/result...
File Source/bindings/tests/results/core/V8TestInterface.cpp (right):

https://codereview.chromium.org/808373002/diff/1/Source/bindings/tests/result...
Source/bindings/tests/results/core/V8TestInterface.cpp:1154: window =
toDOMWindow(info.GetIsolate(), info[0]);
On 2014/12/18 13:47:05, Jens Widell wrote:
> This is a real change; toDOMWindow() does not just use V8Window::hasInstance()
> to check the type. The new code is better, I think.
> 
> This doesn't affect any real methods. Window arguments aren't that common, and
> apparently none of them have [TypeChecking=Interface].

I'm pretty sure that the new code is more correct. In the previous code,
|window| can become 0 in edge cases.

https://codereview.chromium.org/808373002/diff/1/Source/bindings/tests/result...
File Source/bindings/tests/results/core/V8TestObject.cpp (left):

https://codereview.chromium.org/808373002/diff/1/Source/bindings/tests/result...
Source/bindings/tests/results/core/V8TestObject.cpp:10236: if (info.Length() > 1
&& !V8Node::hasInstance(info[1], info.GetIsolate())) {
On 2014/12/18 13:47:05, Jens Widell wrote:
> I dropped the "info.Length() > N" check on purpose, since it's essentially
> redundant.
> 
> For non-optional arguments, the basic arity check at the beginning of the
> function covers it.
> 
> For optional arguments without default value, the code handling the argument
> value is never reached if the argument is missing.
> 
> For optional arguments with default value, the code isn't reached either if
the
> argument is missing, since there's an outer if-statement whose else-branch
sets
> the default instead.
> 
> But for optional non-nullable arguments with [Default=Undefined], we now throw
> if the argument is missing (instead of passing on a null pointer), since
> undefined isn't a valid value for a type-checked interface argument. There are
> no such method arguments in real IDL files today, and any such argument should
> really just be nullable with a "= null" default value.
> 
> This line is the only place it makes a difference. (And this method is a test
> case added to verify that the optimization I now remove was working correctly,
> so could probably be dropped.)

This sounds like a valid reasoning.

[Jens Widell, 2014-12-18 15:37:55]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-12-18 15:38:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/808373002/1

[commit-bot: I haz the power, 2014-12-18 17:03:19]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187476