Supporting more APIs with json_schema_compiler

Adds support for the "choices" and "any" types to json_schema_compiler, as well
as miscellaneous improvements to get at least 3 more schema files compiling:
windows, tabs, and the in-progress experimental.declarative.

For description of the generator, see http://codereview.chromium.org/9114036/

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=122082

[not at google - send to devlin, 2012-02-02 03:37:43]

Only started looking at this, but noticed that. I'll keep looking at it now, but
I thought you might want to fix that up now.

// (Removed CC's since this isn't really a review yet):
chromium-reviews@chromium.org, Aaron Boodman, pam+watch@chromium.org,
mihaip+watch@chromium.org

http://codereview.chromium.org/9309044/diff/2001/generated_files_will_not_sub...
File generated_files_will_not_submit/experimental.declarative.cc (right):

http://codereview.chromium.org/9309044/diff/2001/generated_files_will_not_sub...
generated_files_will_not_submit/experimental.declarative.cc:30:
dict->GetInteger("priority", out->priority.get());
I don't think this is right. priority is optional, so when it's not set it needs
to be NULL. However, as the code currently is, if not set it's given whatever
value "new int()" is initialised to. Possibly 0? In any case, an actual pointer
to some memory, not NULL.

This needs to be more like:

{
  int priority_temp;
  if (dict->GetInteger("priority", &priority_tmp))
    out->priority.reset(new int(priority_tmp));
}

http://codereview.chromium.org/9309044/diff/2001/generated_files_will_not_sub...
generated_files_will_not_submit/experimental.declarative.cc:43:
value->SetWithoutPathExpansion("priority",
Value::CreateIntegerValue(*priority));
Ditto, priority is optional. Maybe the optional/non-optional logic is messed up?

Anyway, this needs to be
if (priority.get())
  ...

http://codereview.chromium.org/9309044/diff/2001/generated_files_will_not_sub...
generated_files_will_not_submit/experimental.declarative.cc:45:
value->SetWithoutPathExpansion("id", Value::CreateStringValue(*id));
ditto

[calamity, 2012-02-03 02:42:35]

http://codereview.chromium.org/9309044/diff/2001/generated_files_will_not_sub...
File generated_files_will_not_submit/experimental.declarative.cc (right):

http://codereview.chromium.org/9309044/diff/2001/generated_files_will_not_sub...
generated_files_will_not_submit/experimental.declarative.cc:30:
dict->GetInteger("priority", out->priority.get());
On 2012/02/02 03:37:43, kalman wrote:
> I don't think this is right. priority is optional, so when it's not set it
needs
> to be NULL. However, as the code currently is, if not set it's given whatever
> value "new int()" is initialised to. Possibly 0? In any case, an actual
pointer
> to some memory, not NULL.
> 
> This needs to be more like:
> 
> {
>   int priority_temp;
>   if (dict->GetInteger("priority", &priority_tmp))
>     out->priority.reset(new int(priority_tmp));
> }

Done.

http://codereview.chromium.org/9309044/diff/2001/generated_files_will_not_sub...
generated_files_will_not_submit/experimental.declarative.cc:43:
value->SetWithoutPathExpansion("priority",
Value::CreateIntegerValue(*priority));
On 2012/02/02 03:37:43, kalman wrote:
> Ditto, priority is optional. Maybe the optional/non-optional logic is messed
up?
> 
> Anyway, this needs to be
> if (priority.get())
>   ...

Done.

http://codereview.chromium.org/9309044/diff/2001/generated_files_will_not_sub...
generated_files_will_not_submit/experimental.declarative.cc:45:
value->SetWithoutPathExpansion("id", Value::CreateStringValue(*id));
On 2012/02/02 03:37:43, kalman wrote:
> ditto

Done.

[not at google - send to devlin, 2012-02-05 23:42:12]

Awesome.

I haven't really gone through the real gritty code generation stuff in the
cc/h/cpp.py classes but I will soon.

I think my round of style-picking is mostly over though, and that's enough to go
on for now...

General note: there seems to be a lot more code in the generator, but not a
whole lot more in the way of tests.

http://codereview.chromium.org/9309044/diff/5001/chrome/common/extensions/api...
File chrome/common/extensions/api/api.gyp (right):

http://codereview.chromium.org/9309044/diff/5001/chrome/common/extensions/api...
chrome/common/extensions/api/api.gyp:10: 'experimental.declarative.json',
you'll need to delete this before submitting

http://codereview.chromium.org/9309044/diff/5001/chrome/common/extensions/api...
File chrome/common/extensions/api/cookies.json (right):

http://codereview.chromium.org/9309044/diff/5001/chrome/common/extensions/api...
chrome/common/extensions/api/cookies.json:4: "compile": true,
better be prepared to follow through with that :)

(or just revert for now)

actually, that makes me think, do we still need to set this property? the way
the build is set up at the moment is to list the appropriate scheme files in the
gyp; is that enough to know to compile it now?

