IDL: Add support for serializer definitions

IDL: Add support for serializer definitions

The code generation for it is very simple. The

  serializer ReturnType name();

form adds the method "ReturnType name()" as a normal method, and in
addition a toJSON() method with identical behavior.

All other forms of serializer definitions, are equivalent to defining
a method

  [CallWith=ScriptState] any toJSON();

unless the serializer definition has additional extended attributes, in
which case those are also applied to the toJSON() method.

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

[Jens Widell, 2015-03-25 14:32:17]

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

[Jens Widell, 2015-03-25 14:32:17]

PTAL

This is simplified relative the WIP CL, by setting the return type to 'any'
(i.e. ScriptValue) instead of V8ObjectBuilder and that adding special handling
of that in the code generator.

This has two main advantages:

- Simplified code generator changes.

- Not all serializers should necessarily return an (plain) object. A serializer
might be defined to return some primitive value, or an array. Not enforcing use
of V8ObjectBuilder avoids making this a problem.

The down-side is that it makes the implementation somewhat less straight-forward
(if it wants to return a plain object), but not that much:

  ScriptValue Foo::toJSONForBinding(ScriptState* scriptState)
  {
    V8ObjectBuilder result;
    result.add(...);
    ...
    return result.scriptValue();
  }

[vivekg, 2015-03-25 15:27:10]

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

[vivekg, 2015-03-25 15:27:11]

I recently added CLs [1] and [2] which supports ScriptValue::from<T>() and
ScriptValue::to<T>() methods. 
So if possible we can reuse them here for JSON serialization purpose. This would
save each implementation of having its own JSON serialization routines.

i.e. the Impl would just need to implement toV8(....) if its not already done.
Now using ScriptValue::from we get the associated ScriptValue.
From this ScriptValue, we can obtain the JSONValue using RefPtr<JSONValue>
jsonValue = ScriptValue::to<JSONValuePtr>(isolate, Local<Value> or ScriptValue,
ExceptionState).

WDYT? 

[1] https://codereview.chromium.org/972153002
[2] https://codereview.chromium.org/1020233002

[Jens Widell, 2015-03-25 15:41:49]

On 2015/03/25 15:27:11, vivekg_ wrote:
> I recently added CLs [1] and [2] which supports ScriptValue::from<T>() and
> ScriptValue::to<T>() methods. 
> So if possible we can reuse them here for JSON serialization purpose. This
would
> save each implementation of having its own JSON serialization routines.
>
> i.e. the Impl would just need to implement toV8(....) if its not already done.
> Now using ScriptValue::from we get the associated ScriptValue.
> From this ScriptValue, we can obtain the JSONValue using RefPtr<JSONValue>
> jsonValue = ScriptValue::to<JSONValuePtr>(isolate, Local<Value> or
ScriptValue,
> ExceptionState).
> 
> WDYT? 
> 
> [1] https://codereview.chromium.org/972153002
> [2] https://codereview.chromium.org/1020233002

Unlike what one might think, toJSON() shouldn't do JSON serialization; it should
return a value (primitive, object or array) that is in turn JSON serialized by
the called (e.g. JSON.stringify()).

Hence, I don't think involving JSONValue is appropriate. ScriptValue is used,
and if it makes sense an implementation of toJSONForBinding() could make use of
at least ScriptValue::from<> (both of those helpers are good stuff!) but that's
not totally likely, I think, given what a toJSON() method is supposed to do
according to Web IDL.

If I misunderstood your suggestion, please dumb it down for me. ;-)

[vivekg, 2015-03-25 16:07:46]

On 2015/03/25 15:41:49, Jens Widell wrote:
> On 2015/03/25 15:27:11, vivekg_ wrote:
> > I recently added CLs [1] and [2] which supports ScriptValue::from<T>() and
> > ScriptValue::to<T>() methods. 
> > So if possible we can reuse them here for JSON serialization purpose. This
> would
> > save each implementation of having its own JSON serialization routines.
> >
> > i.e. the Impl would just need to implement toV8(....) if its not already
done.
> > Now using ScriptValue::from we get the associated ScriptValue.
> > From this ScriptValue, we can obtain the JSONValue using RefPtr<JSONValue>
> > jsonValue = ScriptValue::to<JSONValuePtr>(isolate, Local<Value> or
> ScriptValue,
> > ExceptionState).
> > 
> > WDYT? 
> > 
> > [1] https://codereview.chromium.org/972153002
> > [2] https://codereview.chromium.org/1020233002
> 
> Unlike what one might think, toJSON() shouldn't do JSON serialization; it
should
> return a value (primitive, object or array) that is in turn JSON serialized by
> the called (e.g. JSON.stringify()).
> 
> Hence, I don't think involving JSONValue is appropriate. ScriptValue is used,
> and if it makes sense an implementation of toJSONForBinding() could make use
of
> at least ScriptValue::from<> (both of those helpers are good stuff!) but
that's
> not totally likely, I think, given what a toJSON() method is supposed to do
> according to Web IDL.
> 
> If I misunderstood your suggestion, please dumb it down for me. ;-)

