[WIP] IDL: Add limited serializer support and use for RTCIceCandidate

[WIP] IDL: Add limited serializer support and use for RTCIceCandidate

BUG=469650, 467366

[Jens Widell, 2015-03-23 14:30:13]

Patchset #1 (id:1) has been deleted

[Jens Widell, 2015-03-23 14:37:18]

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

[Jens Widell, 2015-03-23 14:37:19]

This is a draft, and the RTCIceCandidate modification is a (working) example of
how it would be used.

Comments of this approach?

[haraken, 2015-03-23 14:44:28]

I'm OK with V8ObjectBuilder, but it would be great if we could share more code
between V8ObjectBuilder and Dictionary stuff. bashi-san would have thoughts.

[Jens Widell, 2015-03-23 14:55:08]

On 2015/03/23 14:44:28, haraken wrote:
> I'm OK with V8ObjectBuilder, but it would be great if we could share more code
> between V8ObjectBuilder and Dictionary stuff. bashi-san would have thoughts.

Dictionary::createEmpty() and the Dictionary::set() functions are only used in a
few places in modules/crypto/. One way to proceed would be to use
V8ObjectBuilder instead of Dictionary there. This would make Dictionary
read-only, which matches its class comment.

[Jens Widell, 2015-03-23 15:49:08]

On 2015/03/23 14:55:08, Jens Widell wrote:
> On 2015/03/23 14:44:28, haraken wrote:
> > I'm OK with V8ObjectBuilder, but it would be great if we could share more
code
> > between V8ObjectBuilder and Dictionary stuff. bashi-san would have thoughts.
> 
> Dictionary::createEmpty() and the Dictionary::set() functions are only used in
a
> few places in modules/crypto/. One way to proceed would be to use
> V8ObjectBuilder instead of Dictionary there. This would make Dictionary
> read-only, which matches its class comment.

PS#2 changes modules/crypto/ to use V8ObjectBuilder, and drops
Dictionary::createEmpty()/set(). This makes Dictionary and V8ObjectBuilder
non-overlapping.

(I mean to split this into multiple CLs when landing anything, BTW.)

[bashi, 2015-03-24 00:49:05]

I'm supportive on adding V8ObjectBuilder and make Dictionary read-only.

https://codereview.chromium.org/1029093003/diff/40001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ObjectBuilder.cpp (right):

https://codereview.chromium.org/1029093003/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ObjectBuilder.cpp:48: V8ObjectBuilder
V8ObjectBuilder::addObject(String name)
This looks different from other add methds() so addObject sounds a bit strange
to me.

https://codereview.chromium.org/1029093003/diff/40001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ObjectBuilder.h (right):

https://codereview.chromium.org/1029093003/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ObjectBuilder.h:22: void add(String name, const
ScriptValue&);
nit: How about making add methods return *this? This way, call site can use
following pattern.

  builder.addNull("x")
    .addBoolean("y", true)
    .addNumber("z", 42);

I'm sure you know this pattern, and I'm not sure we want to do in Blink, so this
is just a nit.

[Jens Widell, 2015-03-24 07:10:03]

https://codereview.chromium.org/1029093003/diff/40001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ObjectBuilder.cpp (right):

https://codereview.chromium.org/1029093003/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ObjectBuilder.cpp:48: V8ObjectBuilder
V8ObjectBuilder::addObject(String name)
On 2015/03/24 00:49:04, bashi1 wrote:
> This looks different from other add methds() so addObject sounds a bit strange
> to me.

Yes, it's a bit unusual. Would probably be better to make the v8::Value add()
version public and do

  builder1.add("name", builder2.v8Value());

when building and adding a sub-object. Or add a 

  void add(String, const V8ObjectBuilder&);

overload, for that matter.

https://codereview.chromium.org/1029093003/diff/40001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ObjectBuilder.h (right):

https://codereview.chromium.org/1029093003/diff/40001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ObjectBuilder.h:22: void add(String name, const
ScriptValue&);
On 2015/03/24 00:49:05, bashi1 wrote:
> nit: How about making add methods return *this? This way, call site can use
> following pattern.
> 
>   builder.addNull("x")
>     .addBoolean("y", true)
>     .addNumber("z", 42);
> 
> I'm sure you know this pattern, and I'm not sure we want to do in Blink, so
this
> is just a nit.

Yeah, might as well do this, if we drop the current addObject(). (Combining this
with the current addObject() would be quite confusing.)

[perkj_chrome, 2015-11-02 12:09:37]

Description was changed from

==========
[WIP] IDL: Add limited serializer support and use for RTCIceCandidate

BUG=469650, 467366
==========

to

==========
[WIP] IDL: Add limited serializer support and use for RTCIceCandidate

BUG=469650, 467366
==========