Let json schema compiler handle using arrays as types

Allow json schema compiler to use arrays as types

Right now the json schema compiler can only handle objects as types. This allows
the schema compiler to use arrays as types. For an example look at
browserAction.

BUG=122075
TEST=cpp_type_generator_test.py


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

[cduvall, 2012-04-07 00:39:23]

Turned out to be not quite as bad as I thought. Just had to pass around the list
of types to resolve the $refs.

[Aaron Boodman, 2012-04-09 18:28:25]

redirecting to yoz

[Yoyo Zhou, 2012-04-09 23:48:46]

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/cc_...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/cc_...
tools/json_schema_compiler/cc_generator.py:324: if prop.type_  ==
PropertyType.OBJECT or \
style nits: extra space. Also, the style is to continue lines with an unclosed
parenthesis rather than with backslash.

See http://dev.chromium.org/developers/coding-style

Here's my proposal: have helper functions e.g. IsObjectOrObjectRefType and
IsArrayOrArrayRefType (these names are a little unwieldy; feel free to improve
upon them). Each of these would call GetRefType which would tell you the
referenced type of a REF, either OBJECT or ARRAY.

As a function of CCGenerator, GetRefType can then look up the types in the
namespace. This way you'd get types at 'use time', so you don't need to worry
about declaration order, and it would work with types defined in dependent
namespaces, etc.

Let me know if you think this makes sense.

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/cpp...
File tools/json_schema_compiler/cpp_type_generator.py (right):

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/cpp...
tools/json_schema_compiler/cpp_type_generator.py:175: for type_ in types:
I think you may want the same non-array check here.

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/cpp...
File tools/json_schema_compiler/cpp_type_generator_test.py (right):

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/cpp...
tools/json_schema_compiler/cpp_type_generator_test.py:96: def
testGetTypeArray(self):
Does this test the refs to arrays? (i.e. something similar to
testGetTypeIncludedRef)

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/cpp...
File tools/json_schema_compiler/cpp_util.py (right):

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/cpp...
tools/json_schema_compiler/cpp_util.py:48: def GetValueType(prop):
This would need an additional argument (namespace or types).

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/mod...
File tools/json_schema_compiler/model.py (right):

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/mod...
tools/json_schema_compiler/model.py:53: for type_json in json.get('types', []):
This looks fragile. You're building the types and passing the list of types in
as you go along, so it is now dependent on the order in which things are
declared (although I think the ordering is okay in existing API json).

