[bindings] Pass NormalConversion to to[U]Int{8/16/32/64}(...) in v8_types.py and reduce conversion …

[bindings] Pass NormalConversion to to[U]Int{8/16/32/64}(...) in v8_types.py and reduce conversion routines.

There exist multiple routines toXXX(...) for integers of various sizes i.e. 8, 16, 32 and 64. These also include
both signed and unsigned versions. These are mostly redirecting to each other for the final conversion.

Instead of this redirection, if we could pass the 'NormalConversion' as parameter to these methods, most of the
redundant redirections can be avoided.

R=haraken@chromium.org, jl@opera.com

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

[vivekg, 2015-03-17 11:19:19]

vivekg@chromium.org changed reviewers:
+ vivekg@chromium.org

[vivekg, 2015-03-17 11:19:20]

PTAL, thanks.

[Jens Widell, 2015-03-17 11:28:22]

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

https://codereview.chromium.org/1011093002/diff/1/Source/bindings/scripts/v8_...
Source/bindings/scripts/v8_types.py:549: if idl_type.is_integer_type:
The preceding two cases, EnforceRange and Clamp, also affect only integer types.
I think this code would be slightly better restructured as

  if idl_type.is_integer_type:
    configuration = < 'EnforceRange' or 'Clamp' or 'NormalConversion' >
    arguments = ', '.join([v8_value, configuration, 'exceptionState'])
  elif idl_type.v8_conversion_needs_exception_state:
    ...
  else:
    ...

[vivekg, 2015-03-17 11:36:22]

On 2015/03/17 at 11:28:22, jl wrote:
>
https://codereview.chromium.org/1011093002/diff/1/Source/bindings/scripts/v8_...
> File Source/bindings/scripts/v8_types.py (right):
> 
>
https://codereview.chromium.org/1011093002/diff/1/Source/bindings/scripts/v8_...
> Source/bindings/scripts/v8_types.py:549: if idl_type.is_integer_type:
> The preceding two cases, EnforceRange and Clamp, also affect only integer
types. I think this code would be slightly better restructured as
> 
>   if idl_type.is_integer_type:
>     configuration = < 'EnforceRange' or 'Clamp' or 'NormalConversion' >
>     arguments = ', '.join([v8_value, configuration, 'exceptionState'])
>   elif idl_type.v8_conversion_needs_exception_state:
>     ...
>   else:
>     ...

Thank you Jens. How about this one:

    if idl_type.is_integer_type:
        configuration = 'NormalConversion'
        if 'EnforceRange' in extended_attributes:
            configuration = 'EnforceRange'
        elif 'Clamp' in extended_attributes:
            configuration = 'Clamp'
        arguments = ', '.join([v8_value, configuration, 'exceptionState'])
    elif idl_type.v8_conversion_needs_exception_state:
        ...

[Jens Widell, 2015-03-17 11:37:35]

On 2015/03/17 11:36:22, vivekg_ wrote:
> On 2015/03/17 at 11:28:22, jl wrote:
> >
>
https://codereview.chromium.org/1011093002/diff/1/Source/bindings/scripts/v8_...
> > File Source/bindings/scripts/v8_types.py (right):
> > 
> >
>
https://codereview.chromium.org/1011093002/diff/1/Source/bindings/scripts/v8_...
> > Source/bindings/scripts/v8_types.py:549: if idl_type.is_integer_type:
> > The preceding two cases, EnforceRange and Clamp, also affect only integer
> types. I think this code would be slightly better restructured as
> > 
> >   if idl_type.is_integer_type:
> >     configuration = < 'EnforceRange' or 'Clamp' or 'NormalConversion' >
> >     arguments = ', '.join([v8_value, configuration, 'exceptionState'])
> >   elif idl_type.v8_conversion_needs_exception_state:
> >     ...
> >   else:
> >     ...
> 
> Thank you Jens. How about this one:
> 
>     if idl_type.is_integer_type:
>         configuration = 'NormalConversion'
>         if 'EnforceRange' in extended_attributes:
>             configuration = 'EnforceRange'
>         elif 'Clamp' in extended_attributes:
>             configuration = 'Clamp'
>         arguments = ', '.join([v8_value, configuration, 'exceptionState'])
>     elif idl_type.v8_conversion_needs_exception_state:
>         ...

