Add optional schema compiler error messages

tools/json_schema_compiler/h_generator.py

 # Copyright (c) 2012 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.
 
 from code import Code
 from model import PropertyType, Type
 import cpp_util
 import schema_util
 
 class HGenerator(object):
   def __init__(self, type_generator, cpp_namespace):
     self._type_generator = type_generator
     self._cpp_namespace = cpp_namespace
 
   def Generate(self, namespace):
     return _Generator(namespace,
                       self._type_generator,
                       self._cpp_namespace).Generate()
 
 class _Generator(object):
   """A .h generator for a namespace.
   """
   def __init__(self, namespace, cpp_type_generator, cpp_namespace):
     self._namespace = namespace
     self._type_helper = cpp_type_generator
     self._cpp_namespace = cpp_namespace
     self._target_namespace = (
         self._type_helper.GetCppNamespaceName(self._namespace))
+    self._error_parameter = ''
+    if self._namespace.generate_error_messages:
+      self._error_parameter = ', std::string* error = NULL'

[not at google - send to devlin, 2013/06/11 18:33:43]

just make it take an required parameter error, no optional parameter. there
won't be much code to update.

[Aaron Jacobs, 2013/06/14 02:00:37]

tools/json_schema_compiler/util is dependent on the function signature, and
would need to change depending on the |compiler_options|

[not at google - send to devlin, 2013/06/14 02:14:01]

that's ok. Just pass through NULL or something in the case when we're not
generating it.

[Aaron Jacobs, 2013/06/14 21:11:30]

While that will work for this situation, there is a larger issue - if I make the
parameter required, all calls to |FromValue| or |Populate| anywhere within the
chromium codebase will need to be changed to pass in a NULL argument.

[not at google - send to devlin, 2013/06/14 21:14:15]

Why? Still generate the correct Populate, just in the no-error mode make it pass
NULL.

[Aaron Jacobs, 2013/06/14 21:21:24]

If the parameter is not optional, then the function signature between extensions
with error messages and those without are different, which causes issues if they
reference eachother (e.g. chrome/common/extensions/api/extension.cc:48, which
references tabs). In order to keep them compatible, either all calls need to
have the parameter, all need to not have the parameter, or the parameter needs
to be optional.

[not at google - send to devlin, 2013/06/14 21:48:39]

I see. jyasskin@ is there an idiom for this? It seems wasteful to always
generate two constructors. Is an optional parameter justified in this case?

[Jeffrey Yasskin, 2013/06/14 21:58:47]

I see duplicate functions but not duplicate constructors. Lawrence Crowl argued
for the current rule, that overloaded functions should be preferred over default
arguments, and I don't think the reasoning is actually compelling, but for
everything but constructors it's easy and efficient to just generate the inline
forwarding function. For constructors, I'd use a default argument.

 
   def Generate(self):
     """Generates a Code object with the .h for a single namespace.
     """
     c = Code()
     (c.Append(cpp_util.CHROMIUM_LICENSE)
       .Append()
       .Append(cpp_util.GENERATED_FILE_MESSAGE % self._namespace.source_file)
       .Append()
     )
@@ skipping 182 matching lines @@
         c.Comment(type_.description)
       (c.Sblock('struct %(classname)s {')
           .Append('%(classname)s();')
           .Append('~%(classname)s();')
       )
       if type_.origin.from_json:
         (c.Append()
           .Comment('Populates a %s object from a base::Value. Returns'
                    ' whether |out| was successfully populated.' % classname)
           .Append('static bool Populate(const base::Value& value, '
-                  '%(classname)s* out);')
+                  '%s* out%s);' % (classname, self._error_parameter))

[not at google - send to devlin, 2013/06/11 18:33:43]

perhaps:

  params = ['const base::Value& value', '%s out' % classname]
  if self._GenerateErrorMessages():
    params.append('std::string* error')
  ...
  .Append('static bool Populate(%s);' % ', '.join(params))

which involves adding a method _GenerateErrorMessages() which would look up the
compiler options.

[Aaron Jacobs, 2013/06/14 02:00:37]

Done.

         )
         if is_toplevel:
           (c.Append()
             .Comment('Creates a %s object from a base::Value, or NULL on '
                      'failure.' % classname)
-            .Append('static scoped_ptr<%(classname)s> '
-                        'FromValue(const base::Value& value);')
+            .Append('static scoped_ptr<%s> FromValue('
+                    'const base::Value& value%s);' %
+                        (classname, self._error_parameter))

[not at google - send to devlin, 2013/06/11 18:33:43]

likewise. maybe there is a nice way to share that logic here and above and in
cc_generator.

[Aaron Jacobs, 2013/06/14 02:00:37]

Done.

           )
       if type_.origin.from_client:
         (c.Append()
           .Comment('Returns a new base::DictionaryValue representing the'
                    ' serialized form of this %s object.' % classname)
           .Append('scoped_ptr<base::DictionaryValue> ToValue() const;')
         )
       properties = type_.properties.values()
       (c.Append()
         .Cblock(self._GenerateTypes(p.type_ for p in properties))
@@ skipping 23 matching lines @@
       # field of the choice is guaranteed to be set by the compiler.
       (c.Sblock('struct %(classname)s {')
           .Append('%(classname)s();')
           .Append('~%(classname)s();')
           .Append())
       c.Cblock(self._GenerateTypes(type_.choices))
       if type_.origin.from_json:
         (c.Comment('Populates a %s object from a base::Value. Returns'
                    ' whether |out| was successfully populated.' % classname)
           .Append('static bool Populate(const base::Value& value, '
-                  '%(classname)s* out);')
+                  '%s* out%s);' % (classname, self._error_parameter))

[not at google - send to devlin, 2013/06/11 18:33:43]

ditto

[Aaron Jacobs, 2013/06/14 02:00:37]

Done.

           .Append()
         )
       if type_.origin.from_client:
         (c.Comment('Returns a new base::Value representing the'
                    ' serialized form of this %s object.' % classname)
           .Append('scoped_ptr<base::Value> ToValue() const;')
           .Append()
         )
       c.Append('// Choices:')
       for choice_type in type_.choices:
@@ skipping 38 matching lines @@
     return c
 
   def _GenerateFunctionParams(self, function):
     """Generates the struct for passing parameters from JSON to a function.
     """
     if not function.params:
       return Code()
 
     c = Code()
     (c.Sblock('struct Params {')
-      .Append('static scoped_ptr<Params> Create(const base::ListValue& args);')
+      .Append('static scoped_ptr<Params> Create('
+              'const base::ListValue& args%s);' % self._error_parameter)

[not at google - send to devlin, 2013/06/11 18:33:43]

ditto

[Aaron Jacobs, 2013/06/14 02:00:37]

Done.

       .Append('~Params();')
       .Append()
       .Cblock(self._GenerateTypes(p.type_ for p in function.params))
       .Cblock(self._GenerateFields(function.params))
       .Eblock()
       .Append()
       .Sblock(' private:')
         .Append('Params();')
         .Append()
         .Append('DISALLOW_COPY_AND_ASSIGN(Params);')
@@ skipping 32 matching lines @@
   def _GenerateFunctionResults(self, callback):
     """Generates namespace for passing a function's result back.
     """
     c = Code()
     (c.Append('namespace Results {')
       .Append()
       .Concat(self._GenerateCreateCallbackArguments(callback))
       .Append('}  // namespace Results')
     )
     return c