Extend ScalarValueString handling to include constructors.

Extend ScalarValueString handling to include constructors.

ScalarValueString conversion may raise an exception; take that into
account when generating the constructor's code.

Also, common up various string-like and exception-raising tests
internally, making them properties over the underlying IDL types.

R=jsbell@chromium.org,haraken@chromium.org
BUG=

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

[sof, 2014-06-19 19:52:04]

Please take a look.

Ran into some issues when trying to use the shiny-new ScalarValueString support
in a constructor context.

[jsbell, 2014-06-19 20:49:20]

Thanks for taking this on!

There's another use of `idl_type.name in ('ByteString', 'ScalarValueString')` in
v8_attributes.py that can probably be refactored.

https://codereview.chromium.org/338893004/diff/1/Source/bindings/scripts/idl_...
File Source/bindings/scripts/idl_types.py (right):

https://codereview.chromium.org/338893004/diff/1/Source/bindings/scripts/idl_...
Source/bindings/scripts/idl_types.py:80: 
Nit: remove one of these blank lines

https://codereview.chromium.org/338893004/diff/1/Source/bindings/scripts/idl_...
Source/bindings/scripts/idl_types.py:210: def
may_raise_exception_on_conversion(self):
We may want to refactor the conversions even further; DOMString and
ScalarValueString throw for exactly the same reasons (the JS ToString()
conversion throws in V8-land), but we special-case DOMString elsewhere.

(ByteString is the only one that throws for an "extra" reason).

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

https://codereview.chromium.org/338893004/diff/1/Source/bindings/scripts/v8_i...
Source/bindings/scripts/v8_interface.py:763: # • ByteString
This part of the CL will collide with r176531 and need to be rebased. It made a
similar change in this clause (but this refactor is cleaner)

https://codereview.chromium.org/338893004/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/338893004/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:314: idl_type.name in ('String',
'ByteString', 'ScalarValueString') and
Use idl_type.is_string_type here

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

https://codereview.chromium.org/338893004/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:170: if base_idl_type in ('DOMString',
'ByteString', 'ScalarValueString'):
Use idl_type.is_string_type ?

https://codereview.chromium.org/338893004/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:551: if base_idl_type in ('DOMString',
'ByteString', 'ScalarValueString'):
Use idl_type.is_string_type ?

[jsbell, 2014-06-19 20:50:27]

https://codereview.chromium.org/338893004/diff/1/Source/bindings/scripts/idl_...
File Source/bindings/scripts/idl_types.py (right):

https://codereview.chromium.org/338893004/diff/1/Source/bindings/scripts/idl_...
Source/bindings/scripts/idl_types.py:210: def
may_raise_exception_on_conversion(self):
On 2014/06/19 20:49:19, jsbell wrote:
> We may want to refactor the conversions even further; DOMString and
> ScalarValueString throw for exactly the same reasons (the JS ToString()
> conversion throws in V8-land), but we special-case DOMString elsewhere.
> 
> (ByteString is the only one that throws for an "extra" reason).

... but that should be a follow-up CL.

[sof, 2014-06-19 21:41:17]

On 2014/06/19 20:49:20, jsbell wrote:
> Thanks for taking this on!
> 
> There's another use of `idl_type.name in ('ByteString', 'ScalarValueString')`
in
> v8_attributes.py that can probably be refactored.

Thanks, will address that & the other helpful feedback in the morning.

(Ran into this while converting over the URL API.)

[sof, 2014-06-20 07:21:16]

https://codereview.chromium.org/338893004/diff/1/Source/bindings/scripts/idl_...
File Source/bindings/scripts/idl_types.py (right):

https://codereview.chromium.org/338893004/diff/1/Source/bindings/scripts/idl_...
Source/bindings/scripts/idl_types.py:80: 
On 2014/06/19 20:49:19, jsbell wrote:
> Nit: remove one of these blank lines

Done.

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

