mojo::Serialize*_() calls now propogate/return validation errors.

mojo::Serialize*_() calls now propogate/return validation errors.

The public-facing API can use these, but will probably want to use a separate enum of errors (since ValidationError is namespace'd as internal, and should probably remain so).  For now, a public serialization API can return a boolean.

R=viettrungluu@chromium.org
CC=jamesr@chromium.org
BUG=

Committed: https://chromium.googlesource.com/external/mojo/+/b993b3954683f57ef6eab999270509b47d40d1d9

[vardhan, 2015-10-06 00:22:49]

vardhan@google.com changed reviewers:
+ jamesr@chromium.org

[viettrungluu, 2015-10-06 16:40:07]

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

https://codereview.chromium.org/1387993002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/array_serialization.h:61: return
ValidationError::VALIDATION_ERROR_NONE;
Since ValidationError is (not yet!) a class enum, the ValidationError:: is
redundant.

On the other hand, feel free to change it to a class enum, in which case the
value names should be named like NONE (hence ValidationError::NONE) instead of
VALIDATION_ERROR_NONE.

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

https://codereview.chromium.org/1387993002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/map_serialization.h:137:
internal::Map_Data<DataKey, DataValue>* result =
Drive-by: When does the New() fail? Is this an error case? Do we really support
this ("this" = New() failing)?

It also seems very odd to return "no validation error" (even if it's strictly
true).

https://codereview.chromium.org/1387993002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/map_serialization.h:157: return retval;
Are you supposed to set *output in this case? (Ditto below.)

(What exactly is the contract for this function on error?)

https://codereview.chromium.org/1387993002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/map_serialization.h:166: {
These braces seem redundant.

But more generally, I think you should restructure this function to be cleaner
(maybe with some helper functions).

The way errors are handled in this function seem bad.

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

https://codereview.chromium.org/1387993002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/validation_errors.h:120: << description << ").";
Drive-by: It's rather odd that this macro ends with a ';'.

https://codereview.chromium.org/1387993002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/validation_errors.h:124: #define
MOJO_INTERNAL_SERIALIZATION_CHECK_AND_RETURN(condition, return_code,  \
(This is not a well-written macro, for a number of reasons[1][2].)

[1] People will probably use it with a trailing semicolon, in which case there
ends up being an extra semicolon.

This may seem like a trivial point, but it breaks expectations if used in, e.g.:

if (something)
  MOJO_INTERNAL_SERIALIZATION_CHECK_AND_RETURN(...);
else
  ...;

[2] The following is "valid":

MOJO_INTERNAL_SERIALIZATION_CHECK_AND_RETURN(...)
else
  ...;

This may seem unlikely, but removing the semicolon in the example in [1] makes
it compile, but do the wrong thing!

https://codereview.chromium.org/1387993002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/validation_errors.h:131: return return_code;       
                                               \
But in general, I don't think it's a good idea to have a |return| inside a
macro.

I suggest either living with some redundancy[1] (for the sake of clarity) or
factoring out the MOJO_DLOG_IF in MOJO_INTERNAL_DLOG_SERIALIZATION_WARNING() and
using it[2].

[1]:

MOJO_INTERNAL_DLOG_SERIALIZATION_WARNING(my condition, ...);
if (my condition)
  return return_code;

[2]:

if (my condition) {
  MOJO_INTERNAL_WHATEVER_YOU_CALL_IT(...);
  return return_code;
}

where MOJO_INTERNAL_WHATEVER_YOU_CALL_IT() is:

MOJO_DLOG_IF(FATAL, !ReportSerializationWarning(error)) << ...

In which case you should probably rewrite
MOJO_INTERNAL_SERIALIZATION_CHECK_AND_RETURN() as:

do { \
  if (condition) \
    MOJO_INTERNAL_WHATEVER_YOU_CALL_IT(...); \
} while (false)

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

https://codereview.chromium.org/1387993002/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:29: {# A
private macro that prints the C++ log-and-report macro, or test-and-return
Why do we need to generate two versions?

[vardhan, 2015-10-06 22:57:14]

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

https://codereview.chromium.org/1387993002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/array_serialization.h:61: return
ValidationError::VALIDATION_ERROR_NONE;
On 2015/10/06 at 16:40:06, viettrungluu wrote:
> Since ValidationError is (not yet!) a class enum, the ValidationError:: is
redundant.
> 
> On the other hand, feel free to change it to a class enum, in which case the
value names should be named like NONE (hence ValidationError::NONE) instead of
VALIDATION_ERROR_NONE.

That was my plan, but since ValidationError is used in bunch of other files too,
I was thinking of doing a follow up CL to fix those.  I'll do the rename too.

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

https://codereview.chromium.org/1387993002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/map_serialization.h:137:
internal::Map_Data<DataKey, DataValue>* result =
On 2015/10/06 at 16:40:06, viettrungluu wrote:
> Drive-by: When does the New() fail? Is this an error case? Do we really
support this ("this" = New() failing)?
> 
> It also seems very odd to return "no validation error" (even if it's strictly
true).

New() currently check-fails if buf->Allocate() does not have enough space to
allocate, so there are no other errors.  I'll remove the extra checks.

https://codereview.chromium.org/1387993002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/map_serialization.h:157: return retval;
On 2015/10/06 at 16:40:06, viettrungluu wrote:
> Are you supposed to set *output in this case? (Ditto below.)
> (What exactly is the contract for this function on error?)

No, not in this case -- setting "*output = nullptr" needs to happen if the map
we're serializing |is_null()| (and is also nullable), in which case the
serialization is successful.

SerializeMap_ delegates almost all of its logic to serializing two arrays, where
the actual data will lie, so those errors are propogated back.  I can't see any
other validation-related errors here.

https://codereview.chromium.org/1387993002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/map_serialization.h:166: {
On 2015/10/06 at 16:40:06, viettrungluu wrote:
> These braces seem redundant.

This way, I could reuse 'retval' as the name for the return value for keys and
values.  I thought it looked cleaner (and delineated which was serializing keys
and which was values).  But removing..

> But more generally, I think you should restructure this function to be cleaner
(maybe with some helper functions).
> 
> The way errors are handled in this function seem bad.

I removed the unnecessary '::New()' checking that flattened this function out a
bit more, PTAL.

There's not much happening here other than preparing the params for calling
|SerializeELements| and propogating any returned errors.. But i have no problem
doing it if you still think this version looks messy.

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

https://codereview.chromium.org/1387993002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/validation_errors.h:120: << description << ").";
On 2015/10/06 at 16:40:06, viettrungluu wrote:
> Drive-by: It's rather odd that this macro ends with a ';'.

Done.

https://codereview.chromium.org/1387993002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/validation_errors.h:124: #define
MOJO_INTERNAL_SERIALIZATION_CHECK_AND_RETURN(condition, return_code,  \
On 2015/10/06 at 16:40:06, viettrungluu wrote:
> (This is not a well-written macro, for a number of reasons[1][2].)
> 
> [1] People will probably use it with a trailing semicolon, in which case there
ends up being an extra semicolon.
> 
> This may seem like a trivial point, but it breaks expectations if used in,
e.g.:
> 
> if (something)
>   MOJO_INTERNAL_SERIALIZATION_CHECK_AND_RETURN(...);
> else
>   ...;
> 
> [2] The following is "valid":
> 
> MOJO_INTERNAL_SERIALIZATION_CHECK_AND_RETURN(...)
> else
>   ...;
> 
> This may seem unlikely, but removing the semicolon in the example in [1] makes
it compile, but do the wrong thing!

Ah, I didn't think about this enough, that's a good point.
But I changed the way I do this in response to your comment below.

https://codereview.chromium.org/1387993002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/validation_errors.h:131: return return_code;       
                                               \
On 2015/10/06 at 16:40:06, viettrungluu wrote:
> But in general, I don't think it's a good idea to have a |return| inside a
macro.
> 
> I suggest either living with some redundancy[1] (for the sake of clarity) or
factoring out the MOJO_DLOG_IF in MOJO_INTERNAL_DLOG_SERIALIZATION_WARNING() and
using it[2].
> 
> [1]:
> 
> MOJO_INTERNAL_DLOG_SERIALIZATION_WARNING(my condition, ...);
> if (my condition)
>   return return_code;
> 
> [2]:
> 
> if (my condition) {
>   MOJO_INTERNAL_WHATEVER_YOU_CALL_IT(...);
>   return return_code;
> }
> 
> where MOJO_INTERNAL_WHATEVER_YOU_CALL_IT() is:
> 
> MOJO_DLOG_IF(FATAL, !ReportSerializationWarning(error)) << ...
> 
> In which case you should probably rewrite
MOJO_INTERNAL_SERIALIZATION_CHECK_AND_RETURN() as:
> 
> do { \
>   if (condition) \
>     MOJO_INTERNAL_WHATEVER_YOU_CALL_IT(...); \
> } while (false)

I did something like [2]. PTAL!

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

https://codereview.chromium.org/1387993002/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:29: {# A
private macro that prints the C++ log-and-report macro, or test-and-return
On 2015/10/06 at 16:40:06, viettrungluu wrote:
> Why do we need to generate two versions?

Done. (no longer relevant after some changes based on your comments in another
file).

[viettrungluu, 2015-10-08 15:51:25]

(Still have to finish re-reviewing, but may as well send this now, since I don't
know when I'll get to that.)

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

https://codereview.chromium.org/1387993002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/array_serialization.h:61: return
ValidationError::VALIDATION_ERROR_NONE;
On 2015/10/06 22:57:14, vardhan wrote:
> On 2015/10/06 at 16:40:06, viettrungluu wrote:
> > Since ValidationError is (not yet!) a class enum, the ValidationError:: is
> redundant.
> > 
> > On the other hand, feel free to change it to a class enum, in which case the
> value names should be named like NONE (hence ValidationError::NONE) instead of
> VALIDATION_ERROR_NONE.
> 
> That was my plan, but since ValidationError is used in bunch of other files
too,
> I was thinking of doing a follow up CL to fix those.  I'll do the rename too.

OK, but for now, ValidationError::VALIDATION_ERROR_NONE is weird and redundant.
Just writing VALIDATION_ERROR_NONE will make future search-and-replace easier
too.

https://codereview.chromium.org/1387993002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/map_serialization.h (right):

https://codereview.chromium.org/1387993002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_serialization.h:136: if (!input->is_null()) {
I assume that |input| itself shouldn't be a null pointer (which the previous
code allowed).

I suggest you rewrite this as an early exit:

if (input->is_null()) {
  // In this case, |output| is just serialized as null.
  *output = nullptr;
  return VALIDATION_ERROR_NONE;
}

[vardhan, 2015-10-08 20:53:51]

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

https://codereview.chromium.org/1387993002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/array_serialization.h:61: return
ValidationError::VALIDATION_ERROR_NONE;
On 2015/10/08 at 15:51:25, viettrungluu wrote:
> On 2015/10/06 22:57:14, vardhan wrote:
> > On 2015/10/06 at 16:40:06, viettrungluu wrote:
> > > Since ValidationError is (not yet!) a class enum, the ValidationError:: is
> > redundant.
> > > 
> > > On the other hand, feel free to change it to a class enum, in which case
the
> > value names should be named like NONE (hence ValidationError::NONE) instead
of
> > VALIDATION_ERROR_NONE.
> > 
> > That was my plan, but since ValidationError is used in bunch of other files
too,
> > I was thinking of doing a follow up CL to fix those.  I'll do the rename
too.
> 
> OK, but for now, ValidationError::VALIDATION_ERROR_NONE is weird and
redundant. Just writing VALIDATION_ERROR_NONE will make future
search-and-replace easier too.

Done.

https://codereview.chromium.org/1387993002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/map_serialization.h (right):

https://codereview.chromium.org/1387993002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_serialization.h:136: if (!input->is_null()) {
On 2015/10/08 at 15:51:25, viettrungluu wrote:
> I assume that |input| itself shouldn't be a null pointer (which the previous
code allowed).
That's right, thought i'd fix it here.

> I suggest you rewrite this as an early exit:
> 
> if (input->is_null()) {
>   // In this case, |output| is just serialized as null.
>   *output = nullptr;
>   return VALIDATION_ERROR_NONE;
> }

Done.

[viettrungluu, 2015-10-09 01:35:20]

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

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:373: // It is up to the
caller to make the given |Array| is not null if it is
"is not null" -> "non-null"?

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/control_message_handler.cc (right):

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/control_message_handler.cc:59:
MOJO_DCHECK(Serialize_(response_params_ptr.get(), builder.buffer(),
This is wrong, since on a non-Debug build Serialize_() won't get called.

Either MOJO_CHECK() it, or do something like:

auto result = Serialize_(...);
MOJO_DCHECK(result == VALIDATION_ERROR_NONE);

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/control_message_proxy.cc (right):

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/control_message_proxy.cc:55:
MOJO_DCHECK(Serialize_(params_ptr.get(), builder.buffer(), &params) ==
etc.

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/map_serialization.h (right):

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_serialization.h:124: // SerializeMap_ will
return VALIDATION_ERROR_NONE on success and sets
"sets" -> "set"

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_serialization.h:179: 
nit: delete this blank line

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/validation_errors.h (right):

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_errors.h:106: // In debug build, logs a
serialization warning:
nit (pre-existing condition): "debug" -> "a debug"

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_errors.h:113: // In non-debug build,
does nothing (not even compiling |condition|).
Remove the "(not even compiling ...)" bit.

(Incidentally, that comment was wrong to begin with.)

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/array_unittest.cc (right):

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/array_unittest.cc:164:
EXPECT_EQ(mojo::internal::VALIDATION_ERROR_NONE,
nit (here and below): Probably you can skip "mojo::".

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/map_unittest.cc (right):

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/map_unittest.cc:249:
EXPECT_EQ(internal::VALIDATION_ERROR_NONE,
(Like you do here!)

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/serialization_warning_unittest.cc (right):

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/serialization_warning_unittest.cc:66:
EXPECT_EQ(expected_warning, warning_observer_.last_warning());
I wonder if we should get rid of this whole warning observer nonsense. (But we
can worry about that separately.)

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/struct_unittest.cc (right):

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/struct_unittest.cc:58:
EXPECT_EQ(mojo::internal::VALIDATION_ERROR_NONE,
(Apparently, we're very inconsistent.)

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/union_unittest.cc (right):

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/union_unittest.cc:494:
mojo::internal::FixedBufferForTesting buf(size);
mojo::

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/union_unittest.cc:495:
internal::SmallStruct_Data* data = nullptr;
No mojo::

kill me now

[vardhan, 2015-10-09 22:31:29]

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

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/array_serialization.h:373: // It is up to the
caller to make the given |Array| is not null if it is
On 2015/10/09 at 01:35:19, viettrungluu wrote:
> "is not null" -> "non-null"?

Done.  (it was supposed to be "make sure")

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/control_message_handler.cc (right):

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/control_message_handler.cc:59:
MOJO_DCHECK(Serialize_(response_params_ptr.get(), builder.buffer(),
On 2015/10/09 at 01:35:19, viettrungluu wrote:
> This is wrong, since on a non-Debug build Serialize_() won't get called.
> 
> Either MOJO_CHECK() it, or do something like:
> 
> auto result = Serialize_(...);
> MOJO_DCHECK(result == VALIDATION_ERROR_NONE);

cool, i'll do DCHECK the right way :-)

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/control_message_proxy.cc (right):

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/control_message_proxy.cc:55:
MOJO_DCHECK(Serialize_(params_ptr.get(), builder.buffer(), &params) ==
On 2015/10/09 at 01:35:19, viettrungluu wrote:
> etc.

Done.

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/map_serialization.h (right):

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_serialization.h:124: // SerializeMap_ will
return VALIDATION_ERROR_NONE on success and sets
On 2015/10/09 at 01:35:19, viettrungluu wrote:
> "sets" -> "set"

Done.

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_serialization.h:179: 
On 2015/10/09 at 01:35:19, viettrungluu wrote:
> nit: delete this blank line

Done.

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/validation_errors.h (right):

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_errors.h:106: // In debug build, logs a
serialization warning:
On 2015/10/09 at 01:35:19, viettrungluu wrote:
> nit (pre-existing condition): "debug" -> "a debug"

Done.

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_errors.h:113: // In non-debug build,
does nothing (not even compiling |condition|).
On 2015/10/09 at 01:35:20, viettrungluu wrote:
> Remove the "(not even compiling ...)" bit.
> 
> (Incidentally, that comment was wrong to begin with.)

Removed.
(That's really weird!  Why is it DLOG? should we rename.. at some point?)

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/array_unittest.cc (right):

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/array_unittest.cc:164:
EXPECT_EQ(mojo::internal::VALIDATION_ERROR_NONE,
On 2015/10/09 at 01:35:20, viettrungluu wrote:
> nit (here and below): Probably you can skip "mojo::".

Unfortunately internal::VALIDATION_ERROR_NONE conflicts with
mojo::test::internal so I can't (i get a compiler error)

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/map_unittest.cc (right):

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/map_unittest.cc:249:
EXPECT_EQ(internal::VALIDATION_ERROR_NONE,
On 2015/10/09 at 01:35:20, viettrungluu wrote:
> (Like you do here!)

Because there's no conflict with test::internal here :-)

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/serialization_warning_unittest.cc (right):

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/serialization_warning_unittest.cc:66:
EXPECT_EQ(expected_warning, warning_observer_.last_warning());
On 2015/10/09 at 01:35:20, viettrungluu wrote:
> I wonder if we should get rid of this whole warning observer nonsense. (But we
can worry about that separately.)

I was thinking about that..

It's normally a FATAL log on debug builds if the reporting is "turned off" (i.e.
outside of unittests), so I'll make it a WARNING now.  I'll just do it here
since it's very relevant? :-)

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/struct_unittest.cc (right):

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/struct_unittest.cc:58:
EXPECT_EQ(mojo::internal::VALIDATION_ERROR_NONE,
On 2015/10/09 at 01:35:20, viettrungluu wrote:
> (Apparently, we're very inconsistent.)

conflicts mojo::test::internal. (it's also why other things in this file, like
FixedBufferForTesting, are scoped as |mojo::internal| instead of just
|internal|)

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/tests/union_unittest.cc (right):

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/union_unittest.cc:494:
mojo::internal::FixedBufferForTesting buf(size);
On 2015/10/09 at 01:35:20, viettrungluu wrote:
> mojo::

(See comments regarding this in other files)

https://codereview.chromium.org/1387993002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/tests/union_unittest.cc:495:
internal::SmallStruct_Data* data = nullptr;
On 2015/10/09 at 01:35:20, viettrungluu wrote:
> No mojo::
> 
> kill me now

:-)

(this |internal| is a referring to the |internal| namespace that the generated
_Data mojom classes are in)

[viettrungluu, 2015-10-09 23:32:06]

lgtm

[vardhan, 2015-10-09 23:48:44]

Committed patchset #4 (id:60001) manually as
b993b3954683f57ef6eab999270509b47d40d1d9 (presubmit successful).