(and then have a nocompile like there is a nodoc... I know you already had it
like this, but it seems more sensible, unless I'm missing something)

http://codereview.chromium.org/9309044/diff/5001/chrome/common/extensions/api...
File chrome/common/extensions/api/experimental.declarative.json (right):

http://codereview.chromium.org/9309044/diff/5001/chrome/common/extensions/api...
chrome/common/extensions/api/experimental.declarative.json:1: [
you'll need to delete this before submitting

http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub...
File generated_files_will_not_submit/experimental.declarative.cc (right):

http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub...
generated_files_will_not_submit/experimental.declarative.cc:30:
if(dict->GetInteger("priority", &priority_temp))
space between "if" and the (

The reason why I had priority_temp inside
{
  int priority_temp;
  ...
}

is to avoid any naming conflicts, which could happen if some schema has both a
"foo" and a "fooTemp" property defined.  Highly unlikely, but if it's a cheap
thing to avoid then we might as well.

Besides which, I personally just like the idiom.

http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub...
generated_files_will_not_submit/experimental.declarative.cc:32:
if(!json_schema_compiler::util::GetArrayFromDictionary(*dict, "conditions",
&out->conditions))
ditto with the "if" (etc)

http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub...
generated_files_will_not_submit/experimental.declarative.cc:45:
if(priority.get())
and here

http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub...
generated_files_will_not_submit/experimental.declarative.cc:65: 
extra \n

http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub...
File generated_files_will_not_submit/permissions.h (right):

http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub...
generated_files_will_not_submit/permissions.h:8: 
is there any difference between how this file and permissions.cc used to be
generated?

it's a pretty hard thing to determine, I suppose.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:94: .Sblock('bool
%(namespace)s::Populate(const Value& value, %(name)s* out) {')
line wrap

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:117: elif prop.type_ ==
PropertyType.REF or prop.type_ == PropertyType.OBJECT:
line wrap here and above

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:123: .Append(' 
out->%(name)s.reset(new %(type)s(%(name)s_temp));')
yeah, needs space after if.  same below.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:182: param_namespace =
'%s::Params::%s' % (classname, cpp_util.Classname(param.name))
line wrap

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:208: #.Append('  return
scoped_ptr<Params>();')
explain why this is commented out; or just delete the commented-out lines.

seeing code that's commented out without explaining why can be pretty confusing.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:238: if param.type_ ==
PropertyType.REF or param.type_ == PropertyType.OBJECT:
line wrap

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:252: (c.Append('if(!%s)' % 
cpp_util.GetValueFromList(param, 'args', i, dst))
line wrap

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:291: .Append('%s;' %
self._util_cc_helper.SetArrayToList(param, param.unix_name, 'value'))
line wrap

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:295: c.Append('return %s;' %
cpp_util.CreateValueFromSingleProperty(param, param.unix_name))
line wrap

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
File tools/json_schema_compiler/cpp_type_generator.py (right):

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_type_generator.py:56:
self.GetCppNamespaceName(self._namespace))
weird indentation (and below)

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_type_generator.py:128: for dependency_namespace
in sorted(self._NamespaceTypeDependencies().keys()):
line wrap

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
File tools/json_schema_compiler/cpp_util.py (right):

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_util.py:64: var: raw value
"raw value"?

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_util.py:71: raise NotImplementedError('Conversion
of single %s to Value not implemented' %
line wrap

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/h...
File tools/json_schema_compiler/h_generator.py (right):

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/h...
tools/json_schema_compiler/h_generator.py:48: forward_declarations =
self._cpp_type_generator.GenerateForwardDeclarations();
line wrap

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/h...
tools/json_schema_compiler/h_generator.py:101:
(self._cpp_type_generator.GetType(prop, wrap_optional=True), prop.unix_name))
line wrap

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/h...
tools/json_schema_compiler/h_generator.py:153: 
line wrap

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
File tools/json_schema_compiler/model.py (right):

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:62: if param.get('choices'):
I think you need to document what the available properties are throughout the
model, it's a bit hard to follow just by looking through the code.

I don't know what the standard python documentation structure looks like, but
something like (at the top)

"""Model of all namespaces that comprise an API.

Properties:
 - |namespaces| a map of namespace to its |Namespace| model
"""

For namespace

"""An API namespace

Properties:
 - |name| foo
 - |source_file| bar
"""

And, the reason I mention this,

"""A callback parameter to a Function.

Properties:
 - |params| what is params, why does each choice result in something being
appended to it?
"""

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:85: self.params.append(_Choice(self.name,
choice))
(ditto)

I see now why you are appending a different Choice for each choice, but this
definitely needs some extra explanation.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:97: def __init__(self, name, json,
is_choice=False):
is_choice no longer needed

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:98: self.name = name
I think for self-documentation sake, this should specify what the name is
expecting to be. which is... lowerCamelCase I think, right?

something like

if not re.match('^[a-z][a-zA-Z0-9]*$', name)
  throw AppropriateException(name + " must be lowerCamelCase")

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:109: elif json_type ==  'any':
extra space

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:115: elif json_type == 'double' or json_type
== 'number':
We should remove support for "double" and fix up the JSON in the 1 place that
it's used.

(though might as well leave PropertyType as DOUBLE)

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:126: self.properties[key] =
_Choice(self.name, choice)
doesn't this overwrite the same (and incorrect) key each time?

should be something like

if 'choices' in val:
 for choice in val['choices']:
   choice_property = _Choice(self.name, choice)
   self.properties[choice_property.name_] = choice_property

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:155: def _UnixName(name):
document

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:157: for x in re.findall('[A-Z][a-z_]*',
name[0].upper() + name[1:])])
i'll just assume that works...

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:159: def _Choice(name, json):
document

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:162: prop_type = json['$ref'].lower()
I think should structure this slightly differently to make the intent a bit
clearer (and so avoid having to mutate the property that is generated).

def _Choice(name, json):
  """documentation.
  """
  # Adjust name so that it reflects the type of the choice.
  # E.g. for a property with name "ids" with choices ["integer", "array"],
  # we want to generate properties "ids_integer" and "ids_array".
  property_type = json.get('type')
  if not property_type:
    property_type = json['$ref']
  if not property_type:
    throw NotImplementedError(json)
  full_name = "%s%s" % (name, propertyType[0].upper() + propertyType[1].lower())
  return Property(full_name, json)

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
File tools/json_schema_compiler/util.cc (right):

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.cc:28: bool GetItemFromList(const ListValue&
from, int index, linked_ptr<base::DictionaryValue>* out) {
fix line length

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.cc:49: void AddItemToList(const
linked_ptr<base::DictionaryValue> from, base::ListValue* out) {
line length

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
File tools/json_schema_compiler/util.h (right):

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.h:13: #include "base/memory/linked_ptr.h"
:sort

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.h:19: // Creates a new item at |out| from
|from|[|index|].
Explain that these are template specializations for use with the
Get(Optional)ArrayFromDictionary methods.

And group them in there (i.e. group the "getters" and "setters" together)

I'd put it under the actual template declaration for this (i.e. after line 51)

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.h:33: if(!T::Populate(*dict, obj))
space after if

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.h:40: void AddItemToList(const int from,
base::ListValue* out);
Ditto explanation/ordering.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.h:43: // TODO(calamity) does std::string need &?
yes

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.h:56: base::ListValue* out) {
Put on same line as above

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.h:66: base::ListValue* out) {
ditto

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.h:69: SetArrayToList(from, out);
heh, a little more concise as

if (from.get())
  SetArrayToList(from, out);

ditto below

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.h:85: // GetItemFromList to be implemented for
|T|.
run gq over this

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.h:97: // Creates a new vector containing the
fundamentals |list| at |out|. Returns
what does "fundamentals" mean?

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.h:116: // other than |T|.
gq here too

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
File tools/json_schema_compiler/util_cc_helper.py (right):

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util_cc_helper.py:8: """
line length

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util_cc_helper.py:10: self.type_manager =
type_manager
type_manager should be private

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util_cc_helper.py:71: val =
'%(namespace)s::SetOptionalArrayToDictionary(%(src)s, "%(name)s", %(dst)s)'
line wrap

[not at google - send to devlin, 2012-02-06 04:19:17]

a couple of other comments.

I'll hold off having another look until you've gone through the last lot and
these.  ping me when you have.

http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:281: for param in params:
so this generates a different Create method for each parameter?

you'd better explain that this only happens when there are choices, since
otherwise that would be surprising.

(and that there's only either 0 or 1 parameter, which is why this works)

http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:288: arg =  'const %(type)s&
%(name)s'
bit odd seeing this.

how about

if param.type == PropertyType.REF:
  arg = 'const %(type)s& %(name)s'
else
  arg = 'const %(type)s %(name)s'

http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:289: arg = arg % sub
looks like python has an %= operator, you should use that; and inline sub, since
it's only used once before being overridden.

arg %= {
  'type': ...
  'name': ...
}

I mean. But that was probably obvious.

http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:303: c.Substitute(sub)
ditto, inline sub

http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c...
File tools/json_schema_compiler/cpp_type_generator.py (right):

http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_type_generator.py:97: cpp_type =
'linked_ptr<DictionaryValue> '
Why linked_ptr<> over a raw type? I presume it's because they're the most
versatile way of having pointers, but they're only needed inside stl containers.
Hopefully when they're by themselves ("single value" or something you're calling
them?) they can be raw types.

If possible and easy, try doing that; only wrap in linked_ptr if it's an array,
otherwise make raw.

As discussed, we can't find any supported schemas which would actually test
this; so cross this bridge when we come to it. Or better, add an appropriate
NotImplemented exception in the right place.

http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_type_generator.py:114: return '%s' % cpp_type
um, "return cpp_type"; ("%s" % cpp_type) is a bit redundant.

http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/h...
File tools/json_schema_compiler/h_generator.py (right):

http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/h...
tools/json_schema_compiler/h_generator.py:183: for param in params:
same; explain why generating a new Create() function for each parameter makes
sense.

http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/h...
tools/json_schema_compiler/h_generator.py:191: arg = arg % sub
same deal,

arg %= {
  'type': ...
  'name': ...
}

[calamity, 2012-02-06 11:51:18]

http://codereview.chromium.org/9309044/diff/5001/chrome/common/extensions/api...
File chrome/common/extensions/api/cookies.json (right):

http://codereview.chromium.org/9309044/diff/5001/chrome/common/extensions/api...
chrome/common/extensions/api/cookies.json:4: "compile": true,
On 2012/02/05 23:42:12, kalman wrote:
> better be prepared to follow through with that :)
> 
> (or just revert for now)
> 
> actually, that makes me think, do we still need to set this property? the way
> the build is set up at the moment is to list the appropriate scheme files in
the
> gyp; is that enough to know to compile it now?
> 
> (and then have a nocompile like there is a nodoc... I know you already had it
> like this, but it seems more sensible, unless I'm missing something)

It's enough for now, but there's no facility for someone that wants to include
non-compiled types in the model.

I'll think about that but for now, removed.

http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub...
File generated_files_will_not_submit/experimental.declarative.cc (right):

http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub...
generated_files_will_not_submit/experimental.declarative.cc:30:
if(dict->GetInteger("priority", &priority_temp))
On 2012/02/05 23:42:12, kalman wrote:
> space between "if" and the (
> 
> The reason why I had priority_temp inside
> {
>   int priority_temp;
>   ...
> }
> 
> is to avoid any naming conflicts, which could happen if some schema has both a
> "foo" and a "fooTemp" property defined.  Highly unlikely, but if it's a cheap
> thing to avoid then we might as well.
> 
> Besides which, I personally just like the idiom.

Done.

http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub...
generated_files_will_not_submit/experimental.declarative.cc:32:
if(!json_schema_compiler::util::GetArrayFromDictionary(*dict, "conditions",
&out->conditions))
On 2012/02/05 23:42:12, kalman wrote:
> ditto with the "if" (etc)

Done.

http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub...
generated_files_will_not_submit/experimental.declarative.cc:45:
if(priority.get())
On 2012/02/05 23:42:12, kalman wrote:
> and here

Done.

http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub...
generated_files_will_not_submit/experimental.declarative.cc:65: 
On 2012/02/05 23:42:12, kalman wrote:
> extra \n

Done.

http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub...
File generated_files_will_not_submit/permissions.h (right):

http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub...
generated_files_will_not_submit/permissions.h:8: 
On 2012/02/05 23:42:12, kalman wrote:
> is there any difference between how this file and permissions.cc used to be
> generated?
> 
> it's a pretty hard thing to determine, I suppose.

Not really? I just included it for sake of completeness (and laziness).

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:94: .Sblock('bool
%(namespace)s::Populate(const Value& value, %(name)s* out) {')
On 2012/02/05 23:42:12, kalman wrote:
> line wrap

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:208: #.Append('  return
scoped_ptr<Params>();')
On 2012/02/05 23:42:12, kalman wrote:
> explain why this is commented out; or just delete the commented-out lines.
> 
> seeing code that's commented out without explaining why can be pretty
confusing.

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:238: if param.type_ ==
PropertyType.REF or param.type_ == PropertyType.OBJECT:
On 2012/02/05 23:42:12, kalman wrote:
> line wrap

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:252: (c.Append('if(!%s)' % 
cpp_util.GetValueFromList(param, 'args', i, dst))
On 2012/02/05 23:42:12, kalman wrote:
> line wrap

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:291: .Append('%s;' %
self._util_cc_helper.SetArrayToList(param, param.unix_name, 'value'))
On 2012/02/05 23:42:12, kalman wrote:
> line wrap

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:295: c.Append('return %s;' %
cpp_util.CreateValueFromSingleProperty(param, param.unix_name))
On 2012/02/05 23:42:12, kalman wrote:
> line wrap

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
File tools/json_schema_compiler/cpp_type_generator.py (right):

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_type_generator.py:56:
self.GetCppNamespaceName(self._namespace))
On 2012/02/05 23:42:12, kalman wrote:
> weird indentation (and below)

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_type_generator.py:128: for dependency_namespace
in sorted(self._NamespaceTypeDependencies().keys()):
On 2012/02/05 23:42:12, kalman wrote:
> line wrap

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
File tools/json_schema_compiler/cpp_util.py (right):

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_util.py:71: raise NotImplementedError('Conversion
of single %s to Value not implemented' %
On 2012/02/05 23:42:12, kalman wrote:
> line wrap

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/h...
File tools/json_schema_compiler/h_generator.py (right):

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/h...
tools/json_schema_compiler/h_generator.py:48: forward_declarations =
self._cpp_type_generator.GenerateForwardDeclarations();
On 2012/02/05 23:42:12, kalman wrote:
> line wrap

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/h...
tools/json_schema_compiler/h_generator.py:101:
(self._cpp_type_generator.GetType(prop, wrap_optional=True), prop.unix_name))
On 2012/02/05 23:42:12, kalman wrote:
> line wrap

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/h...
tools/json_schema_compiler/h_generator.py:153: 
On 2012/02/05 23:42:12, kalman wrote:
> line wrap

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
File tools/json_schema_compiler/model.py (right):

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:62: if param.get('choices'):
On 2012/02/05 23:42:12, kalman wrote:
> I think you need to document what the available properties are throughout the
> model, it's a bit hard to follow just by looking through the code.
> 
> I don't know what the standard python documentation structure looks like, but
> something like (at the top)
> 
> """Model of all namespaces that comprise an API.
> 
> Properties:
>  - |namespaces| a map of namespace to its |Namespace| model
> """
> 
> For namespace
> 
> """An API namespace
> 
> Properties:
>  - |name| foo
>  - |source_file| bar
> """
> 
> And, the reason I mention this,
> 
> """A callback parameter to a Function.
> 
> Properties:
>  - |params| what is params, why does each choice result in something being
> appended to it?
> """

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:85: self.params.append(_Choice(self.name,
choice))
On 2012/02/05 23:42:12, kalman wrote:
> (ditto)
> 
> I see now why you are appending a different Choice for each choice, but this
> definitely needs some extra explanation.

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:97: def __init__(self, name, json,
is_choice=False):
On 2012/02/05 23:42:12, kalman wrote:
> is_choice no longer needed

I think I'll need something eventually in order to generate the type indicator
for which choice is populated. The choices code is certainly not finished.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:98: self.name = name
On 2012/02/05 23:42:12, kalman wrote:
> I think for self-documentation sake, this should specify what the name is
> expecting to be. which is... lowerCamelCase I think, right?
> 
> something like
> 
> if not re.match('^[a-z][a-zA-Z0-9]*$', name)
>   throw AppropriateException(name + " must be lowerCamelCase")

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:109: elif json_type ==  'any':
On 2012/02/05 23:42:12, kalman wrote:
> extra space

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:115: elif json_type == 'double' or json_type
== 'number':
On 2012/02/05 23:42:12, kalman wrote:
> We should remove support for "double" and fix up the JSON in the 1 place that
> it's used.
> 
> (though might as well leave PropertyType as DOUBLE)

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:126: self.properties[key] =
_Choice(self.name, choice)
On 2012/02/05 23:42:12, kalman wrote:
> doesn't this overwrite the same (and incorrect) key each time?
> 
> should be something like
> 
> if 'choices' in val:
>  for choice in val['choices']:
>    choice_property = _Choice(self.name, choice)
>    self.properties[choice_property.name_] = choice_property

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:155: def _UnixName(name):
On 2012/02/05 23:42:12, kalman wrote:
> document

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:159: def _Choice(name, json):
On 2012/02/05 23:42:12, kalman wrote:
> document

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
File tools/json_schema_compiler/util.cc (right):

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.cc:28: bool GetItemFromList(const ListValue&
from, int index, linked_ptr<base::DictionaryValue>* out) {
On 2012/02/05 23:42:12, kalman wrote:
> fix line length

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
File tools/json_schema_compiler/util.h (right):

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.h:13: #include "base/memory/linked_ptr.h"
On 2012/02/05 23:42:12, kalman wrote:
> :sort

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.h:19: // Creates a new item at |out| from
|from|[|index|].
On 2012/02/05 23:42:12, kalman wrote:
> Explain that these are template specializations for use with the
> Get(Optional)ArrayFromDictionary methods.
> 
> And group them in there (i.e. group the "getters" and "setters" together)
> 
> I'd put it under the actual template declaration for this (i.e. after line 51)

Done. Couldn't move these under the templates that use them because the
templates needed to know the functions existed.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.h:33: if(!T::Populate(*dict, obj))
On 2012/02/05 23:42:12, kalman wrote:
> space after if

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.h:43: // TODO(calamity) does std::string need &?
On 2012/02/05 23:42:12, kalman wrote:
> yes

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.h:56: base::ListValue* out) {
On 2012/02/05 23:42:12, kalman wrote:
> Put on same line as above

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.h:66: base::ListValue* out) {
On 2012/02/05 23:42:12, kalman wrote:
> ditto

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.h:69: SetArrayToList(from, out);
On 2012/02/05 23:42:12, kalman wrote:
> heh, a little more concise as
> 
> if (from.get())
>   SetArrayToList(from, out);
> 
> ditto below

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.h:85: // GetItemFromList to be implemented for
|T|.
On 2012/02/05 23:42:12, kalman wrote:
> run gq over this

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.h:97: // Creates a new vector containing the
fundamentals |list| at |out|. Returns
On 2012/02/05 23:42:12, kalman wrote:
> what does "fundamentals" mean?

Sorry, forgot to change this doc.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util.h:116: // other than |T|.
On 2012/02/05 23:42:12, kalman wrote:
> gq here too

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
File tools/json_schema_compiler/util_cc_helper.py (right):

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util_cc_helper.py:8: """
On 2012/02/05 23:42:12, kalman wrote:
> line length

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util_cc_helper.py:10: self.type_manager =
type_manager
On 2012/02/05 23:42:12, kalman wrote:
> type_manager should be private

Done.

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/u...
tools/json_schema_compiler/util_cc_helper.py:71: val =
'%(namespace)s::SetOptionalArrayToDictionary(%(src)s, "%(name)s", %(dst)s)'
On 2012/02/05 23:42:12, kalman wrote:
> line wrap

Done.

http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:281: for param in params:
On 2012/02/06 04:19:17, kalman wrote:
> so this generates a different Create method for each parameter?
> 
> you'd better explain that this only happens when there are choices, since
> otherwise that would be surprising.
> 
> (and that there's only either 0 or 1 parameter, which is why this works)

Done.

http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:288: arg =  'const %(type)s&
%(name)s'
On 2012/02/06 04:19:17, kalman wrote:
> bit odd seeing this.
> 
> how about
> 
> if param.type == PropertyType.REF:
>   arg = 'const %(type)s& %(name)s'
> else
>   arg = 'const %(type)s %(name)s'

Done.

http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:289: arg = arg % sub
On 2012/02/06 04:19:17, kalman wrote:
> looks like python has an %= operator, you should use that; and inline sub,
since
> it's only used once before being overridden.
> 
> arg %= {
>   'type': ...
>   'name': ...
> }
> 
> I mean. But that was probably obvious.

Done.

http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:303: c.Substitute(sub)
On 2012/02/06 04:19:17, kalman wrote:
> ditto, inline sub

Done.

http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c...
File tools/json_schema_compiler/cpp_type_generator.py (right):

http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_type_generator.py:97: cpp_type =
'linked_ptr<DictionaryValue> '
On 2012/02/06 04:19:17, kalman wrote:
> Why linked_ptr<> over a raw type? I presume it's because they're the most
> versatile way of having pointers, but they're only needed inside stl
containers.
> Hopefully when they're by themselves ("single value" or something you're
calling
> them?) they can be raw types.
> 
> If possible and easy, try doing that; only wrap in linked_ptr if it's an
array,
> otherwise make raw.
> 
> As discussed, we can't find any supported schemas which would actually test
> this; so cross this bridge when we come to it. Or better, add an appropriate
> NotImplemented exception in the right place.

Done.

http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_type_generator.py:114: return '%s' % cpp_type
On 2012/02/06 04:19:17, kalman wrote:
> um, "return cpp_type"; ("%s" % cpp_type) is a bit redundant.

Yes, yes it is.

http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/h...
File tools/json_schema_compiler/h_generator.py (right):

http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/h...
tools/json_schema_compiler/h_generator.py:183: for param in params:
On 2012/02/06 04:19:17, kalman wrote:
> same; explain why generating a new Create() function for each parameter makes
> sense.

Done.

http://codereview.chromium.org/9309044/diff/8001/tools/json_schema_compiler/h...
tools/json_schema_compiler/h_generator.py:191: arg = arg % sub
On 2012/02/06 04:19:17, kalman wrote:
> same deal,
> 
> arg %= {
>   'type': ...
>   'name': ...
> }

Done.

[not at google - send to devlin, 2012-02-06 13:14:47]

Looking really nice, adding back the CC list (plus koz/benwells) and reviewers
(aa/yoz).  Hi guys.  Also CC'ed battre who's writing the declarative API.

I hope this CL description/contents makes sense out of context?  Basically we're
working towards committing a version of the compiler that will work for Dominic
as he's writing the declarative API (which is already in review, and doing this
stuff by hand).

Basically yeah:
 - my style eyes are happy, though they're not good at spotting pythons
 - structure is good, and your code seems really nice to iterate on
 - there is just that "choices" thing which might need a bit of a reshuffle
(sorry about that again)
 - also, tests still seem meagre.  I've observed you doing a lot of compiling
and running browser tests to test this stuff; you're much better off writing
comprehensive tests here and iterating that way, rather than on the browser. 
Plus, APIs that people write in the future won't have that luxury obviously.
 - also, run try jobs :)

http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub...
File generated_files_will_not_submit/permissions.h (right):

http://codereview.chromium.org/9309044/diff/5001/generated_files_will_not_sub...
generated_files_will_not_submit/permissions.h:8: 
On 2012/02/06 11:51:18, calamity wrote:
> On 2012/02/05 23:42:12, kalman wrote:
> > is there any difference between how this file and permissions.cc used to be
> > generated?
> > 
> > it's a pretty hard thing to determine, I suppose.
> 
> Not really? I just included it for sake of completeness (and laziness).

(not quite what I meant but nvm, not important)

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
File tools/json_schema_compiler/model.py (right):

http://codereview.chromium.org/9309044/diff/5001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:97: def __init__(self, name, json,
is_choice=False):
On 2012/02/06 11:51:18, calamity wrote:
> On 2012/02/05 23:42:12, kalman wrote:
> > is_choice no longer needed
> 
> I think I'll need something eventually in order to generate the type indicator
> for which choice is populated. The choices code is certainly not finished.

Yeah, see the other couple of comments I've made regarding this elsewhere.

http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:213: if last != param.name:
Why the "last" check?  Is it something to do with choices?  If so, wouldn't this
be a bit dangerous to assume they all have the wrong name (see other comment).

Yes, must be choices.  That's a bummer :(

Hmm, a few options here I suppose
 - cache those values directly in the model (we can test it that way, but it
makes the model a bit bigger and that's a shame)
 - tag the parameters in a way that can detect whether they're part of the same
choices group (sounds arcane and complicated; too much logic in the C
generators)
 - do choices differently; push that "expansion" logic out of the model and into
the c generators.  params just becomes literally the params (with a CHOICES
option) and h_generator/cc_generator/cpp_types_generator know how to expand
them.

I reckon the last sounds the best. This method then becomes

required = 0
for param in function.params:
  if not param.optional
    required += 1

c = Code()
  'if %(var)s.GetSize() < %(required)d'
  '  return scoped_ptr<Params>();'
  'if %(var)s.GetSize() > %(total)d'
  '  return scoped_ptr<Params>();'

return c.sub({
  'var': var,
  'required': required,
  'total': params.length
})

http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:222: check = 'if (%(var)s.GetSize() <
%(req)d && %(var)s.GetSize() > %(sum)d)'
&& should be || right?

http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c...
File tools/json_schema_compiler/compiler.py (right):

http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c...
tools/json_schema_compiler/compiler.py:82: filename_suffix)
Note here, and in general, indentation here makes it look like filename_suffix
is an argument to AddNamespace().

The c++ style guide would say to format this call like

type_generator.AddNamespace(
    referenced_namespace,
    cpp_util.ClassName(referenced_namespace.name).lower() + filename_suffix)

and if that doesn't fit then

type_generator.AddNamespace(
    referenced_namespace,
    cpp_util.ClassName(referenced_namespace.name).lower() +
        filename_suffix)

i.e. all arguments/parameters should be on same line, or in line.  That's open a
bit to interpretation I suppose, but I think the same sentiment should translate
across to Python with these things.

^
apply to all line-splitting choices.

http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c...
File tools/json_schema_compiler/cpp_type_generator.py (right):

http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_type_generator.py:98: if prop.item_type.type_ in
(PropertyType.REF, PropertyType.ANY, PropertyType.OBJECT):
Cool

Line wrapping though :)

