* Change C++ serialization/deserialization to not be move-only operations (with the except of |Ha…

* Change C++ serialization/deserialization to not be move-only operations   (with the except of |Handle|, which is move-only). * Change deserialization of structs and unions to take in a pre-initialized   struct instead of initializing it inside Deserialize_().

This is all related to issue #419.

Details on what this change has:
* Generated Serialize_(), SerializeArray_(), and related functions operate on
  pointers to their types, since these types are move-only.
* Generated [De]Serialize_() for mojom structs and unions don't operate on |StructPtr|, but
  instead on |Struct*|.  This is also true for Serialize[Array|Map]_().
* |Map| will uses iterator_util.h to help serialize its keys and values, instead
  of constructing an |Array| by moving its elements and calling
  SerializeArray_().

R=viettrungluu@chromium.org, jamesr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/1e92ff607c598220dbbb31a1e33c999c6c06a200

[viettrungluu, 2015-09-23 18:15:55]

A bunch of stuff is busted -- see the red bots.

(Also, I haven't reviewed the entire change yet.)

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/array_serialization.h (right):

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/array_serialization.h:35: // To avoid a circular
dependency on map_serialization.h, we forward declare:
Maybe we should have a map_serialization_forward.h?

(Possibly this applies to other things which we have forward declarations for.)

Side point: I wonder if we should have the trailing _, for things that are
"exposed" (and if they're not exposed, maybe they should be in the |internal|
namespace).

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/array_serialization.h:59: // ArraySerializer
template contains static methods for serializing |Array|s of
*The* ArraySerializer template ...

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/array_serialization.h:197: static void
SerializeElements(Array<ScopedHandleBase<H>>* input,
Do you really need this? (Not having it would also allow you to keep the
"SerializeElements" name for the one that takes an iterator.)

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/array_serialization.h:216: // since you cannot
specialize a struct within a class definition.
Are you allowed to specialize static member functions?

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/control_message_handler.cc (right):

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/control_message_handler.cc:54: size_t size =
GetSerializedSize_(*response_params_ptr.get());
Doesn't |*response_params_ptr| work?

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/control_message_proxy.cc (right):

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/control_message_proxy.cc:51: size_t size =
GetSerializedSize_(*params_ptr.get());
... (etc.)

[vardhan, 2015-09-23 22:07:00]

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/array_serialization.h (right):

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/array_serialization.h:35: // To avoid a circular
dependency on map_serialization.h, we forward declare:
On 2015/09/23 18:15:55, viettrungluu wrote:
> Maybe we should have a map_serialization_forward.h?
> 
> (Possibly this applies to other things which we have forward declarations
for.)
> 
> Side point: I wonder if we should have the trailing _, for things that are
> "exposed" (and if they're not exposed, maybe they should be in the |internal|
> namespace).

I think SerializeMap_ should be internal (not available to mojo apps to use), 
because SerializeMap_ is not enough to serialize a map and write it to disk --
you also need to encode handles and pointers.

But  if we had a public version and our internal version, I think having a
mojo::SerializeMap() version and an internal mojo::internal::SerializeMap() is a
little unambiguous, so I was thinking we should keep the trailing '_'?

LMK what you think, I'll do a follow up CL for this.

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/array_serialization.h:59: // ArraySerializer
template contains static methods for serializing |Array|s of
On 2015/09/23 18:15:55, viettrungluu wrote:
> *The* ArraySerializer template ...

Done.

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/array_serialization.h:197: static void
SerializeElements(Array<ScopedHandleBase<H>>* input,
On 2015/09/23 18:15:55, viettrungluu wrote:
> Do you really need this? (Not having it would also allow you to keep the
> "SerializeElements" name for the one that takes an iterator.)

The only reason we have both SerializeElements and SerializeIterableElements is
so that SerializeElements can memcpy() Array<POD type> (minus bool) since it's
just a vector instead of having to work with an iterator (see line 114)

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/array_serialization.h:216: // since you cannot
specialize a struct within a class definition.
On 2015/09/23 18:15:55, viettrungluu wrote:
> Are you allowed to specialize static member functions?

Yeah, but explicitly specialized definitions of a member function needs to live
outside of the struct definition still.  Most of this trouble is because I'm
fully specializing for String (ie SerializeCaller<String>)

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/control_message_handler.cc (right):

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/control_message_handler.cc:54: size_t size =
GetSerializedSize_(*response_params_ptr.get());
On 2015/09/23 18:15:55, viettrungluu wrote:
> Doesn't |*response_params_ptr| work?

You're right, it does!  Done.

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/control_message_proxy.cc (right):

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/control_message_proxy.cc:51: size_t size =
GetSerializedSize_(*params_ptr.get());
On 2015/09/23 18:15:55, viettrungluu wrote:
> ... (etc.)

Done.

[vardhan, 2015-09-23 22:12:16]

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/array_serialization.h (right):

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/array_serialization.h:35: // To avoid a circular
dependency on map_serialization.h, we forward declare:
On 2015/09/23 22:07:00, vardhan wrote:
> On 2015/09/23 18:15:55, viettrungluu wrote:
> > Maybe we should have a map_serialization_forward.h?
> > 
> > (Possibly this applies to other things which we have forward declarations
> for.)
> > 
> > Side point: I wonder if we should have the trailing _, for things that are
> > "exposed" (and if they're not exposed, maybe they should be in the
|internal|
> > namespace).
> 
> I think SerializeMap_ should be internal (not available to mojo apps to use), 
> because SerializeMap_ is not enough to serialize a map and write it to disk --
> you also need to encode handles and pointers.
> 
> But  if we had a public version and our internal version, I think having a
> mojo::SerializeMap() version and an internal mojo::internal::SerializeMap() is
a
> little unambiguous, so I was thinking we should keep the trailing '_'?
> 
> LMK what you think, I'll do a follow up CL for this.

Sorry, I meant its a little ambigious*

[viettrungluu, 2015-09-23 23:20:24]

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/array_serialization.h (right):

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/array_serialization.h:35: // To avoid a circular
dependency on map_serialization.h, we forward declare:
On 2015/09/23 22:07:00, vardhan wrote:
> On 2015/09/23 18:15:55, viettrungluu wrote:
> > Maybe we should have a map_serialization_forward.h?
> > 
> > (Possibly this applies to other things which we have forward declarations
> for.)
> > 
> > Side point: I wonder if we should have the trailing _, for things that are
> > "exposed" (and if they're not exposed, maybe they should be in the
|internal|
> > namespace).
> 
> I think SerializeMap_ should be internal (not available to mojo apps to use), 
> because SerializeMap_ is not enough to serialize a map and write it to disk --
> you also need to encode handles and pointers.
> 
> But  if we had a public version and our internal version, I think having a
> mojo::SerializeMap() version and an internal mojo::internal::SerializeMap() is
a
> little unambiguous, so I was thinking we should keep the trailing '_'?
> 
> LMK what you think, I'll do a follow up CL for this.

Probably it should be moved to internal:: (without the trailing _).

(The only reason for trailing _s, is when we generate stuff in someone else's
namespace, or similarly generating members/methods in a class whose
members/methods are controlled by someone else.)

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/array_serialization.h:197: static void
SerializeElements(Array<ScopedHandleBase<H>>* input,
On 2015/09/23 22:06:59, vardhan wrote:
> On 2015/09/23 18:15:55, viettrungluu wrote:
> > Do you really need this? (Not having it would also allow you to keep the
> > "SerializeElements" name for the one that takes an iterator.)
> 
> The only reason we have both SerializeElements and SerializeIterableElements
is
> so that SerializeElements can memcpy() Array<POD type> (minus bool) since it's
> just a vector instead of having to work with an iterator (see line 114)

Is it possible to do this via some appropriate specialization? All the extra
boilerplate is really unfortunate.

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/array_serialization.h:216: // since you cannot
specialize a struct within a class definition.
On 2015/09/23 22:06:59, vardhan wrote:
> On 2015/09/23 18:15:55, viettrungluu wrote:
> > Are you allowed to specialize static member functions?
> 
> Yeah, but explicitly specialized definitions of a member function needs to
live
> outside of the struct definition still.  Most of this trouble is because I'm
> fully specializing for String (ie SerializeCaller<String>)

Well, the definition would be declared outside, but at least you wouldn't be
dumping all of this inside the global namespace?

(And not having the extra struct would also reduce verbiage too.)

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

https://codereview.chromium.org/1358353002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:60: if (num_elements) {
This seems redundant.

https://codereview.chromium.org/1358353002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:61: for (size_t i = 0; i <
num_elements; ++i, ++it) {
nit: For consistency with other stuff in this file (and probably other files in
this directory), probably you should omit the braces for single ("physical")
line blocks.

https://codereview.chromium.org/1358353002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:76: if (input->size()) {
Assuming that output->storage() and &input->storage()[0] are valid, this is
unnecessary.

https://codereview.chromium.org/1358353002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:110: for (size_t i = 0; i <
num_elements; ++i, ++it) {
Omit {}.

(Probably do so elsewhere too.)

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

https://codereview.chromium.org/1358353002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_serialization_forward.h:8: // Headers such as
array_serialization.h can include this file to avoid circular
Nit: I'd move this above the include guard (#ifndef).

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

https://codereview.chromium.org/1358353002/diff/60001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:20: size
+= {{input_field_pattern|format(pf.field.name)}}.is_null() ? 0 :
GetSerializedSize_(*{{input_field_pattern|format(pf.field.name)}}.get());
Can you break this line in some reasonable way?

Also, is the .get() here needed?

https://codereview.chromium.org/1358353002/diff/60001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl
(right):

https://codereview.chromium.org/1358353002/diff/60001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:104:
{#- TODO:  There is a lot of overlap with struct_macros.deserialize() here,
TODO -> TODO(vardhan)

Muahahaha.

https://codereview.chromium.org/1358353002/diff/60001/services/url_response_d...
File services/url_response_disk_cache/url_response_disk_cache_db.cc (right):

https://codereview.chromium.org/1358353002/diff/60001/services/url_response_d...
services/url_response_disk_cache/url_response_disk_cache_db.cc:31: size_t size =
GetSerializedSize_(*input.get());
Is .get() needed?

[vardhan, 2015-09-25 19:33:05]

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/array_serialization.h (right):

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/array_serialization.h:35: // To avoid a circular
dependency on map_serialization.h, we forward declare:
On 2015/09/23 23:20:24, viettrungluu wrote:
> On 2015/09/23 22:07:00, vardhan wrote:
> > On 2015/09/23 18:15:55, viettrungluu wrote:
> > > Maybe we should have a map_serialization_forward.h?
> > > 
> > > (Possibly this applies to other things which we have forward declarations
> > for.)
> > > 
> > > Side point: I wonder if we should have the trailing _, for things that are
> > > "exposed" (and if they're not exposed, maybe they should be in the
> |internal|
> > > namespace).
> > 
> > I think SerializeMap_ should be internal (not available to mojo apps to
use), 
> > because SerializeMap_ is not enough to serialize a map and write it to disk
--
> > you also need to encode handles and pointers.
> > 
> > But  if we had a public version and our internal version, I think having a
> > mojo::SerializeMap() version and an internal mojo::internal::SerializeMap()
is
> a
> > little unambiguous, so I was thinking we should keep the trailing '_'?
> > 
> > LMK what you think, I'll do a follow up CL for this.
> 
> Probably it should be moved to internal:: (without the trailing _).
> 
> (The only reason for trailing _s, is when we generate stuff in someone else's
> namespace, or similarly generating members/methods in a class whose
> members/methods are controlled by someone else.)

Another thought is that I would have to leave Deserialize_ (with the
underscore), which creates an underscore inconsistency..

Would you mind if i left this renaming to another CL?

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/array_serialization.h:197: static void
SerializeElements(Array<ScopedHandleBase<H>>* input,
On 2015/09/23 23:20:24, viettrungluu wrote:
> On 2015/09/23 22:06:59, vardhan wrote:
> > On 2015/09/23 18:15:55, viettrungluu wrote:
> > > Do you really need this? (Not having it would also allow you to keep the
> > > "SerializeElements" name for the one that takes an iterator.)
> > 
> > The only reason we have both SerializeElements and SerializeIterableElements
> is
> > so that SerializeElements can memcpy() Array<POD type> (minus bool) since
it's
> > just a vector instead of having to work with an iterator (see line 114)
> 
> Is it possible to do this via some appropriate specialization? All the extra
> boilerplate is really unfortunate.

Done :(

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/array_serialization.h:216: // since you cannot
specialize a struct within a class definition.
On 2015/09/23 23:20:24, viettrungluu wrote:
> On 2015/09/23 22:06:59, vardhan wrote:
> > On 2015/09/23 18:15:55, viettrungluu wrote:
> > > Are you allowed to specialize static member functions?
> > 
> > Yeah, but explicitly specialized definitions of a member function needs to
> live
> > outside of the struct definition still.  Most of this trouble is because I'm
> > fully specializing for String (ie SerializeCaller<String>)
> 
> Well, the definition would be declared outside, but at least you wouldn't be
> dumping all of this inside the global namespace?
> 
> (And not having the extra struct would also reduce verbiage too.)

Done.

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

https://codereview.chromium.org/1358353002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:60: if (num_elements) {
On 2015/09/23 23:20:24, viettrungluu wrote:
> This seems redundant.

oops

https://codereview.chromium.org/1358353002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:61: for (size_t i = 0; i <
num_elements; ++i, ++it) {
On 2015/09/23 23:20:24, viettrungluu wrote:
> nit: For consistency with other stuff in this file (and probably other files
in
> this directory), probably you should omit the braces for single ("physical")
> line blocks.

Done.

https://codereview.chromium.org/1358353002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:76: if (input->size()) {
On 2015/09/23 23:20:24, viettrungluu wrote:
> Assuming that output->storage() and &input->storage()[0] are valid, this is
> unnecessary.

output->storage() could point to memory we shouldn't access if the array size is
0 (see Array_Data::storage() in array_internal.h, it's whack)

https://codereview.chromium.org/1358353002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:110: for (size_t i = 0; i <
num_elements; ++i, ++it) {
On 2015/09/23 23:20:24, viettrungluu wrote:
> Omit {}.
> 
> (Probably do so elsewhere too.)

Done.

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

https://codereview.chromium.org/1358353002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_serialization_forward.h:8: // Headers such as
array_serialization.h can include this file to avoid circular
On 2015/09/23 23:20:24, viettrungluu wrote:
> Nit: I'd move this above the include guard (#ifndef).

Done.

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

https://codereview.chromium.org/1358353002/diff/60001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:20: size
+= {{input_field_pattern|format(pf.field.name)}}.is_null() ? 0 :
GetSerializedSize_(*{{input_field_pattern|format(pf.field.name)}}.get());
On 2015/09/23 23:20:24, viettrungluu wrote:
> Can you break this line in some reasonable way?
> 
> Also, is the .get() here needed?

Done.

https://codereview.chromium.org/1358353002/diff/60001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl
(right):

https://codereview.chromium.org/1358353002/diff/60001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:104:
{#- TODO:  There is a lot of overlap with struct_macros.deserialize() here,
On 2015/09/23 23:20:24, viettrungluu wrote:
> TODO -> TODO(vardhan)
> 
> Muahahaha.

Done :(

https://codereview.chromium.org/1358353002/diff/60001/services/url_response_d...
File services/url_response_disk_cache/url_response_disk_cache_db.cc (right):

https://codereview.chromium.org/1358353002/diff/60001/services/url_response_d...
services/url_response_disk_cache/url_response_disk_cache_db.cc:31: size_t size =
GetSerializedSize_(*input.get());
On 2015/09/23 23:20:24, viettrungluu wrote:
> Is .get() needed?

Done.

[viettrungluu, 2015-09-25 20:08:08]

lgtm w/nits, you break it you buy it

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/array_serialization.h (right):

https://codereview.chromium.org/1358353002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/array_serialization.h:35: // To avoid a circular
dependency on map_serialization.h, we forward declare:
On 2015/09/25 19:33:05, vardhan wrote:
> On 2015/09/23 23:20:24, viettrungluu wrote:
> > On 2015/09/23 22:07:00, vardhan wrote:
> > > On 2015/09/23 18:15:55, viettrungluu wrote:
> > > > Maybe we should have a map_serialization_forward.h?
> > > > 
> > > > (Possibly this applies to other things which we have forward
declarations
> > > for.)
> > > > 
> > > > Side point: I wonder if we should have the trailing _, for things that
are
> > > > "exposed" (and if they're not exposed, maybe they should be in the
> > |internal|
> > > > namespace).
> > > 
> > > I think SerializeMap_ should be internal (not available to mojo apps to
> use), 
> > > because SerializeMap_ is not enough to serialize a map and write it to
disk
> --
> > > you also need to encode handles and pointers.
> > > 
> > > But  if we had a public version and our internal version, I think having a
> > > mojo::SerializeMap() version and an internal
mojo::internal::SerializeMap()
> is
> > a
> > > little unambiguous, so I was thinking we should keep the trailing '_'?
> > > 
> > > LMK what you think, I'll do a follow up CL for this.
> > 
> > Probably it should be moved to internal:: (without the trailing _).
> > 
> > (The only reason for trailing _s, is when we generate stuff in someone
else's
> > namespace, or similarly generating members/methods in a class whose
> > members/methods are controlled by someone else.)
> 
> Another thought is that I would have to leave Deserialize_ (with the
> underscore), which creates an underscore inconsistency..
> 
> Would you mind if i left this renaming to another CL?

OK

https://codereview.chromium.org/1358353002/diff/70024/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/array_serialization.h (right):

https://codereview.chromium.org/1358353002/diff/70024/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:72: if (num_elements) {
nit: omit {}

https://codereview.chromium.org/1358353002/diff/70024/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:204: // serialize
nit: please re-wrap

https://codereview.chromium.org/1358353002/diff/70024/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:298:
SerializeUnion_((*it).get(), buf, &result, true);
nit: it->get() instead of (*it).get()?

https://codereview.chromium.org/1358353002/diff/70024/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:337: if (input && *input) {
Is input ever allowed to be null?

Maybe you should just MOJO_DCHECK(input)?

https://codereview.chromium.org/1358353002/diff/70024/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/map_serialization_forward.h (right):

https://codereview.chromium.org/1358353002/diff/70024/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_serialization_forward.h:7: #ifndef
MOJO_PUBLIC_CPP_BINDINGS_LIB_MAP_SERIALIZATION_FORWARD_H_
nit: add blank line above

[vardhan, 2015-09-28 21:55:44]

https://codereview.chromium.org/1358353002/diff/70024/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/array_serialization.h (right):

https://codereview.chromium.org/1358353002/diff/70024/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:72: if (num_elements) {
On 2015/09/25 20:08:08, viettrungluu wrote:
> nit: omit {}

Done.

https://codereview.chromium.org/1358353002/diff/70024/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:204: // serialize
On 2015/09/25 20:08:08, viettrungluu wrote:
> nit: please re-wrap

Done.

https://codereview.chromium.org/1358353002/diff/70024/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:298:
SerializeUnion_((*it).get(), buf, &result, true);
On 2015/09/25 20:08:08, viettrungluu wrote:
> nit: it->get() instead of (*it).get()?

Done.

https://codereview.chromium.org/1358353002/diff/70024/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:337: if (input && *input) {
On 2015/09/25 20:08:08, viettrungluu wrote:
> Is input ever allowed to be null?
> 
> Maybe you should just MOJO_DCHECK(input)?

Doesn't look like it'll ever be null if it goes through the rest of the bindings
code, unless the caller purposely passes in null.  I'll make it a MOJO_DCHECK.

https://codereview.chromium.org/1358353002/diff/70024/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/map_serialization_forward.h (right):

https://codereview.chromium.org/1358353002/diff/70024/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_serialization_forward.h:7: #ifndef
MOJO_PUBLIC_CPP_BINDINGS_LIB_MAP_SERIALIZATION_FORWARD_H_
On 2015/09/25 20:08:08, viettrungluu wrote:
> nit: add blank line above

Done.

[vardhan, 2015-09-28 21:57:26]

Committed patchset #6 (id:90001) manually as
1e92ff607c598220dbbb31a1e33c999c6c06a200 (presubmit successful).