json_schema_compiler: any, additionalProperties, functions on types

json_schema_compiler: any, additionalProperties, functions on types

Add support and tests for more json types. Also fixed a number of API jsons.

BUG=
TEST=


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

[Yoyo Zhou, 2012-02-28 22:23:40]

TM, mostly LG.

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

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:139:
c.Append('out->additional_properties'
The additional properties can't have any name conflicts with the existing keys?

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/c...
File tools/json_schema_compiler/code.py (right):

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/c...
tools/json_schema_compiler/code.py:45: for substitute, line in
zip(obj._substitute, obj._code):
itertools.izip?

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/c...
tools/json_schema_compiler/code.py:84: Unaffected by code.Substitute().
You could make substitute a parameter to this function too, if that could be
useful.

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

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/h...
tools/json_schema_compiler/h_generator.py:252: "because it's redundant.")
nit: indent to paren

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

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:65: - |properties| a map of property names
to their model.Property
Add |functions| to the doc.

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:74: 'functions' in json), name + " has no
properties or functions"
nit: indent to paren

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:84: self.functions[function.name] = function
This could fit on one line (same with 56-57).

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:95: prop.type_ =
PropertyType.ADDITIONAL_PROPERTIES
Seems like an abstraction violation to have to set this here and not in
Property.__init__.

[not at google - send to devlin, 2012-02-28 22:41:17]

This is so cool.

I haven't looked at the tests yet, will do now.

http://codereview.chromium.org/9491002/diff/3001/build/json_schema_compile.gypi
File build/json_schema_compile.gypi (right):

http://codereview.chromium.org/9491002/diff/3001/build/json_schema_compile.gy...
build/json_schema_compile.gypi:28: '<(api_gen_dir)/any.h',
alphabetical order

http://codereview.chromium.org/9491002/diff/3001/chrome/common/extensions/api...
File chrome/common/extensions/api/fileBrowserPrivate.json (right):

http://codereview.chromium.org/9491002/diff/3001/chrome/common/extensions/api...
chrome/common/extensions/api/fileBrowserPrivate.json:365: "properties": {},
FYI: I just submitted a patch that makes this unnecessary ("properties": {});
the lack of a properties property -> empty properties.

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/a...
File tools/json_schema_compiler/any.cc (right):

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/a...
tools/json_schema_compiler/any.cc:18: value_.reset(from_value.DeepCopy());
Is it true that this should be called exactly once? Perhaps it makes sense to
call it Init then, and CHECK beforehand that value_ is empty.

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/a...
File tools/json_schema_compiler/any.h (right):

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/a...
tools/json_schema_compiler/any.h:19: class Any {
document

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/a...
File tools/json_schema_compiler/any_helper.py (right):

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/a...
tools/json_schema_compiler/any_helper.py:16: dst: Type*
The parameters and documentation seem wrong. Where does src come into it? Is
having a Create method worth the additional overhead that just using ANY_CLASS
directly would have?

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/a...
File tools/json_schema_compiler/api_gen_util.gyp (right):

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/a...
tools/json_schema_compiler/api_gen_util.gyp:14: 'any.cc',
keep in alphabetical order

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/c...
File tools/json_schema_compiler/code.py (right):

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/c...
tools/json_schema_compiler/code.py:84: Unaffected by code.Substitute().
Might a better solution be to just escape all % characters as %% ? Nothing else
woud need to change, then.

You should also say in the documentation that string substitutions are
automatically escaped, therefore not supported in comments.

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/c...
File tools/json_schema_compiler/cpp_type_generator.py (left):

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_type_generator.py:147: elif prop.type_ ==
PropertyType.OBJECT:
Confused, why not support OBJECT?  Even if you don't, still have an explicit
branch for it with a comment and raised error there.

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

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/h...
tools/json_schema_compiler/h_generator.py:111: c.Append('%s %s_type;' %
(enum_name, prop.unix_name))
browsing through generated code (specifically the storage API); could you add a
new line (as in Append()) here, the KeysType is right above keys_string which
looks strange.

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/h...
tools/json_schema_compiler/h_generator.py:127: # instantiable because they're
dealt with renderer side.
See my comment about this comment in model.py.  Maybe just reference that
comment here?

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/h...
tools/json_schema_compiler/h_generator.py:129: assert not type_.properties
Also: again, asserts are for internal errors not errors with input, so this
should raise an exception

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

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:48: # TODO(calamity): Implement this
business.
What is "this business"?  Implementation of properties on namespaces?

Yeah, I can't think of when it'd be useful, at the moment, to support this.

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:74: 'functions' in json), name + " has no
properties or functions"
nit... I like arranging multiple linked conditionals like this by lining them up
like:

