IDL: Add toRestricted{Float,Double}() helpers to V8Binding.h

IDL: Add toRestricted{Float,Double}() helpers to V8Binding.h

These implement conversion from V8 value to a float/double C++ that throws
a TypeError if the numeric value is NaN or infinite. This is how the IDL
types 'float' and 'double' should behave according to WebIDL, but in Blink
they currently only do if an interface/method/attribute has the extended
attribute [TypeChecking=Unrestricted].

This CL changes two observable things:

1) The error message changes from being specific to the method argument
   being converted ("double parameter N is non-finite"), to being generic
   ("The provided double value is non-finite").

2) 'float'/'double' members in union types go from being always
   unrestricted to always being restricted, which is correct per
   specification.

Since [TypeChecking=Unrestricted] wasn't supported either for dictionary
members or union type members, it was previously impossible to get the
correct behavior for these type conversions. An unrestricted conversion
for a union member can be achieved by declaring the member as 'unrestricted
float' or 'unrestricted double', also per specification. A restricted
conversion for a dictionary member can now be achieved by adding
[TypeChecking=Unrestricted] on the dictionary as a whole or on the member.

BUG=354298, 450252
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188797

[Jens Widell, 2015-01-21 13:35:09]

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

[Jens Widell, 2015-01-21 13:35:10]

haraken, bashi: PTAL
fs: FYI

Slightly bigger patch than I had intended initially, but I think it's a step in
a good direction.

https://codereview.chromium.org/860353002/diff/1/Source/bindings/core/v8/V8Bi...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/860353002/diff/1/Source/bindings/core/v8/V8Bi...
Source/bindings/core/v8/V8Binding.h:472: float
toRestrictedFloat(v8::Handle<v8::Value>, ExceptionState&);
From a WebIDL POV, it could be argued to make more sense to have toFloat() and
toUnrestrictedFloat() here.

From a C++ POV, toFloat() and toRestrictedFloat() makes some sense, since the
C++ float type is unrestricted by default. And this is C++.

It doesn't look like toFloat()/toDouble() are used from non-generated code, so
changing their behavior should be non-intrusive. And, to make things clear, we
could of course have toRestrictedFloat() and toUnrestrictedFloat(), and no
toFloat() at all.

Opinions?

https://codereview.chromium.org/860353002/diff/1/Source/bindings/scripts/v8_a...
File Source/bindings/scripts/v8_attributes.py (left):

https://codereview.chromium.org/860353002/diff/1/Source/bindings/scripts/v8_a...
Source/bindings/scripts/v8_attributes.py:332:
context['has_type_checking_unrestricted'] or
Dropping this is safe since idl_type.v8_conversion_needs_exception_state is
always true for float and double types.

https://codereview.chromium.org/860353002/diff/1/Source/bindings/scripts/v8_i...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/860353002/diff/1/Source/bindings/scripts/v8_i...
Source/bindings/scripts/v8_interface.py:1187: # FIXME: check interface's
[TypeChecking] attribute too.
I intend to address this in a different CL shortly.

https://codereview.chromium.org/860353002/diff/1/Source/bindings/scripts/v8_t...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/860353002/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:557: if base_idl_type in ('float', 'double')
and not restricted_float:
This is king of backwards. The ultimate solution is to drop
[TypeChecking=Unrestricted] (make it apply always) and switch all
attributes/arguments that should be unrestricted over to an unrestricted type.
Then these two lines can be dropped.

[bashi, 2015-01-22 02:11:15]

I like this approach. LGTM.

