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

mojo/public/cpp/bindings/lib/map_serialization.h

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef MOJO_PUBLIC_CPP_BINDINGS_LIB_MAP_SERIALIZATION_H_
 #define MOJO_PUBLIC_CPP_BINDINGS_LIB_MAP_SERIALIZATION_H_
 
 #include "mojo/public/cpp/bindings/lib/array_internal.h"
 #include "mojo/public/cpp/bindings/lib/array_serialization.h"
 #include "mojo/public/cpp/bindings/lib/bindings_internal.h"
@@ skipping 110 matching lines @@
          value_data_size;
 }
 
 // We don't need an ArrayValidateParams instance for key validation since
 // we can deduce it from the Key type. (which can only be primitive types or
 // non-nullable strings.)
 template <typename MapKey,
           typename MapValue,
           typename DataKey,
           typename DataValue>
-inline void SerializeMap_(
+inline internal::ValidationError SerializeMap_(
     Map<MapKey, MapValue>* input,
     internal::Buffer* buf,
     internal::Map_Data<DataKey, DataValue>** output,
     const internal::ArrayValidateParams* value_validate_params) {
   if (input && *input) {
     internal::Map_Data<DataKey, DataValue>* result =

[viettrungluu, 2015/10/06 16:40:06]

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).

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

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.

         internal::Map_Data<DataKey, DataValue>::New(buf);
     internal::Array_Data<DataKey>* keys_data =
         internal::Array_Data<DataKey>::New(input->size(), buf);
 
     if (result && keys_data) {
       result->keys.ptr = keys_data;
 
       // We *must* serialize the keys before we allocate an Array_Data for the
       // values.
       internal::MapKeyIterator<MapKey, MapValue> key_iter(input);
       const internal::ArrayValidateParams* key_validate_params =
           internal::MapKeyValidateParamsFactory<DataKey>::Get();
 
-      internal::ArraySerializer<MapKey, DataKey>::SerializeElements(
-          key_iter.begin(), input->size(), buf, result->keys.ptr,
-          key_validate_params);
+      {
+        auto retval =
+            internal::ArraySerializer<MapKey, DataKey>::SerializeElements(
+                key_iter.begin(), input->size(), buf, result->keys.ptr,
+                key_validate_params);
+        if (retval != internal::ValidationError::VALIDATION_ERROR_NONE)
+          return retval;

[viettrungluu, 2015/10/06 16:40:06]

Are you supposed to set *output in this case? (Ditto below.)

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

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

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.

+      }
 
       // Now we try allocate an Array_Data for the values
       internal::Array_Data<DataValue>* values_data =
           internal::Array_Data<DataValue>::New(input->size(), buf);
       if (values_data) {
         result->values.ptr = values_data;
         internal::MapValueIterator<MapKey, MapValue> value_iter(input);
-        internal::ArraySerializer<MapValue, DataValue>::SerializeElements(
-            value_iter.begin(), input->size(), buf, result->values.ptr,
-            value_validate_params);
+        {

[viettrungluu, 2015/10/06 16:40:06]

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.

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

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..
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.

+          auto retval =
+              internal::ArraySerializer<MapValue, DataValue>::SerializeElements(
+                  value_iter.begin(), input->size(), buf, result->values.ptr,
+                  value_validate_params);
+          if (retval != internal::ValidationError::VALIDATION_ERROR_NONE)
+            return retval;
+        }
       }
     }
     *output = result;
   } else {
     *output = nullptr;
   }
+  return internal::ValidationError::VALIDATION_ERROR_NONE;
 }
 
 template <typename MapKey,
           typename MapValue,
           typename DataKey,
           typename DataValue>
 inline void Deserialize_(internal::Map_Data<DataKey, DataValue>* input,
                          Map<MapKey, MapValue>* output) {
   if (input) {
     Array<MapKey> keys;
     Array<MapValue> values;
 
     Deserialize_(input->keys.ptr, &keys);
     Deserialize_(input->values.ptr, &values);
 
     *output = Map<MapKey, MapValue>(keys.Pass(), values.Pass());
   } else {
     output->reset();
   }
 }
 
 }  // namespace mojo
 
 #endif  // MOJO_PUBLIC_CPP_BINDINGS_LIB_MAP_SERIALIZATION_H_