An alternative approach is to not try to establish what kind of referenced type
you have until you need to use it. (I wrote more about that in the other
comment.)

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/mod...
tools/json_schema_compiler/model.py:58: self.functions[function_json['name']] =
Function(self, function_json,
nit: line continuation should be 4 spaces if you put the first arg on the new
line, otherwise it should line up with the open paren. (Sadly, that isn't the
style in most of the file.)

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/mod...
tools/json_schema_compiler/model.py:175: Properties:
Make sure to update these.

[cduvall, 2012-04-10 03:06:08]

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/cc_...
File tools/json_schema_compiler/cc_generator.py (right):

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/cc_...
tools/json_schema_compiler/cc_generator.py:324: if prop.type_  ==
PropertyType.OBJECT or \
On 2012/04/09 23:48:47, Yoyo Zhou wrote:
> style nits: extra space. Also, the style is to continue lines with an unclosed
> parenthesis rather than with backslash.
> 
> See http://dev.chromium.org/developers/coding-style
> 
> Here's my proposal: have helper functions e.g. IsObjectOrObjectRefType and
> IsArrayOrArrayRefType (these names are a little unwieldy; feel free to improve
> upon them). Each of these would call GetRefType which would tell you the
> referenced type of a REF, either OBJECT or ARRAY.
> 
> As a function of CCGenerator, GetRefType can then look up the types in the
> namespace. This way you'd get types at 'use time', so you don't need to worry
> about declaration order, and it would work with types defined in dependent
> namespaces, etc.
> 
> Let me know if you think this makes sense.

That makes it way cleaner. Good idea.

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/cpp...
File tools/json_schema_compiler/cpp_type_generator.py (right):

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/cpp...
tools/json_schema_compiler/cpp_type_generator.py:175: for type_ in types:
On 2012/04/09 23:48:47, Yoyo Zhou wrote:
> I think you may want the same non-array check here.

Done.

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/cpp...
File tools/json_schema_compiler/cpp_type_generator_test.py (right):

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/cpp...
tools/json_schema_compiler/cpp_type_generator_test.py:96: def
testGetTypeArray(self):
On 2012/04/09 23:48:47, Yoyo Zhou wrote:
> Does this test the refs to arrays? (i.e. something similar to
> testGetTypeIncludedRef)

This tests if a type that is an array will generate the correct typedef in the
.h file.

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/cpp...
File tools/json_schema_compiler/cpp_util.py (right):

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/cpp...
tools/json_schema_compiler/cpp_util.py:48: def GetValueType(prop):
On 2012/04/09 23:48:47, Yoyo Zhou wrote:
> This would need an additional argument (namespace or types).

Ended up resolving the type before I passed it here, so it doesn't need any more
parameters.

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/mod...
File tools/json_schema_compiler/model.py (right):

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/mod...
tools/json_schema_compiler/model.py:53: for type_json in json.get('types', []):
On 2012/04/09 23:48:47, Yoyo Zhou wrote:
> This looks fragile. You're building the types and passing the list of types in
> as you go along, so it is now dependent on the order in which things are
> declared (although I think the ordering is okay in existing API json).
> 
> An alternative approach is to not try to establish what kind of referenced
type
> you have until you need to use it. (I wrote more about that in the other
> comment.)

Fixed this to not pass types around everywhere.

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/mod...
tools/json_schema_compiler/model.py:58: self.functions[function_json['name']] =
Function(self, function_json,
On 2012/04/09 23:48:47, Yoyo Zhou wrote:
> nit: line continuation should be 4 spaces if you put the first arg on the new
> line, otherwise it should line up with the open paren. (Sadly, that isn't the
> style in most of the file.)

Done.

http://codereview.chromium.org/10022005/diff/1/tools/json_schema_compiler/mod...
tools/json_schema_compiler/model.py:175: Properties:
On 2012/04/09 23:48:47, Yoyo Zhou wrote:
> Make sure to update these.

Done.

[Yoyo Zhou, 2012-04-10 19:26:48]

Yes, this definitely looks better.

https://chromiumcodereview.appspot.com/10022005/diff/1/tools/json_schema_comp...
File tools/json_schema_compiler/cpp_type_generator_test.py (right):

https://chromiumcodereview.appspot.com/10022005/diff/1/tools/json_schema_comp...
tools/json_schema_compiler/cpp_type_generator_test.py:96: def
testGetTypeArray(self):
On 2012/04/10 03:06:08, cduvall wrote:
> On 2012/04/09 23:48:47, Yoyo Zhou wrote:
> > Does this test the refs to arrays? (i.e. something similar to
> > testGetTypeIncludedRef)
> 
> This tests if a type that is an array will generate the correct typedef in the
> .h file.

Ok, but it's probably worth having a test with the array type in a more complex
structure, like that test.

https://chromiumcodereview.appspot.com/10022005/diff/9001/tools/json_schema_c...
File tools/json_schema_compiler/cc_generator.py (right):

https://chromiumcodereview.appspot.com/10022005/diff/9001/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:633: def _GetRef(self, prop):
I'm not sure this name is clear enoguh. How about GetReferencedType?

https://chromiumcodereview.appspot.com/10022005/diff/9001/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:634: """Returns the item a $ref is
referring to.
s/item/type/, and add a note on what it returns if prop is not a REF.

https://chromiumcodereview.appspot.com/10022005/diff/9001/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:643: return
(self._cpp_type_generator._type_namespaces[qualified_name]
The _ is supposed to prefix private members, so you're getting at
_cpp_type_generator's private _type_namespaces. It seems like what you want is
cpp_type_generator._ResolveTypeNamespace to get the namespace; you can move this
into its public interface (removing the _) if you need to. But I would look
carefully to see if there's an even better fit that is already there.

https://chromiumcodereview.appspot.com/10022005/diff/9001/tools/json_schema_c...
File tools/json_schema_compiler/cpp_type_generator.py (right):

https://chromiumcodereview.appspot.com/10022005/diff/9001/tools/json_schema_c...
tools/json_schema_compiler/cpp_type_generator.py:176: if
(self._type_namespaces[self._QualifyName(namespace, type_)]
I feel like this all should be type_.type_ (as below), or this line should be
self.namespace. You definitely don't need the type-to-namespace map here.

[cduvall, 2012-04-12 00:41:23]

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

http://codereview.chromium.org/10022005/diff/9001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:633: def _GetRef(self, prop):
On 2012/04/10 19:26:48, Yoyo Zhou wrote:
> I'm not sure this name is clear enoguh. How about GetReferencedType?

This function is actually returning the whole property (see line 488), not just
the type, so I called it GetReferencedProperty. I also moved it into
cpp_type_generator, since I think it fits better there.

http://codereview.chromium.org/10022005/diff/9001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:634: """Returns the item a $ref is
referring to.
On 2012/04/10 19:26:48, Yoyo Zhou wrote:
> s/item/type/, and add a note on what it returns if prop is not a REF.

Done. See previous comment.

http://codereview.chromium.org/10022005/diff/9001/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:643: return
(self._cpp_type_generator._type_namespaces[qualified_name]
On 2012/04/10 19:26:48, Yoyo Zhou wrote:
> The _ is supposed to prefix private members, so you're getting at
> _cpp_type_generator's private _type_namespaces. It seems like what you want is
> cpp_type_generator._ResolveTypeNamespace to get the namespace; you can move
this
> into its public interface (removing the _) if you need to. But I would look
> carefully to see if there's an even better fit that is already there.

Done.

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

http://codereview.chromium.org/10022005/diff/9001/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:176: if
(self._type_namespaces[self._QualifyName(namespace, type_)]
On 2012/04/10 19:26:48, Yoyo Zhou wrote:
> I feel like this all should be type_.type_ (as below), or this line should be
> self.namespace. You definitely don't need the type-to-namespace map here.

Done.

[Yoyo Zhou, 2012-04-12 00:54:57]

LGTM, with a few cleanup comments.

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

http://codereview.chromium.org/10022005/diff/12002/tools/json_schema_compiler...
tools/json_schema_compiler/cc_generator.py:16: def __init__(self, namespace,
cpp_type_generator, referenced_schemas):
You can remove this since you don't use this anymore.

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

http://codereview.chromium.org/10022005/diff/12002/tools/json_schema_compiler...
tools/json_schema_compiler/cpp_type_generator.py:224: if prop.ref_type in
self._ResolveTypeNamespace(prop.ref_type).types.keys():
Use ....types.get(prop.ref_type, None) for all this.

[commit-bot: I haz the power, 2012-04-12 01:12:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/10022005/12005

[commit-bot: I haz the power, 2012-04-12 04:30:25]

Change committed as 131927