assert('properties' in json or
       'additionalProperties' in json or
       'functions' in json, name + ' has no properties or functions')

I don't think an assert is right here though.  Asserts should be for sanity
checking the correctness of your own code (that is, json_schema_compiler) but
this is validating the correctness of input to the program.  It should raise
some appropriate error instead.

Oh, and can types really have additionalProperties?  That seems odd.  You'd
expect types to be... well, entirely typed.

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:86: # Ignore callbacks in objects because
they're handled renderer-side
Reference to "renderer-side" doesn't make much sense out of context in the
compiler.  You might want to say like

TODO(calamity): support functions (callbacks) on properties.  The model doesn't
support it yet because to h/cc generators don't -- this is because we'd need to
hook it into a base::Callback or something.

However, pragmatically it's not necessary to support them anyway, since the
instances of functions-on-properties in the extension APIs are all handled in
pure Javascript on the render process (and .: never reach C++ let alone the
browser).

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:96: self.properties['additionalProperties']
= prop
So... additionalProperties is stored as a property called
'additionalProperties'?  I guess this might make code generation easier, but it
could cause problems if there actually is a property called
'additionalProperties' I assume.

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:207: self.from_json = from_json
you have repeated assignments here.

[not at google - send to devlin, 2012-02-29 03:06:30]

comments on tests

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/t...
File tools/json_schema_compiler/test/additionalProperties.json (right):

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/t...
tools/json_schema_compiler/test/additionalProperties.json:5: ],
seems unnecessary

actually, what about additionalProperties in a type, didn't you support that...?
though as I mentioned in previous comments, it seems surprising that you'd
support it.

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/t...
tools/json_schema_compiler/test/additionalProperties.json:45: 
some more things to test: (and in C++)
- more than one property, only setting one of them (should error)
- setting a property to something wrong (like int property set to string, should
error)

just to make sure that additionalProperties doesn't interfere with the error
checking.

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/t...
File tools/json_schema_compiler/test/additional_properties_unittest.cc (right):

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/t...
tools/json_schema_compiler/test/additional_properties_unittest.cc:21: int
int_temp = 0;
you could replace all of this to the bottom with

EXPECT_TRUE(params->Equals(&param_object_value));

... except you can't, because you release() it. So how about:
1. have param_object_value on the stack.
2. DeepCopy() rather than releasing it
3. profit.

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/t...
tools/json_schema_compiler/test/additional_properties_unittest.cc:42:
DictionaryValue* result_dict = NULL;
same comment as above, try to use Equals on the value (you'd have to remove one
of the keys from result_object_value first).

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/t...
File tools/json_schema_compiler/test/any_unittest.cc (right):

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/t...
tools/json_schema_compiler/test/any_unittest.cc:13: scoped_ptr<AnyType>
any_type(new AnyType());
just have any_type on the stack

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/t...
tools/json_schema_compiler/test/any_unittest.cc:17:
EXPECT_TRUE(any_type->any.value().IsType(Value::TYPE_STRING));
again, try to use Equals rather than all this manual checking

that would also let you have a more complex test example, like, multiple
keys/types.

ditto for rest of file

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/t...
File tools/json_schema_compiler/test/functionsOnTypes.json (right):

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/t...
tools/json_schema_compiler/test/functionsOnTypes.json:25: }
would be better to test something that has a function too (like storage.get or
something)

[calamity, 2012-03-01 04:47:09]

https://chromiumcodereview.appspot.com/9491002/diff/3001/build/json_schema_co...
File build/json_schema_compile.gypi (right):

