Add ByteString support to IDL bindings

Add ByteString support to IDL bindings

ByteString is like DOMString, but should throw TypeError 
during the V8->C++ hop if script tries to set a string with a 
code unit greater than 0x00FF. No enforcement is done on the 
C++->V8 side.

R=nbarth@chromium.org
BUG=347426
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175995

[jsbell, 2014-05-29 18:33:21]

nbarth@ - please take a look?

I tackled this since it came up with ServiceWorker and Fetch, and I hadn't done
anything with the python bindings yet. It's not urgent, and I'm happy to throw
away this CL if you've got something better planned.

It seems a bit hacky with too many `== 'ByteString'` but it's a primitive type
and there isn't a general class of string-like types with conversion rules,
although it looks like ScalarValueString might get added (throws if there are
unmatched surrogates?).

Anyway, feedback wanted.

[jsbell, 2014-05-30 16:02:16]

On 2014/05/29 18:33:21, jsbell wrote:
> nbarth@ - please take a look?
> 
> I tackled this since it came up with ServiceWorker and Fetch, and I hadn't
done
> anything with the python bindings yet. It's not urgent, and I'm happy to throw
> away this CL if you've got something better planned.
> 
> It seems a bit hacky with too many `== 'ByteString'` but it's a primitive type
> and there isn't a general class of string-like types with conversion rules,
> although it looks like ScalarValueString might get added (throws if there are
> unmatched surrogates?).
> 
> Anyway, feedback wanted.

FYI, filed crbug.com/379009 for ScalarValueString. (And it doesn't throw, it
just replaces unmatched surrogates)

[jsbell, 2014-05-30 19:14:54]

https://codereview.chromium.org/309553002/diff/1/LayoutTests/fast/js/webidl-t...
File LayoutTests/fast/js/webidl-type-mapping.html (right):

https://codereview.chromium.org/309553002/diff/1/LayoutTests/fast/js/webidl-t...
LayoutTests/fast/js/webidl-type-mapping.html:584:
evalAndLog("converter.testByteString = '!@#123ABCabc\\x00\\x80\\xFF\\r\\n\\t'");
I need to add a case that assigns a non-string (e.g. a number, null, undefined,
etc) and ensure it coerces just like to a DOMString.

[jsbell, 2014-05-30 19:16:23]

Also, this may require additional hackery to support [Default=NullString] if we
need that.

[jsbell, 2014-05-30 21:53:28]

https://codereview.chromium.org/309553002/diff/1/Source/bindings/v8/V8Binding...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/309553002/diff/1/Source/bindings/v8/V8Binding...
Source/bindings/v8/V8Binding.cpp:497: String string =
toCoreString(value.As<v8::String>());
Yeah, this As<v8::String> is bogus if it's not a string. I'll dig further.

[Nils Barth (inactive), 2014-06-02 03:09:49]

On 2014/05/30 19:16:23, jsbell wrote:
> Also, this may require additional hackery to support [Default=NullString] if
we
> need that.

This should be fine -- we can handle it when it comes up, if it does.

Default value handling should be fixed to standard soon; Pavane @Samsung has a
CL:
Support WebIDL optional default value syntax
https://codereview.chromium.org/272213005/

...and Jens @Opera is also interested in it:

Support WebIDL optional default value syntax
https://code.google.com/p/chromium/issues/detail?id=258153

[Nils Barth (inactive), 2014-06-02 03:19:32]

On 2014/05/29 18:33:21, jsbell wrote:
> I tackled this since it came up with ServiceWorker and Fetch, and I hadn't
done
> anything with the python bindings yet. It's not urgent, and I'm happy to throw
> away this CL if you've got something better planned.

Thanks for tackling this, and hope you're enjoying the Python IDL compiler!
No plans for doing this myself short-term, so feel free to have at it!

Generally looks fine; a few nits and one substantive issue on the C++ of the
actual conversion.

> It seems a bit hacky with too many `== 'ByteString'` but it's a primitive type
> and there isn't a general class of string-like types with conversion rules,
> although it looks like ScalarValueString might get added (throws if there are
> unmatched surrogates?).

It's fine to have 'ByteString', as it's a *basic* type.
It's not a "primitive" type: those are just boolean and numeric types:
http://heycam.github.io/webidl/#dfn-primitive-type

The Web IDL spec doesn't have a name for the basic/built-in types it defines
(like DOMString or Date), so we call these "basic" types, which seems the best
name, per Wikipedia:
http://en.wikipedia.org/wiki/Primitive_data_type

It's fine to have a flat list of types, since that matches the spec;
if we want to have more generic treatment (via a deeper hierarchy),
we can add subtyping in *v8_types* (not in idl_types), like IdlType.is_string =
property(...)

That probably makes sense if we add more string types, given DOMString,
ByteString, ScalarValueString, and enumerations.
(Currently have an auxiliary function for preprocessing the type to simplify
treatment a bit.)
...but for now flat seems fine.

[Nils Barth (inactive), 2014-06-02 03:20:16]