That's about exactly what I meant, but was too lazy to actually write myself.
:-)

[vivekg, 2015-03-17 11:40:09]

On 2015/03/17 at 11:37:35, jl wrote:
> On 2015/03/17 11:36:22, vivekg_ wrote:
> > On 2015/03/17 at 11:28:22, jl wrote:
> > >
> >
https://codereview.chromium.org/1011093002/diff/1/Source/bindings/scripts/v8_...
> > > File Source/bindings/scripts/v8_types.py (right):
> > > 
> > >
> >
https://codereview.chromium.org/1011093002/diff/1/Source/bindings/scripts/v8_...
> > > Source/bindings/scripts/v8_types.py:549: if idl_type.is_integer_type:
> > > The preceding two cases, EnforceRange and Clamp, also affect only integer
> > types. I think this code would be slightly better restructured as
> > > 
> > >   if idl_type.is_integer_type:
> > >     configuration = < 'EnforceRange' or 'Clamp' or 'NormalConversion' >
> > >     arguments = ', '.join([v8_value, configuration, 'exceptionState'])
> > >   elif idl_type.v8_conversion_needs_exception_state:
> > >     ...
> > >   else:
> > >     ...
> > 
> > Thank you Jens. How about this one:
> > 
> >     if idl_type.is_integer_type:
> >         configuration = 'NormalConversion'
> >         if 'EnforceRange' in extended_attributes:
> >             configuration = 'EnforceRange'
> >         elif 'Clamp' in extended_attributes:
> >             configuration = 'Clamp'
> >         arguments = ', '.join([v8_value, configuration, 'exceptionState'])
> >     elif idl_type.v8_conversion_needs_exception_state:
> >         ...
> 
> That's about exactly what I meant, but was too lazy to actually write myself.
:-)

Hehe thanks. Done, PTAL.

[Jens Widell, 2015-03-17 11:42:50]

LGTM

[vivekg, 2015-03-17 11:50:33]

On 2015/03/17 at 11:42:50, jl wrote:
> LGTM

Thank you. The gn bots are red as its not clobbering old UnionTypesCore.cpp.

[Jens Widell, 2015-03-17 12:00:07]

On 2015/03/17 11:50:33, vivekg_ wrote:
> On 2015/03/17 at 11:42:50, jl wrote:
> > LGTM
> 
> Thank you. The gn bots are red as its not clobbering old UnionTypesCore.cpp.

I think it's a bug in bindings/scripts/scripts.gni in the definition of the
'idl_impl' template. Unlike the 'idl_compiler' template, the 'idl_impl' template
doesn't list 'idl_lexer_parser_files'/'idl_compiler_files' and other stuff as
inputs, which I guess it should since it depends on them equally much.

[vivekg, 2015-03-17 12:12:53]

On 2015/03/17 at 12:00:07, jl wrote:
> On 2015/03/17 11:50:33, vivekg_ wrote:
> > On 2015/03/17 at 11:42:50, jl wrote:
> > > LGTM
> > 
> > Thank you. The gn bots are red as its not clobbering old UnionTypesCore.cpp.
> 
> I think it's a bug in bindings/scripts/scripts.gni in the definition of the
'idl_impl' template. Unlike the 'idl_compiler' template, the 'idl_impl' template
doesn't list 'idl_lexer_parser_files'/'idl_compiler_files' and other stuff as
inputs, which I guess it should since it depends on them equally much.

hmm thanks for the details. If I get some bandwidth, I will see to it getting
fixed unless someone is already fixing it hehe.

[Jens Widell, 2015-03-17 12:13:39]