http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/m...
File tools/json_schema_compiler/model.py (right):

http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:130: its model.Type
line wrap

http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:205: def _Choice(name, json):
I made a comment here earlier about structuring this method differently, but it
seems to have gotten lost.  No matter, since I've come to realise something
after looking at that checking code.  Sorry to have not have formed an opinion
about this earlier... you're going to hate me...

but the way choices are done at the moment is hard to understand and easy to
misuse.  There is logic in in the model to expand choice parameters, but not
enough (as we saw earlier) and it's not applicable to all scenarios anyway.

To write this differently, how about:
 - Have another PropertyType CHOICE.
 - If a Property is CHOICE, have another member of Property called like
"choices", an array of possible Properties.
 - Code that uses parameters needs to know how to expand out choices.  A lot of
this, I think, should be able to be abstracted nicely away into the
CppTypeGenerator class; like a method along the lines of ExpandChoicesInParams
or something which returns an array of Properties, like function.params, but
expanded in the same way you're doing here, basically.

(and then the h/cc generator classes in ToValue which used function.params and
assumed the Choices were taken care of would use
cpp_type_generator.ExpandChoicesInParams(function.params) instead)

Yeah, there will be a few non-trivial changes to do something like this, but
nothing overly structural, and I think overall it will result in easier to
follow code.