https://chromiumcodereview.appspot.com/9491002/diff/3001/build/json_schema_co...
build/json_schema_compile.gypi:28: '<(api_gen_dir)/any.h',
On 2012/02/28 22:41:17, kalman wrote:
> alphabetical order

Done.

https://chromiumcodereview.appspot.com/9491002/diff/3001/chrome/common/extens...
File chrome/common/extensions/api/fileBrowserPrivate.json (right):

https://chromiumcodereview.appspot.com/9491002/diff/3001/chrome/common/extens...
chrome/common/extensions/api/fileBrowserPrivate.json:365: "properties": {},
On 2012/02/28 22:41:17, kalman wrote:
> FYI: I just submitted a patch that makes this unnecessary ("properties": {});
> the lack of a properties property -> empty properties.

Removed.

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
File tools/json_schema_compiler/any.cc (right):

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/any.cc:18: value_.reset(from_value.DeepCopy());
On 2012/02/28 22:41:17, kalman wrote:
> Is it true that this should be called exactly once? Perhaps it makes sense to
> call it Init then, and CHECK beforehand that value_ is empty.

Done.

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
File tools/json_schema_compiler/any.h (right):

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/any.h:19: class Any {
On 2012/02/28 22:41:17, kalman wrote:
> document

Done.

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
File tools/json_schema_compiler/api_gen_util.gyp (right):

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/api_gen_util.gyp:14: 'any.cc',
On 2012/02/28 22:41:17, kalman wrote:
> keep in alphabetical order

Done.

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

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/cc_generator.py:139:
c.Append('out->additional_properties'
On 2012/02/28 22:23:41, Yoyo Zhou wrote:
> The additional properties can't have any name conflicts with the existing
keys?

The additional properties are the keys in the dictionary that didn't map to a
defined property.

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
File tools/json_schema_compiler/code.py (right):

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/code.py:45: for substitute, line in
zip(obj._substitute, obj._code):
On 2012/02/28 22:23:41, Yoyo Zhou wrote:
> itertools.izip?

Done.

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/code.py:84: Unaffected by code.Substitute().
On 2012/02/28 22:41:17, kalman wrote:
> Might a better solution be to just escape all % characters as %% ? Nothing
else
> woud need to change, then.
> 
> You should also say in the documentation that string substitutions are
> automatically escaped, therefore not supported in comments.

Escaping is a little more tricky that that. Since strings can get substituted
after a Concat, we need to know which lines are comments so we don't escape
them.

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/code.py:84: Unaffected by code.Substitute().
On 2012/02/28 22:23:41, Yoyo Zhou wrote:
> You could make substitute a parameter to this function too, if that could be
> useful.

I think it's safer to disallow that. The problem here was % characters in
descriptions and I'd hate for someone else to accidentally allow this and
scratch their heads over why there's an error.

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
File tools/json_schema_compiler/cpp_type_generator.py (left):

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/cpp_type_generator.py:147: elif prop.type_ ==
PropertyType.OBJECT:
On 2012/02/28 22:41:17, kalman wrote:
> Confused, why not support OBJECT?  Even if you don't, still have an explicit
> branch for it with a comment and raised error there.

Moved this up, sorry. wanted all the simple, 1-liner things together.

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
File tools/json_schema_compiler/h_generator.py (right):

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/h_generator.py:111: c.Append('%s %s_type;' %
(enum_name, prop.unix_name))
On 2012/02/28 22:41:17, kalman wrote:
> browsing through generated code (specifically the storage API); could you add
a
> new line (as in Append()) here, the KeysType is right above keys_string which
> looks strange.

Done.

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/h_generator.py:127: # instantiable because they're
dealt with renderer side.
On 2012/02/28 22:41:17, kalman wrote:
> See my comment about this comment in model.py.  Maybe just reference that
> comment here?

Done.

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/h_generator.py:129: assert not type_.properties
On 2012/02/28 22:41:17, kalman wrote:
> Also: again, asserts are for internal errors not errors with input, so this
> should raise an exception

Done.

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/h_generator.py:252: "because it's redundant.")
On 2012/02/28 22:23:41, Yoyo Zhou wrote:
> nit: indent to paren

Done.

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

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/model.py:48: # TODO(calamity): Implement this
business.
On 2012/02/28 22:41:17, kalman wrote:
> What is "this business"?  Implementation of properties on namespaces?
> 
> Yeah, I can't think of when it'd be useful, at the moment, to support this. 

Leaving this TODO as discussed (for shared structures or constants across a
namespace).

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/model.py:65: - |properties| a map of property names
to their model.Property
On 2012/02/28 22:23:41, Yoyo Zhou wrote:
> Add |functions| to the doc.

Done.

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/model.py:74: 'functions' in json), name + " has no
properties or functions"
On 2012/02/28 22:41:17, kalman wrote:
> nit... I like arranging multiple linked conditionals like this by lining them
up
> like:
> 
> assert('properties' in json or
>        'additionalProperties' in json or
>        'functions' in json, name + ' has no properties or functions')
> 
> I don't think an assert is right here though.  Asserts should be for sanity
> checking the correctness of your own code (that is, json_schema_compiler) but
> this is validating the correctness of input to the program.  It should raise
> some appropriate error instead.
> 
> Oh, and can types really have additionalProperties?  That seems odd.  You'd
> expect types to be... well, entirely typed.

