Improve handling of failed integer type conversions.

Improve handling of failed integer type conversions.

Change over the V8Binding methods for converting to integer types
(to*Int*()) to take an ExceptionState argument instead of using a
bool reference argument to indicate success/failure.

Improve the exception messages from failed type conversion as a
result; adjust the binding code generator to emit compatible calls and
exception propagation.

Reworked some of the error reporting in the WebGL custom binding code
while switching over; more informative exception messages will now
flow.

R=jsbell,nbarth,haraken
BUG=

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

[sof, 2014-01-02 12:01:28]

Please take a look when you next have a moment.

While I haven't intentionally tried to break the code generated by the spiffy
new IDL compiler, I haven't verified this (how might one go about doing that?)

jsbell: mainly just the IDBCursor.advance() change - if you could look it over,
that'd be great.

[jsbell, 2014-01-02 18:03:22]

modules/indexeddb (and corresponding tests) lgtm. I skimmed but did not
throughly review the code generator changes (since I was involved in some of the
EnforceRange impl), questions below.

re: testing the python generator: run-bindings-tests --test-python

https://codereview.chromium.org/121113004/diff/200001/Source/bindings/scripts...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/121113004/diff/200001/Source/bindings/scripts...
Source/bindings/scripts/code_generator_v8.pm:2373: #   - weird SVG stuff.
nit: inconsistent punctuation in the list

https://codereview.chromium.org/121113004/diff/200001/Source/bindings/tests/r...
File Source/bindings/tests/results/V8TestObject.cpp (right):

https://codereview.chromium.org/121113004/diff/200001/Source/bindings/tests/r...
Source/bindings/tests/results/V8TestObject.cpp:246: ExceptionState
exceptionState(ExceptionState::SetterContext, "byteAttr", "TestObject",
info.Holder(), info.GetIsolate());
Is there going to be any perf impact from this extra initialization in
attributes that don't throw (i.e. don't have EnforceRange, etc) ?

https://codereview.chromium.org/121113004/diff/200001/Source/modules/indexedd...
File Source/modules/indexeddb/IDBCursor.cpp (right):

https://codereview.chromium.org/121113004/diff/200001/Source/modules/indexedd...
Source/modules/indexeddb/IDBCursor.cpp:145: if (!count) {
Any particular reason for changing the order? lgtm anyway, since this case is
listed first in the spec (even though we couldn't get consensus on a strict
ordering in the spec).

[sof, 2014-01-02 19:30:30]

https://codereview.chromium.org/121113004/diff/200001/Source/bindings/tests/r...
File Source/bindings/tests/results/V8TestObject.cpp (right):

https://codereview.chromium.org/121113004/diff/200001/Source/bindings/tests/r...
Source/bindings/tests/results/V8TestObject.cpp:246: ExceptionState
exceptionState(ExceptionState::SetterContext, "byteAttr", "TestObject",
info.Holder(), info.GetIsolate());
On 2014/01/02 18:03:23, jsbell wrote:
> Is there going to be any perf impact from this extra initialization in
> attributes that don't throw (i.e. don't have EnforceRange, etc) ?

The setter here being passed a simple number not bundled up in a valueOf() which
potentially throws? The overhead added:

 - stack allocation of an ExceptionState (with a trivial destructor.)
 - an UNLIKELY() check post-conversion, checking for exceptions.
 
I wouldn't think these combine to make much of a difference, but I don't have
hard data.

(FWIW, a semi-related data point: I worried about the same when adding
creationContext to ExceptionState objects, but haven't come across any
performance regressions against its addition.)

https://codereview.chromium.org/121113004/diff/200001/Source/modules/indexedd...
File Source/modules/indexeddb/IDBCursor.cpp (right):