And... you'll need tests for it.

http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:208: raise NotImplementedError('%s\nChoices
is not fully implemented' % json)
Choices is implemented for return values right?  Or one of them?

I just meant to raise a NotImplementedError where you said it didn't work, not
for _all_ attempts to use choices :)

That's a bit harsh on yourself...

[not at google - send to devlin, 2012-02-06 13:25:34]

http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/m...
File tools/json_schema_compiler/model.py (right):

http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:205: def _Choice(name, json):
>  - If a Property is CHOICE, have another member of Property called like
> "choices", an array of possible Properties.

Sorry, I should have said an array of possible PropertyTypes.

This is tough though, since for array/object you also need the information about
the items/properties.

Perhaps you'll need to eventually refactor this so that PropertyType is a class
not an enum, so that it can have those set.  That's a big change though, and we
can talk about it tomorrow.  If all that's needed to get this out the door now
is for this actually to be an array of Propertys after all (with those redundant
extra properties like descrition/name/unix_name/etc copyied over) then let's do
that for now.

[calamity, 2012-02-08 00:52:31]

Started supporting 'choices' and did some rework.

Forgot to do line wrapping style fixes. Will come in next patch. Also going to
integrate the generated choices code somewhere to make sure it works.

https://chromiumcodereview.appspot.com/9309044/diff/2003/tools/json_schema_co...
File tools/json_schema_compiler/cc_generator.py (right):

https://chromiumcodereview.appspot.com/9309044/diff/2003/tools/json_schema_co...
tools/json_schema_compiler/cc_generator.py:213: if last != param.name:
On 2012/02/06 13:14:48, kalman wrote:
> Why the "last" check?  Is it something to do with choices?  If so, wouldn't
this
> be a bit dangerous to assume they all have the wrong name (see other comment).
> 
> Yes, must be choices.  That's a bummer :(
> 
> Hmm, a few options here I suppose
>  - cache those values directly in the model (we can test it that way, but it
> makes the model a bit bigger and that's a shame)
>  - tag the parameters in a way that can detect whether they're part of the
same
> choices group (sounds arcane and complicated; too much logic in the C
> generators)
>  - do choices differently; push that "expansion" logic out of the model and
into
> the c generators.  params just becomes literally the params (with a CHOICES
> option) and h_generator/cc_generator/cpp_types_generator know how to expand
> them.
> 
> I reckon the last sounds the best. This method then becomes
> 
> required = 0
> for param in function.params:
>   if not param.optional
>     required += 1
> 
> c = Code()
>   'if %(var)s.GetSize() < %(required)d'
>   '  return scoped_ptr<Params>();'
>   'if %(var)s.GetSize() > %(total)d'
>   '  return scoped_ptr<Params>();'
> 
> return c.sub({
>   'var': var,
>   'required': required,
>   'total': params.length
> })

Done.

https://chromiumcodereview.appspot.com/9309044/diff/2003/tools/json_schema_co...
tools/json_schema_compiler/cc_generator.py:222: check = 'if (%(var)s.GetSize() <
%(req)d && %(var)s.GetSize() > %(sum)d)'
On 2012/02/06 13:14:48, kalman wrote:
> && should be || right?

Wups, removed anyways.

https://chromiumcodereview.appspot.com/9309044/diff/2003/tools/json_schema_co...
File tools/json_schema_compiler/cpp_type_generator.py (right):

https://chromiumcodereview.appspot.com/9309044/diff/2003/tools/json_schema_co...
tools/json_schema_compiler/cpp_type_generator.py:98: if prop.item_type.type_ in
(PropertyType.REF, PropertyType.ANY, PropertyType.OBJECT):
On 2012/02/06 13:14:48, kalman wrote:
> Cool
> 
> Line wrapping though :)

Done.

https://chromiumcodereview.appspot.com/9309044/diff/2003/tools/json_schema_co...
File tools/json_schema_compiler/model.py (right):

https://chromiumcodereview.appspot.com/9309044/diff/2003/tools/json_schema_co...
tools/json_schema_compiler/model.py:130: its model.Type
On 2012/02/06 13:14:48, kalman wrote:
> line wrap

Done.

https://chromiumcodereview.appspot.com/9309044/diff/2003/tools/json_schema_co...
tools/json_schema_compiler/model.py:205: def _Choice(name, json):
On 2012/02/06 13:14:48, kalman wrote:
> I made a comment here earlier about structuring this method differently, but
it
> seems to have gotten lost.  No matter, since I've come to realise something
> after looking at that checking code.  Sorry to have not have formed an opinion
> about this earlier... you're going to hate me...
> 
> but the way choices are done at the moment is hard to understand and easy to
> misuse.  There is logic in in the model to expand choice parameters, but not
> enough (as we saw earlier) and it's not applicable to all scenarios anyway.
> 
> To write this differently, how about:
>  - Have another PropertyType CHOICE.
>  - If a Property is CHOICE, have another member of Property called like
> "choices", an array of possible Properties.
>  - Code that uses parameters needs to know how to expand out choices.  A lot
of
> this, I think, should be able to be abstracted nicely away into the
> CppTypeGenerator class; like a method along the lines of ExpandChoicesInParams
> or something which returns an array of Properties, like function.params, but
> expanded in the same way you're doing here, basically.
> 
> (and then the h/cc generator classes in ToValue which used function.params and
> assumed the Choices were taken care of would use
> cpp_type_generator.ExpandChoicesInParams(function.params) instead)
> 
> Yeah, there will be a few non-trivial changes to do something like this, but
> nothing overly structural, and I think overall it will result in easier to
> follow code.
> 
> And... you'll need tests for it.

Done.

https://chromiumcodereview.appspot.com/9309044/diff/2003/tools/json_schema_co...
tools/json_schema_compiler/model.py:208: raise NotImplementedError('%s\nChoices
is not fully implemented' % json)
On 2012/02/06 13:14:48, kalman wrote:
> Choices is implemented for return values right?  Or one of them?
> 
> I just meant to raise a NotImplementedError where you said it didn't work, not
> for _all_ attempts to use choices :)
> 
> That's a bit harsh on yourself...

It's done now, but I thought it made sense to disable choices everywhere since
having NotImplemented anywhere would break as soon as you had choices anyway.

[not at google - send to devlin, 2012-02-08 05:02:07]

Looking forward to that Choices stuff as discussed :)

http://codereview.chromium.org/9309044/diff/10001/generated_files_will_not_su...
File generated_files_will_not_submit/tabs.h (right):

http://codereview.chromium.org/9309044/diff/10001/generated_files_will_not_su...
generated_files_will_not_submit/tabs.h:48: // The ID of the tab that opened this
tab, if any. This will only be present if
looks like the comment wrapping thing got a bit broken

