Mojo C++ bindings: add a new mode to generator to use native STL/WTF types

Mojo C++ bindings: add a new mode to generator to use native STL/WTF types

Users can add "use_new_wrapper_types = true" to their mojom targets to make the generator map mojom array/map/string to STL types (for chromium variant) or WTF types (for blink variant).

This CL also switches bindings tests to use the new mode

BUG=624136
Committed: https://crrev.com/c875b8baff46f35eb650e1f837d13d8c9ea1a862
Cr-Commit-Position: refs/heads/master@{#405273}

[yzshen1, 2016-07-11 21:00:30]

Description was changed from

==========
[WIP]

BUG=
==========

to

==========
Mojo C++ bindings: add a new mode to generator to use native STL/WTF types

Users can add "use_new_wrapper_types = true" to their mojom targets to make the
generator map mojom array/map/string to STL types (for chromium variant) or WTF
types (for blink variant).

This CL also switches bindings tests to use the new mode

BUG=624136
==========

[yzshen1, 2016-07-11 21:00:30]

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

[yzshen1, 2016-07-11 21:03:42]

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

[yzshen1, 2016-07-11 21:03:43]

Hi, Ken and Daniel.

Would you please take a look? Thanks!

Ken: everything

Daniel: device/bluetooth/public/interfaces/bluetooth_uuid_struct_traits.h
This CL enables using T's StructTraits to handle base::Optional<T>
serialization, hence the change.

[yzshen1, 2016-07-11 22:43:53]

On 2016/07/11 21:03:43, yzshen1 wrote:
> Hi, Ken and Daniel.
> 
> Would you please take a look? Thanks!
> 
> Ken: everything
> 
> Daniel: device/bluetooth/public/interfaces/bluetooth_uuid_struct_traits.h
> This CL enables using T's StructTraits to handle base::Optional<T>
> serialization, hence the change.

Hmmm... this fails on Android and Mac because the following code fails
compilation:

std::vector<std::vector<MoveOnlyType>> vec;
vec.resize(10);

This works fine on Linux and Windows.
Looking at it...

[yzshen1, 2016-07-11 23:57:19]

On 2016/07/11 22:43:53, yzshen1 wrote:
> On 2016/07/11 21:03:43, yzshen1 wrote:
> > Hi, Ken and Daniel.
> > 
> > Would you please take a look? Thanks!
> > 
> > Ken: everything
> > 
> > Daniel: device/bluetooth/public/interfaces/bluetooth_uuid_struct_traits.h
> > This CL enables using T's StructTraits to handle base::Optional<T>
> > serialization, hence the change.
> 
> Hmmm... this fails on Android and Mac because the following code fails
> compilation:
> 
> std::vector<std::vector<MoveOnlyType>> vec;
> vec.resize(10);
> 
> This works fine on Linux and Windows.
> Looking at it...

Okay, I think the compilers are happy now.

Please take a look. Thanks!

[Ken Rockot(use gerrit already), 2016-07-11 23:59:03]

fantastic. LGTM!

https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/array_traits_stl.h (right):

https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/array_traits_stl.h:49: //
std::vector<std::vector<MoveOnlyType>>.
weird

[dcheng, 2016-07-12 04:19:42]

Is the eventual plan to convert all bindings, or will we end up keeping both
modes?

https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/bindings_internal.h (right):

https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bindings_internal.h:321: template <typename T>
Nit: maybe static_assert that is_enum<T> is true, since using underlying_type on
something that's not a complete enum type is undefined behavior

https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/map_traits_wtf_hash_map.h (right):

https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/map_traits_wtf_hash_map.h:27: // WTF::HashMap<> doesn't
support null state. Set it to empty instead.
I don't know if anyone depends on this behavior, but it seems like it'd be
better to static assert instead.

[yzshen1, 2016-07-12 16:29:00]

On 2016/07/12 04:19:42, dcheng wrote:
> Is the eventual plan to convert all bindings, or will we end up keeping both
> modes?

The plan is to:
 - flip the flag and make use_new_wrapper_types=true the default;
 - convert all users to use the new mode;
 - remove support for the old mode.

I have updated the comment in mojom.gni to be more clear about the plan

> 
>
https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/lib/bindings_internal.h (right):
> 
>
https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/lib/bindings_internal.h:321: template <typename T>
> Nit: maybe static_assert that is_enum<T> is true, since using underlying_type
on
> something that's not a complete enum type is undefined behavior
> 
>
https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/map_traits_wtf_hash_map.h (right):
> 
>
https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/map_traits_wtf_hash_map.h:27: // WTF::HashMap<>
doesn't
> support null state. Set it to empty instead.
> I don't know if anyone depends on this behavior, but it seems like it'd be
> better to static assert instead.

[yzshen1, 2016-07-12 16:29:20]

https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/array_traits_stl.h (right):

https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/array_traits_stl.h:49: //
std::vector<std::vector<MoveOnlyType>>.
On 2016/07/11 23:59:03, Ken Rockot wrote:
> weird

It is possible that the compiler version is not fully compliant with C++11.

https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/bindings_internal.h (right):

https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bindings_internal.h:321: template <typename T>
On 2016/07/12 04:19:42, dcheng wrote:
> Nit: maybe static_assert that is_enum<T> is true, since using underlying_type
on
> something that's not a complete enum type is undefined behavior

Done.

https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/map_traits_wtf_hash_map.h (right):

https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/map_traits_wtf_hash_map.h:27: // WTF::HashMap<> doesn't
support null state. Set it to empty instead.
On 2016/07/12 04:19:42, dcheng wrote:
> I don't know if anyone depends on this behavior, but it seems like it'd be
> better to static assert instead.

The bindings use WTF::Optional<WTF::HashMap<>> to represent mojom nullable map,
so IsNull/SetToNull is not used by the auto-generated code. If a user doesn't
care about null state, however, in StructTraits::Read() he could read a nullable
map field directly into a WTF::HashMap<> and convert null to empty; or he could
return HashMap<> from a StructTraits getter which serializes to nullable map. So
it seems useful to me to keep IsNull()/SetToNull() here.

Besides, it is not possible to static assert here because the "target mojom
type" is not part of this MapTraits.

WDYT? Thanks!

[dcheng, 2016-07-13 14:04:44]

LGTM

https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/map_traits_wtf_hash_map.h (right):

https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/map_traits_wtf_hash_map.h:27: // WTF::HashMap<> doesn't
support null state. Set it to empty instead.
On 2016/07/12 16:29:20, yzshen1 wrote:
> On 2016/07/12 04:19:42, dcheng wrote:
> > I don't know if anyone depends on this behavior, but it seems like it'd be
> > better to static assert instead.
> 
> The bindings use WTF::Optional<WTF::HashMap<>> to represent mojom nullable
map,
> so IsNull/SetToNull is not used by the auto-generated code. If a user doesn't
> care about null state, however, in StructTraits::Read() he could read a
nullable
> map field directly into a WTF::HashMap<> and convert null to empty; or he
could
> return HashMap<> from a StructTraits getter which serializes to nullable map.
So
> it seems useful to me to keep IsNull()/SetToNull() here.
> 
> Besides, it is not possible to static assert here because the "target mojom
> type" is not part of this MapTraits.
> 
> WDYT? Thanks!

It just seems weird that it's not symmetric: we declare that this thing will
never be null when serializing, and yet we have to handle it when deserializing.

I don't think it should block this CL, just something to think about.

[yzshen1, 2016-07-13 15:24:11]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2016-07-13 15:24:12]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2136733002/#ps80001
(title: ".")