https://codereview.chromium.org/338893004/diff/1/Source/bindings/scripts/v8_i...
Source/bindings/scripts/v8_interface.py:763: # • ByteString
On 2014/06/19 20:49:19, jsbell wrote:
> This part of the CL will collide with r176531 and need to be rebased. It made
a
> similar change in this clause (but this refactor is cleaner)

This patchset was already on top of it; it generalizes the change here slightly.

https://codereview.chromium.org/338893004/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/338893004/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:314: idl_type.name in ('String',
'ByteString', 'ScalarValueString') and
On 2014/06/19 20:49:19, jsbell wrote:
> Use idl_type.is_string_type here

Thanks, done. Notice that "idl_type.name" appends a "OrNull" suffix to a
string-like type that's nullable. I _think_ we want the nullable case to also
apply here, so I added an extra check for that. See the V8TestObject.cpp change
that resulted - correctness preserving?

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

https://codereview.chromium.org/338893004/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:170: if base_idl_type in ('DOMString',
'ByteString', 'ScalarValueString'):
On 2014/06/19 20:49:20, jsbell wrote:
> Use idl_type.is_string_type ?

Done.

https://codereview.chromium.org/338893004/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:551: if base_idl_type in ('DOMString',
'ByteString', 'ScalarValueString'):
On 2014/06/19 20:49:19, jsbell wrote:
> Use idl_type.is_string_type ?

Done.

[Jens Widell, 2014-06-20 08:07:06]

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

https://codereview.chromium.org/338893004/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/idl_types.py:74: # http://heycam.github.io/webidl/,
Section 4.5.1.1 (step 10.11)
Add "#es-interface-call" to the link, instead of the "Section 4.5.1.1"?

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

https://codereview.chromium.org/338893004/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:314: (idl_type.is_string_type or
idl_type.name == 'StringOrNull') and
Isn't 'is_string_type' true for StringOrNull?

[sof, 2014-06-20 08:15:10]

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

https://codereview.chromium.org/338893004/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/idl_types.py:74: # http://heycam.github.io/webidl/,
Section 4.5.1.1 (step 10.11)
On 2014/06/20 08:07:06, Jens Lindström wrote:
> Add "#es-interface-call" to the link, instead of the "Section 4.5.1.1"?

Done.

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

https://codereview.chromium.org/338893004/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_methods.py:314: (idl_type.is_string_type or
idl_type.name == 'StringOrNull') and
On 2014/06/20 08:07:06, Jens Lindström wrote:
> Isn't 'is_string_type' true for StringOrNull?

The RHS of the disjunction doesn't add anything, removed. The comment/issue
https://codereview.chromium.org/338893004/diff/1/Source/bindings/scripts/v8_m...
still applies, however.

[Jens Widell, 2014-06-20 08:29:10]

https://codereview.chromium.org/338893004/diff/1/Source/bindings/scripts/v8_m...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/338893004/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:314: idl_type.name in ('String',
'ByteString', 'ScalarValueString') and
On 2014/06/20 07:21:16, sof wrote:
> On 2014/06/19 20:49:19, jsbell wrote:
> > Use idl_type.is_string_type here
> 
> Thanks, done. Notice that "idl_type.name" appends a "OrNull" suffix to a
> string-like type that's nullable. I _think_ we want the nullable case to also
> apply here, so I added an extra check for that. See the V8TestObject.cpp
change
> that resulted - correctness preserving?

The V8TestObject.cpp change looks good to me; previous handling of nullable
string arguments with null defaults ought to have been a bug.

This whole block of code also goes away with
https://codereview.chromium.org/335113002/.

[sof, 2014-06-23 05:43:14]

Would be good to land these fixes; what remains?

[haraken, 2014-06-23 06:06:52]

This is a nice change, LGTM.

[sof, 2014-06-23 06:45:26]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-06-23 06:45:59]

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

[commit-bot: I haz the power, 2014-06-23 08:24:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/13084)

[commit-bot: I haz the power, 2014-06-23 09:04:42]

Change committed as 176732