Extending ONC validator's logging. Completing toplevel validation.

chrome/browser/chromeos/network_settings/onc_mapper.h

Index: chrome/browser/chromeos/network_settings/onc_mapper.h
diff --git a/chrome/browser/chromeos/network_settings/onc_mapper.h b/chrome/browser/chromeos/network_settings/onc_mapper.h
index 0f6705b1a237d1f9f34b844262d896ab2d57b521..465063e1aa18fc646f7ee47d0c20091a7fa45412 100644
--- a/chrome/browser/chromeos/network_settings/onc_mapper.h
+++ b/chrome/browser/chromeos/network_settings/onc_mapper.h
@@ -40,58 +40,66 @@ class Mapper {
 
  protected:
   // Calls |MapObject|, |MapArray| and |MapPrimitive| according to |onc_value|'s
-  // type. By default aborts on nested errors in arrays. Result of the mapping
-  // is returned. On error returns NULL.
-  virtual scoped_ptr<base::Value> MapValue(
-    const OncValueSignature& signature,
-    const base::Value& onc_value);
+  // type, which always return an object of the according type. Result of the
+  // mapping is returned. On error sets |error| to true.
+  virtual scoped_ptr<base::Value> MapValue(const OncValueSignature& signature,
+                                           const base::Value& onc_value,
+                                           bool* error);
 
   // Maps objects/dictionaries. By default calls |MapFields|, which recurses
-  // into each field of |onc_object|, and aborts on unknown fields. Result of
-  // the mapping is returned. On error returns NULL.
+  // into each field of |onc_object|, and drops unknown fields. Result of the
+  // mapping is returned. On error sets |error| to true. In this implementation
+  // only unknown fields are errors.
   virtual scoped_ptr<base::DictionaryValue> MapObject(
       const OncValueSignature& signature,
-      const base::DictionaryValue& onc_object);
+      const base::DictionaryValue& onc_object,
+      bool* error);

[Greg Spencer (Chromium), 2012/12/10 22:59:01]

Seems kind of backwards to return the scoped_ptr, and take a bool* to get the
success/failure.  Would it make sense to pass in a base::DictionaryValue*
instead and return a bool?  The ownership model in the classic calling
convention (passing in the pointer) is pretty well defined, and doesn't need a
scoped_ptr then.

What was the reason to change from returning NULL for failure?  That seemed a
lot simpler...

[pneubeck (no reviews), 2012/12/11 08:25:22]

Here, the |error| argument indicates something different than the return value.
Before this patch, I was just not consistent and added the |error| flag only to
a few methods.

The idea was that the Mapper class doesn't know about what a derived class
considers to be a fatal error. E.g. if we find an unknown field then we might
either see that as an fatal error (e.g. strict validation) or ignore the problem
(e.g. liberal validation).

Thus, the scheme I followed here is: |error| is set to true if anywhere in this
subtree an error occurred. The return value is a "repaired" object (with
problematic fields removed).

I think, the choice return object/error argument is fine here because the
primary usage of this function is the mapping and the resulting object. The
|error| value is an additional detail.

This usage is also similar to what was discussed on chromium-dev a while ago.
The seemed to prefer
  scoped_ptr<X> MyFunction(/* optional outparam for detailed error code */);
over
  RetValue MyFunction(scoped_ptr<X>* p);

https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/scoped...

On 2012/12/10 22:59:01, Greg Spencer (Chromium) wrote:

 
   // Maps primitive values like BinaryValue, StringValue, IntegerValue... (all
   // but dictionaries and lists). By default copies |onc_primitive|. Result of
-  // the mapping is returned. On error returns NULL.
+  // the mapping is returned. On error sets |error| to true.
   virtual scoped_ptr<base::Value> MapPrimitive(
-    const OncValueSignature& signature,
-    const base::Value& onc_primitive);
-
-  // Maps each field of the given |onc_object| according to
-  // |object_signature|. Adds the mapping of each field to |result| using
-  // |MapField| and drops unknown fields by default. Sets
-  // |found_unknown_field| to true if this dictionary contains any unknown
-  // fields. Set |nested_error_occured| to true if nested errors occured.
-  virtual void MapFields(
-      const OncValueSignature& object_signature,
-      const base::DictionaryValue& onc_object,
-      bool* found_unknown_field,
-      bool* nested_error_occured,
-      base::DictionaryValue* result);
+      const OncValueSignature& signature,
+      const base::Value& onc_primitive,
+      bool* error);
+
+  // Maps each field of the given |onc_object| according to |object_signature|.
+  // Adds the mapping of each field to |result| using |MapField| and drops
+  // unknown fields by default. Sets |found_unknown_field| to true if this
+  // dictionary contains any unknown fields. Set |nested_error| to true if
+  // nested errors occured.
+  virtual void MapFields(const OncValueSignature& object_signature,
+                         const base::DictionaryValue& onc_object,
+                         bool* found_unknown_field,
+                         bool* nested_error,
+                         base::DictionaryValue* result);
 
   // Maps the value |onc_value| of field |field_name| according to its field
   // signature in |object_signature| using |MapValue|. Sets
-  // |found_unknown_field| to true if |field_name| cannot be found in
-  // |object_signature|, which by default is an error. Result of the mapping is
-  // returned. On error returns NULL.
+  // |found_unknown_field| to true and returns NULL if |field_name| cannot be
+  // found in |object_signature|. Otherwise returns the mapping of |onc_value|.
   virtual scoped_ptr<base::Value> MapField(
       const std::string& field_name,
       const OncValueSignature& object_signature,
       const base::Value& onc_value,
-      bool* found_unknown_field);
+      bool* found_unknown_field,
+      bool* error);
 
   // Maps the array |onc_array| according to |array_signature|, which defines
   // the type of the entries. Maps each entry by calling |MapValue|. If any of
-  // the nested mappings failed, the flag |nested_error_occured| is set to true
-  // and the entry is dropped from the result. The resulting array is
-  // returned. On error returns NULL.
+  // the nested mappings failed, the flag |nested_error| is set to true and the
+  // entry is dropped from the result. The resulting array is returned.
   virtual scoped_ptr<base::ListValue> MapArray(
       const OncValueSignature& array_signature,
       const base::ListValue& onc_array,
-      bool* nested_error_occured);
+      bool* nested_error);
+
+  // Calls |MapValue| and returns its result. Called by |MapArray| for each
+  // entry and its index in the enclosing array.
+  virtual scoped_ptr<base::Value> MapEntry(int index,
+                                           const OncValueSignature& signature,
+                                           const base::Value& onc_value,
+                                           bool* error);
 
  private:
   DISALLOW_COPY_AND_ASSIGN(Mapper);