Types as in OBJECT properties. Should I have a flag to disallow
additionalProperties on a root type or just test this case?

Added a ParseException class that prints the hierarchy to the model object.

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/model.py:84: self.functions[function.name] = function
On 2012/02/28 22:23:41, Yoyo Zhou wrote:
> This could fit on one line (same with 56-57).
like
self.functions[function_json['name']] = Function(function_json)?

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/model.py:86: # Ignore callbacks in objects because
they're handled renderer-side
On 2012/02/28 22:41:17, kalman wrote:
> Reference to "renderer-side" doesn't make much sense out of context in the
> compiler.  You might want to say like
> 
> TODO(calamity): support functions (callbacks) on properties.  The model
doesn't
> support it yet because to h/cc generators don't -- this is because we'd need
to
> hook it into a base::Callback or something.
> 
> However, pragmatically it's not necessary to support them anyway, since the
> instances of functions-on-properties in the extension APIs are all handled in
> pure Javascript on the render process (and .: never reach C++ let alone the
> browser).

Done.

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/model.py:95: prop.type_ =
PropertyType.ADDITIONAL_PROPERTIES
On 2012/02/28 22:23:41, Yoyo Zhou wrote:
> Seems like an abstraction violation to have to set this here and not in
> Property.__init__.

Made a flag in Property.__init__.

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/model.py:96: self.properties['additionalProperties']
= prop
On 2012/02/28 22:41:17, kalman wrote:
> So... additionalProperties is stored as a property called
> 'additionalProperties'?  I guess this might make code generation easier, but
it
> could cause problems if there actually is a property called
> 'additionalProperties' I assume.

Raised an exception if any 2 properties have the same unix_name.

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/model.py:207: self.from_json = from_json
On 2012/02/28 22:41:17, kalman wrote:
> you have repeated assignments here.

Done.

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
File tools/json_schema_compiler/test/additionalProperties.json (right):

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/test/additionalProperties.json:5: ],
On 2012/02/29 03:06:30, kalman wrote:
> seems unnecessary
> 
> actually, what about additionalProperties in a type, didn't you support
that...?
> though as I mentioned in previous comments, it seems surprising that you'd
> support it.

Done.

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/test/additionalProperties.json:45: 
On 2012/02/29 03:06:30, kalman wrote:
> some more things to test: (and in C++)
> - more than one property, only setting one of them (should error)
> - setting a property to something wrong (like int property set to string,
should
> error)
> 
> just to make sure that additionalProperties doesn't interfere with the error
> checking.

Done.

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
File tools/json_schema_compiler/test/additional_properties_unittest.cc (right):

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/test/additional_properties_unittest.cc:21: int
int_temp = 0;
On 2012/02/29 03:06:30, kalman wrote:
> you could replace all of this to the bottom with
> 
> EXPECT_TRUE(params->Equals(&param_object_value));
> 
> ... except you can't, because you release() it. So how about:
> 1. have param_object_value on the stack.
> 2. DeepCopy() rather than releasing it
> 3. profit.