Various nits, one substantive issue (C++ conversion), generally looks fine --
thanks!

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

https://codereview.chromium.org/309553002/diff/1/Source/bindings/scripts/idl_...
Source/bindings/scripts/idl_types.py:41: #
http://www.w3.org/TR/WebIDL/#idl-types
Could you update this link to the ED?
(Otherwise we don't have ByteString there.)
http://heycam.github.io/webidl/#idl-types

https://codereview.chromium.org/309553002/diff/1/Source/bindings/scripts/idl_...
Source/bindings/scripts/idl_types.py:67: }
Could you add
'Date': 'Date',
here?

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

https://codereview.chromium.org/309553002/diff/1/Source/bindings/scripts/v8_a...
Source/bindings/scripts/v8_attributes.py:111: idl_type.is_integer_type or
idl_type.base_type == 'ByteString',
This should probably be .name instead of .base_type
(.name is simpler and more specific, and ensures we aren't actually
using an array or nullable type or whatever.)

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

https://codereview.chromium.org/309553002/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:124: argument.idl_type.base_type ==
'ByteString'),
Ditto about .name, in which case this can instead be
if argument.idl_type.name in ('ByteString', 'SerializedScriptValue') or
...

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

https://codereview.chromium.org/309553002/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:174: if base_idl_type == 'ByteString':
Should this be merged with the DOMString above?
(Presumably we want to handle them the same?)

https://codereview.chromium.org/309553002/diff/1/Source/bindings/tests/idls/T...
File Source/bindings/tests/idls/TestObject.idl (right):

https://codereview.chromium.org/309553002/diff/1/Source/bindings/tests/idls/T...
Source/bindings/tests/idls/TestObject.idl:280: void
voidMethodByteStringArg(ByteString stringArg);
Could you also add a method *returning* a ByteString?

https://codereview.chromium.org/309553002/diff/1/Source/bindings/v8/V8Binding...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/309553002/diff/1/Source/bindings/v8/V8Binding...
Source/bindings/v8/V8Binding.cpp:497: String string =
toCoreString(value.As<v8::String>());
On 2014/05/30 21:53:28, jsbell wrote:
> Yeah, this As<v8::String> is bogus if it's not a string. I'll dig further.

Should it be ToString() instead then?
(JS values are all stringifable.)
Anyway, that's what the spec says, right?:
http://heycam.github.io/webidl/#es-ByteString

In any case, could you link to the spec here (per above URL),
and quote line-by-line so the spec <-> code correspondence is explicit?
(V. useful if the spec changes!)

E.g. (with whatever the correct code is):
// 1. Let x be ToString(V).
String string = value.ToString();
// 2. If the value of any element of x is greater than 255, then throw a
TypeError.
if (!...)

[jsbell, 2014-06-03 23:22:36]

Thanks for the initial look. This should be much better, but should still be
inspected with a fine toothed comb.

(I have ScalarValueString hacked up locally, but will rework it on top of this,
and sort out defaults so may wait until jl's patch lands.)

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

https://codereview.chromium.org/309553002/diff/1/Source/bindings/scripts/idl_...
Source/bindings/scripts/idl_types.py:41: #
http://www.w3.org/TR/WebIDL/#idl-types
On 2014/06/02 03:20:16, Nils Barth wrote:
> Could you update this link to the ED?
> (Otherwise we don't have ByteString there.)
> http://heycam.github.io/webidl/#idl-types

Done.

https://codereview.chromium.org/309553002/diff/1/Source/bindings/scripts/idl_...
Source/bindings/scripts/idl_types.py:67: }
On 2014/06/02 03:20:16, Nils Barth wrote:
> Could you add
> 'Date': 'Date',
> here?

Done.

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

https://codereview.chromium.org/309553002/diff/1/Source/bindings/scripts/v8_a...
Source/bindings/scripts/v8_attributes.py:111: idl_type.is_integer_type or
idl_type.base_type == 'ByteString',
On 2014/06/02 03:20:16, Nils Barth wrote:
> This should probably be .name instead of .base_type
> (.name is simpler and more specific, and ensures we aren't actually
> using an array or nullable type or whatever.)

Done.

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

https://codereview.chromium.org/309553002/diff/1/Source/bindings/scripts/v8_m...
Source/bindings/scripts/v8_methods.py:124: argument.idl_type.base_type ==
'ByteString'),
On 2014/06/02 03:20:16, Nils Barth wrote:
> Ditto about .name, in which case this can instead be
> if argument.idl_type.name in ('ByteString', 'SerializedScriptValue') or
> ...

Done.

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

https://codereview.chromium.org/309553002/diff/1/Source/bindings/scripts/v8_t...
Source/bindings/scripts/v8_types.py:174: if base_idl_type == 'ByteString':
On 2014/06/02 03:20:16, Nils Barth wrote:
> Should this be merged with the DOMString above?
> (Presumably we want to handle them the same?)

This failed due to the generated (and de-macro-ified):