https://codereview.chromium.org/860353002/diff/1/Source/bindings/core/v8/V8Bi...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/860353002/diff/1/Source/bindings/core/v8/V8Bi...
Source/bindings/core/v8/V8Binding.h:472: float
toRestrictedFloat(v8::Handle<v8::Value>, ExceptionState&);
On 2015/01/21 13:35:10, Jens Widell wrote:
> From a WebIDL POV, it could be argued to make more sense to have toFloat() and
> toUnrestrictedFloat() here.
> 
> From a C++ POV, toFloat() and toRestrictedFloat() makes some sense, since the
> C++ float type is unrestricted by default. And this is C++.
> 
> It doesn't look like toFloat()/toDouble() are used from non-generated code, so
> changing their behavior should be non-intrusive. And, to make things clear, we
> could of course have toRestrictedFloat() and toUnrestrictedFloat(), and no
> toFloat() at all.
> 
> Opinions?

I don't have strong opinion here, but I prefer having toFloat() and
toRestrictedFloat() (i.e. this CL) because it wouldn't change the behavior of
toFloat() and I generally see this kind of mismatch from a C++ POV (not sure
it's a common among Blink developers, though). We might want to add a comment to
mention the mismatch.

https://codereview.chromium.org/860353002/diff/1/Source/bindings/scripts/v8_t...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/860353002/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:557: if base_idl_type in ('float', 'double')
and not restricted_float:
On 2015/01/21 13:35:10, Jens Widell wrote:
> This is king of backwards. The ultimate solution is to drop
> [TypeChecking=Unrestricted] (make it apply always) and switch all
> attributes/arguments that should be unrestricted over to an unrestricted type.
> Then these two lines can be dropped.

I'm not suggesting to do in this CL, but I think it's great to drop
TypeChecking=Unrestricted if possible. It would make Blink more spec-compatible.

[haraken, 2015-01-22 02:43:06]

LGTM

https://codereview.chromium.org/860353002/diff/1/Source/bindings/core/v8/V8Bi...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/860353002/diff/1/Source/bindings/core/v8/V8Bi...
Source/bindings/core/v8/V8Binding.h:472: float
toRestrictedFloat(v8::Handle<v8::Value>, ExceptionState&);
On 2015/01/22 02:11:14, bashi1 wrote:
> On 2015/01/21 13:35:10, Jens Widell wrote:
> > From a WebIDL POV, it could be argued to make more sense to have toFloat()
and
> > toUnrestrictedFloat() here.
> > 
> > From a C++ POV, toFloat() and toRestrictedFloat() makes some sense, since
the
> > C++ float type is unrestricted by default. And this is C++.
> > 
> > It doesn't look like toFloat()/toDouble() are used from non-generated code,
so
> > changing their behavior should be non-intrusive. And, to make things clear,
we
> > could of course have toRestrictedFloat() and toUnrestrictedFloat(), and no
> > toFloat() at all.
> > 
> > Opinions?
> 
> I don't have strong opinion here, but I prefer having toFloat() and
> toRestrictedFloat() (i.e. this CL) because it wouldn't change the behavior of
> toFloat() and I generally see this kind of mismatch from a C++ POV (not sure
> it's a common among Blink developers, though). We might want to add a comment
to
> mention the mismatch.

+1 to toFloat() and toRestrictedFloat().

https://codereview.chromium.org/860353002/diff/1/Source/bindings/scripts/v8_t...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/860353002/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:557: if base_idl_type in ('float', 'double')
and not restricted_float:
On 2015/01/22 02:11:14, bashi1 wrote:
> On 2015/01/21 13:35:10, Jens Widell wrote:
> > This is king of backwards. The ultimate solution is to drop
> > [TypeChecking=Unrestricted] (make it apply always) and switch all
> > attributes/arguments that should be unrestricted over to an unrestricted
type.
> > Then these two lines can be dropped.
> 
> I'm not suggesting to do in this CL, but I think it's great to drop
> TypeChecking=Unrestricted if possible. It would make Blink more
spec-compatible.

Yes, let's try to drop TypeChecking=Unrestricted :)

https://codereview.chromium.org/860353002/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/860353002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:557: if base_idl_type in ('float', 'double')
and not restricted_float:

Shall we add a FIXME and mention that we should have 'unrestricted' in actual
IDL files instead of adding 'unrestricted' here?

[Jens Widell, 2015-01-22 06:59:10]

Thanks for reviews!

https://codereview.chromium.org/860353002/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/860353002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_types.py:557: if base_idl_type in ('float', 'double')
and not restricted_float:
On 2015/01/22 02:43:06, haraken wrote:
> 
> Shall we add a FIXME and mention that we should have 'unrestricted' in actual
> IDL files instead of adding 'unrestricted' here?

Comment added.

[Jens Widell, 2015-01-22 06:59:15]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2015-01-22 06:59:46]

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

[commit-bot: I haz the power, 2015-01-22 08:17:50]

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

[a.suchit2, 2015-02-20 15:03:11]

a.suchit@chromium.org changed reviewers:
+ a.suchit@chromium.org

[a.suchit2, 2015-02-20 15:03:12]

https://codereview.chromium.org/860353002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8Binding.cpp (right):

https://codereview.chromium.org/860353002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8Binding.cpp:528: exceptionState.throwTypeError("The
provided float value is non-finite.");
Why are we not throwing NotSupportedError from here based on specification
www.w3.org/TR/2dcontext/

Many of the canvas API should throw NotSupportedError in the case of
infinite/Nan. Because control never goes to our code for throwing these
exceptions based on specifications.

https://codereview.chromium.org/860353002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8Binding.cpp:555: exceptionState.throwTypeError("The
provided double value is non-finite.");
same as above.

[Jens Widell, 2015-02-20 15:16:47]

https://codereview.chromium.org/860353002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8Binding.cpp (right):

https://codereview.chromium.org/860353002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8Binding.cpp:528: exceptionState.throwTypeError("The
provided float value is non-finite.");
On 2015/02/20 15:03:12, a.suchit2 wrote:
> Why are we not throwing NotSupportedError from here based on specification
> http://www.w3.org/TR/2dcontext/
> 
> Many of the canvas API should throw NotSupportedError in the case of
> infinite/Nan. Because control never goes to our code for throwing these
> exceptions based on specifications.

We're throwing TypeError here because that's what WebIDL says to do, and WebIDL
is the specification this code primarily implements.

The specification you link to is probably simply wrong. (And also probably
obsolete: don't read /TR/ specifications!) The description of the API in
http://html.spec.whatwg.org/ does not say to throw NotSupportedError (except in
a non-normative section, which in turn is probably simply a bug). The
specification you link also says

  "Interfaces are defined in terms of Web IDL."

which is incompatible with requiring that a different type of exception should
be throw in error conditions also covered by Web IDL.

[a.suchit2, 2015-02-21 09:35:26]

https://codereview.chromium.org/860353002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8Binding.cpp (right):

https://codereview.chromium.org/860353002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8Binding.cpp:528: exceptionState.throwTypeError("The
provided float value is non-finite.");
On 2015/02/20 15:16:46, Jens Widell wrote:
> On 2015/02/20 15:03:12, a.suchit2 wrote:
> > Why are we not throwing NotSupportedError from here based on specification
> > http://www.w3.org/TR/2dcontext/
> > 
> > Many of the canvas API should throw NotSupportedError in the case of
> > infinite/Nan. Because control never goes to our code for throwing these
> > exceptions based on specifications.
> 
> We're throwing TypeError here because that's what WebIDL says to do, and
WebIDL
> is the specification this code primarily implements.
> 
> The specification you link to is probably simply wrong. (And also probably
> obsolete: don't read /TR/ specifications!) The description of the API in
> http://html.spec.whatwg.org/ does not say to throw NotSupportedError (except
in
> a non-normative section, which in turn is probably simply a bug). The
> specification you link also says
> 
>   "Interfaces are defined in terms of Web IDL."
> 
> which is incompatible with requiring that a different type of exception should
> be throw in error conditions also covered by Web IDL.

Thanks Jens