Add support for type checking of floating point arguments as [TypeChecking=Unrestricted]

Add support for type checking of floating point arguments as [TypeChecking=Unrestricted]

This adds an new extended attribute value, [TypeChecking=Unrestricted],
which turns on type checking of floating point arguments for non-finite
values, and switches CanvasRenderingContext2D.createImageData over as a
test case.

This does not implement type-checking for floating point attributes yet.

Docs updated at:
http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-TypeChecking-i-m-a-

R=haraken
R=fs
BUG=354298
BUG=367648
TEST=run_webkit_tests.sh fast/canvas/canvas-2d-imageData-create-nonfinite.html

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

[Nils Barth (inactive), 2014-04-28 09:16:14]

First cut at adding type checking for floating point args.

haraken, how does bindings code look?
Fredrik, how does usage in Blink look?
Jochen, is this sane, or is there a better V8 way to do it?
(...or can we change V8 to make this easier?)

Thanks!

https://codereview.chromium.org/259773008/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/259773008/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:107: if (info.Length() >
{{argument.index}} && {% if argument.is_nullable
%}!isUndefinedOrNull(info[{{argument.index}}]) && {% endif
%}WTF::double_conversion::Double(info[{{argument.index}}]->NumberValue()).IsSpecial())
{
This would be much slicker (and faster) as:
info[{{argument.index}}]->IsSpecial()

...instead of:

WTF::double_conversion::Double(
    info[{{argument.index}}]->NumberValue())
    .IsSpecial()

[haraken, 2014-04-28 10:35:33]

Discussed offline. This CL looks OK, but it will become simpler if V8 exposes
the IsSpecial() method (in src/double.h) to V8 APIs.

[Nils Barth (inactive), 2014-04-28 10:40:45]

Simplified slightly.
Fredrik, Jochen?

https://codereview.chromium.org/259773008/diff/40001/Source/bindings/template...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/259773008/diff/40001/Source/bindings/template...
Source/bindings/templates/methods.cpp:107: if (info.Length() >
{{argument.index}} && {% if argument.is_nullable
%}!isUndefinedOrNull(info[{{argument.index}}]) && {% endif
%}!std::isfinite(info[{{argument.index}}]->NumberValue())) {
Actually it's not that bad;
can just use std::isfinite:
!std::isfinite(info[{{argument.index}}]->NumberValue())
...but it would still be nicer to do via V8 as:
info[{{argument.index}}]->IsSpecial()
...as we just need to do a simple check, not compute the value.

[fs, 2014-04-28 10:58:33]

core/ changes LGTM, but I have question on the bindings change.

https://codereview.chromium.org/259773008/diff/40001/Source/bindings/template...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/259773008/diff/40001/Source/bindings/template...
Source/bindings/templates/methods.cpp:107: if (info.Length() >
{{argument.index}} && {% if argument.is_nullable
%}!isUndefinedOrNull(info[{{argument.index}}]) && {% endif
%}!std::isfinite(info[{{argument.index}}]->NumberValue())) {
On 2014/04/28 10:40:45, Nils Barth wrote:
> Actually it's not that bad;
> can just use std::isfinite:
> !std::isfinite(info[{{argument.index}}]->NumberValue())
> ...but it would still be nicer to do via V8 as:
> info[{{argument.index}}]->IsSpecial()
> ...as we just need to do a simple check, not compute the value.

But in a case like:

{ valueOf: function() { return NaN; } }

it really won't be a "simple" check. But maybe that's uncommon enough to not
matter much... (Or to put it another way: What would IsSpecial() mean on a
generic v8::Value?)

That being said though - why not do the finiteness check after
v8_value_to_local_cpp_value (i.e, on the cppValue)?

[Nils Barth (inactive), 2014-04-30 01:58:02]

Updated per Fredrik's comments;
haraken, WDYT?

https://codereview.chromium.org/259773008/diff/40001/Source/bindings/template...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/259773008/diff/40001/Source/bindings/template...
Source/bindings/templates/methods.cpp:107: if (info.Length() >
{{argument.index}} && {% if argument.is_nullable
%}!isUndefinedOrNull(info[{{argument.index}}]) && {% endif
%}!std::isfinite(info[{{argument.index}}]->NumberValue())) {
On 2014/04/28 10:58:34, fs wrote:
> But in a case like:
> 
> { valueOf: function() { return NaN; } }
> 
> it really won't be a "simple" check. But maybe that's uncommon enough to not
> matter much... (Or to put it another way: What would IsSpecial() mean on a
> generic v8::Value?)

Good point; presumably would need to be IsSpecialDouble or something, but
wouldn't make sense for non-numbers.

> That being said though - why not do the finiteness check after
> v8_value_to_local_cpp_value (i.e, on the cppValue)?

That's even better!
Done; now it's just a simple std::isfinite check, which is as efficient as we
can get.
This presumably just checks the biased exponent field, which is how non-numbers
are stored:
http://en.wikipedia.org/wiki/IEEE_754-1985#Representation_of_non-numbers
...and is the same check that V8 does:
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/double.h&q=...

Currently type checking is in two places: before and after storing the cppValue,
and it's generally simpler to put it after, as you suggest.

[haraken, 2014-04-30 02:50:27]

LGTM.

std::isfinite looks nice enough to me, so you won't need try hard to expose
IsSpecial to V8 APIs.

[Nils Barth (inactive), 2014-04-30 02:52:52]

On 2014/04/30 02:50:27, haraken wrote:
> LGTM.
> 
> std::isfinite looks nice enough to me, so you won't need try hard to expose
> IsSpecial to V8 APIs.

Yup, agreed!
Jochen, no action needed by V8: we can handle this fine in Blink!

[Nils Barth (inactive), 2014-04-30 02:53:16]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-04-30 02:54:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nbarth@chromium.org/259773008/60001

[commit-bot: I haz the power, 2014-04-30 03:25:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-30 03:25:17]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink

[Nils Barth (inactive), 2014-04-30 04:31:38]

On 2014/04/30 03:25:17, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_blink_rel on tryserver.blink

Test failures are due to old tests (4 years ago: 2010-06) with outdated
hard-coded expectations of specific exceptions:
canvas/philip/tests/2d.imageData.create2.nonfinite.html

This is completely redundant with our up-to-date one (exactly the same test
cases):
fast/canvas/canvas-2d-imageData-create-nonfinite.html
fast/canvas/resources/canvas-2d-imageData-create-nonfinite.js
...so deleted.

[Nils Barth (inactive), 2014-04-30 04:31:57]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-04-30 04:32:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nbarth@chromium.org/259773008/80001

[commit-bot: I haz the power, 2014-04-30 05:32:52]

Change committed as 172945