On 2015/03/17 12:12:53, vivekg_ wrote:
> On 2015/03/17 at 12:00:07, jl wrote:
> > On 2015/03/17 11:50:33, vivekg_ wrote:
> > > On 2015/03/17 at 11:42:50, jl wrote:
> > > > LGTM
> > > 
> > > Thank you. The gn bots are red as its not clobbering old
UnionTypesCore.cpp.
> > 
> > I think it's a bug in bindings/scripts/scripts.gni in the definition of the
> 'idl_impl' template. Unlike the 'idl_compiler' template, the 'idl_impl'
template
> doesn't list 'idl_lexer_parser_files'/'idl_compiler_files' and other stuff as
> inputs, which I guess it should since it depends on them equally much.
> 
> hmm thanks for the details. If I get some bandwidth, I will see to it getting
> fixed unless someone is already fixing it hehe.

I'm preparing a fix. Once landed, landing this CL should be safe.

[vivekg, 2015-03-17 12:15:44]

On 2015/03/17 at 12:13:39, jl wrote:
> On 2015/03/17 12:12:53, vivekg_ wrote:
> > On 2015/03/17 at 12:00:07, jl wrote:
> > > On 2015/03/17 11:50:33, vivekg_ wrote:
> > > > On 2015/03/17 at 11:42:50, jl wrote:
> > > > > LGTM
> > > > 
> > > > Thank you. The gn bots are red as its not clobbering old
UnionTypesCore.cpp.
> > > 
> > > I think it's a bug in bindings/scripts/scripts.gni in the definition of
the
> > 'idl_impl' template. Unlike the 'idl_compiler' template, the 'idl_impl'
template
> > doesn't list 'idl_lexer_parser_files'/'idl_compiler_files' and other stuff
as
> > inputs, which I guess it should since it depends on them equally much.
> > 
> > hmm thanks for the details. If I get some bandwidth, I will see to it
getting
> > fixed unless someone is already fixing it hehe.
> 
> I'm preparing a fix. Once landed, landing this CL should be safe.

Oh Sure... I will wait till it lands. Btw, on the 2nd run, these bots have
turned green though, so its flaky as of now.

[haraken, 2015-03-17 15:08:12]

https://codereview.chromium.org/1011093002/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/1011093002/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/PrivateScriptRunner.cpp:246:
exceptionState.throwDOMException(toInt32(isolate, code, NormalConversion,
nonThrowableExceptionState), messageString);

Why can't we use the |exceptionState| for toInt32?

https://codereview.chromium.org/1011093002/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/custom/V8BlobCustomHelpers.cpp (right):

https://codereview.chromium.org/1011093002/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8BlobCustomHelpers.cpp:101:
NonThrowableExceptionState exceptionState;

Ditto. Can we use the existing |exceptionState| ?

[vivekg_samsung, 2015-03-17 16:28:32]

Patchset #3 (id:40001) has been deleted

[vivekg_samsung, 2015-03-17 16:29:48]

Patchset #3 (id:60001) has been deleted

[vivekg, 2015-03-17 16:47:18]

On 2015/03/17 at 15:08:12, haraken wrote:
>
https://codereview.chromium.org/1011093002/diff/20001/Source/bindings/core/v8...
> File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):
> 
>
https://codereview.chromium.org/1011093002/diff/20001/Source/bindings/core/v8...
> Source/bindings/core/v8/PrivateScriptRunner.cpp:246:
exceptionState.throwDOMException(toInt32(isolate, code, NormalConversion,
nonThrowableExceptionState), messageString);
> 
> Why can't we use the |exceptionState| for toInt32?

Done.

> 
>
https://codereview.chromium.org/1011093002/diff/20001/Source/bindings/core/v8...
> File Source/bindings/core/v8/custom/V8BlobCustomHelpers.cpp (right):
> 
>
https://codereview.chromium.org/1011093002/diff/20001/Source/bindings/core/v8...
> Source/bindings/core/v8/custom/V8BlobCustomHelpers.cpp:101:
NonThrowableExceptionState exceptionState;
> 
> Ditto. Can we use the existing |exceptionState| ?

Done. 

PTAL, thanks.

[haraken, 2015-03-17 17:19:03]

https://codereview.chromium.org/1011093002/diff/70001/Source/bindings/core/v8...
File Source/bindings/core/v8/custom/V8BlobCustomHelpers.cpp (right):