Done.

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/test/additional_properties_unittest.cc:42:
DictionaryValue* result_dict = NULL;
On 2012/02/29 03:06:30, kalman wrote:
> same comment as above, try to use Equals on the value (you'd have to remove
one
> of the keys from result_object_value first).

Done.

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
File tools/json_schema_compiler/test/any_unittest.cc (right):

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/test/any_unittest.cc:13: scoped_ptr<AnyType>
any_type(new AnyType());
On 2012/02/29 03:06:30, kalman wrote:
> just have any_type on the stack

Done.

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/test/any_unittest.cc:17:
EXPECT_TRUE(any_type->any.value().IsType(Value::TYPE_STRING));
On 2012/02/29 03:06:30, kalman wrote:
> again, try to use Equals rather than all this manual checking
> 
> that would also let you have a more complex test example, like, multiple
> keys/types.
> 
> ditto for rest of file

Done.

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
File tools/json_schema_compiler/test/functionsOnTypes.json (right):

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/test/functionsOnTypes.json:25: }
On 2012/02/29 03:06:30, kalman wrote:
> would be better to test something that has a function too (like storage.get or
> something)

Done.

[not at google - send to devlin, 2012-03-01 07:09:37]

just 1 more I promise...

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/a...
File tools/json_schema_compiler/any_helper.py (right):

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/a...
tools/json_schema_compiler/any_helper.py:16: dst: Type*
On 2012/02/28 22:41:17, kalman wrote:
> The parameters and documentation seem wrong. Where does src come into it? Is
> having a Create method worth the additional overhead that just using ANY_CLASS
> directly would have?

Ping. I can't see this function being called anywhere.

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

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:139:
c.Append('out->additional_properties'
On 2012/03/01 04:47:09, calamity wrote:
> On 2012/02/28 22:23:41, Yoyo Zhou wrote:
> > The additional properties can't have any name conflicts with the existing
> keys?
> 
> The additional properties are the keys in the dictionary that didn't map to a
> defined property. 

I think he meant what I pointed out, that if you have additional properties then
it means you can't have a property with name additional properties.

Which as discussed, is hard to avoid if you want to maintain the simple struct
interface of these things.

One way you could avoid this would be to generate a GetAdditionalProperties
method and additional_properties_ private variable... probably not worth it
though.

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/c...
File tools/json_schema_compiler/code.py (right):

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/c...
tools/json_schema_compiler/code.py:84: Unaffected by code.Substitute().
On 2012/03/01 04:47:09, calamity wrote:
> On 2012/02/28 22:41:17, kalman wrote:
> > Might a better solution be to just escape all % characters as %% ? Nothing
> else
> > woud need to change, then.
> > 
> > You should also say in the documentation that string substitutions are
> > automatically escaped, therefore not supported in comments.
> 
> Escaping is a little more tricky that that. Since strings can get substituted
> after a Concat, we need to know which lines are comments so we don't escape
> them.

Ah, I see the problem.  Cool.

I think that parallel lists (as opposed to a mini class/struct thing) is a bit
unfortunate, but not a biggie. Well... actually that would be kind of nice I
think. You could store the line and a substitute flag, and after substituting
always set that to false (and if it's a comment then start off as false). And
then you wouldn't need that parallel list / zip / etc nastiness.  Eh, something
to consider if you want.

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

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:74: 'functions' in json), name + " has no
properties or functions"
On 2012/03/01 04:47:09, calamity wrote:
> On 2012/02/28 22:41:17, kalman wrote:
> > nit... I like arranging multiple linked conditionals like this by lining
them
> up
> > like:
> > 
> > assert('properties' in json or
> >        'additionalProperties' in json or
> >        'functions' in json, name + ' has no properties or functions')
> > 
> > I don't think an assert is right here though.  Asserts should be for sanity
> > checking the correctness of your own code (that is, json_schema_compiler)
but
> > this is validating the correctness of input to the program.  It should raise
> > some appropriate error instead.
> > 
> > Oh, and can types really have additionalProperties?  That seems odd.  You'd
> > expect types to be... well, entirely typed.
> 
> Types as in OBJECT properties. Should I have a flag to disallow
> additionalProperties on a root type or just test this case?
> 
> Added a ParseException class that prints the hierarchy to the model object.

Eh, the more you support the better I suppose. Might as well test it too since
it's cheap.

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/t...
File tools/json_schema_compiler/test/additional_properties_unittest.cc (right):

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/t...
tools/json_schema_compiler/test/additional_properties_unittest.cc:21: int
int_temp = 0;
On 2012/03/01 04:47:09, calamity wrote:
> On 2012/02/29 03:06:30, kalman wrote:
> > you could replace all of this to the bottom with
> > 
> > EXPECT_TRUE(params->Equals(&param_object_value));
> > 
> > ... except you can't, because you release() it. So how about:
> > 1. have param_object_value on the stack.
> > 2. DeepCopy() rather than releasing it
> > 3. profit.
> 
> Done.

param_object_value still isn't on the stack... I mean, it's not a big point, but
it does clean things up ever so slightly.

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

http://codereview.chromium.org/9491002/diff/12001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:92: '\nCannot generate both functions
and properties on a type')
'\n'.join of a singleton? Did you mean to use a , instead of a + and remove the
starting \n?

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

http://codereview.chromium.org/9491002/diff/12001/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:135: '\nCannot generate both functions
and properties on a type')
ditto comment on \n.join

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

http://codereview.chromium.org/9491002/diff/12001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:118: self.properties[prop.unix_name] = prop
umm duplicated code?

http://codereview.chromium.org/9491002/diff/12001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:314: class ParseException(Exception):
docs

http://codereview.chromium.org/9491002/diff/12001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:319: self, 'Model parse exception at: \n' +
'\n'.join(hierarchy))
unnecessary space in the middle of "at: \n"