http://codereview.chromium.org/9309044/diff/10001/generated_files_will_not_su...
generated_files_will_not_submit/tabs.h:139: 
where did this extra blank line come from?

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:109: .Eblock('}')
indent 2 spaces. I can see why you might have done it this way (so that it lines
up with the C++ indentation) but it looks weird.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:113: def
_GenerateTypePopulateProperty(self, prop, src, dst):
As discussed, most of this code will be replaced with the choices implementation
in GeneratePopulateChoices; you'll just need to factor it a bit differently to
deal with a Value*. Then,
 - this method will get the value from src[prop.name]
 - GeneratePopulateChoices will get it from src[index]
or something like that.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:174: 'type':
self._cpp_type_generator.GetType(prop)
too much indentation, and spread out each member on its own line. hard to read
this way.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:176: return c
Can delay substituting src until the end?

c.Substitute({
  'src': src
})

then remove it above.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:280: return c
y u no write it like I wrote?

I can see why you did the num_optional thing, but those checks aren't necessary
(the "if num_{optional,required}:") because they cause a weakening of the
checks. Looking at permissions.cc for example, it used to be

if (args.GetSize() != 1)
  return NULL;

but now it's

if (args.GetSize() < 1)
  return NULL;

which isn't as strong. Instead, we want like (generated C++)

if (args.GetSize() < 1)
  return NULL;
if (args.GetSize() > 1)
  return NULL;

or just

if (args.GetSize() < 1 || args.GetSize() > 1)
  return NULL;

which is the same as != 1.  In fact, you could have like (Python)

if num_required == params.length:
  c.Append('if (%(var)s.GetSize() != %(total)d')
else:
  c.Append('if (%(var)s.GetSize() < %(required)d'
           ' || %(var)s.GetSize() > %(total)d')
c.Append('  return scoped_ptr<Params>();')

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:345: def
_GeneratePopulateChoices(self, param, src, index, return_line):
confused by the use of the word "populate" here but it's actually used as part
of the Params::Create() method, not Populate?

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:351: """
as discussed, refactoring to do here.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:358: value_var = param.unix_name +
'_value'
Refactoring being making this method accept the name of value_var rather than a
ListValue and its index.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:361: c.Append(return_line)
This means that optional parameters can only appear as the last parameter in
function calls. You should check that the JSON behaves that way, and if it does:
assert this in the model, and add a comment here.

If it doesn't, then you'll need to deal with it by not returning from the
function, but putting everything in an "if" block (you can figure it out I'm
sure).

Actually, if it's not too hard, you can save yourself and other people the
cognitive effort of reading the comment(s) and reason for the assertion(s) and
just implement it in the more general way to start off with.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:363: c.Substitute({'src': src,
'index': index, 'value': value_var})
IMO try to put substitutions at the end of these code blocks. When you
substitute early like this, I wonder if it's because the values change later
on... and here they don't.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:374: (c.Append('  %(type)s =
%(enum)s;')
why the extra spaces before %(type)s?

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:393: 'enum':
self._cpp_type_generator.GetChoiceEnum(param, choice.type_)})
I don't know whether enum refers to the type or the value by reading the code. I
think it's value, so call enum_value.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
File tools/json_schema_compiler/cpp_type_generator.py (right):

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:33: def
ExpandedChoicesInParams(self, params):
nice

though try to make method names verbs, like "GetExpandedChoicesInParams".

(I didn't suggest "ExpandChoicesInParams" because that might imply that the
params passed through is mutated)

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:91: assert prop.choices[type_]
won't prop.choices[type_] raise a TypeError if type_ isn't there, so the assert
is unnecessary?

Could be missing something...

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:93: 'NONE' if type_ is None
else repr(type_))
Make this two separate methods rather than overloading the method with being
able to support both NONE and actual values.

Like

def GetChoiceEnumValue(self, prop, type_):
  return '%s_%s' % (prop.unix_name.upper(), type_.name)

def GetChoiceEnumNoneValue(self, prop):
  return '%s_NONE' % (prop.unix_name_upper())

And yeah -- why the repr()? type_.name would suffice right?

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:95: def
GetChoicesEnumName(self, prop):
This function name is confusing me. It generates the type right? like

enum GetChoicesEnumName {
  GetChoiceEnum(),
  GetChoiceEnum(),
}

GetChoicesEnumType would be better, I think.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
File tools/json_schema_compiler/h_generator.py (right):

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:96: for prop in [x for x in props if
x.type_ == PropertyType.CHOICES]:
I think this needs a comment, like

# Generate the enums needed for any fields with "choices".

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:105: c.Append(enum)
I don't understand why you've done it this way rather than generating the code
as you go, like:

===

enum_type = self._cpp_type_generator.GetChoicesEnumType(prop)
c.Sblock('enum %s {' % enum_type)

c.Append('%s,' % ...GetChoiceEnumNoneType(prop))
for choice in prop.choices.values():
  s.Append('%s,' % ...GetChoiceEnumType(prop, choice.type_)

c.Eblock('};')

(c.Append()
  .Append('%s %s_type;' % (enum_type, prop.unix_name))
)

===

note that c++ is ok with trailing commas on enums.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
File tools/json_schema_compiler/model.py (right):

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:189: unix_name = property(GetUnixName,
SetUnixName)
Getter/Setter can be a bit simpler to take advantage of their complementary
access restrictions (you don't need the comments, I'm just adding them for
illustrative purposes).

===

def GetUnicName(self):
  # There has to be a valid _unix_name since we set one in the
  # constructor, and SetUnixName ensures that it's only ever
  # changed to something valid.
  self._unix_name_used = True
  return self._unix_name

def SetUnixName(self, unix_name):
  if unix_name is None
    raise AssertionErrorOrSomething(...)
  if self._unix_name_used
    raise AttributeError(...)
  # This is up to you, but it seems like you only want to be
  # able to set it once, too.
  self._unix_name_used = true
  self._unix_name = unix_name

[calamity, 2012-02-08 07:01:17]

Refactored choices and other small fixes.

http://codereview.chromium.org/9309044/diff/10001/generated_files_will_not_su...
File generated_files_will_not_submit/tabs.h (right):

http://codereview.chromium.org/9309044/diff/10001/generated_files_will_not_su...
generated_files_will_not_submit/tabs.h:48: // The ID of the tab that opened this
tab, if any. This will only be present if
On 2012/02/08 05:02:08, kalman wrote:
> looks like the comment wrapping thing got a bit broken

It wraps to 80 characters but then got indented 2. I could fix this.. but not
now.

http://codereview.chromium.org/9309044/diff/10001/generated_files_will_not_su...
generated_files_will_not_submit/tabs.h:139: 
On 2012/02/08 05:02:08, kalman wrote:
> where did this extra blank line come from?

Done.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:109: .Eblock('}')
On 2012/02/08 05:02:08, kalman wrote:
> indent 2 spaces. I can see why you might have done it this way (so that it
lines
> up with the C++ indentation) but it looks weird.

Done.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:113: def
_GenerateTypePopulateProperty(self, prop, src, dst):
On 2012/02/08 05:02:08, kalman wrote:
> As discussed, most of this code will be replaced with the choices
implementation
> in GeneratePopulateChoices; you'll just need to factor it a bit differently to
> deal with a Value*. Then,
>  - this method will get the value from src[prop.name]
>  - GeneratePopulateChoices will get it from src[index]
> or something like that.

Done.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:174: 'type':
self._cpp_type_generator.GetType(prop)
On 2012/02/08 05:02:08, kalman wrote:
> too much indentation, and spread out each member on its own line. hard to read
> this way.

Done.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:176: return c
On 2012/02/08 05:02:08, kalman wrote:
> Can delay substituting src until the end?
> 
> c.Substitute({
>   'src': src
> })
> 
> then remove it above.

Done.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:280: return c
On 2012/02/08 05:02:08, kalman wrote:
> y u no write it like I wrote?
> 
> I can see why you did the num_optional thing, but those checks aren't
necessary
> (the "if num_{optional,required}:") because they cause a weakening of the
> checks. Looking at permissions.cc for example, it used to be
> 
> if (args.GetSize() != 1)
>   return NULL;
> 
> but now it's
> 
> if (args.GetSize() < 1)
>   return NULL;
> 
> which isn't as strong. Instead, we want like (generated C++)
> 
> if (args.GetSize() < 1)
>   return NULL;
> if (args.GetSize() > 1)
>   return NULL;
> 
> or just
> 
> if (args.GetSize() < 1 || args.GetSize() > 1)
>   return NULL;
> 
> which is the same as != 1.  In fact, you could have like (Python)
> 
> if num_required == params.length:
>   c.Append('if (%(var)s.GetSize() != %(total)d')
> else:
>   c.Append('if (%(var)s.GetSize() < %(required)d'
>            ' || %(var)s.GetSize() > %(total)d')
> c.Append('  return scoped_ptr<Params>();')

Wups, my bad. Note that you can't do args.GetSize() < 0 because C++ herps a derp
about it always being true.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:345: def
_GeneratePopulateChoices(self, param, src, index, return_line):
On 2012/02/08 05:02:08, kalman wrote:
> confused by the use of the word "populate" here but it's actually used as part
> of the Params::Create() method, not Populate?

Now it populates. Woo.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:351: """
On 2012/02/08 05:02:08, kalman wrote:
> as discussed, refactoring to do here.

Done.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:358: value_var = param.unix_name +
'_value'
On 2012/02/08 05:02:08, kalman wrote:
> Refactoring being making this method accept the name of value_var rather than
a
> ListValue and its index.

Done.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:361: c.Append(return_line)
On 2012/02/08 05:02:08, kalman wrote:
> This means that optional parameters can only appear as the last parameter in
> function calls. You should check that the JSON behaves that way, and if it
does:
> assert this in the model, and add a comment here.
> 
> If it doesn't, then you'll need to deal with it by not returning from the
> function, but putting everything in an "if" block (you can figure it out I'm
> sure).
> 
> Actually, if it's not too hard, you can save yourself and other people the
> cognitive effort of reading the comment(s) and reason for the assertion(s) and
> just implement it in the more general way to start off with.