[commit-bot: I haz the power, 2016-07-13 15:24:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yzshen1, 2016-07-13 15:34:02]

On 2016/07/13 14:04:44, dcheng wrote:
> LGTM
> 
>
https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/map_traits_wtf_hash_map.h (right):
> 
>
https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/map_traits_wtf_hash_map.h:27: // WTF::HashMap<>
doesn't
> support null state. Set it to empty instead.
> On 2016/07/12 16:29:20, yzshen1 wrote:
> > On 2016/07/12 04:19:42, dcheng wrote:
> > > I don't know if anyone depends on this behavior, but it seems like it'd be
> > > better to static assert instead.
> > 
> > The bindings use WTF::Optional<WTF::HashMap<>> to represent mojom nullable
> map,
> > so IsNull/SetToNull is not used by the auto-generated code. If a user
doesn't
> > care about null state, however, in StructTraits::Read() he could read a
> nullable
> > map field directly into a WTF::HashMap<> and convert null to empty; or he
> could
> > return HashMap<> from a StructTraits getter which serializes to nullable
map.
> So
> > it seems useful to me to keep IsNull()/SetToNull() here.
> > 
> > Besides, it is not possible to static assert here because the "target mojom
> > type" is not part of this MapTraits.
> > 
> > WDYT? Thanks!
> 
> It just seems weird that it's not symmetric: we declare that this thing will
> never be null when serializing, and yet we have to handle it when
deserializing.

