Use StrictTypeChecking for methods on CRC2D that accepts Path2D

Use StrictTypeChecking for methods on CRC2D that accepts Path2D

This moves the null-check out into generated bindings code. Since there's
no longer a need to throw (explicitly) the ExceptionState argument can be
dropped from the signatures.

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

[fs, 2014-04-11 17:40:23]

This is the first in a series CLs that will attempt to transition
CanvasRenderingContext2D (and related interfaces) to use StrictTypeChecking.
Hopefully this should mean less "manual" checks.

[Justin Novosad, 2014-04-11 17:52:35]

Thanks for taking this on, but there are problems with the new error messages.

https://codereview.chromium.org/232913004/diff/1/LayoutTests/fast/canvas/canv...
File LayoutTests/fast/canvas/canvas-isPointInPath-winding-expected.txt (right):

https://codereview.chromium.org/232913004/diff/1/LayoutTests/fast/canvas/canv...
LayoutTests/fast/canvas/canvas-isPointInPath-winding-expected.txt:29: PASS
ctx.isPointInPath(null, 50, 50) threw exception TypeError: Failed to execute
'isPointInPath' on 'CanvasRenderingContext2D': parameter 3 ('50') is not a valid
enum value..
This is the wrong error message.

https://codereview.chromium.org/232913004/diff/1/LayoutTests/fast/canvas/canv...
LayoutTests/fast/canvas/canvas-isPointInPath-winding-expected.txt:30: PASS
ctx.isPointInPath(null, 50, 50, 'nonzero') threw exception TypeError: Failed to
execute 'isPointInPath' on 'CanvasRenderingContext2D': No function was found
that matched the signature provided..
This error message is less informative than what we had before. I would consider
this to be a regression.

[fs, 2014-04-11 18:31:19]

+nbarth
+haraken

For additional WebIDL/overload resolution know-how. Please check my ramblings
below.

https://codereview.chromium.org/232913004/diff/1/LayoutTests/fast/canvas/canv...
File LayoutTests/fast/canvas/canvas-isPointInPath-winding-expected.txt (right):

https://codereview.chromium.org/232913004/diff/1/LayoutTests/fast/canvas/canv...
LayoutTests/fast/canvas/canvas-isPointInPath-winding-expected.txt:29: PASS
ctx.isPointInPath(null, 50, 50) threw exception TypeError: Failed to execute
'isPointInPath' on 'CanvasRenderingContext2D': parameter 3 ('50') is not a valid
enum value..
On 2014/04/11 17:52:36, junov wrote:
> This is the wrong error message.

Well, it's not a great error message for sure... I believe this is due to the
overload handling (which needs some work) - essentially since the 'null' value
is not of type 'Path2D' the next overload is tried - <number, number, enum>. And
since the number 50 coerces into the string '50' you get this message. Thinking
about, I'm not even sure it'd work with better overload handling - a Path2D
object will never need conversion, and basically everything else can be turned
into 'float' (actually double - unrestricted). I'm not enough of a WebIDL expert
though.

https://codereview.chromium.org/232913004/diff/1/LayoutTests/fast/canvas/canv...
LayoutTests/fast/canvas/canvas-isPointInPath-winding-expected.txt:30: PASS
ctx.isPointInPath(null, 50, 50, 'nonzero') threw exception TypeError: Failed to
execute 'isPointInPath' on 'CanvasRenderingContext2D': No function was found
that matched the signature provided..
On 2014/04/11 17:52:36, junov wrote:
> This error message is less informative than what we had before. I would
consider
> this to be a regression.

Same issue as above, although here there's no additional overload to try so we
get the generic "No function matched" message.

[Nils Barth (inactive), 2014-04-14 02:32:18]

Replies regarding overload resolution and errors:
the new errors are actually correct, due to overload resolution being
complicated.

Correct overload resolution is:
Implement the Web IDL overload resolution algorithm
http://crbug.com/293561
...but current behavior is enough for this case.

https://codereview.chromium.org/232913004/diff/1/LayoutTests/fast/canvas/canv...
File LayoutTests/fast/canvas/canvas-isPointInPath-winding-expected.txt (right):