Couldn't figure a case where requires args happen after optional args. Can't
assert because there are 'optional' first args but they need to be specified as
null/undefined.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:363: c.Substitute({'src': src,
'index': index, 'value': value_var})
On 2012/02/08 05:02:08, kalman wrote:
> IMO try to put substitutions at the end of these code blocks. When you
> substitute early like this, I wonder if it's because the values change later
> on... and here they don't.

Done. I'm wary of someone else changing a value in the middle of a huge code
block and breaking things for the substitute at the bottom.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:374: (c.Append('  %(type)s =
%(enum)s;')
On 2012/02/08 05:02:08, kalman wrote:
> why the extra spaces before %(type)s?

Done.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:393: 'enum':
self._cpp_type_generator.GetChoiceEnum(param, choice.type_)})
On 2012/02/08 05:02:08, kalman wrote:
> I don't know whether enum refers to the type or the value by reading the code.
I
> think it's value, so call enum_value.

Done.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
File tools/json_schema_compiler/cpp_type_generator.py (right):

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:33: def
ExpandedChoicesInParams(self, params):
On 2012/02/08 05:02:08, kalman wrote:
> nice
> 
> though try to make method names verbs, like "GetExpandedChoicesInParams".
> 
> (I didn't suggest "ExpandChoicesInParams" because that might imply that the
> params passed through is mutated)

Done.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:91: assert prop.choices[type_]
On 2012/02/08 05:02:08, kalman wrote:
> won't prop.choices[type_] raise a TypeError if type_ isn't there, so the
assert
> is unnecessary?
> 
> Could be missing something...

We don't actually use prop.choice[type_] otherwise. (unless I just use
prop.choice[type_] as its own statement but that's kind of confusing too?)

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:93: 'NONE' if type_ is None
else repr(type_))
On 2012/02/08 05:02:08, kalman wrote:
> Make this two separate methods rather than overloading the method with being
> able to support both NONE and actual values.
> 
> Like
> 
> def GetChoiceEnumValue(self, prop, type_):
>   return '%s_%s' % (prop.unix_name.upper(), type_.name)
> 
> def GetChoiceEnumNoneValue(self, prop):
>   return '%s_NONE' % (prop.unix_name_upper())
> 
> And yeah -- why the repr()? type_.name would suffice right?

Hah, wups. Yeah.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:95: def
GetChoicesEnumName(self, prop):
On 2012/02/08 05:02:08, kalman wrote:
> This function name is confusing me. It generates the type right? like
> 
> enum GetChoicesEnumName {
>   GetChoiceEnum(),
>   GetChoiceEnum(),
> }
> 
> GetChoicesEnumType would be better, I think.

Okay. Didn't know if enums were classified as types.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
File tools/json_schema_compiler/h_generator.py (right):

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:96: for prop in [x for x in props if
x.type_ == PropertyType.CHOICES]:
On 2012/02/08 05:02:08, kalman wrote:
> I think this needs a comment, like
> 
> # Generate the enums needed for any fields with "choices".

Done.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:105: c.Append(enum)
On 2012/02/08 05:02:08, kalman wrote:
> I don't understand why you've done it this way rather than generating the code
> as you go, like:
> 
> ===
> 
> enum_type = self._cpp_type_generator.GetChoicesEnumType(prop)
> c.Sblock('enum %s {' % enum_type)
> 
> c.Append('%s,' % ...GetChoiceEnumNoneType(prop))
> for choice in prop.choices.values():
>   s.Append('%s,' % ...GetChoiceEnumType(prop, choice.type_)
> 
> c.Eblock('};')
> 
> (c.Append()
>   .Append('%s %s_type;' % (enum_type, prop.unix_name))
> )
> 
> ===
> 
> note that c++ is ok with trailing commas on enums.

Hah. Didn't know that. My default assumption is that C++ will whine.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
File tools/json_schema_compiler/model.py (right):

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:189: unix_name = property(GetUnixName,
SetUnixName)
On 2012/02/08 05:02:08, kalman wrote:
> Getter/Setter can be a bit simpler to take advantage of their complementary
> access restrictions (you don't need the comments, I'm just adding them for
> illustrative purposes).
> 
> ===
> 
> def GetUnicName(self):
>   # There has to be a valid _unix_name since we set one in the
>   # constructor, and SetUnixName ensures that it's only ever
>   # changed to something valid.
>   self._unix_name_used = True
>   return self._unix_name
> 
> def SetUnixName(self, unix_name):
>   if unix_name is None
>     raise AssertionErrorOrSomething(...)
>   if self._unix_name_used
>     raise AttributeError(...)
>   # This is up to you, but it seems like you only want to be
>   # able to set it once, too.
>   self._unix_name_used = true
>   self._unix_name = unix_name

If the property is a choice, we unset it. GetUnixName then throws the exception
on attempted access.

We probably don't want to set more than once but we don't really care (unless it
gets used). Actually I'm going to allow this because it lets me avoid a copy in
GetExpandedChoicesInParams.

[battre, 2012-02-08 09:15:19]

Hi.

I had a look at the generated stuff and added some comments.

This is great stuff. I am looking forward to using it.

Best,
Dominic

http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su...
File generated_files_will_not_submit/experimental.declarative.cc (right):

http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su...
generated_files_will_not_submit/experimental.declarative.cc:86: // Rules that
were registered, the optional parameters are filled with values.
I think it is sufficient to have this comment in the header file.

http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su...
File generated_files_will_not_submit/experimental.declarative.h (right):

http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su...
generated_files_will_not_submit/experimental.declarative.h:10: #define
CHROME_COMMON_EXTENSIONS_API_EXPERIMENTAL_DECLARATIVE_H__
Should this go to chrome/common?

http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su...
generated_files_will_not_submit/experimental.declarative.h:21: using
base::Value;
I think the style guide prohibits "bleeding using statements" in header files
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su...
generated_files_will_not_submit/experimental.declarative.h:55: // Rule object.
Passesownership to caller.
nit: space after "Passes"

http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su...
generated_files_will_not_submit/experimental.declarative.h:56: DictionaryValue*
ToValue() const;
should this return a scoped_ptr?

http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su...
generated_files_will_not_submit/experimental.declarative.h:71: // Name of the
event this function affects.
Why is the "Name of the event this function affects." line here twice?

[Yoyo Zhou, 2012-02-08 19:04:12]

I have a few comments (on older drafts) that I wanted to send out before I
forget and they become obsolete. =)

http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c...
File tools/json_schema_compiler/cpp_type_generator.py (right):

http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_type_generator.py:74: Use wrap_optional to wrap
the type in a scoped_ptr<T> if the Property is
When would you not want to use this?

http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_type_generator.py:123: c.Append('namespace %s {'
% namespace.name)
Any possibility of nested namespaces?

http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_type_generator.py:157: if not
dependency_namespaces.get(namespace):
You could use dependency_namespaces.setdefault(namespace, [])

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
File tools/json_schema_compiler/model.py (right):

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:33: - |source_file| the file that contained
the namespace definition
This seems redundant given the 2 below it.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:165: # A choice needs to have its unix_name
set elsewhere
Can you discuss where SetUnixName might be called from? As it is, I can't find
it.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:187: 'Cannot set the unix_name on %s; it is
already used elsewhere as %s')
missing substitutions here?

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:213: """Returns the unix_style name for a
given string.
Comment here that the name is expected to be lowerCamelCase.

[calamity, 2012-02-09 00:56:52]

Pulled from master so now I'm working against Dominic's actual code.

http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c...
File tools/json_schema_compiler/cpp_type_generator.py (right):

http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_type_generator.py:74: Use wrap_optional to wrap
the type in a scoped_ptr<T> if the Property is
On 2012/02/08 19:04:12, Yoyo Zhou wrote:
> When would you not want to use this?

Anywhere that isn't a declaration.

e.g scoped_ptr<X> var(new X()) 
would be
'%s var(new %s())' %
(GetType(prop, wrap_optional=True), GetType(prop))

http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_type_generator.py:123: c.Append('namespace %s {'
% namespace.name)
On 2012/02/08 19:04:12, Yoyo Zhou wrote:
> Any possibility of nested namespaces?

I don't think so. These should only be types (e.g Window, Tab) which should all
be in the top level of a generated namespace.

http://codereview.chromium.org/9309044/diff/2003/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_type_generator.py:157: if not
dependency_namespaces.get(namespace):
On 2012/02/08 19:04:12, Yoyo Zhou wrote:
> You could use dependency_namespaces.setdefault(namespace, [])

Done.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
File tools/json_schema_compiler/model.py (right):

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:33: - |source_file| the file that contained
the namespace definition
On 2012/02/08 19:04:12, Yoyo Zhou wrote:
> This seems redundant given the 2 below it.

It's really just for convenience, otherwise, I'd need to import os.path and join
the split version back together.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:165: # A choice needs to have its unix_name
set elsewhere
On 2012/02/08 19:04:12, Yoyo Zhou wrote:
> Can you discuss where SetUnixName might be called from? As it is, I can't find
> it.

unix_name is set as a property of this class (line 189 in this patch). When the
property accessed, (i.e prop.unix_name) the setter or getter is called depending
on context.

tl;dr python magic

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:187: 'Cannot set the unix_name on %s; it is
already used elsewhere as %s')
On 2012/02/08 19:04:12, Yoyo Zhou wrote:
> missing substitutions here?

Done.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:213: """Returns the unix_style name for a
given string.
On 2012/02/08 19:04:12, Yoyo Zhou wrote:
> Comment here that the name is expected to be lowerCamelCase.

Done.

http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su...
File generated_files_will_not_submit/experimental.declarative.cc (right):

