Add wider support to json_schema_compiler

json_schema_compiler: Added wider support for OBJECTs and ENUMs. Used the new 'dependencies' property in the jsons. Refactored to avoid code duplication. Added tests for new features and where tests were lacking.

BUG=
TEST=unit_tests --gtest_filter=JsonSchemaCompiler*


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

[calamity, 2012-02-23 11:31:00]

Sorry about the huge CL, got a lot done that would be really slow to submit
separately.

compiler.py has the 'dependencies' property change.

We generate a CreateEnumValue and util.h now creates Values to reduce special
handling in cc_generator.

OBJECT and ENUM can be used as return types. Last check, with this patch, 40/60
of the extensions apis compiled.

Bunch of tests to top it all off.

[not at google - send to devlin, 2012-02-24 03:53:05]

Basically LG, just a bunch of nits and requests to add comments where things
confuse me.

https://chromiumcodereview.appspot.com/9456007/diff/1/build/json_schema_compi...
File build/json_schema_compile.gypi (left):

https://chromiumcodereview.appspot.com/9456007/diff/1/build/json_schema_compi...
build/json_schema_compile.gypi:42: '<@(json_schema_files)',
what's this change for?

the comment up the top still references json_schema_files, btw.

https://chromiumcodereview.appspot.com/9456007/diff/1/tools/json_schema_compi...
File tools/json_schema_compiler/cc_generator.py (right):

https://chromiumcodereview.appspot.com/9456007/diff/1/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:165: def
_CreateValueFromProperty(self, prop, var, dst):
just noticed that dst isn't documented, can you document this.

Also, what is this function "serialize"ing to? It's serializing to a Value
within a DictionaryValue given by |dst|... so mention that.

oh, kind of like the comment in GenerateTypeToValue.

TBH I think all these method comments need example code. It would be much easier
to understand that way. You can probably do that in a follow-up CL, but please
do it.

https://chromiumcodereview.appspot.com/9456007/diff/1/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:177: c.Sblock('if (%s.get())' % var)
will this work for CHOICES?

https://chromiumcodereview.appspot.com/9456007/diff/1/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:215: c.Sblock('scoped_ptr<Value>
%(namespace)s::CreateEnumValue(%(arg)s) {')
call string substitution same name as variable, so %(cpp_namespace)s

https://chromiumcodereview.appspot.com/9456007/diff/1/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:322: c.Append('// %s' % prop.name)
2 spaces between the // and the %s (sorry... yeah, it's just what people do)

https://chromiumcodereview.appspot.com/9456007/diff/1/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:422: properties and enum to Value
conversion.
Ugh, I don't understand how the method name and comment relate to each other.
Also the comment doesn't parse to me.

https://chromiumcodereview.appspot.com/9456007/diff/1/tools/json_schema_compi...
tools/json_schema_compiler/cc_generator.py:452: classname + '::Result',
expanded_params))
too much indentation

https://chromiumcodereview.appspot.com/9456007/diff/1/tools/json_schema_compi...
File tools/json_schema_compiler/cpp_util.py (right):

https://chromiumcodereview.appspot.com/9456007/diff/1/tools/json_schema_compi...
tools/json_schema_compiler/cpp_util.py:58: def
CreateValueFromSingleProperty(prop, var, util_cc_helper):
I don't think this method needs to be in cpp_util, it can be moved to a private
method in cpp_generator.

(In general: avoid public util methods).

https://chromiumcodereview.appspot.com/9456007/diff/1/tools/json_schema_compi...
File tools/json_schema_compiler/model.py (left):

