[mojo] Add pickling support for native-only structs

[mojo] Add pickling support for native-only structs

This uses IPC::ParamTraits definitions as serializers for
native-only structs declared in mojom.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. https://codereview.chromium.org/1520153002
  5. https://codereview.chromium.org/1524613002
  6. This CL
  7. https://codereview.chromium.org/1524703002

BUG=569669
TBR=jam@chromium.org for DEPS +ipc/ipc_param_traits.h

Committed: https://crrev.com/2312b0ae91169d466ed79311698cf2f2eedf7c73
Cr-Commit-Position: refs/heads/master@{#365847}

[Ken Rockot(use gerrit already), 2015-12-14 07:16:10]

Description was changed from

==========
[mojo] Add pickling support for native-only structs

This uses IPC::ParamTraits definitions as serializers for
native-only structs declared in mojom.

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

to

==========
[mojo] Add pickling support for native-only structs

This uses IPC::ParamTraits definitions as serializers for
native-only structs declared in mojom.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1524613002
  2. https://codereview.chromium.org/1515423002
  3. https://codereview.chromium.org/1517043004
  4. https://codereview.chromium.org/1524693002
  5. https://codereview.chromium.org/1520153002
  6. This CL
  7. https://codereview.chromium.org/1524703002

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

[Ken Rockot(use gerrit already), 2015-12-14 18:46:39]

Description was changed from

==========
[mojo] Add pickling support for native-only structs

This uses IPC::ParamTraits definitions as serializers for
native-only structs declared in mojom.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1524613002
  2. https://codereview.chromium.org/1515423002
  3. https://codereview.chromium.org/1517043004
  4. https://codereview.chromium.org/1524693002
  5. https://codereview.chromium.org/1520153002
  6. This CL
  7. https://codereview.chromium.org/1524703002

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

to

==========
[mojo] Add pickling support for native-only structs

This uses IPC::ParamTraits definitions as serializers for
native-only structs declared in mojom.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. https://codereview.chromium.org/1520153002
  5. This CL
  6. https://codereview.chromium.org/1524703002

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

[Ken Rockot(use gerrit already), 2015-12-14 20:19:51]

Description was changed from

==========
[mojo] Add pickling support for native-only structs

This uses IPC::ParamTraits definitions as serializers for
native-only structs declared in mojom.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. https://codereview.chromium.org/1520153002
  5. This CL
  6. https://codereview.chromium.org/1524703002

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

to

==========
[mojo] Add pickling support for native-only structs

This uses IPC::ParamTraits definitions as serializers for
native-only structs declared in mojom.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. https://codereview.chromium.org/1520153002
  5. https://codereview.chromium.org/1524613002
  6. This CL
  7. https://codereview.chromium.org/1524703002

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

[Ken Rockot(use gerrit already), 2015-12-14 20:43:08]

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

[Ken Rockot(use gerrit already), 2015-12-14 20:43:10]

Here's the patch which actually starts using ParamTraits for native
serialization. Without needing to understand how mojo messages are produced, you
might be interested in native_serialization.h, as well as the ParamTraits
definitions in pickled_struct_{blink,chromium}.*

[Ken Rockot(use gerrit already), 2015-12-14 21:09:02]

Description was changed from

==========
[mojo] Add pickling support for native-only structs

This uses IPC::ParamTraits definitions as serializers for
native-only structs declared in mojom.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. https://codereview.chromium.org/1520153002
  5. https://codereview.chromium.org/1524613002
  6. This CL
  7. https://codereview.chromium.org/1524703002

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

to

==========
[mojo] Add pickling support for native-only structs

This uses IPC::ParamTraits definitions as serializers for
native-only structs declared in mojom.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. https://codereview.chromium.org/1520153002
  5. https://codereview.chromium.org/1524613002
  6. This CL
  7. https://codereview.chromium.org/1524703002

BUG=569669
==========

[jam, 2015-12-14 21:55:11]

Description was changed from

==========
[mojo] Add pickling support for native-only structs

This uses IPC::ParamTraits definitions as serializers for
native-only structs declared in mojom.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. https://codereview.chromium.org/1520153002
  5. https://codereview.chromium.org/1524613002
  6. This CL
  7. https://codereview.chromium.org/1524703002

BUG=569669
==========

to

==========
[mojo] Add pickling support for native-only structs

This uses IPC::ParamTraits definitions as serializers for
native-only structs declared in mojom.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. https://codereview.chromium.org/1520153002
  5. https://codereview.chromium.org/1524613002
  6. This CL
  7. https://codereview.chromium.org/1524703002

BUG=569669
==========

[jam, 2015-12-14 21:55:11]

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

[jam, 2015-12-14 21:55:25]

+Yuzhu as reviewer. I'm not familiar with the bindings code.

[yzshen1, 2015-12-15 21:20:27]

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/DEPS
File mojo/public/DEPS (right):

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/DEPS#newcode5
mojo/public/DEPS:5: "+ipc",
Please see my comments in BUILD.gn.

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/BUILD.gn (right):

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/BUILD.gn:89: "//ipc",
It seems too much to depend on //ipc. It may cause circular dependencies as
well.

Is it sufficient to depend on "param_traits" target?

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

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bindings_internal.h:40: union NativePointer {
Please define it as an alias of ArrayPointer<uint8_t>.

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/native_serialization.h (right):

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/native_serialization.h:37: header->num_bytes =
sizeof(ArrayHeader);
header->num_bytes should be the total size of the array, including the array
header and the payload.

Making it a standard array has the benefits:
- avoid confusion
- use the standard Array_Data<uint_8>::Validate() (Please see my comment in
validation_util)
- other language bindings can handle the object as an opaque array if they want
to.

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/native_serialization.h:54: bool
DeserializeNative_(Array_Data<uint8_t>* data,
Please handle the case |data| is nullptr, in that case, |out| should be set to
default.

With the current code, we won't encountered a null |data| because the
serialization code never generate null. But it is legal to declare a native
struct field to be nullable. And theoretically other bindings (if they deal with
opaque arrays) could send us null.

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/native_serialization.h:70:
DCHECK_EQ(header->num_bytes, 8u);
Please see my comment on line 37.

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/native_serialization.h:81: header->num_elements +
8);
Please use sizeof(ArrayHeader) instead of 8.

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/validation_util.cc (right):

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_util.cc:143: return true;
I think here we need to use the Validate() of Array_Data<uint_8> to check that
it is a valid array. It claims the memory range from |bounds_checker|, making
sure that it doesn't overlap with other objects in the message, etc.

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/validation_util.h (right):

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_util.h:127: bool ValidateNative(const
NativePointer& input, BoundsChecker* bounds_checker);
Please add validation tests. (Not necessarily in this CL. You could add a TODO
if you decide to do it later.)

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/pickle_unittest.cc (right):

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/pickle_unittest.cc:52: void
ExpectError(InterfacePtr<T> *proxy, const base::Closure& callback) {
style nit: * should be adjacent to the type instead of the parameter name.

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/pickled_struct_blink.h (right):

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/pickled_struct_blink.h:35: void set_foo(int foo)
{ DCHECK_GE(foo, 0); foo_ = foo; }
Style! :) Please put each statement on a separate line.

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/pickled_struct_blink.h:38: void set_bar(int bar)
{ DCHECK_GE(bar, 0); bar_ = bar; }
ditto.

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/pickled_struct_blink.h:41: int foo_ = 0, bar_ =
0;
I believe they should be on separate lines, too.

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/pickled_struct_chromium.h (right):

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/pickled_struct_chromium.h:36: int foo_ = 0, bar_
= 0;
separate lines, please.

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl
(right):

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:18: size
+= mojo::internal::GetSerializedSizeNative_(
Please consider naming those Native-related functions simply as
GetSerializedSize_() / Serialize_() / Deserialize_(). We use the same names for
string, struct, array, etc.

I understand that some functions are named with a suffix, such as
mojo::SerializeMap_(), those have extra parameters so I didn't name them in the
same way.

WDYT?

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:144:
return false;
We cannot short-circuit deserialization, so that we won't leak resources.

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_definition.tmpl
(right):

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_definition.tmpl:29:
NOTREACHED() << "Object cloning not supported with typemapped fields.";
It seems reasonable to add support if the typemapped object supports copy/clone,
right?
(No need to be in this CL though. Maybe a TODO.)

Besides, instead of adding a NOTREACHED() here, please consider changing the
definition of is_cloneable_kind.

If a struct is not cloneable (say, having handle fields), we don't generate this
method at all.

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_definition.tmpl:43:
NOTREACHED() << "Equality testing not supported with typemapped fields.";
Similarly, seems reasonable to add support if the object supports equality
testing.

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/mojom_cpp_generator.py (right):

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:101: def
GetCppType(kind):
Do you need to also update this method?

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:130: def
GetCppArrayArgWrapperType(kind):
Do you need to also update this method?

[Ken Rockot(use gerrit already), 2015-12-15 23:31:08]

Note: I haven't updated the generated C++ types used for array/map elements
because they won't actually work yet anyway. It's a little bit of a yak shave to
try to do them here as I think we'll need to generated special-case array
serialization code to use a different element serializer.

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/DEPS
File mojo/public/DEPS (right):

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/DEPS#newcode5
mojo/public/DEPS:5: "+ipc",
On 2015/12/15 at 21:20:26, yzshen1 wrote:
> Please see my comments in BUILD.gn.

Changed to specifically "+ipc/ipc_param_traits.h"

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/BUILD.gn (right):

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/BUILD.gn:89: "//ipc",
On 2015/12/15 at 21:20:26, yzshen1 wrote:
> It seems too much to depend on //ipc. It may cause circular dependencies as
well.
> 
> Is it sufficient to depend on "param_traits" target?

That's fair, though gyp doesn't really have a notion of a param_traits target. I
updated the GN deps.

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

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bindings_internal.h:40: union NativePointer {
On 2015/12/15 at 21:20:26, yzshen1 wrote:
> Please define it as an alias of ArrayPointer<uint8_t>.

Actually I've decided to just get rid of this altogether. Instead the generator
just emits an ArrayPointer<uint8_t>, and I've added code so that array
validation params for native-only fields basically match those of string
validation. WDYT?

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/native_serialization.h (right):

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/native_serialization.h:37: header->num_bytes =
sizeof(ArrayHeader);
On 2015/12/15 at 21:20:26, yzshen1 wrote:
> header->num_bytes should be the total size of the array, including the array
header and the payload.
> 
> Making it a standard array has the benefits:
> - avoid confusion
> - use the standard Array_Data<uint_8>::Validate() (Please see my comment in
validation_util)
> - other language bindings can handle the object as an opaque array if they
want to.

Oops, I was confused by this and at some point convinced myself it was supposed
to be the header size. Fixed.

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/native_serialization.h:54: bool
DeserializeNative_(Array_Data<uint8_t>* data,
On 2015/12/15 at 21:20:26, yzshen1 wrote:
> Please handle the case |data| is nullptr, in that case, |out| should be set to
default.
> 
> With the current code, we won't encountered a null |data| because the
serialization code never generate null. But it is legal to declare a native
struct field to be nullable. And theoretically other bindings (if they deal with
opaque arrays) could send us null.

Done, sort of. I'd rather not force native types to be copyable, and since the
bindings already provide the address of a default-constructed output, I simply
return true in the null case and assume |out| is already a default.

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/native_serialization.h:70:
DCHECK_EQ(header->num_bytes, 8u);
On 2015/12/15 at 21:20:26, yzshen1 wrote:
> Please see my comment on line 37.

Fixed

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/native_serialization.h:81: header->num_elements +
8);
On 2015/12/15 at 21:20:26, yzshen1 wrote:
> Please use sizeof(ArrayHeader) instead of 8.

Done

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/validation_util.cc (right):

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_util.cc:143: return true;
On 2015/12/15 at 21:20:27, yzshen1 wrote:
> I think here we need to use the Validate() of Array_Data<uint_8> to check that
it is a valid array. It claims the memory range from |bounds_checker|, making
sure that it doesn't overlap with other objects in the message, etc.

Done.

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/validation_util.h (right):

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_util.h:127: bool ValidateNative(const
NativePointer& input, BoundsChecker* bounds_checker);
On 2015/12/15 at 21:20:27, yzshen1 wrote:
> Please add validation tests. (Not necessarily in this CL. You could add a TODO
if you decide to do it later.)

No longer needed as we just use array validation now

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/pickle_unittest.cc (right):

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/pickle_unittest.cc:52: void
ExpectError(InterfacePtr<T> *proxy, const base::Closure& callback) {
On 2015/12/15 at 21:20:27, yzshen1 wrote:
> style nit: * should be adjacent to the type instead of the parameter name.

oops, Done

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/pickled_struct_blink.h (right):

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/pickled_struct_blink.h:35: void set_foo(int foo)
{ DCHECK_GE(foo, 0); foo_ = foo; }
On 2015/12/15 at 21:20:27, yzshen1 wrote:
> Style! :) Please put each statement on a separate line.

OK - I took some liberties because it's test-only code, but I think you're
right:)

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/pickled_struct_blink.h:38: void set_bar(int bar)
{ DCHECK_GE(bar, 0); bar_ = bar; }
On 2015/12/15 at 21:20:27, yzshen1 wrote:
> ditto.

done

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/pickled_struct_blink.h:41: int foo_ = 0, bar_ =
0;
On 2015/12/15 at 21:20:27, yzshen1 wrote:
> I believe they should be on separate lines, too.

done

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/pickled_struct_chromium.h (right):

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/pickled_struct_chromium.h:36: int foo_ = 0, bar_
= 0;
On 2015/12/15 at 21:20:27, yzshen1 wrote:
> separate lines, please.

done

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl
(right):

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:18: size
+= mojo::internal::GetSerializedSizeNative_(
On 2015/12/15 at 21:20:27, yzshen1 wrote:
> Please consider naming those Native-related functions simply as
GetSerializedSize_() / Serialize_() / Deserialize_(). We use the same names for
string, struct, array, etc.
> 
> I understand that some functions are named with a suffix, such as
mojo::SerializeMap_(), those have extra parameters so I didn't name them in the
same way.
> 
> WDYT?

This won't work. We don't know the types at bindings generation time, so these
need to be templated over native type. Serialize_ et al aren't templated, so we
need a new name

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:144:
return false;
On 2015/12/15 at 21:20:27, yzshen1 wrote:
> We cannot short-circuit deserialization, so that we won't leak resources.

Oops, fixed

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_definition.tmpl
(right):

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_definition.tmpl:29:
NOTREACHED() << "Object cloning not supported with typemapped fields.";
On 2015/12/15 at 21:20:27, yzshen1 wrote:
> It seems reasonable to add support if the typemapped object supports
copy/clone, right?
> (No need to be in this CL though. Maybe a TODO.)
> 

I agree we might want to, but I'd rather wait until we know for sure. It
complicates things a little, and the simplest path forward might be to add
explicit flags to typemaps (to mark types as copyable or move-only) so we can
get things like this right without piles of template voodoo.

Granted, detecting copyability at compile-time isn't very complex, but it would
make this code just that much harder to read.

> Besides, instead of adding a NOTREACHED() here, please consider changing the
definition of is_cloneable_kind.
> 
> If a struct is not cloneable (say, having handle fields), we don't generate
this method at all.

Done.

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_definition.tmpl:43:
NOTREACHED() << "Equality testing not supported with typemapped fields.";
On 2015/12/15 at 21:20:27, yzshen1 wrote:
> Similarly, seems reasonable to add support if the object supports equality
testing.

Agreed

[yzshen1, 2015-12-15 23:55:29]

On 2015/12/15 23:31:08, Ken Rockot wrote:
> Note: I haven't updated the generated C++ types used for array/map elements
> because they won't actually work yet anyway. It's a little bit of a yak shave
to
> try to do them here as I think we'll need to generated special-case array
> serialization code to use a different element serializer.

It is fine to support it later. I just think it is nice to explicitly raise an
exception in places like GetCppArrayArgWrapperType (saying that it is not
supported yet) / GetCppType (saying that it shouldn't be reached).

That part of code is not pretty currently, and I hope it at least won't get more
confusing. :)

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/native_serialization.h (right):

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/native_serialization.h:54: bool
DeserializeNative_(Array_Data<uint8_t>* data,
On 2015/12/15 23:31:07, Ken Rockot wrote:
> On 2015/12/15 at 21:20:26, yzshen1 wrote:
> > Please handle the case |data| is nullptr, in that case, |out| should be set
to
> default.
> > 
> > With the current code, we won't encountered a null |data| because the
> serialization code never generate null. But it is legal to declare a native
> struct field to be nullable. And theoretically other bindings (if they deal
with
> opaque arrays) could send us null.
> 
> Done, sort of. I'd rather not force native types to be copyable, and since the
> bindings already provide the address of a default-constructed output, I simply
> return true in the null case and assume |out| is already a default.

Okay. The other deserialization routines don't make that assumption (which may
be useful when we expose serialization/deserialization as a public API), but I
don't think we need to worry too much now.

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl
(right):

https://codereview.chromium.org/1526533002/diff/80001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:18: size
+= mojo::internal::GetSerializedSizeNative_(
On 2015/12/15 23:31:08, Ken Rockot wrote:
> On 2015/12/15 at 21:20:27, yzshen1 wrote:
> > Please consider naming those Native-related functions simply as
> GetSerializedSize_() / Serialize_() / Deserialize_(). We use the same names
for
> string, struct, array, etc.
> > 
> > I understand that some functions are named with a suffix, such as
> mojo::SerializeMap_(), those have extra parameters so I didn't name them in
the
> same way.
> > 
> > WDYT?
> 
> This won't work. We don't know the types at bindings generation time, so these
> need to be templated over native type. Serialize_ et al aren't templated, so
we
> need a new name

Right. I forgot that was why I used different names for some of those functions.
:)

[Ken Rockot(use gerrit already), 2015-12-16 00:39:15]

On 2015/12/15 at 23:55:29, yzshen wrote:
> On 2015/12/15 23:31:08, Ken Rockot wrote:
> > Note: I haven't updated the generated C++ types used for array/map elements
> > because they won't actually work yet anyway. It's a little bit of a yak
shave to
> > try to do them here as I think we'll need to generated special-case array
> > serialization code to use a different element serializer.
> 
> It is fine to support it later. I just think it is nice to explicitly raise an
exception in places like GetCppArrayArgWrapperType (saying that it is not
supported yet) / GetCppType (saying that it shouldn't be reached).
> 
> That part of code is not pretty currently, and I hope it at least won't get
more confusing. :)

OK, these both raise now!

> 
>
https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/lib/native_serialization.h (right):
> 
>
https://codereview.chromium.org/1526533002/diff/80001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/lib/native_serialization.h:54: bool
DeserializeNative_(Array_Data<uint8_t>* data,
> On 2015/12/15 23:31:07, Ken Rockot wrote:
> > On 2015/12/15 at 21:20:26, yzshen1 wrote:
> > > Please handle the case |data| is nullptr, in that case, |out| should be
set to
> > default.
> > > 
> > > With the current code, we won't encountered a null |data| because the
> > serialization code never generate null. But it is legal to declare a native
> > struct field to be nullable. And theoretically other bindings (if they deal
with
> > opaque arrays) could send us null.
> > 
> > Done, sort of. I'd rather not force native types to be copyable, and since
the
> > bindings already provide the address of a default-constructed output, I
simply
> > return true in the null case and assume |out| is already a default.
> 
> Okay. The other deserialization routines don't make that assumption (which may
be useful when we expose serialization/deserialization as a public API), but I
don't think we need to worry too much now.

I agree. I think we can discuss this more a bit later. One possibility may be
that we should just disallow nullable native-only fields.

> 
>
https://codereview.chromium.org/1526533002/diff/80001/mojo/public/tools/bindi...
> File mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl
(right):
> 
>
https://codereview.chromium.org/1526533002/diff/80001/mojo/public/tools/bindi...
> mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:18:
size += mojo::internal::GetSerializedSizeNative_(
> On 2015/12/15 23:31:08, Ken Rockot wrote:
> > On 2015/12/15 at 21:20:27, yzshen1 wrote:
> > > Please consider naming those Native-related functions simply as
> > GetSerializedSize_() / Serialize_() / Deserialize_(). We use the same names
for
> > string, struct, array, etc.
> > > 
> > > I understand that some functions are named with a suffix, such as
> > mojo::SerializeMap_(), those have extra parameters so I didn't name them in
the
> > same way.
> > > 
> > > WDYT?
> > 
> > This won't work. We don't know the types at bindings generation time, so
these
> > need to be templated over native type. Serialize_ et al aren't templated, so
we
> > need a new name
> 
> Right. I forgot that was why I used different names for some of those
functions. :)

[yzshen1, 2015-12-16 00:42:35]

LGTM Thanks!

[Ken Rockot(use gerrit already), 2015-12-17 06:15:52]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-17 06:16:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1526533002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1526533002/200001

[commit-bot: I haz the power, 2015-12-17 06:21:59]

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

[commit-bot: I haz the power, 2015-12-17 06:22:00]

Dry run: 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_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Ken Rockot(use gerrit already), 2015-12-17 06:23:27]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[Ken Rockot(use gerrit already), 2015-12-17 06:23:44]

The CQ bit was unchecked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2015-12-17 16:32:09]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2015-12-17 16:32:10]

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

[commit-bot: I haz the power, 2015-12-17 16:32:55]

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

[commit-bot: I haz the power, 2015-12-17 16:42:46]

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

[commit-bot: I haz the power, 2015-12-17 16:42:47]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Ken Rockot(use gerrit already), 2015-12-17 16:44:14]