V8StringResource<> cppValue(toByteString(v8Value, exceptionState));

.. but now that I've poked around in the code a bit more, it seems that
something like this should fix it:

https://codereview.chromium.org/312683005/diff/1/Source/bindings/v8/V8StringR...

https://codereview.chromium.org/309553002/diff/1/Source/bindings/tests/idls/T...
File Source/bindings/tests/idls/TestObject.idl (right):

https://codereview.chromium.org/309553002/diff/1/Source/bindings/tests/idls/T...
Source/bindings/tests/idls/TestObject.idl:280: void
voidMethodByteStringArg(ByteString stringArg);
On 2014/06/02 03:20:16, Nils Barth wrote:
> Could you also add a method *returning* a ByteString?

Done.

https://codereview.chromium.org/309553002/diff/1/Source/bindings/v8/V8Binding...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/309553002/diff/1/Source/bindings/v8/V8Binding...
Source/bindings/v8/V8Binding.cpp:497: String string =
toCoreString(value.As<v8::String>());
On 2014/06/02 03:20:16, Nils Barth wrote:
> On 2014/05/30 21:53:28, jsbell wrote:
> > Yeah, this As<v8::String> is bogus if it's not a string. I'll dig further.
> 
> Should it be ToString() instead then?

Yeah; handling should be identical to js->DOMString, except for the extra
throwing case.

That does mean the ToString needs to handle the conversion itself throwing. Was
afraid of the macros, but I've got that sorted now.

[jsbell, 2014-06-10 18:16:58]

Added [Default=NullString] support + tests, so that it's not bolted on by the
ScalarValueString patch.

nbarth@ - PTAL?

[Nils Barth (inactive), 2014-06-11 03:25:14]

LGTM, thanks for a great CL!

[haraken, 2014-06-11 03:54:52]

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

https://codereview.chromium.org/309553002/diff/40001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.cpp:505: String x = toCoreString(stringObject);

I'm not quite sure, but probably this should be:

TOSTRING_DEFAULT(V8StringResource<>, string, value, String());

?

[Nils Barth (inactive), 2014-06-11 03:59:47]

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

https://codereview.chromium.org/309553002/diff/40001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.cpp:505: String x = toCoreString(stringObject);
On 2014/06/11 03:54:52, haraken wrote:
> I'm not quite sure, but probably this should be:
> 
> TOSTRING_DEFAULT(V8StringResource<>, string, value, String());
> 
> ?

Oh, that's a good point, I think.
We're only throwing the exception here, not in the string conversion, so we
don't need to pass exceptionState to the macro, right?

(OTOH, in the *generated bindings* we do need to use the ES macro b/c this code
can throw an exception.)

[jsbell, 2014-06-11 16:55:07]

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

https://codereview.chromium.org/309553002/diff/40001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.cpp:505: String x = toCoreString(stringObject);
On 2014/06/11 03:59:47, Nils Barth wrote:
> On 2014/06/11 03:54:52, haraken wrote:
> > I'm not quite sure, but probably this should be:
> > 
> > TOSTRING_DEFAULT(V8StringResource<>, string, value, String());
> > 
> > ?
> 
> Oh, that's a good point, I think.
> We're only throwing the exception here, not in the string conversion, so we
> don't need to pass exceptionState to the macro, right?

Just to make sure I'm understanding things - TOSTRING_DEFAULT uses
V8StringResource and calls prepare(). If the ES 'ToString()' conversion itself
throws, the prepare() catches and rethrows it. But the C++ code should just
blithely carry on? Does that work properly in a context such as this where
exceptionState is passed in?

[jsbell, 2014-06-11 16:57:43]

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

https://codereview.chromium.org/309553002/diff/40001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.cpp:505: String x = toCoreString(stringObject);
And to be clear, the TONATIVE_DEFAULT_EXCEPTIONSTATE is handling the ToString()
throwing (which sets an exception via exceptionState, then returns early).

I believe I can switch to TOSTRING_DEFAULT() but that seems... weird. I don't
see it being used in conjunction with an exceptionState anywhere.

[haraken, 2014-06-11 23:55:17]

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

https://codereview.chromium.org/309553002/diff/40001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.cpp:505: String x = toCoreString(stringObject);
On 2014/06/11 16:57:43, jsbell wrote:
> And to be clear, the TONATIVE_DEFAULT_EXCEPTIONSTATE is handling the
ToString()
> throwing (which sets an exception via exceptionState, then returns early).
> 
> I believe I can switch to TOSTRING_DEFAULT() but that seems... weird. I don't
> see it being used in conjunction with an exceptionState anywhere.

You're right. Your programming pattern matches what toInt32() does. So LGTM.

Sorry about the noise!

[jsbell, 2014-06-12 00:03:34]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-06-12 00:03:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jsbell@chromium.org/309553002/40001

[commit-bot: I haz the power, 2014-06-12 03:43:34]

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/11652)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18994)

[commit-bot: I haz the power, 2014-06-12 06:00:27]

Change committed as 175995