https://chromiumcodereview.appspot.com/9456007/diff/1/tools/json_schema_compi...
tools/json_schema_compiler/model.py:130: raise AssertionError('Name %s must be
lowerCamelCase' % name)
is it worthwhile having a warning here? are there any problems if it isn't camel
case?

https://chromiumcodereview.appspot.com/9456007/diff/1/tools/json_schema_compi...
File tools/json_schema_compiler/model.py (right):

https://chromiumcodereview.appspot.com/9456007/diff/1/tools/json_schema_compi...
tools/json_schema_compiler/model.py:64: the input format
i don't understand this comment in isolation. what's the "input format"? a
Value?

there are a lot of True/False scattered throughout this file. Could you also
explain why (in this comment) something is serializable or not? as in, what
determines that.

also, since boolean values as arguments are evil (very hard to read), can you
make it named? like serializable=False or whatever.

https://chromiumcodereview.appspot.com/9456007/diff/1/tools/json_schema_compi...
tools/json_schema_compiler/model.py:128: - |properties| the properties of an
OBJECT parameter
add |serializable|

https://chromiumcodereview.appspot.com/9456007/diff/1/tools/json_schema_compi...
tools/json_schema_compiler/model.py:130: def __init__(self, name, json,
serializable_objects):
this name makes it sound like an array or something. what is it, a flag that
says whether the objects are serializable? why would that matter to Property?

https://chromiumcodereview.appspot.com/9456007/diff/1/tools/json_schema_compi...
File tools/json_schema_compiler/test/crossref_unittest.cc (right):

https://chromiumcodereview.appspot.com/9456007/diff/1/tools/json_schema_compi...
tools/json_schema_compiler/test/crossref_unittest.cc:29: value->Set("testType",
test_type_value.release());
inline CreateTestTypeDictionary()?

https://chromiumcodereview.appspot.com/9456007/diff/1/tools/json_schema_compi...
tools/json_schema_compiler/test/crossref_unittest.cc:74:
param_object_value->Set("testType", test_type_value.release());
ditto (and everywhere else)

https://chromiumcodereview.appspot.com/9456007/diff/1/tools/json_schema_compi...
File tools/json_schema_compiler/util.h (right):

https://chromiumcodereview.appspot.com/9456007/diff/1/tools/json_schema_compi...
tools/json_schema_compiler/util.h:158: return scoped_ptr<Value>(list);
TBH a more idomatic way of writing this method would be

scoped_ptr<base::ListValue> list(new base::ListValue());
PopulateListFromArray(from, list.get());
return list.Pass();

[Yoyo Zhou, 2012-02-24 04:02:22]

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

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/cc_g...
tools/json_schema_compiler/cc_generator.py:211: """Generates a function that
returns the Value representation of an enum.
Should this mention that they are string values?

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/comp...
File tools/json_schema_compiler/compiler.py (right):

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/comp...
tools/json_schema_compiler/compiler.py:64: for namespace in referenced_api_defs:
Should this be inside the with block?

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

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/mode...
tools/json_schema_compiler/model.py:130: def __init__(self, name, json,
serializable_objects):
objects_serializable would be a clearer name, I think.

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/mode...
tools/json_schema_compiler/model.py:162: self.serializable =
serializable_objects
self.type_.serializable?

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/test...
File tools/json_schema_compiler/test/objects_unittest.cc (right):

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/objects_unittest.cc:11:
TEST(JsonSchemaCompilerObjectsTest, ReturnsObjectResultCreate) {
nit: these tests seem to be in reverse order

[calamity, 2012-02-24 15:10:40]

I had to add nocompile to extension.json because it's a dependency of tabs.json
so this CL needs an extensions reviewer now?

http://codereview.chromium.org/9456007/diff/1/build/json_schema_compile.gypi
File build/json_schema_compile.gypi (left):

http://codereview.chromium.org/9456007/diff/1/build/json_schema_compile.gypi#...
build/json_schema_compile.gypi:42: '<@(json_schema_files)',
On 2012/02/24 03:53:05, kalman wrote:
> what's this change for?
> 
> the comment up the top still references json_schema_files, btw.

Now that we use dependencies, no need to pass in all the files that comprise the
model.

Left the comment up top because it SHOULD be in inputs, but isn't because of
win-gyp.

Meant to leave a comment saying so, forgot to write the file, will come in the
next patch.

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

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/cc_g...
tools/json_schema_compiler/cc_generator.py:177: c.Sblock('if (%s.get())' % var)
On 2012/02/24 03:53:05, kalman wrote:
> will this work for CHOICES?

No. There is currently no facility for turning a CHOICES Property into a Value.
Conveniently, I think choices are only ever used for Parameters or Results and
never in a type.

Commented this.

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/cc_g...
tools/json_schema_compiler/cc_generator.py:211: """Generates a function that
returns the Value representation of an enum.
On 2012/02/24 04:02:22, Yoyo Zhou wrote:
> Should this mention that they are string values?

Done.

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/cc_g...
tools/json_schema_compiler/cc_generator.py:215: c.Sblock('scoped_ptr<Value>
%(namespace)s::CreateEnumValue(%(arg)s) {')
On 2012/02/24 03:53:05, kalman wrote:
> call string substitution same name as variable, so %(cpp_namespace)s

Done.

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/cc_g...
tools/json_schema_compiler/cc_generator.py:322: c.Append('// %s' % prop.name)
On 2012/02/24 03:53:05, kalman wrote:
> 2 spaces between the // and the %s (sorry... yeah, it's just what people do)

Removed. This made the code uglier. I must have left it in by accident.

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/cc_g...
tools/json_schema_compiler/cc_generator.py:422: properties and enum to Value
conversion.
On 2012/02/24 03:53:05, kalman wrote:
> Ugh, I don't understand how the method name and comment relate to each other.
> Also the comment doesn't parse to me.

Done.

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/cc_g...
tools/json_schema_compiler/cc_generator.py:452: classname + '::Result',
expanded_params))
On 2012/02/24 03:53:05, kalman wrote:
> too much indentation

Done.

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/comp...
File tools/json_schema_compiler/compiler.py (right):

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/comp...
tools/json_schema_compiler/compiler.py:64: for namespace in referenced_api_defs:
On 2012/02/24 04:02:22, Yoyo Zhou wrote:
> Should this be inside the with block?

It can be outside because referenced_api_defs is already defined and loaded.
There's no need for a handle to the file.

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

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/cpp_...
tools/json_schema_compiler/cpp_util.py:58: def
CreateValueFromSingleProperty(prop, var, util_cc_helper):
On 2012/02/24 03:53:05, kalman wrote:
> I don't think this method needs to be in cpp_util, it can be moved to a
private
> method in cpp_generator.
> 
> (In general: avoid public util methods).

Done.

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/mode...
File tools/json_schema_compiler/model.py (left):

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/mode...
tools/json_schema_compiler/model.py:130: raise AssertionError('Name %s must be
lowerCamelCase' % name)
On 2012/02/24 03:53:05, kalman wrote:
> is it worthwhile having a warning here? are there any problems if it isn't
camel
> case?

Minimal, at worst, names might be dodgy (possibly dodgy enough to not compile).

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

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/mode...
tools/json_schema_compiler/model.py:64: the input format
On 2012/02/24 03:53:05, kalman wrote:
> i don't understand this comment in isolation. what's the "input format"? a
> Value?
> 
> there are a lot of True/False scattered throughout this file. Could you also
> explain why (in this comment) something is serializable or not? as in, what
> determines that.
> 
> also, since boolean values as arguments are evil (very hard to read), can you
> make it named? like serializable=False or whatever.

Made the changes we discussed. Settled on to_api and from_api as the names. Let
me know if you think of something better.

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/mode...
tools/json_schema_compiler/model.py:128: - |properties| the properties of an
OBJECT parameter
On 2012/02/24 03:53:05, kalman wrote:
> add |serializable|

Done.

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/mode...
tools/json_schema_compiler/model.py:162: self.serializable =
serializable_objects
On 2012/02/24 04:02:22, Yoyo Zhou wrote:
> self.type_.serializable?

Now that I've switched this to to_api/from_api, they're probably more describing
the object itself than self.type_ which is the PropertyType.

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/test...
File tools/json_schema_compiler/test/crossref_unittest.cc (right):

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/crossref_unittest.cc:29: value->Set("testType",
test_type_value.release());
On 2012/02/24 03:53:05, kalman wrote:
> inline CreateTestTypeDictionary()?

Done.

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/crossref_unittest.cc:74:
param_object_value->Set("testType", test_type_value.release());
On 2012/02/24 03:53:05, kalman wrote:
> ditto (and everywhere else)

Done.

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/test...
File tools/json_schema_compiler/test/objects_unittest.cc (right):

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/test...
tools/json_schema_compiler/test/objects_unittest.cc:11:
TEST(JsonSchemaCompilerObjectsTest, ReturnsObjectResultCreate) {
On 2012/02/24 04:02:22, Yoyo Zhou wrote:
> nit: these tests seem to be in reverse order

Done.

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

http://codereview.chromium.org/9456007/diff/1/tools/json_schema_compiler/util...
tools/json_schema_compiler/util.h:158: return scoped_ptr<Value>(list);
On 2012/02/24 03:53:05, kalman wrote:
> TBH a more idomatic way of writing this method would be
> 
> scoped_ptr<base::ListValue> list(new base::ListValue());
> PopulateListFromArray(from, list.get());
> return list.Pass();

Hmm, I tried this to start, got this error:

/tools/json_schema_compiler/util.h:158:3: error: no viable constructor returning
object of type 'scoped_ptr<base::Value>'
  return list.Pass();
  ^      ~~~~~~~~~~~
./base/memory/scoped_ptr.h:113:29: note: candidate constructor not viable: no
known conversion from 'scoped_ptr<base::Value>' to 'scoped_ptr<base::Value> &'
for 1st argument;
  MOVE_ONLY_TYPE_FOR_CPP_03(scoped_ptr, RValue)
                            ^
./base/move.h:204:3: note: expanded from macro 'MOVE_ONLY_TYPE_FOR_CPP_03'
  type(type&); \
  ^

[Yoyo Zhou, 2012-02-24 23:39:47]

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

http://codereview.chromium.org/9456007/diff/7003/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:140: from_api=False,
Unless I'm reading something incorrectly, it looks like from_api/to_api are only
ever read from Type (and never set to anything but True), and they're only ever
set on Property (and never read).

[not at google - send to devlin, 2012-02-26 23:51:53]

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

http://codereview.chromium.org/9456007/diff/7003/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:152: CHOICES properties are not
supported.
Similar comment to the one in cpp_type_generator.

Rather than this comment, check for it in the code and print a warning / ignore
if you can. If you can't fail gracefully then raise an error. Either way,
explain why it isn't and how you'd implement it in case somebody needs to later.

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

http://codereview.chromium.org/9456007/diff/7003/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_type_generator.py:24: def
CreateValueFromProperty(self, util_cc_helper, prop, var):
Why did you move this method into cpp_type_generator? It doesn't seem to
reference self... and I was under the impression that the practical purpose for
this class is to that it can be unit tested... and I don't see any updated unit
tests for it.

http://codereview.chromium.org/9456007/diff/7003/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_type_generator.py:26: to caller. Does not support
CHOICES.
Rather than this little comment, could you add an explicit branch for CHOICES
which either:
 - prints a warning and doesn't generate anything (if possible).
 - if not possible, raises an error.
Either way, in that branch explain why it isn't implemented and how you would
implement it (in case somebody needs to).

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

http://codereview.chromium.org/9456007/diff/7003/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:64: - |to_api| indicates that the type is
going to the API caller
This makes a lot more sense. I think it could still do with some improvements
documentation-wise.

Firstly the use of "api" is confusing because it can refer to either side (in
the c++ case, either Value or the compiled code). I was surprised reading
from_api and to_api uses and it seeming to be mixed up -- but I was thinking of
"api" as the "code generated to deal with the API".

A concept that will be common to all generators it the fact that they're
generating code, so perhaps we can use that as part of the documentation. So, we
could have |to_generator| and |from_generator| or similar? Like

|from_user| indicates that instances of the Type can originate from the users of
generated code, such as top-level types and function results.

|from_json| indicates that instances of Type can originate from the JSON (as
described by the schema), such as top-level types and function parameters.

WDYT? Does that make sense to you as well as me?

http://codereview.chromium.org/9456007/diff/7003/tools/json_schema_compiler/t...
File tools/json_schema_compiler/test/objects_unittest.cc (right):

http://codereview.chromium.org/9456007/diff/7003/tools/json_schema_compiler/t...
tools/json_schema_compiler/test/objects_unittest.cc:26: EXPECT_EQ(2UL,
params->info.strings.size());
This may cause size problems on other platforms where size_t isn't an unsigned
long.

You may need to just cast 2 to a size_t.

[not at google - send to devlin, 2012-02-26 23:53:34]

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

http://codereview.chromium.org/9456007/diff/7003/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:64: - |to_api| indicates that the type is
going to the API caller
> we could have |to_generator| and |from_generator| or similar?

Heh, fail, changed my mind right after writing that.

[calamity, 2012-02-27 04:57:30]

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

http://codereview.chromium.org/9456007/diff/7003/tools/json_schema_compiler/c...
tools/json_schema_compiler/cc_generator.py:152: CHOICES properties are not
supported.
On 2012/02/26 23:51:53, kalman wrote:
> Similar comment to the one in cpp_type_generator.
> 
> Rather than this comment, check for it in the code and print a warning /
ignore
> if you can. If you can't fail gracefully then raise an error. Either way,
> explain why it isn't and how you'd implement it in case somebody needs to
later.

Put the comment in _CreateValueFromProperty, this calls that anyway.

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

http://codereview.chromium.org/9456007/diff/7003/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_type_generator.py:24: def
CreateValueFromProperty(self, util_cc_helper, prop, var):
On 2012/02/26 23:51:53, kalman wrote:
> Why did you move this method into cpp_type_generator? It doesn't seem to
> reference self... and I was under the impression that the practical purpose
for
> this class is to that it can be unit tested... and I don't see any updated
unit
> tests for it.

Mismove.
Moved into cc_generator.

http://codereview.chromium.org/9456007/diff/7003/tools/json_schema_compiler/c...
tools/json_schema_compiler/cpp_type_generator.py:26: to caller. Does not support
CHOICES.
On 2012/02/26 23:51:53, kalman wrote:
> Rather than this little comment, could you add an explicit branch for CHOICES
> which either:
>  - prints a warning and doesn't generate anything (if possible).
>  - if not possible, raises an error.
> Either way, in that branch explain why it isn't implemented and how you would
> implement it (in case somebody needs to).

Just raised an error for now. If we want to do fault-tolerance, it'll involve
and try-catches around basically every function. Let me know if you want that
included into this patch.

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

http://codereview.chromium.org/9456007/diff/7003/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:64: - |to_api| indicates that the type is
going to the API caller
On 2012/02/26 23:53:34, kalman wrote:
> > we could have |to_generator| and |from_generator| or similar?
> 
> Heh, fail, changed my mind right after writing that.

Done. Changed to from_user/from_json

http://codereview.chromium.org/9456007/diff/7003/tools/json_schema_compiler/m...
tools/json_schema_compiler/model.py:140: from_api=False,
On 2012/02/24 23:39:47, Yoyo Zhou wrote:
> Unless I'm reading something incorrectly, it looks like from_api/to_api are
only
> ever read from Type (and never set to anything but True), and they're only
ever
> set on Property (and never read).

The Property is used as a Type if it is an OBJECT. It's passed into GenerateType
in the generators. Commented.

http://codereview.chromium.org/9456007/diff/7003/tools/json_schema_compiler/t...
File tools/json_schema_compiler/test/objects_unittest.cc (right):

http://codereview.chromium.org/9456007/diff/7003/tools/json_schema_compiler/t...
tools/json_schema_compiler/test/objects_unittest.cc:26: EXPECT_EQ(2UL,
params->info.strings.size());
On 2012/02/26 23:51:53, kalman wrote:
> This may cause size problems on other platforms where size_t isn't an unsigned
> long.
> 
> You may need to just cast 2 to a size_t.

Done.

[not at google - send to devlin, 2012-02-27 06:04:47]

lgtm, just those last couple of things.

http://codereview.chromium.org/9456007/diff/7003/tools/json_schema_compiler/t...
File tools/json_schema_compiler/test/objects_unittest.cc (right):

http://codereview.chromium.org/9456007/diff/7003/tools/json_schema_compiler/t...
tools/json_schema_compiler/test/objects_unittest.cc:26: EXPECT_EQ(2UL,
params->info.strings.size());
On 2012/02/27 04:57:30, calamity wrote:
> On 2012/02/26 23:51:53, kalman wrote:
> > This may cause size problems on other platforms where size_t isn't an
unsigned
> > long.
> > 
> > You may need to just cast 2 to a size_t.
> 
> Done.

nit: space between (size_t) and 2.

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

http://codereview.chromium.org/9456007/diff/12003/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:91: if type_.from_json:
Am I missing something... isn't this inverted? if the type comes from the client
then it needs a ToValue, if it comes from the json it needs a Populate?  Looking
at the model, I think it's just misnamed there.

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

http://codereview.chromium.org/9456007/diff/12003/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:142: if type_.from_json:
ditto

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

http://codereview.chromium.org/9456007/diff/12003/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:129: (as described by the schema), such as
top-level types and function parameters
need to keep commenting style consistent... elsewhere it's |from_client| so keep
it that way here.

actually, this comment shouldn't be here, it should be part of the docs for
__init__.

http://codereview.chromium.org/9456007/diff/12003/tools/json_schema_compiler/...
File tools/json_schema_compiler/test/arrays_unittest.cc (right):

http://codereview.chromium.org/9456007/diff/12003/tools/json_schema_compiler/...
tools/json_schema_compiler/test/arrays_unittest.cc:64: EXPECT_EQ((size_t)3,
ref_array_type->refs.size());
nit: space between size_t and 3 (and everywhere else this change has been made)

[Yoyo Zhou, 2012-02-28 00:29:46]

LGTM

http://codereview.chromium.org/9456007/diff/7003/tools/json_schema_compiler/t...
File tools/json_schema_compiler/test/objects_unittest.cc (right):

http://codereview.chromium.org/9456007/diff/7003/tools/json_schema_compiler/t...
tools/json_schema_compiler/test/objects_unittest.cc:26: EXPECT_EQ(2UL,
params->info.strings.size());
On 2012/02/27 06:04:47, kalman wrote:
> On 2012/02/27 04:57:30, calamity wrote:
> > On 2012/02/26 23:51:53, kalman wrote:
> > > This may cause size problems on other platforms where size_t isn't an
> unsigned
> > > long.
> > > 
> > > You may need to just cast 2 to a size_t.
> > 
> > Done.
> 
> nit: space between (size_t) and 2.

2U is pretty common, I don't think you need this casting.

http://code.google.com/p/chromium/source/search?q=expect_eq\(2u&btnG=Search+T...

http://codereview.chromium.org/9456007/diff/12003/build/json_schema_compile.gypi
File build/json_schema_compile.gypi (right):

http://codereview.chromium.org/9456007/diff/12003/build/json_schema_compile.g...
build/json_schema_compile.gypi:31: # platforms.
Can you expand on this in the comment? What behavior is needed?

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

http://codereview.chromium.org/9456007/diff/12003/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:165: c.Sblock('if (%s.get())' %
prop.unix_name)
Does the Sblock without Eblock mess up the indent level?

[calamity, 2012-02-28 01:01:45]

Thanks for the review.

http://codereview.chromium.org/9456007/diff/12003/build/json_schema_compile.gypi
File build/json_schema_compile.gypi (right):

http://codereview.chromium.org/9456007/diff/12003/build/json_schema_compile.g...
build/json_schema_compile.gypi:31: # platforms.
On 2012/02/28 00:29:46, Yoyo Zhou wrote:
> Can you expand on this in the comment? What behavior is needed?

Done.

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

http://codereview.chromium.org/9456007/diff/12003/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:91: if type_.from_json:
On 2012/02/27 06:04:47, kalman wrote:
> Am I missing something... isn't this inverted? if the type comes from the
client
> then it needs a ToValue, if it comes from the json it needs a Populate? 
Looking
> at the model, I think it's just misnamed there.

Done.

http://codereview.chromium.org/9456007/diff/12003/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:165: c.Sblock('if (%s.get())' %
prop.unix_name)
On 2012/02/28 00:29:46, Yoyo Zhou wrote:
> Does the Sblock without Eblock mess up the indent level?

Done.

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

http://codereview.chromium.org/9456007/diff/12003/tools/json_schema_compiler/...
tools/json_schema_compiler/model.py:129: (as described by the schema), such as
top-level types and function parameters
On 2012/02/27 06:04:47, kalman wrote:
> need to keep commenting style consistent... elsewhere it's |from_client| so
keep
> it that way here.
> 
> actually, this comment shouldn't be here, it should be part of the docs for
> __init__.

Done.

http://codereview.chromium.org/9456007/diff/12003/tools/json_schema_compiler/...
File tools/json_schema_compiler/test/arrays_unittest.cc (right):

http://codereview.chromium.org/9456007/diff/12003/tools/json_schema_compiler/...
tools/json_schema_compiler/test/arrays_unittest.cc:64: EXPECT_EQ((size_t)3,
ref_array_type->refs.size());
On 2012/02/27 06:04:47, kalman wrote:
> nit: space between size_t and 3 (and everywhere else this change has been
made)

Done.

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

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

[commit-bot: I haz the power, 2012-02-28 01:30:22]

Try job failure for 9456007-15002 (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-02-28 02:18:53]

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

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

Try job failure for 9456007-18003 (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-02-28 03:22:12]

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

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

Can't process patch for file
tools/json_schema_compiler/cpp_type_generator_test.py.
File's status is None, patchset upload is incomplete.

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

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

[commit-bot: I haz the power, 2012-02-28 05:51:45]

Change committed as 123909