https://codereview.chromium.org/232913004/diff/1/LayoutTests/fast/canvas/canv...
LayoutTests/fast/canvas/canvas-isPointInPath-winding-expected.txt:29: PASS
ctx.isPointInPath(null, 50, 50) threw exception TypeError: Failed to execute
'isPointInPath' on 'CanvasRenderingContext2D': parameter 3 ('50') is not a valid
enum value..
On 2014/04/11 18:31:19, fs wrote:
> On 2014/04/11 17:52:36, junov wrote:
> > This is the wrong error message.
> 
> Well, it's not a great error message for sure... I believe this is due to the
> overload handling (which needs some work) - essentially since the 'null' value
> is not of type 'Path2D' the next overload is tried - <number, number, enum>.
And
> since the number 50 coerces into the string '50' you get this message.
Thinking
> about, I'm not even sure it'd work with better overload handling - a Path2D
> object will never need conversion, and basically everything else can be turned
> into 'float' (actually double - unrestricted). I'm not enough of a WebIDL
expert
> though.

As Fredrik notes, this is actually the correct error message, due to Web IDL's
implicit conversion rules:
(null, 50, 50) matches (float, float, DOMString),
and thus is converted to
(0f, 50f, "50").
This then fails semantic validation once it is passed to the operation:
enumerations are resolved as strings, with checks happening afterwards.

The spec is as follows:
overload resolution algorithm:
http://heycam.github.io/webidl/#es-interface-call
(Narrowly speaking, the exact match is at step 11.)

...and then the TypeError is thrown in the enumeration handling:
http://heycam.github.io/webidl/#es-enumeration

So the Web IDL semantics are confusing (as they always are if you have
overloading and implicit conversion), but the code is an accurate reflection of
the reality.

An ideal solution for this case error message-wise would probably be to:
1. State which overload was matched.
2. State the raw inputs and converted values.
...though that's a bit involved.

https://codereview.chromium.org/232913004/diff/1/LayoutTests/fast/canvas/canv...
LayoutTests/fast/canvas/canvas-isPointInPath-winding-expected.txt:30: PASS
ctx.isPointInPath(null, 50, 50, 'nonzero') threw exception TypeError: Failed to
execute 'isPointInPath' on 'CanvasRenderingContext2D': No function was found
that matched the signature provided..
On 2014/04/11 18:31:19, fs wrote:
> On 2014/04/11 17:52:36, junov wrote:
> > This error message is less informative than what we had before. I would
> consider
> > this to be a regression.
> 
> Same issue as above, although here there's no additional overload to try so we
> get the generic "No function matched" message.

As Fredrik says: the new message is correct, the old message was wrong.
If you provide null for an argument and there's no function that has a nullable
parameter there, it simply shouldn't match.

It might be possible to give better error messages stating *which* argument
failed to match, but in general we can't say more than "didn't match", because
the overload resolution algorithm is very complicated.
Even in this case, we can't actually give a simple answer.
See, (null, 50) matches (Float, Float), converted to (0f, 50f),
so is the problem that you have a |null| instead of a Path,
or that you have (..., 50, 'nonzero')?

In simple cases we *may* be able to add some logic to give clearer error
messages, but this case is complex enough that we can't honestly give a clearer
answer.

[fs, 2014-04-14 07:36:53]

On 2014/04/14 02:32:18, Nils Barth wrote:
> Replies regarding overload resolution and errors:
> the new errors are actually correct, due to overload resolution being
> complicated.

Thanks for having a look Nils - much appreciated (and thanks for the [[Call]]
spec reference I've been meaning to read that more carefullly.)
 
[snipped all the good stuff]

[Nils Barth (inactive), 2014-04-14 07:40:11]

On 2014/04/14 07:36:53, fs wrote:
> Thanks for having a look Nils - much appreciated (and thanks for the [[Call]]
> spec reference I've been meaning to read that more carefullly.)
>  
> [snipped all the good stuff]

n/p, and thanks for fixing this!

[Justin Novosad, 2014-04-14 14:11:40]

lgtm

[fs, 2014-04-14 15:01:09]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-04-14 15:01:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fs@opera.com/232913004/20001

[commit-bot: I haz the power, 2014-04-14 15:11:14]

Change committed as 171471