http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su...
generated_files_will_not_submit/experimental.declarative.cc:86: // Rules that
were registered, the optional parameters are filled with values.
On 2012/02/08 09:15:20, battre wrote:
> I think it is sufficient to have this comment in the header file.

Done.

http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su...
File generated_files_will_not_submit/experimental.declarative.h (right):

http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su...
generated_files_will_not_submit/experimental.declarative.h:10: #define
CHROME_COMMON_EXTENSIONS_API_EXPERIMENTAL_DECLARATIVE_H__
On 2012/02/08 09:15:20, battre wrote:
> Should this go to chrome/common?

Sorry, I'm not sure what you mean? The file is generated to a temporary
directory during the build. chrome/common is simply the include path (generated
from the relative path of root to the api json).

http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su...
generated_files_will_not_submit/experimental.declarative.h:21: using
base::Value;
On 2012/02/08 09:15:20, battre wrote:
> I think the style guide prohibits "bleeding using statements" in header files
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

Moved to the .cc file.

http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su...
generated_files_will_not_submit/experimental.declarative.h:55: // Rule object.
Passesownership to caller.
On 2012/02/08 09:15:20, battre wrote:
> nit: space after "Passes"

Done.

http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su...
generated_files_will_not_submit/experimental.declarative.h:56: DictionaryValue*
ToValue() const;
On 2012/02/08 09:15:20, battre wrote:
> should this return a scoped_ptr?

Done.

http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su...
generated_files_will_not_submit/experimental.declarative.h:71: // Name of the
event this function affects.
On 2012/02/08 09:15:20, battre wrote:
> Why is the "Name of the event this function affects." line here twice?

Done.

[not at google - send to devlin, 2012-02-09 02:49:36]

I'm losing track of this patch a bit due to the number of changes that have
happened... I think basically it LGTM after you fix up those comments.

As you write those compiler-level tests and possibly find bugs, and iterate on
the compiler, you might (probably will) find things to fix up. But as it stands
-- this looks good and generates correct code at the moment, so cool.

Make sure it passes try-jobs though.

And wait for Yoyo's LGTM.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:176: return c
On 2012/02/08 07:01:18, calamity wrote:
> On 2012/02/08 05:02:08, kalman wrote:
> > Can delay substituting src until the end?
> > 
> > c.Substitute({
> >   'src': src
> > })
> > 
> > then remove it above.
> 
> Done.

Not done?

Also you have the same thing above now, substituting "src" in.

http://codereview.chromium.org/9309044/diff/10001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:280: return c
On 2012/02/08 07:01:18, calamity wrote:
> On 2012/02/08 05:02:08, kalman wrote:
> > y u no write it like I wrote?
> > 
> > I can see why you did the num_optional thing, but those checks aren't
> necessary
> > (the "if num_{optional,required}:") because they cause a weakening of the
> > checks. Looking at permissions.cc for example, it used to be
> > 
> > if (args.GetSize() != 1)
> >   return NULL;
> > 
> > but now it's
> > 
> > if (args.GetSize() < 1)
> >   return NULL;
> > 
> > which isn't as strong. Instead, we want like (generated C++)
> > 
> > if (args.GetSize() < 1)
> >   return NULL;
> > if (args.GetSize() > 1)
> >   return NULL;
> > 
> > or just
> > 
> > if (args.GetSize() < 1 || args.GetSize() > 1)
> >   return NULL;
> > 
> > which is the same as != 1.  In fact, you could have like (Python)
> > 
> > if num_required == params.length:
> >   c.Append('if (%(var)s.GetSize() != %(total)d')
> > else:
> >   c.Append('if (%(var)s.GetSize() < %(required)d'
> >            ' || %(var)s.GetSize() > %(total)d')
> > c.Append('  return scoped_ptr<Params>();')
> 
> Wups, my bad. Note that you can't do args.GetSize() < 0 because C++ herps a
derp
> about it always being true.

Hah.

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:123:
self._util_cc_helper.GetArray(prop, src, prop.name, dst_member))
Add a comment around here saying that the C++ helper which gets the array
handles required vs optional (took me a moment to remember this).

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:128: c.Append('Value* %(value)s =
NULL;')
It might be a good general rule, if you have local variables that end up being
substituted for strings, to keep the names the same. That would make things a
bit clearer.

So c.Append('Value* %(value_var)s = NULL') etc.

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:129: c.Append('if
(!(%(src)s->Get("%(key)s", &%(value)s)))')
I think you have some redundant parens around the "Get" here

also, should be GetWithoutPathExpansion

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:286: return_line = '  return
scoped_ptr<Params>();'
eh, IMO I'd do the same kind of thing as I suggested below, and assign to a
failure_value argument; a bit easier to read the Python below as C++ that way.

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:291: .Append('if
(!args.GetList(%(index)d, &%(var)s))')
Like I commented above, call this string substitution character %(index) ->
%(i), %(var) -> %(param_var)

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:305: 'return scoped_ptr<Params>();'))
should be passing in return_line (aka failure_value) in here instead of
scoped_ptr<Param>()?

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:333: .Eblock('}')
like comment from last round, indent this in the normal python style.

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:338: def
_GeneratePopulateChoices(self, prop, value, dst, return_line):
A few things:

 - Call "value" "value_var" instead, since that's what you're always calling the
variable that's being passed in.

 - Maybe change return_line to return_value or even failure_value, and use like
.Append('  return %s;' % failure_value)

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:387: 'type': type_var,
Same comment as earlier; call the string substitution %(type_var)

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
File tools/json_schema_compiler/cpp_type_generator.py (right):

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:83: """Gets the enum of the
choices in the given model.Property indicating no
"enum value"

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:89: """Gets the enum of the
choices in the given model.Property of the given
"enum value"

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:98: """Gets the name of the
enum for the given model.Property.
"Gets the type"

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
File tools/json_schema_compiler/h_generator.py (right):

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:99: c.Sblock('enum %s {' % enum_name)
enum_name -> enum_type

(any reason you're not using Substitute in this method? just curious)

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
File tools/json_schema_compiler/util.h (right):

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/util.h:149: 
hm?

[battre, 2012-02-09 09:00:47]

http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su...
File generated_files_will_not_submit/experimental.declarative.h (right):

http://codereview.chromium.org/9309044/diff/16005/generated_files_will_not_su...
generated_files_will_not_submit/experimental.declarative.h:10: #define
CHROME_COMMON_EXTENSIONS_API_EXPERIMENTAL_DECLARATIVE_H__
On 2012/02/09 00:56:52, calamity wrote:
> On 2012/02/08 09:15:20, battre wrote:
> > Should this go to chrome/common?
> 
> Sorry, I'm not sure what you mean? The file is generated to a temporary
> directory during the build. chrome/common is simply the include path
(generated
> from the relative path of root to the api json).

I was wondering whether chrome/browser/extensions/ would be more appropriate
than chrome/common/extensions/ because the API implementations are in
chrome/browser/extensions.

[Yoyo Zhou, 2012-02-10 01:49:33]

Looks pretty good for the most part.

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:281: # optional argument at position
4 cannot exist without an argument at
Aaron: is mtytel's work on optional arguments to functions going to change this
at all?

http://codereview.chromium.org/9309044/diff/22001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/permissions/permissions_api.cc (right):

http://codereview.chromium.org/9309044/diff/22001/chrome/browser/extensions/a...
chrome/browser/extensions/api/permissions/permissions_api.cc:50: LOG(WARNING) <<
"IN HERE\n\n\n";
Are these no longer needed?

http://codereview.chromium.org/9309044/diff/22001/generated_files_will_not_su...
File generated_files_will_not_submit/windows.cc (right):

http://codereview.chromium.org/9309044/diff/22001/generated_files_will_not_su...
generated_files_will_not_submit/windows.cc:114: return params.Pass();
So invalid optional arguments are treated as though they are missing. But it
looks like the existing behavior is to treat these as errors.

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:292: .Append(return_line)
I think this return_line would be different if you wanted to abort on invalid
optional parameters.

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:345: BEFORE you call this method as
return_line will be used to indicate a
Maybe s/you call/the code generated by/

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:359: PropertyType.STRING:
'Value::TYPE_STRING',
These "Value::..." look like they could be attributes of PropertyType, or
perhaps the PropertyType name could be used here.

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:414: for param in
self._cpp_type_generator.GetExpandedChoicesInParams(params):
This logic looks somewhat duplicated between here and the h_generator. Consider
a utility function that you could use in both places.

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
File tools/json_schema_compiler/h_generator.py (right):

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:94: for prop in [x for x in props if
x.type_ == PropertyType.CHOICES]:
Why not just
for prop in props:
  if prop.type__ == ...:

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
File tools/json_schema_compiler/model.py (right):

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:164: # A choice needs to have its unix_name
set elsewhere
Ok. What I meant was "elsewhere" is vague.  I found GetExpandedChoicesInParams
-- it would help understanding to have it in the comment here.

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:192: unix_name = property(GetUnixName,
SetUnixName)
Ah, this line (the python magic) was hiding here. It should probably have a
newline above it.

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
File tools/json_schema_compiler/util.h (right):

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/util.h:40: // Creates a new vector containing Llist|
at |out|. Returns
typo: L -> |
Also, this doesn't appear to create a new vector.

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/util.h:129: // Set |out| to the the contents of
|from|. Requires GetItemFromList to be
This one doesn't appear to erase |out|'s existing contents.

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/util.h:132: void SetArrayToList(
This name is a little awkward. I'd think it means
array = list;
when it's actually vice versa.
What about AddArrayItemsToList or some such? Maybe ...From...?

Same for the names below.

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
File tools/json_schema_compiler/util_cc_helper.py (right):

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/util_cc_helper.py:60: """Sets dst.name to the array
at src
Generates code to...

[calamity, 2012-02-10 03:52:50]

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:123:
self._util_cc_helper.GetArray(prop, src, prop.name, dst_member))
On 2012/02/09 02:49:36, kalman wrote:
> Add a comment around here saying that the C++ helper which gets the array
> handles required vs optional (took me a moment to remember this).