Description was changed from

==========
[mojo] Add pickling support for native-only structs

This uses IPC::ParamTraits definitions as serializers for
native-only structs declared in mojom.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. https://codereview.chromium.org/1520153002
  5. https://codereview.chromium.org/1524613002
  6. This CL
  7. https://codereview.chromium.org/1524703002

BUG=569669
==========

to

==========
[mojo] Add pickling support for native-only structs

This uses IPC::ParamTraits definitions as serializers for
native-only structs declared in mojom.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. https://codereview.chromium.org/1520153002
  5. https://codereview.chromium.org/1524613002
  6. This CL
  7. https://codereview.chromium.org/1524703002

BUG=569669
TBR=jam@chromium.org for DEPS +ipc/ipc_param_traits.h
==========

[Ken Rockot(use gerrit already), 2015-12-17 16:44:18]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2015-12-17 16:44:45]

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

[commit-bot: I haz the power, 2015-12-17 17:45:44]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-12-17 17:46:25]

Description was changed from

==========
[mojo] Add pickling support for native-only structs

This uses IPC::ParamTraits definitions as serializers for
native-only structs declared in mojom.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. https://codereview.chromium.org/1520153002
  5. https://codereview.chromium.org/1524613002
  6. This CL
  7. https://codereview.chromium.org/1524703002

BUG=569669
TBR=jam@chromium.org for DEPS +ipc/ipc_param_traits.h
==========

to

==========
[mojo] Add pickling support for native-only structs

This uses IPC::ParamTraits definitions as serializers for
native-only structs declared in mojom.

Part of a series of changes to support custom mojom serialization:

  1. https://codereview.chromium.org/1515423002
  2. https://codereview.chromium.org/1517043004
  3. https://codereview.chromium.org/1524693002
  4. https://codereview.chromium.org/1520153002
  5. https://codereview.chromium.org/1524613002
  6. This CL
  7. https://codereview.chromium.org/1524703002

BUG=569669
TBR=jam@chromium.org for DEPS +ipc/ipc_param_traits.h

Committed: https://crrev.com/2312b0ae91169d466ed79311698cf2f2eedf7c73
Cr-Commit-Position: refs/heads/master@{#365847}
==========

[commit-bot: I haz the power, 2015-12-17 17:46:26]

Patchset 12 (id:??) landed as
https://crrev.com/2312b0ae91169d466ed79311698cf2f2eedf7c73
Cr-Commit-Position: refs/heads/master@{#365847}