https://codereview.chromium.org/1011093002/diff/70001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8BlobCustomHelpers.cpp:101:
TONATIVE_DEFAULT(long long, lastModifiedInt, toInt64(isolate, lastModified,
NormalConversion, exceptionState), false);

If toInt64 fails and exceptionState is set, should we probably return early
before executing TONATIVE_DEFAULT?

[Jens Widell, 2015-03-17 17:25:00]

https://codereview.chromium.org/1011093002/diff/70001/Source/bindings/core/v8...
File Source/bindings/core/v8/custom/V8BlobCustomHelpers.cpp (right):

https://codereview.chromium.org/1011093002/diff/70001/Source/bindings/core/v8...
Source/bindings/core/v8/custom/V8BlobCustomHelpers.cpp:101:
TONATIVE_DEFAULT(long long, lastModifiedInt, toInt64(isolate, lastModified,
NormalConversion, exceptionState), false);
On 2015/03/17 17:19:03, haraken wrote:
> 
> If toInt64 fails and exceptionState is set, should we probably return early
> before executing TONATIVE_DEFAULT?

Yeah, we should just have

  long long lastModifiedInt = toInt64(...);
  if (exceptionState.hadException())
    return false;

here.

[vivekg, 2015-03-17 17:54:43]

On 2015/03/17 at 17:25:00, jl wrote:
>
https://codereview.chromium.org/1011093002/diff/70001/Source/bindings/core/v8...
> File Source/bindings/core/v8/custom/V8BlobCustomHelpers.cpp (right):
> 
>
https://codereview.chromium.org/1011093002/diff/70001/Source/bindings/core/v8...
> Source/bindings/core/v8/custom/V8BlobCustomHelpers.cpp:101:
TONATIVE_DEFAULT(long long, lastModifiedInt, toInt64(isolate, lastModified,
NormalConversion, exceptionState), false);
> On 2015/03/17 17:19:03, haraken wrote:
> > 
> > If toInt64 fails and exceptionState is set, should we probably return early
> > before executing TONATIVE_DEFAULT?
> 
> Yeah, we should just have
> 
>   long long lastModifiedInt = toInt64(...);
>   if (exceptionState.hadException())
>     return false;
> 
> here.

Sure. Done now.. Sorry about the previous patch.

[Jens Widell, 2015-03-17 17:56:33]

LGTM

[Jens Widell, 2015-03-17 18:03:14]

https://codereview.chromium.org/1011093002/diff/90001/Source/bindings/core/v8...
File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/1011093002/diff/90001/Source/bindings/core/v8...
Source/bindings/core/v8/PrivateScriptRunner.cpp:245:
exceptionState.throwDOMException(toInt32(isolate, code, NormalConversion,
exceptionState), messageString);
Actually, technically it's wrong to use the existing exceptionState here, I
think. It's for throwing an exception in a different context, is it not?

(It won't matter much since toInt32() won't throw anything; the value is known
to be an int32 here.)

[vivekg_samsung, 2015-03-17 18:10:04]

On 2015/03/17 18:03:14, Jens Widell wrote:
>
https://codereview.chromium.org/1011093002/diff/90001/Source/bindings/core/v8...
> File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):
> 
>
https://codereview.chromium.org/1011093002/diff/90001/Source/bindings/core/v8...
> Source/bindings/core/v8/PrivateScriptRunner.cpp:245:
> exceptionState.throwDOMException(toInt32(isolate, code, NormalConversion,
> exceptionState), messageString);
> Actually, technically it's wrong to use the existing exceptionState here, I
> think. It's for throwing an exception in a different context, is it not?
> 
> (It won't matter much since toInt32() won't throw anything; the value is known
> to be an int32 here.)

Yeah I had thought so initially. Should I change it back so that its done right?

[vivekg_samsung, 2015-03-17 18:10:04]