The reason is that users may serialize from a type T and deserialize to a type
U. T and U are different types so different StructTraits are used.
A StructTraits' IsNull() is used during serialization, whlie SetToNull() is used
during deserialization. So it is expected there is no symmetry between the two.

I considered splitting StructTraits into
StructSerializationTraits/StructDeserializationTraits, that seems slightly less
convenient for users. The benefit is that sometimes users write a StructTraits<>
only for one side (either serialization or deserialization), in that case,
splitting StructTraits gives a more clear interface.

I don't have a strong opinion here but seems too much trouble to initiate a
change. :)


> 
> I don't think it should block this CL, just something to think about.

[dcheng, 2016-07-13 16:22:04]

On 2016/07/13 15:34:02, yzshen1 wrote:
> On 2016/07/13 14:04:44, dcheng wrote:
> > LGTM
> > 
> >
>
https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
> > File mojo/public/cpp/bindings/map_traits_wtf_hash_map.h (right):
> > 
> >
>
https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
> > mojo/public/cpp/bindings/map_traits_wtf_hash_map.h:27: // WTF::HashMap<>
> doesn't
> > support null state. Set it to empty instead.
> > On 2016/07/12 16:29:20, yzshen1 wrote:
> > > On 2016/07/12 04:19:42, dcheng wrote:
> > > > I don't know if anyone depends on this behavior, but it seems like it'd
be
> > > > better to static assert instead.
> > > 
> > > The bindings use WTF::Optional<WTF::HashMap<>> to represent mojom nullable
> > map,
> > > so IsNull/SetToNull is not used by the auto-generated code. If a user
> doesn't
> > > care about null state, however, in StructTraits::Read() he could read a
> > nullable
> > > map field directly into a WTF::HashMap<> and convert null to empty; or he
> > could
> > > return HashMap<> from a StructTraits getter which serializes to nullable
> map.
> > So
> > > it seems useful to me to keep IsNull()/SetToNull() here.
> > > 
> > > Besides, it is not possible to static assert here because the "target
mojom
> > > type" is not part of this MapTraits.
> > > 
> > > WDYT? Thanks!
> > 
> > It just seems weird that it's not symmetric: we declare that this thing will
> > never be null when serializing, and yet we have to handle it when
> deserializing.
> 
> The reason is that users may serialize from a type T and deserialize to a type
> U. T and U are different types so different StructTraits are used.
> A StructTraits' IsNull() is used during serialization, whlie SetToNull() is
used
> during deserialization. So it is expected there is no symmetry between the
two.
> 
> I considered splitting StructTraits into
> StructSerializationTraits/StructDeserializationTraits, that seems slightly
less
> convenient for users. The benefit is that sometimes users write a
StructTraits<>
> only for one side (either serialization or deserialization), in that case,
> splitting StructTraits gives a more clear interface.
> 
> I don't have a strong opinion here but seems too much trouble to initiate a
> change. :)
> 
> 
> > 
> > I don't think it should block this CL, just something to think about.

I think this mismatch leads to trouble more than it helps: for example,
url::Origin and blink::WebSecurityOrigin. One side is nullable, while the other
is not: that means if you go from WebSecurityOrigin -> WebSecurityOrigin, you're
fine, but if you go from WebSecurityOrigin -> url::Origin, you explode.

[yzshen1, 2016-07-13 19:12:22]