Done.

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:128: c.Append('Value* %(value)s =
NULL;')
On 2012/02/09 02:49:36, kalman wrote:
> It might be a good general rule, if you have local variables that end up being
> substituted for strings, to keep the names the same. That would make things a
> bit clearer.
> 
> So c.Append('Value* %(value_var)s = NULL') etc.

Done.

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:129: c.Append('if
(!(%(src)s->Get("%(key)s", &%(value)s)))')
On 2012/02/09 02:49:36, kalman wrote:
> I think you have some redundant parens around the "Get" here
> 
> also, should be GetWithoutPathExpansion

Done.

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:286: return_line = '  return
scoped_ptr<Params>();'
On 2012/02/09 02:49:36, kalman wrote:
> eh, IMO I'd do the same kind of thing as I suggested below, and assign to a
> failure_value argument; a bit easier to read the Python below as C++ that way.

Done. I wanted to Substitute it but the line is everywhere and python will
complain if you try to partially substitute a string so I couldn't find a nice
place to do the substitution.

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:291: .Append('if
(!args.GetList(%(index)d, &%(var)s))')
On 2012/02/09 02:49:36, kalman wrote:
> Like I commented above, call this string substitution character %(index) ->
> %(i), %(var) -> %(param_var)

Done.

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:305: 'return scoped_ptr<Params>();'))
On 2012/02/09 02:49:36, kalman wrote:
> should be passing in return_line (aka failure_value) in here instead of
> scoped_ptr<Param>()?

By this point we've done our optional check. Anything that goes wrong here is a
proper failure.

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:333: .Eblock('}')
On 2012/02/09 02:49:36, kalman wrote:
> like comment from last round, indent this in the normal python style.

Done.

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:338: def
_GeneratePopulateChoices(self, prop, value, dst, return_line):
On 2012/02/09 02:49:36, kalman wrote:
> A few things:
> 
>  - Call "value" "value_var" instead, since that's what you're always calling
the
> variable that's being passed in.
> 
>  - Maybe change return_line to return_value or even failure_value, and use
like
> .Append('  return %s;' % failure_value)

Done.

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:387: 'type': type_var,
On 2012/02/09 02:49:36, kalman wrote:
> Same comment as earlier; call the string substitution %(type_var)

Done.

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
File tools/json_schema_compiler/cpp_type_generator.py (right):

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:83: """Gets the enum of the
choices in the given model.Property indicating no
On 2012/02/09 02:49:36, kalman wrote:
> "enum value"

Done.

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:89: """Gets the enum of the
choices in the given model.Property of the given
On 2012/02/09 02:49:36, kalman wrote:
> "enum value"

Done.

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:98: """Gets the name of the
enum for the given model.Property.
On 2012/02/09 02:49:36, kalman wrote:
> "Gets the type"

Done.

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
File tools/json_schema_compiler/h_generator.py (right):

http://codereview.chromium.org/9309044/diff/16005/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:99: c.Sblock('enum %s {' % enum_name)
On 2012/02/09 02:49:36, kalman wrote:
> enum_name -> enum_type
> 
> (any reason you're not using Substitute in this method? just curious)

Done. Just got lazy I guess.

http://codereview.chromium.org/9309044/diff/22001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/permissions/permissions_api.cc (right):

http://codereview.chromium.org/9309044/diff/22001/chrome/browser/extensions/a...
chrome/browser/extensions/api/permissions/permissions_api.cc:50: LOG(WARNING) <<
"IN HERE\n\n\n";
On 2012/02/10 01:49:33, Yoyo Zhou wrote:
> Are these no longer needed?

Done.

http://codereview.chromium.org/9309044/diff/22001/generated_files_will_not_su...
File generated_files_will_not_submit/windows.cc (right):

http://codereview.chromium.org/9309044/diff/22001/generated_files_will_not_su...
generated_files_will_not_submit/windows.cc:114: return params.Pass();
On 2012/02/10 01:49:33, Yoyo Zhou wrote:
> So invalid optional arguments are treated as though they are missing. But it
> looks like the existing behavior is to treat these as errors.

Good catch. This is a big-ish change. I think it should go in the next patch
(which will have badass tests which can actually check for this sort of
behavior).

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:292: .Append(return_line)
On 2012/02/10 01:49:33, Yoyo Zhou wrote:
> I think this return_line would be different if you wanted to abort on invalid
> optional parameters.

Agreed. I want to do a pretty major refactor regarding this for my next patch so
I'll just leave it til then?

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:345: BEFORE you call this method as
return_line will be used to indicate a
On 2012/02/10 01:49:33, Yoyo Zhou wrote:
> Maybe s/you call/the code generated by/

Done.

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:359: PropertyType.STRING:
'Value::TYPE_STRING',
On 2012/02/10 01:49:33, Yoyo Zhou wrote:
> These "Value::..." look like they could be attributes of PropertyType, or
> perhaps the PropertyType name could be used here.

I feel that this would couple the python code too closely to the C++ code? Using
the PropertyType.name for names that we generate is one thing, using it to refer
to code that isn't ours seems dodgy.

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:414: for param in
self._cpp_type_generator.GetExpandedChoicesInParams(params):
On 2012/02/10 01:49:33, Yoyo Zhou wrote:
> This logic looks somewhat duplicated between here and the h_generator.
Consider
> a utility function that you could use in both places.

Done.

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
File tools/json_schema_compiler/h_generator.py (right):

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:94: for prop in [x for x in props if
x.type_ == PropertyType.CHOICES]:
On 2012/02/10 01:49:33, Yoyo Zhou wrote:
> Why not just
> for prop in props:
>   if prop.type__ == ...:

Hah, that's what is was before. I thought it'd be more python-y to use a list
comprehension. Reverted.

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
File tools/json_schema_compiler/model.py (right):

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:164: # A choice needs to have its unix_name
set elsewhere
On 2012/02/10 01:49:33, Yoyo Zhou wrote:
> Ok. What I meant was "elsewhere" is vague.  I found GetExpandedChoicesInParams
> -- it would help understanding to have it in the comment here.

Done.

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:192: unix_name = property(GetUnixName,
SetUnixName)
On 2012/02/10 01:49:33, Yoyo Zhou wrote:
> Ah, this line (the python magic) was hiding here. It should probably have a
> newline above it.

Done.

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
File tools/json_schema_compiler/util.h (right):

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/util.h:40: // Creates a new vector containing Llist|
at |out|. Returns
On 2012/02/10 01:49:33, Yoyo Zhou wrote:
> typo: L -> |
> Also, this doesn't appear to create a new vector.

Done. Also erases |out|.

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/util.h:129: // Set |out| to the the contents of
|from|. Requires GetItemFromList to be
On 2012/02/10 01:49:33, Yoyo Zhou wrote:
> This one doesn't appear to erase |out|'s existing contents.

Done. Erases now.

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/util.h:132: void SetArrayToList(
On 2012/02/10 01:49:33, Yoyo Zhou wrote:
> This name is a little awkward. I'd think it means
> array = list;
> when it's actually vice versa.
> What about AddArrayItemsToList or some such? Maybe ...From...?
> 
> Same for the names below.

You're right. Since I added the erasure of |out|, I didn't want to use Add in
the name. I chose Populate instead? Let me know if you think of something
better.

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
File tools/json_schema_compiler/util_cc_helper.py (right):

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/util_cc_helper.py:60: """Sets dst.name to the array
at src
On 2012/02/10 01:49:33, Yoyo Zhou wrote:
> Generates code to...

Done.

[Yoyo Zhou, 2012-02-10 19:43:21]

LGTM

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:292: .Append(return_line)
On 2012/02/10 03:52:50, calamity wrote:
> On 2012/02/10 01:49:33, Yoyo Zhou wrote:
> > I think this return_line would be different if you wanted to abort on
invalid
> > optional parameters.
> 
> Agreed. I want to do a pretty major refactor regarding this for my next patch
so
> I'll just leave it til then?

Ok, just add a TODO for it.

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:359: PropertyType.STRING:
'Value::TYPE_STRING',
On 2012/02/10 03:52:50, calamity wrote:
> On 2012/02/10 01:49:33, Yoyo Zhou wrote:
> > These "Value::..." look like they could be attributes of PropertyType, or
> > perhaps the PropertyType name could be used here.
> 
> I feel that this would couple the python code too closely to the C++ code?
Using
> the PropertyType.name for names that we generate is one thing, using it to
refer
> to code that isn't ours seems dodgy.

Ok, makes sense.

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
File tools/json_schema_compiler/h_generator.py (right):

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:94: for prop in [x for x in props if
x.type_ == PropertyType.CHOICES]:
On 2012/02/10 03:52:50, calamity wrote:
> On 2012/02/10 01:49:33, Yoyo Zhou wrote:
> > Why not just
> > for prop in props:
> >   if prop.type__ == ...:
> 
> Hah, that's what is was before. I thought it'd be more python-y to use a list
> comprehension. Reverted.

It is more python-y if it becomes less code, but in this case you're just
iterating over the values, not making a list.

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
File tools/json_schema_compiler/util.h (right):

http://codereview.chromium.org/9309044/diff/22001/tools/json_schema_compiler/...
tools/json_schema_compiler/util.h:132: void SetArrayToList(
On 2012/02/10 03:52:50, calamity wrote:
> On 2012/02/10 01:49:33, Yoyo Zhou wrote:
> > This name is a little awkward. I'd think it means
> > array = list;
> > when it's actually vice versa.
> > What about AddArrayItemsToList or some such? Maybe ...From...?
> > 
> > Same for the names below.
> 
> You're right. Since I added the erasure of |out|, I didn't want to use Add in
> the name. I chose Populate instead? Let me know if you think of something
> better.

All the populating looks pretty consistent.

[commit-bot: I haz the power, 2012-02-15 03:58:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/9309044/34001

[commit-bot: I haz the power, 2012-02-15 11:11:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/9309044/42001

[commit-bot: I haz the power, 2012-02-15 14:52:36]

Change committed as 122082