On 2015/03/17 18:03:14, Jens Widell wrote:
>
https://codereview.chromium.org/1011093002/diff/90001/Source/bindings/core/v8...
> File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):
> 
>
https://codereview.chromium.org/1011093002/diff/90001/Source/bindings/core/v8...
> Source/bindings/core/v8/PrivateScriptRunner.cpp:245:
> exceptionState.throwDOMException(toInt32(isolate, code, NormalConversion,
> exceptionState), messageString);
> Actually, technically it's wrong to use the existing exceptionState here, I
> think. It's for throwing an exception in a different context, is it not?
> 
> (It won't matter much since toInt32() won't throw anything; the value is known
> to be an int32 here.)

Yeah I had thought so initially. Should I change it back so that its done right?

[Jens Widell, 2015-03-17 18:11:30]

On 2015/03/17 18:10:04, vivekg wrote:
> On 2015/03/17 18:03:14, Jens Widell wrote:
> >
>
https://codereview.chromium.org/1011093002/diff/90001/Source/bindings/core/v8...
> > File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1011093002/diff/90001/Source/bindings/core/v8...
> > Source/bindings/core/v8/PrivateScriptRunner.cpp:245:
> > exceptionState.throwDOMException(toInt32(isolate, code, NormalConversion,
> > exceptionState), messageString);
> > Actually, technically it's wrong to use the existing exceptionState here, I
> > think. It's for throwing an exception in a different context, is it not?
> > 
> > (It won't matter much since toInt32() won't throw anything; the value is
known
> > to be an int32 here.)
> 
> Yeah I had thought so initially. Should I change it back so that its done
right?

Let's see what haraken thinks.

[haraken, 2015-03-18 01:23:13]

LGTM

https://codereview.chromium.org/1011093002/diff/90001/Source/bindings/core/v8...
File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):

https://codereview.chromium.org/1011093002/diff/90001/Source/bindings/core/v8...
Source/bindings/core/v8/PrivateScriptRunner.cpp:245:
exceptionState.throwDOMException(toInt32(isolate, code, NormalConversion,
exceptionState), messageString);
On 2015/03/17 18:03:13, Jens Widell wrote:
> Actually, technically it's wrong to use the existing exceptionState here, I
> think. It's for throwing an exception in a different context, is it not?
> 
> (It won't matter much since toInt32() won't throw anything; the value is known
> to be an int32 here.)

Ah, you're right; sorry, let's revert it back to the NonThrowableExceptionState.

Also it would be better to call toInt32() just after the code->isInt32() check.

[vivekg_samsung, 2015-03-18 02:34:17]

On 2015/03/18 01:23:13, haraken wrote:
> LGTM
> 
>
https://codereview.chromium.org/1011093002/diff/90001/Source/bindings/core/v8...
> File Source/bindings/core/v8/PrivateScriptRunner.cpp (right):
> 
>
https://codereview.chromium.org/1011093002/diff/90001/Source/bindings/core/v8...
> Source/bindings/core/v8/PrivateScriptRunner.cpp:245:
> exceptionState.throwDOMException(toInt32(isolate, code, NormalConversion,
> exceptionState), messageString);
> On 2015/03/17 18:03:13, Jens Widell wrote:
> > Actually, technically it's wrong to use the existing exceptionState here, I
> > think. It's for throwing an exception in a different context, is it not?
> > 
> > (It won't matter much since toInt32() won't throw anything; the value is
known
> > to be an int32 here.)
> 
> Ah, you're right; sorry, let's revert it back to the
NonThrowableExceptionState.
> 
> Also it would be better to call toInt32() just after the code->isInt32()
check.

Done, thank you.

[haraken, 2015-03-18 02:37:40]

LGTM

[vivekg_samsung, 2015-03-18 02:41:43]

The CQ bit was checked by vivek.vg@samsung.com

[vivekg_samsung, 2015-03-18 02:41:43]

The patchset sent to the CQ was uploaded after l-g-t-m from jl@opera.com
Link to the patchset: https://codereview.chromium.org/1011093002/#ps110001
(title: "Patch for landing after review comments! :)")

[commit-bot: I haz the power, 2015-03-18 02:41:49]

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

[commit-bot: I haz the power, 2015-03-18 05:52:09]

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