cos nobody likes a trailing space!

http://codereview.chromium.org/9491002/diff/12001/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:321: def HierarchyOf(prop):
write ALL the docs

call it GetModelHierarchy... why "prop", is it always a property?

[calamity, 2012-03-01 11:05:51]

https://chromiumcodereview.appspot.com/9491002/diff/12001/tools/json_schema_c...
File tools/json_schema_compiler/model.py (right):

https://chromiumcodereview.appspot.com/9491002/diff/12001/tools/json_schema_c...
tools/json_schema_compiler/model.py:118: self.properties[prop.unix_name] = prop
On 2012/03/01 07:09:37, kalman wrote:
> umm duplicated code?

Done.

https://chromiumcodereview.appspot.com/9491002/diff/12001/tools/json_schema_c...
tools/json_schema_compiler/model.py:314: class ParseException(Exception):
On 2012/03/01 07:09:37, kalman wrote:
> docs

Done.

https://chromiumcodereview.appspot.com/9491002/diff/12001/tools/json_schema_c...
tools/json_schema_compiler/model.py:319: self, 'Model parse exception at: \n' +
'\n'.join(hierarchy))
On 2012/03/01 07:09:37, kalman wrote:
> unnecessary space in the middle of "at: \n"
> 
> cos nobody likes a trailing space!

Done.

https://chromiumcodereview.appspot.com/9491002/diff/12001/tools/json_schema_c...
tools/json_schema_compiler/model.py:321: def HierarchyOf(prop):
On 2012/03/01 07:09:37, kalman wrote:
> write ALL the docs
> 
> call it GetModelHierarchy... why "prop", is it always a property?

Done.

[not at google - send to devlin, 2012-03-01 11:36:20]

Well there are a few comments that you seem to have not noticed (e.g. deleting
Create in any_helper.py, and some others) so please fix those, but then
definitely go ahead and submit. lgtm.

[Yoyo Zhou, 2012-03-01 19:02:37]

LGTM

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

https://chromiumcodereview.appspot.com/9491002/diff/3001/tools/json_schema_co...
tools/json_schema_compiler/model.py:84: self.functions[function.name] = function
On 2012/03/01 04:47:09, calamity wrote:
> On 2012/02/28 22:23:41, Yoyo Zhou wrote:
> > This could fit on one line (same with 56-57).
> like
> self.functions[function_json['name']] = Function(function_json)?