https://codereview.chromium.org/121113004/diff/200001/Source/modules/indexedd...
Source/modules/indexeddb/IDBCursor.cpp:145: if (!count) {
On 2014/01/02 18:03:23, jsbell wrote:
> Any particular reason for changing the order? lgtm anyway, since this case is
> listed first in the spec (even though we couldn't get consensus on a strict
> ordering in the spec).

The spec was the motivation for moving it up, but also that it makes logical
sense to have it first -- to my mind, the >0 constraint being an extension of
[EnforceRange] validation of the argument. Hence having other observable
exceptions between these two parts would be unnatural.

But that's all API implementation machinery details -- if it is preferable to
keep it the way it was, happy to revert the placement.

[sof, 2014-01-02 20:53:48]

jsbell writes:
>
..
> re: testing the python generator: run-bindings-tests --test-python

Great, thanks. 

I've updated it to mirror the functional changes on the Perl side. Adding nbarth
as a reviewer.

[Nils Barth (inactive), 2014-01-06 02:54:16]

On 2014/01/02 20:53:48, sof wrote:
> jsbell writes:
> >
> ..
> > re: testing the python generator: run-bindings-tests --test-python
> 
> Great, thanks.

Thanks Joshua!

> I've updated it to mirror the functional changes on the Perl side. Adding
nbarth
> as a reviewer.

Thanks for doing the Python side Sigbjørn!
...and for this CL, full stop!

I've a few comments on code organization (mostly Python, a bit Perl), but
otherwise LGTM!

[Nils Barth (inactive), 2014-01-06 02:58:08]

Comments on CG changes;
main one is to have an INTEGER_TYPES set
(instead of using 3 C++-specific sets), as 'integer type'
is a well-defined Web IDL concept.

https://codereview.chromium.org/121113004/diff/280001/Source/bindings/scripts...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/121113004/diff/280001/Source/bindings/scripts...
Source/bindings/scripts/code_generator_v8.pm:175: my %integerTypeHash = ("byte"
=> "Int8",
While you're at it, you could remove the duplication from the 'primitive' types,
which redundantly lists integer types as well.

I.e., you could remove the integers from %primitiveTypeHash
and then just change IsPrimitiveType to have:
    return 1 if $primitiveTypeHash{$type};
    return 1 if $integerTypeHash{$type};

https://codereview.chromium.org/121113004/diff/280001/Source/bindings/scripts...
Source/bindings/scripts/code_generator_v8.pm:1936: my $isNumberType =
IsIntegerType($attribute->type);
Would this be clearer as $isIntegerType ?
('Number' is ambiguous and could suggest float.)

https://codereview.chromium.org/121113004/diff/280001/Source/bindings/scripts...
File Source/bindings/scripts/unstable/v8_methods.py (right):

https://codereview.chromium.org/121113004/diff/280001/Source/bindings/scripts...
Source/bindings/scripts/unstable/v8_methods.py:83:
has_exception_raising_argument = any(
Could you inline this into the value for
  'has_exception_state': ...
?

The auxiliary variable isn't used for anything else, now that you've eliminated
|has_serialized_script_value_argument|, and so we'd rather keep everything
together, despite it being a bit long.

https://codereview.chromium.org/121113004/diff/280001/Source/bindings/scripts...
File Source/bindings/scripts/unstable/v8_types.py (right):

https://codereview.chromium.org/121113004/diff/280001/Source/bindings/scripts...
Source/bindings/scripts/unstable/v8_types.py:146: return any(idl_type in s for s
in [CPP_INT_TYPES, CPP_UNSIGNED_TYPES, CPP_LONG_LONG_TYPES])
Simpler:
return idl_type in CPP_INT_TYPES | CPP_UNSIGNED_TYPES | CPP_LONG_LONG_TYPES

Since this is IDL (independent of C++), could we instead split off INTEGER_TYPES
from BASIC_TYPES, as:
BASIC_TYPES = set([
    # Built-in, non-composite, non-object data types
    # (integer types are listed separately below)
    # http://www.w3.org/TR/WebIDL/#dfn-primitive-type
    'boolean',
    'float',
    # unrestricted float is not supported
    'double',
    # unrestricted double is not supported
    # http://www.w3.org/TR/WebIDL/#idl-types
    'DOMString',
    'Date',
    # http://www.w3.org/TR/WebIDL/#es-type-mapping
    'void',
])
INTEGER_TYPES = set([
    # http://www.w3.org/TR/WebIDL/#dfn-integer-type
    'byte',
    'octet',
    'short',
    'unsigned short',
    # int and unsigned are not IDL types
    'long',
    'unsigned long',
    'long long',
    'unsigned long long',
])
BASIC_TYPES.update(INTEGER_TYPES)

...and then we have these two functions:
def is_basic_type(idl_type):
    return idl_type in BASIC_TYPES

def is_integer_type(idl_type):
    return idl_type in INTEGER_TYPES

https://codereview.chromium.org/121113004/diff/280001/Source/bindings/scripts...
Source/bindings/scripts/unstable/v8_types.py:243: CPP_LONG_LONG_TYPES = set([
Could you put this set up *above* CPP_SPECIAL_CONVERSION_RULES?
(So the 3 sets of types are together.)

Actually, we don't need this if we instead use INTEGER_TYPES (above),
as this isn't C++-specific.

https://codereview.chromium.org/121113004/diff/280001/Source/bindings/scripts...
Source/bindings/scripts/unstable/v8_types.py:388: elif
is_integer_type(idl_type):  # NormalConversion
Comment '# NormalConversion' should go on the default 'else:' line.

[sof, 2014-01-06 21:01:24]

https://codereview.chromium.org/121113004/diff/280001/Source/bindings/scripts...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/121113004/diff/280001/Source/bindings/scripts...
Source/bindings/scripts/code_generator_v8.pm:175: my %integerTypeHash = ("byte"
=> "Int8",
On 2014/01/06 02:58:09, Nils Barth wrote:
> While you're at it, you could remove the duplication from the 'primitive'
types,
> which redundantly lists integer types as well.
> 
> I.e., you could remove the integers from %primitiveTypeHash
> and then just change IsPrimitiveType to have:
>     return 1 if $primitiveTypeHash{$type};
>     return 1 if $integerTypeHash{$type};

Good idea, done.

https://codereview.chromium.org/121113004/diff/280001/Source/bindings/scripts...
Source/bindings/scripts/code_generator_v8.pm:1936: my $isNumberType =
IsIntegerType($attribute->type);
On 2014/01/06 02:58:09, Nils Barth wrote:
> Would this be clearer as $isIntegerType ?
> ('Number' is ambiguous and could suggest float.)

It certainly would be, done.

https://codereview.chromium.org/121113004/diff/280001/Source/bindings/scripts...
File Source/bindings/scripts/unstable/v8_methods.py (right):

https://codereview.chromium.org/121113004/diff/280001/Source/bindings/scripts...
Source/bindings/scripts/unstable/v8_methods.py:83:
has_exception_raising_argument = any(
On 2014/01/06 02:58:09, Nils Barth wrote:
> Could you inline this into the value for
>   'has_exception_state': ...
> ?
> 
> The auxiliary variable isn't used for anything else, now that you've
eliminated
> |has_serialized_script_value_argument|, and so we'd rather keep everything
> together, despite it being a bit long.

Done.

https://codereview.chromium.org/121113004/diff/280001/Source/bindings/scripts...
File Source/bindings/scripts/unstable/v8_types.py (right):

https://codereview.chromium.org/121113004/diff/280001/Source/bindings/scripts...
Source/bindings/scripts/unstable/v8_types.py:146: return any(idl_type in s for s
in [CPP_INT_TYPES, CPP_UNSIGNED_TYPES, CPP_LONG_LONG_TYPES])
On 2014/01/06 02:58:09, Nils Barth wrote:
> Simpler:
> return idl_type in CPP_INT_TYPES | CPP_UNSIGNED_TYPES | CPP_LONG_LONG_TYPES
> 
> Since this is IDL (independent of C++), could we instead split off
INTEGER_TYPES
> from BASIC_TYPES, as:
> BASIC_TYPES = set([
>     # Built-in, non-composite, non-object data types
>     # (integer types are listed separately below)
>     # http://www.w3.org/TR/WebIDL/#dfn-primitive-type
>     'boolean',
>     'float',
>     # unrestricted float is not supported
>     'double',
>     # unrestricted double is not supported
>     # http://www.w3.org/TR/WebIDL/#idl-types
>     'DOMString',
>     'Date',
>     # http://www.w3.org/TR/WebIDL/#es-type-mapping
>     'void',
> ])
> INTEGER_TYPES = set([
>     # http://www.w3.org/TR/WebIDL/#dfn-integer-type
>     'byte',
>     'octet',
>     'short',
>     'unsigned short',
>     # int and unsigned are not IDL types
>     'long',
>     'unsigned long',
>     'long long',
>     'unsigned long long',
> ])
> BASIC_TYPES.update(INTEGER_TYPES)
> 
> ...and then we have these two functions:
> def is_basic_type(idl_type):
>     return idl_type in BASIC_TYPES
> 
> def is_integer_type(idl_type):
>     return idl_type in INTEGER_TYPES

Adopted wholesale; the transformation added here is over IDL types, not over
what it maps to in C++.

https://codereview.chromium.org/121113004/diff/280001/Source/bindings/scripts...
Source/bindings/scripts/unstable/v8_types.py:243: CPP_LONG_LONG_TYPES = set([
On 2014/01/06 02:58:09, Nils Barth wrote:
> Could you put this set up *above* CPP_SPECIAL_CONVERSION_RULES?
> (So the 3 sets of types are together.)
> 
> Actually, we don't need this if we instead use INTEGER_TYPES (above),
> as this isn't C++-specific.

Removed.

https://codereview.chromium.org/121113004/diff/280001/Source/bindings/scripts...
Source/bindings/scripts/unstable/v8_types.py:388: elif
is_integer_type(idl_type):  # NormalConversion
On 2014/01/06 02:58:09, Nils Barth wrote:
> Comment '# NormalConversion' should go on the default 'else:' line.

Isn't this the correct placement for it? i.e., this is for NormalConversion over
integer types.

[Nils Barth (inactive), 2014-01-07 02:11:35]

Thanks for revisions, CG part looks great!

https://codereview.chromium.org/121113004/diff/280001/Source/bindings/scripts...
File Source/bindings/scripts/unstable/v8_types.py (right):

https://codereview.chromium.org/121113004/diff/280001/Source/bindings/scripts...
Source/bindings/scripts/unstable/v8_types.py:388: elif
is_integer_type(idl_type):  # NormalConversion
On 2014/01/06 21:01:25, sof wrote:
> Isn't this the correct placement for it? i.e., this is for NormalConversion
over
> integer types.

OIC, you're right - sorry, thanks!
(NormalConversion as opposed to EnforceRange for integer types;
 for non-integer types this doesn't make sense.)

[haraken, 2014-01-07 02:19:04]

LGTM but I'm a bit concerned that this might regress performance. Integer
conversion can be used in performance-sensitive places, and one ExceptionState
construction is heavier than one bool allocation. Please watch perf bots when
landing.

[sof, 2014-01-07 07:11:48]

On 2014/01/07 02:19:04, haraken wrote:
> LGTM but I'm a bit concerned that this might regress performance. Integer
> conversion can be used in performance-sensitive places, and one ExceptionState
> construction is heavier than one bool allocation. Please watch perf bots when
> landing.

I will do that; let's try.

[commit-bot: I haz the power, 2014-01-07 09:56:45]

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

[commit-bot: I haz the power, 2014-01-07 12:37:10]

Change committed as 164568