On 2016/07/13 16:22:04, dcheng wrote:
> On 2016/07/13 15:34:02, yzshen1 wrote:
> > On 2016/07/13 14:04:44, dcheng wrote:
> > > LGTM
> > > 
> > >
> >
>
https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
> > > File mojo/public/cpp/bindings/map_traits_wtf_hash_map.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2136733002/diff/60001/mojo/public/cpp/binding...
> > > mojo/public/cpp/bindings/map_traits_wtf_hash_map.h:27: // WTF::HashMap<>
> > doesn't
> > > support null state. Set it to empty instead.
> > > On 2016/07/12 16:29:20, yzshen1 wrote:
> > > > On 2016/07/12 04:19:42, dcheng wrote:
> > > > > I don't know if anyone depends on this behavior, but it seems like
it'd
> be
> > > > > better to static assert instead.
> > > > 
> > > > The bindings use WTF::Optional<WTF::HashMap<>> to represent mojom
nullable
> > > map,
> > > > so IsNull/SetToNull is not used by the auto-generated code. If a user
> > doesn't
> > > > care about null state, however, in StructTraits::Read() he could read a
> > > nullable
> > > > map field directly into a WTF::HashMap<> and convert null to empty; or
he
> > > could
> > > > return HashMap<> from a StructTraits getter which serializes to nullable
> > map.
> > > So
> > > > it seems useful to me to keep IsNull()/SetToNull() here.
> > > > 
> > > > Besides, it is not possible to static assert here because the "target
> mojom
> > > > type" is not part of this MapTraits.
> > > > 
> > > > WDYT? Thanks!
> > > 
> > > It just seems weird that it's not symmetric: we declare that this thing
will
> > > never be null when serializing, and yet we have to handle it when
> > deserializing.
> > 
> > The reason is that users may serialize from a type T and deserialize to a
type
> > U. T and U are different types so different StructTraits are used.
> > A StructTraits' IsNull() is used during serialization, whlie SetToNull() is
> used
> > during deserialization. So it is expected there is no symmetry between the
> two.
> > 
> > I considered splitting StructTraits into
> > StructSerializationTraits/StructDeserializationTraits, that seems slightly
> less
> > convenient for users. The benefit is that sometimes users write a
> StructTraits<>
> > only for one side (either serialization or deserialization), in that case,
> > splitting StructTraits gives a more clear interface.
> > 
> > I don't have a strong opinion here but seems too much trouble to initiate a
> > change. :)
> > 
> > 
> > > 
> > > I don't think it should block this CL, just something to think about.
> 
> I think this mismatch leads to trouble more than it helps: for example,
> url::Origin and blink::WebSecurityOrigin. One side is nullable, while the
other
> is not: that means if you go from WebSecurityOrigin -> WebSecurityOrigin,
you're
> fine, but if you go from WebSecurityOrigin -> url::Origin, you explode.

In this case, a null value going to be deserialized as url::Origin will be
considered a deserialization failure and disconnect the pipe.
It is just like other cases where you return "false" from StructTratis::Read().
It doesn't seem so bad to me.

I guess I haven't understood your idea: do you suggest that all StructTraits
have to support null state?

[commit-bot: I haz the power, 2016-07-13 20:24:32]

Description was changed from

==========
Mojo C++ bindings: add a new mode to generator to use native STL/WTF types

Users can add "use_new_wrapper_types = true" to their mojom targets to make the
generator map mojom array/map/string to STL types (for chromium variant) or WTF
types (for blink variant).

This CL also switches bindings tests to use the new mode

BUG=624136
==========

to

==========
Mojo C++ bindings: add a new mode to generator to use native STL/WTF types

Users can add "use_new_wrapper_types = true" to their mojom targets to make the
generator map mojom array/map/string to STL types (for chromium variant) or WTF
types (for blink variant).

This CL also switches bindings tests to use the new mode

BUG=624136
==========

[commit-bot: I haz the power, 2016-07-13 20:24:33]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-13 20:24:37]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 20:26:52]

Description was changed from

==========
Mojo C++ bindings: add a new mode to generator to use native STL/WTF types

Users can add "use_new_wrapper_types = true" to their mojom targets to make the
generator map mojom array/map/string to STL types (for chromium variant) or WTF
types (for blink variant).

This CL also switches bindings tests to use the new mode

BUG=624136
==========

to

==========
Mojo C++ bindings: add a new mode to generator to use native STL/WTF types

Users can add "use_new_wrapper_types = true" to their mojom targets to make the
generator map mojom array/map/string to STL types (for chromium variant) or WTF
types (for blink variant).

This CL also switches bindings tests to use the new mode

BUG=624136

Committed: https://crrev.com/c875b8baff46f35eb650e1f837d13d8c9ea1a862
Cr-Commit-Position: refs/heads/master@{#405273}
==========

[commit-bot: I haz the power, 2016-07-13 20:26:53]

Patchset 5 (id:??) landed as
https://crrev.com/c875b8baff46f35eb650e1f837d13d8c9ea1a862
Cr-Commit-Position: refs/heads/master@{#405273}