Yes, exactly.

[calamity, 2012-03-01 23:58:48]

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/c...
File tools/json_schema_compiler/code.py (right):

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/c...
tools/json_schema_compiler/code.py:84: Unaffected by code.Substitute().
On 2012/03/01 07:09:37, kalman wrote:
> On 2012/03/01 04:47:09, calamity wrote:
> > On 2012/02/28 22:41:17, kalman wrote:
> > > Might a better solution be to just escape all % characters as %% ? Nothing
> > else
> > > woud need to change, then.
> > > 
> > > You should also say in the documentation that string substitutions are
> > > automatically escaped, therefore not supported in comments.
> > 
> > Escaping is a little more tricky that that. Since strings can get
substituted
> > after a Concat, we need to know which lines are comments so we don't escape
> > them.
> 
> Ah, I see the problem.  Cool.
> 
> I think that parallel lists (as opposed to a mini class/struct thing) is a bit
> unfortunate, but not a biggie. Well... actually that would be kind of nice I
> think. You could store the line and a substitute flag, and after substituting
> always set that to false (and if it's a comment then start off as false). And
> then you wouldn't need that parallel list / zip / etc nastiness.  Eh,
something
> to consider if you want.

Done.

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

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:74: 'functions' in json), name + " has no
properties or functions"
On 2012/03/01 07:09:37, kalman wrote:
> On 2012/03/01 04:47:09, calamity wrote:
> > On 2012/02/28 22:41:17, kalman wrote:
> > > nit... I like arranging multiple linked conditionals like this by lining
> them
> > up
> > > like:
> > > 
> > > assert('properties' in json or
> > >        'additionalProperties' in json or
> > >        'functions' in json, name + ' has no properties or functions')
> > > 
> > > I don't think an assert is right here though.  Asserts should be for
sanity
> > > checking the correctness of your own code (that is, json_schema_compiler)
> but
> > > this is validating the correctness of input to the program.  It should
raise
> > > some appropriate error instead.
> > > 
> > > Oh, and can types really have additionalProperties?  That seems odd. 
You'd
> > > expect types to be... well, entirely typed.
> > 
> > Types as in OBJECT properties. Should I have a flag to disallow
> > additionalProperties on a root type or just test this case?
> > 
> > Added a ParseException class that prints the hierarchy to the model object.
> 
> Eh, the more you support the better I suppose. Might as well test it too since
> it's cheap.

Done.

http://codereview.chromium.org/9491002/diff/3001/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:84: self.functions[function.name] = function
On 2012/03/01 19:02:38, Yoyo Zhou wrote:
> On 2012/03/01 04:47:09, calamity wrote:
> > On 2012/02/28 22:23:41, Yoyo Zhou wrote:
> > > This could fit on one line (same with 56-57).
> > like
> > self.functions[function_json['name']] = Function(function_json)?
> 
> Yes, exactly.

Done.

[commit-bot: I haz the power, 2012-03-01 23:59:11]

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

[commit-bot: I haz the power, 2012-03-01 23:59:23]

Can't apply patch for file chrome/common/extensions/api/fileBrowserPrivate.json.
While running patch -p1 --forward --force;
patching file chrome/common/extensions/api/fileBrowserPrivate.json
Hunk #1 FAILED at 293.
Hunk #2 succeeded at 360 (offset -1 lines).
Hunk #3 succeeded at 470 (offset -1 lines).
1 out of 3 hunks FAILED -- saving rejects to file
chrome/common/extensions/api/fileBrowserPrivate.json.rej

[commit-bot: I haz the power, 2012-03-02 02:07:26]

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

[commit-bot: I haz the power, 2012-03-02 02:57:39]

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

[commit-bot: I haz the power, 2012-03-02 03:28:20]

Try job failure for 9491002-16009 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-03-02 04:35:24]

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

[commit-bot: I haz the power, 2012-03-02 06:25:51]

Try job failure for 9491002-16012 (retry) on mac_rel for step "ui_tests".
It's a second try, previously, step "ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-03-02 06:31:57]

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

[commit-bot: I haz the power, 2012-03-02 15:05:28]

Change committed as 124643