Mojo C++ bindings: switch union to use the new serialization interface.

Mojo C++ bindings: switch union to use the new serialization interface.

BUG=577686
Committed: https://crrev.com/5527dbce795f2d3daaf079f9adb0b236f716c572
Cr-Commit-Position: refs/heads/master@{#392653}

[yzshen1, 2016-05-05 22:34:00]

yzshen@chromium.org changed reviewers:
+ sammc@chromium.org

[yzshen1, 2016-05-05 22:34:01]

Hi, Sam.

Would you please take a look? Thanks!

[Sam McNally, 2016-05-09 01:15:00]

https://codereview.chromium.org/1956603002/diff/1/mojo/public/tools/bindings/...
File
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_declaration.tmpl
(right):

https://codereview.chromium.org/1956603002/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_declaration.tmpl:15:
static void Serialize({{mojom_type}}Ptr& input,
Why are these taking a non-const lvalue reference?

[yzshen1, 2016-05-09 06:13:40]

https://codereview.chromium.org/1956603002/diff/1/mojo/public/tools/bindings/...
File
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_declaration.tmpl
(right):

https://codereview.chromium.org/1956603002/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_declaration.tmpl:15:
static void Serialize({{mojom_type}}Ptr& input,
On 2016/05/09 01:14:59, Sam McNally wrote:
> Why are these taking a non-const lvalue reference?

The old serialization API takes |input| by value, because struct/union/etc. are
move-only and may contain things like handles.

In the new API, I changed the input to a reference, which may or may not be
const. (You can see I used MaybeConstUserType for struct here:
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/tools/...
)
That is because with custom type mapping, sometimes the user input is passed in
by const-ref (example: GURL as the custom type of a struct), and sometimes by
value (example: standard mojo struct ptr).

For now I am not adding support for custom typemapping of  unions. But I want
the API to be consistent, so I made it a non-const ref of the standard mojo
union ptr.

[Sam McNally, 2016-05-09 07:02:29]

https://codereview.chromium.org/1956603002/diff/1/mojo/public/tools/bindings/...
File
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_declaration.tmpl
(right):

https://codereview.chromium.org/1956603002/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_declaration.tmpl:15:
static void Serialize({{mojom_type}}Ptr& input,
On 2016/05/09 06:13:40, yzshen1 wrote:
> On 2016/05/09 01:14:59, Sam McNally wrote:
> > Why are these taking a non-const lvalue reference?
> 
> The old serialization API takes |input| by value, because struct/union/etc.
are
> move-only and may contain things like handles.
> 
> In the new API, I changed the input to a reference, which may or may not be
> const. (You can see I used MaybeConstUserType for struct here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/tools/...
> )
> That is because with custom type mapping, sometimes the user input is passed
in
> by const-ref (example: GURL as the custom type of a struct), and sometimes by
> value (example: standard mojo struct ptr).
> 
> For now I am not adding support for custom typemapping of  unions. But I want
> the API to be consistent, so I made it a non-const ref of the standard mojo
> union ptr.

I don't see why the input needs to be non-const. If this is only for StructPtr,
a const StructPtr& is sufficient, since StructPtr<T>::operator-> and friends are
const and return a T*.

[yzshen1, 2016-05-09 16:05:26]

On 2016/05/09 07:02:29, Sam McNally wrote:
>
https://codereview.chromium.org/1956603002/diff/1/mojo/public/tools/bindings/...
> File
>
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_declaration.tmpl
> (right):
> 
>
https://codereview.chromium.org/1956603002/diff/1/mojo/public/tools/bindings/...
>
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_declaration.tmpl:15:
> static void Serialize({{mojom_type}}Ptr& input,
> On 2016/05/09 06:13:40, yzshen1 wrote:
> > On 2016/05/09 01:14:59, Sam McNally wrote:
> > > Why are these taking a non-const lvalue reference?
> > 
> > The old serialization API takes |input| by value, because struct/union/etc.
> are
> > move-only and may contain things like handles.
> > 
> > In the new API, I changed the input to a reference, which may or may not be
> > const. (You can see I used MaybeConstUserType for struct here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/tools/...
> > )
> > That is because with custom type mapping, sometimes the user input is passed
> in
> > by const-ref (example: GURL as the custom type of a struct), and sometimes
by
> > value (example: standard mojo struct ptr).
> > 
> > For now I am not adding support for custom typemapping of  unions. But I
want
> > the API to be consistent, so I made it a non-const ref of the standard mojo
> > union ptr.
> 
> I don't see why the input needs to be non-const. If this is only for
StructPtr,
> a const StructPtr& is sufficient, since StructPtr<T>::operator-> and friends
are
> const and return a T*.

Ah, StructPtr may be a bad example, but there are plenty of examples that
require non-const input.

Example 1: Please note that with custom type mapping, the input of struct
serialization could be arbitrary type that the user would like to use. Take the
following mojo struct as example:
struct Foo {
  Bar bar;  // |bar| is a interface pointer.
};

Instead of using StructPtr<Foo>, the user may want to map it to, for example,
base::Optional<Foo>. ("const base::Optional<Foo>&" doesn't return a non-const
Foo pointer.)
Or the user may want to map it to something like:

class CustomFoo {
  ...
  BarPtr PassBar();  // non-const |this| is required.
};


Example 2: Array<ScopedMessagePipeHandle> cannot be serialized with a non-const
ref.

I agree that a const-ref works for unions, if we don't support
custom-typemapping for unions. I feel that it is fairly counter-intuitive, but I
am fine to make the change if it makes more sense to you that way.
In the case of structs/arrays/maps which we will support custom-typemapping,
however, I don't think non-const ref is sufficient.

WDYT?

[Sam McNally, 2016-05-10 00:37:16]

lgtm

[yzshen1, 2016-05-10 15:42:00]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2016-05-10 15:42:17]

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

[commit-bot: I haz the power, 2016-05-10 17:08:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-10 17:08:31]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[yzshen1, 2016-05-10 17:21:18]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2016-05-10 17:21:59]

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

[commit-bot: I haz the power, 2016-05-10 18:22:16]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-05-10 18:23:29]

Description was changed from

==========
Mojo C++ bindings: switch union to use the new serialization interface.

BUG=577686
==========

to

==========
Mojo C++ bindings: switch union to use the new serialization interface.

BUG=577686

Committed: https://crrev.com/5527dbce795f2d3daaf079f9adb0b236f716c572
Cr-Commit-Position: refs/heads/master@{#392653}
==========

[commit-bot: I haz the power, 2016-05-10 18:23:30]

Patchset 1 (id:??) landed as
https://crrev.com/5527dbce795f2d3daaf079f9adb0b236f716c572
Cr-Commit-Position: refs/heads/master@{#392653}