Oh sorry I misunderstood the semantics of JSON serialization. I think you are
right in terms of toJSONForBinding. Sorry for the noise ;)

[haraken, 2015-03-26 01:39:20]

LGTM

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

https://codereview.chromium.org/1004503004/diff/1/Source/bindings/scripts/v8_...
Source/bindings/scripts/v8_interface.py:480: if 'CallWith' not in
serializer_ext_attrs:

What if the serializer has [CallWith] but the [CallWith=...] doesn't have
'ScriptState'?

https://codereview.chromium.org/1004503004/diff/1/Source/bindings/tests/resul...
File Source/bindings/tests/results/core/V8TestInterface2.cpp (right):

https://codereview.chromium.org/1004503004/diff/1/Source/bindings/tests/resul...
Source/bindings/tests/results/core/V8TestInterface2.cpp:373:
v8SetReturnValueString(info, impl->stringifierMethod(), info.GetIsolate());

Is this change expected?

[bashi, 2015-03-26 02:26:17]

LGTM

https://codereview.chromium.org/1004503004/diff/1/Source/bindings/tests/idls/...
File Source/bindings/tests/idls/core/TestInterface.idl (right):

https://codereview.chromium.org/1004503004/diff/1/Source/bindings/tests/idls/...
Source/bindings/tests/idls/core/TestInterface.idl:83: [NotEnumerable,
RaisesException] serializer;
Just a question: Does this CL support all forms of serializer described in the
spec? Or only following forms?
- serializer
- serializer type identifier()

[Jens Widell, 2015-03-26 05:34:11]

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

https://codereview.chromium.org/1004503004/diff/1/Source/bindings/scripts/v8_...
Source/bindings/scripts/v8_interface.py:480: if 'CallWith' not in
serializer_ext_attrs:
On 2015/03/26 01:39:20, haraken wrote:
> 
> What if the serializer has [CallWith] but the [CallWith=...] doesn't have
> 'ScriptState'?

We could call that a bug in the IDL; it needs a ScriptState to construct a
ScriptValue, after all.

To be honest, I just didn't want to complicate this code by supporting extending
an existing CallWith attribute.

https://codereview.chromium.org/1004503004/diff/1/Source/bindings/tests/idls/...
File Source/bindings/tests/idls/core/TestInterface.idl (right):

https://codereview.chromium.org/1004503004/diff/1/Source/bindings/tests/idls/...
Source/bindings/tests/idls/core/TestInterface.idl:83: [NotEnumerable,
RaisesException] serializer;
On 2015/03/26 02:26:17, bashi1 wrote:
> Just a question: Does this CL support all forms of serializer described in the
> spec? Or only following forms?
> - serializer
> - serializer type identifier()

The base parser supports all forms. Thus CL mostly doesn't care what the
definition looks like, but it will allow all forms.

I added one test each for the two output variations this CL supports.

[haraken, 2015-03-26 05:36:02]

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

https://codereview.chromium.org/1004503004/diff/1/Source/bindings/scripts/v8_...
Source/bindings/scripts/v8_interface.py:480: if 'CallWith' not in
serializer_ext_attrs:
On 2015/03/26 05:34:10, Jens Widell wrote:
> On 2015/03/26 01:39:20, haraken wrote:
> > 
> > What if the serializer has [CallWith] but the [CallWith=...] doesn't have
> > 'ScriptState'?
> 
> We could call that a bug in the IDL; it needs a ScriptState to construct a
> ScriptValue, after all.
> 
> To be honest, I just didn't want to complicate this code by supporting
extending
> an existing CallWith attribute.

Makes sense.

[Jens Widell, 2015-03-26 09:27:52]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2015-03-26 09:28:12]

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

[commit-bot: I haz the power, 2015-03-26 10:43:06]

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