JSON Schema Compiler: Added event compilation.

JSON Schema Compiler: Add event compilation and allow multiple parameters to callbacks

Compiles events to objects that look similar to this:
namespace OnObjectFired {
  struct SomeObject {
    ~SomeObject();
    SomeObject();

    enum State {
      STATE_FOO,
      STATE_BAR,
      STATE_BAZ,
    };

    static scoped_ptr<Value> CreateEnumValue(State state);
    State state;

    // Returns a new DictionaryValue representing the serialized form of this
    // SomeObject object. Passes ownership to caller.
    scoped_ptr<DictionaryValue> ToValue() const;

   private:
    DISALLOW_COPY_AND_ASSIGN(SomeObject);
  };

  Value* Create(const SomeObject& some_object);
};


BUG=133757, 135237
TEST=

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

[Matt Tytel, 2012-06-27 23:11:47]

Still working on fixing a hack and adding tests.

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

https://chromiumcodereview.appspot.com/10701012/diff/1/tools/json_schema_comp...
tools/json_schema_compiler/h_generator.py:299: if prop.from_json:
This is a bit of a hack. I'm currently looking for a better fix, the comment
explains the necessity.

[Matt Tytel, 2012-06-28 19:59:11]

kalman++, event compilation is working. there is one slight hack I commented on
in the first patch but i'm not sure i can make it prettier without reworking a
fair amount of code (still looking though).

this change also fixed enum parameters from client functions.

[Matt Tytel, 2012-06-29 01:15:52]

https://chromiumcodereview.appspot.com/10701012/diff/7001/tools/json_schema_c...
File tools/json_schema_compiler/test/enums_unittest.cc (right):

https://chromiumcodereview.appspot.com/10701012/diff/7001/tools/json_schema_c...
tools/json_schema_compiler/test/enums_unittest.cc:150:
TEST(JsonSchemaCompilerEnumsTest, OnEnumFiredCreate) {
Erp. Fixing.

[Matt Tytel, 2012-06-29 20:57:10]

ready for your review.

[Matt Tytel, 2012-06-29 23:06:40]

https://chromiumcodereview.appspot.com/10701012/diff/10002/tools/json_schema_...
File tools/json_schema_compiler/h_generator.py (right):

https://chromiumcodereview.appspot.com/10701012/diff/10002/tools/json_schema_...
tools/json_schema_compiler/h_generator.py:299: if prop.from_json:
On alternative to this is to define this function here in the header file. Then
we could put in static either way.

[not at google - send to devlin, 2012-07-03 14:21:39]

http://codereview.chromium.org/10701012/diff/10002/chrome/common/extensions/a...
File chrome/common/extensions/api/storage.json (right):

http://codereview.chromium.org/10701012/diff/10002/chrome/common/extensions/a...
chrome/common/extensions/api/storage.json:164: "name": "storageNamespace",
I always wanted to change references to this in here from "namespace" to
"areaName", so let's do that instead.

Also in the descriptions where "namespace" is mentioned, reword like:

"The name of the storage area (\"sync\" or \"local\") the changes are for."

http://codereview.chromium.org/10701012/diff/10002/tools/json_schema_compiler...
File tools/json_schema_compiler/cc_generator.py (left):

http://codereview.chromium.org/10701012/diff/10002/tools/json_schema_compiler...
tools/json_schema_compiler/cc_generator.py:93: # TODO(calamity): Events
hooray!

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

http://codereview.chromium.org/10701012/diff/10002/tools/json_schema_compiler...
tools/json_schema_compiler/cc_generator.py:293:
c.Concat(self._GenerateCallbackCreate(
Some of the function names in json_schema_compiler are quite vague / hard to
parse. Let's try to break that tradition, like "GenerateCreateCallbackArguments"
or something.

http://codereview.chromium.org/10701012/diff/10002/tools/json_schema_compiler...
tools/json_schema_compiler/cc_generator.py:637: def
_GenerateCallbackCreate(self, function_scope, callback):
the name "function_scope" keeps tripping me up. just make it function_name?

document args while you're here?

http://codereview.chromium.org/10701012/diff/10002/tools/json_schema_compiler...
tools/json_schema_compiler/cc_generator.py:641: E.g for event "Gar", generate
Gar::Create
ITYM "Baz".

http://codereview.chromium.org/10701012/diff/10002/tools/json_schema_compiler...
tools/json_schema_compiler/cc_generator.py:659: # time.
This will, sadly, require some more reshuffling to support events than this.

It's kind of explained in the comment above here, but "expanded_params" comes
from GetExpandedChoicesInParams, which returns a list of "fake" parameter
objects representing each "choice"... if the parameter is of type "choices".
Otherwise it returns just the singleton parameter.

The reason this works is because callbacks can only ever take a single argument.
By, err, odd convention, and because that's what ExtensionFunction expects.
Which is why these methods return Value not ListValue.

On the other hand, event arguments take *multiple* arguments. A more
general/sensible system, and it's what ExtensionEventRouter expects (actually it
expects JSON of a list, to add to the confusion).

I think the solution to this problem is still mostly what you've done, you'll
just need to do a bit more.

Firstly, make this method accept a flag like single_argument, and pass that
through when you generate the function. We might as well make the default
behaviour the "sensible" one i.e. generating the full arguments... particularly
since you're changing function callbacks to be able to take multiple arguments?

If single_argument is False (i.e. events case), the return type should be
scoped_ptr<ListValue>. This code will also generally have to change to accept
multiple arguments in Create, know how to make up a ListValue, etc.

If single_argument is True (i.e. functions case), the return type should be
scoped_ptr<Value> and asserted that there's only a single parameter to the
callback.

(note that we can't just say if len(params) == 1 because many events have a
single parameter but we still need to generate ListValue for them)

That leaves the problem with the expanded params stuff. To solve this *properly*
the algorithm which expands choices will need to expand *all permutations* of
choices... e.g. if an event has 5 arguments, each with 3 choices, that's a lot
of stuff to generate :\ not that it would actually ever happen, hopefully, but
you get the idea.

http://codereview.chromium.org/10701012/diff/10002/tools/json_schema_compiler...
tools/json_schema_compiler/cc_generator.py:668: c.Sblock('Value*
%(function_scope)s::Create(const %(arg)s) {')
possible cleanup: make this return scoped_ptr<Value>

you might want to do this in a separate change, since there will be a bunch of
other code to update... thought it'd just be a matter of changing cases of

result_.reset(Foo::Result::Create(...));

with

result_ = Foo::Result::Create(...);

which is quite nice actually, and everybody will thank you.

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

http://codereview.chromium.org/10701012/diff/10002/tools/json_schema_compiler...
tools/json_schema_compiler/h_generator.py:299: if prop.from_json:
On 2012/06/29 23:06:40, Matt Tytel wrote:
> On alternative to this is to define this function here in the header file.
Then
> we could put in static either way.

Don't follow?

This approach is fine I think. I'd tweak the comment a bit though:

# If the property is "from_json" then we're generating populated structs for it.
# This means that the ::Create utility methods are inside that struct{} and
# need to be "static".
# If not, they're probably being generated inside a namespace{} block as a
# client utility, so "static" would be incorrect.

You'll need to make the same change in cc_generator, by the way. It prints "//
static" incorrectly under these circumstances.

http://codereview.chromium.org/10701012/diff/10002/tools/json_schema_compiler...
tools/json_schema_compiler/h_generator.py:304: (enum_name, prop.unix_name))
Clean this up a bit:

create_enum_value = 'scoped_ptr<Value> CreateEnumValue(%s %s);' %
                     (enum_name, prop.unix_name)

# comment.
if prop.from_json:
  create_enum_value = 'static ' + create_enum_value
return c.Append(create_enum_value)

http://codereview.chromium.org/10701012/diff/10002/tools/json_schema_compiler...
tools/json_schema_compiler/h_generator.py:307: def
_GenerateFromClientFunction(self, function):
Same comments in here as in cc_generator.

Also: call this function GenerateCreateCallbackArguments like in there?

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

http://codereview.chromium.org/10701012/diff/10002/tools/json_schema_compiler...
tools/json_schema_compiler/model.py:146: for param in json['parameters']:
Same fix here as above (using json.get('parameters', []))?

I checked other places in the file where this might be a problem and found only
one, kinda. That's what the seemingly unrelated choices comment is.

Thanks for fixing this.

http://codereview.chromium.org/10701012/diff/10002/tools/json_schema_compiler...
tools/json_schema_compiler/model.py:229: raise ParseException(self, 'Choices has
no choices')
also add check that there is >0 choices.

http://codereview.chromium.org/10701012/diff/10002/tools/json_schema_compiler...
tools/json_schema_compiler/model.py:354: event = Function(model, event_json,
from_client=True)
Casual observation that might actually change a whole bunch of things: I think
it would make more sense to be creating a Callback here, not a Function.

http://codereview.chromium.org/10701012/diff/10002/tools/json_schema_compiler...
File tools/json_schema_compiler/schema_bundle_generator.py (right):

http://codereview.chromium.org/10701012/diff/10002/tools/json_schema_compiler...
tools/json_schema_compiler/schema_bundle_generator.py:88:
c.Append("registry->RegisterFunction<%sFunction>();" % (function_name))
You can leave out the surrounding () in %(function_name) ?

http://codereview.chromium.org/10701012/diff/10002/tools/json_schema_compiler...
File tools/json_schema_compiler/test/enums.json (right):

http://codereview.chromium.org/10701012/diff/10002/tools/json_schema_compiler...
tools/json_schema_compiler/test/enums.json:116: }
Also test events with multiple parameters.

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

http://codereview.chromium.org/10701012/diff/10002/tools/json_schema_compiler...
tools/json_schema_compiler/test/objects_unittest.cc:70: EXPECT_EQ("bar", state);
Another alternative to this is to build up the expected DictionaryValue then
assert that result_value.Equals(expected).

Check out chrome/common/extensions/value_builder.h for a new/nice way to
construct DictionaryValues.

TBH not sure we actually link that in from here, so try that first.

http://codereview.chromium.org/10701012/diff/10002/tools/json_schema_compiler...
File tools/json_schema_compiler/test/simple_api_unittest.cc (right):

http://codereview.chromium.org/10701012/diff/10002/tools/json_schema_compiler...
tools/json_schema_compiler/test/simple_api_unittest.cc:173:
some_test_type.boolean = true;
same comment(s)

[Matt Tytel, 2012-07-11 02:08:50]

Ready for more reveiew, but still working on prettying up the tests.

https://chromiumcodereview.appspot.com/10701012/diff/10002/chrome/common/exte...
File chrome/common/extensions/api/storage.json (right):

https://chromiumcodereview.appspot.com/10701012/diff/10002/chrome/common/exte...
chrome/common/extensions/api/storage.json:164: "name": "storageNamespace",
On 2012/07/03 14:21:39, kalman wrote:
> I always wanted to change references to this in here from "namespace" to
> "areaName", so let's do that instead.
> 
> Also in the descriptions where "namespace" is mentioned, reword like:
> 
> "The name of the storage area (\"sync\" or \"local\") the changes are for."

Done.

https://chromiumcodereview.appspot.com/10701012/diff/10002/tools/json_schema_...
File tools/json_schema_compiler/cc_generator.py (right):

https://chromiumcodereview.appspot.com/10701012/diff/10002/tools/json_schema_...
tools/json_schema_compiler/cc_generator.py:293:
c.Concat(self._GenerateCallbackCreate(
On 2012/07/03 14:21:39, kalman wrote:
> Some of the function names in json_schema_compiler are quite vague / hard to
> parse. Let's try to break that tradition, like
"GenerateCreateCallbackArguments"
> or something.

Done.

https://chromiumcodereview.appspot.com/10701012/diff/10002/tools/json_schema_...
tools/json_schema_compiler/cc_generator.py:637: def
_GenerateCallbackCreate(self, function_scope, callback):
On 2012/07/03 14:21:39, kalman wrote:
> the name "function_scope" keeps tripping me up. just make it function_name?
> 
> document args while you're here?

In this meaning, for a function A::B::C(), A::B is the passed in function_scope.
I don't think function_name captures this, any alternatives?

https://chromiumcodereview.appspot.com/10701012/diff/10002/tools/json_schema_...
tools/json_schema_compiler/cc_generator.py:641: E.g for event "Gar", generate
Gar::Create
On 2012/07/03 14:21:39, kalman wrote:
> ITYM "Baz".

ITYR
Done.

https://chromiumcodereview.appspot.com/10701012/diff/10002/tools/json_schema_...
tools/json_schema_compiler/cc_generator.py:659: # time.
On 2012/07/03 14:21:39, kalman wrote:
> This will, sadly, require some more reshuffling to support events than this.
> 
> It's kind of explained in the comment above here, but "expanded_params" comes
> from GetExpandedChoicesInParams, which returns a list of "fake" parameter
> objects representing each "choice"... if the parameter is of type "choices".
> Otherwise it returns just the singleton parameter.
> 
> The reason this works is because callbacks can only ever take a single
argument.
> By, err, odd convention, and because that's what ExtensionFunction expects.
> Which is why these methods return Value not ListValue.
> 
> On the other hand, event arguments take *multiple* arguments. A more
> general/sensible system, and it's what ExtensionEventRouter expects (actually
it
> expects JSON of a list, to add to the confusion).
> 
> I think the solution to this problem is still mostly what you've done, you'll
> just need to do a bit more.
> 
> Firstly, make this method accept a flag like single_argument, and pass that
> through when you generate the function. We might as well make the default
> behaviour the "sensible" one i.e. generating the full arguments...
particularly
> since you're changing function callbacks to be able to take multiple
arguments?
> 
> If single_argument is False (i.e. events case), the return type should be
> scoped_ptr<ListValue>. This code will also generally have to change to accept
> multiple arguments in Create, know how to make up a ListValue, etc.
> 
> If single_argument is True (i.e. functions case), the return type should be
> scoped_ptr<Value> and asserted that there's only a single parameter to the
> callback.
> 
> (note that we can't just say if len(params) == 1 because many events have a
> single parameter but we still need to generate ListValue for them)
> 
> That leaves the problem with the expanded params stuff. To solve this
*properly*
> the algorithm which expands choices will need to expand *all permutations* of
> choices... e.g. if an event has 5 arguments, each with 3 choices, that's a lot
> of stuff to generate :\ not that it would actually ever happen, hopefully, but
> you get the idea.

Done.

https://chromiumcodereview.appspot.com/10701012/diff/10002/tools/json_schema_...
tools/json_schema_compiler/cc_generator.py:668: c.Sblock('Value*
%(function_scope)s::Create(const %(arg)s) {')
I really like this, I'll file a separate issue if you haven't already.

https://chromiumcodereview.appspot.com/10701012/diff/10002/tools/json_schema_...
File tools/json_schema_compiler/h_generator.py (right):

https://chromiumcodereview.appspot.com/10701012/diff/10002/tools/json_schema_...
tools/json_schema_compiler/h_generator.py:299: if prop.from_json:
On 2012/07/03 14:21:39, kalman wrote:
> On 2012/06/29 23:06:40, Matt Tytel wrote:
> > On alternative to this is to define this function here in the header file.
> Then
> > we could put in static either way.
> 
> Don't follow?
If we define CreateEnumValue here in the header file instead of in the cc file,
then we can make it static regardless if we are in a class or a namespace. Trade
off is we have less logic in the json_schema_compiler but worse style
> 
> This approach is fine I think. I'd tweak the comment a bit though:
> 
> # If the property is "from_json" then we're generating populated structs for
it.
> # This means that the ::Create utility methods are inside that struct{} and
> # need to be "static".
> # If not, they're probably being generated inside a namespace{} block as a
> # client utility, so "static" would be incorrect.
> 
> You'll need to make the same change in cc_generator, by the way. It prints "//
> static" incorrectly under these circumstances.

https://chromiumcodereview.appspot.com/10701012/diff/10002/tools/json_schema_...
tools/json_schema_compiler/h_generator.py:304: (enum_name, prop.unix_name))
On 2012/07/03 14:21:39, kalman wrote:
> Clean this up a bit:
> 
> create_enum_value = 'scoped_ptr<Value> CreateEnumValue(%s %s);' %
>                      (enum_name, prop.unix_name)
> 
> # comment.
> if prop.from_json:
>   create_enum_value = 'static ' + create_enum_value
> return c.Append(create_enum_value)

Done.

https://chromiumcodereview.appspot.com/10701012/diff/10002/tools/json_schema_...
tools/json_schema_compiler/h_generator.py:307: def
_GenerateFromClientFunction(self, function):
On 2012/07/03 14:21:39, kalman wrote:
> Same comments in here as in cc_generator.
> 
> Also: call this function GenerateCreateCallbackArguments like in there?

Done.

https://chromiumcodereview.appspot.com/10701012/diff/10002/tools/json_schema_...
File tools/json_schema_compiler/model.py (right):

https://chromiumcodereview.appspot.com/10701012/diff/10002/tools/json_schema_...
tools/json_schema_compiler/model.py:146: for param in json['parameters']:
On 2012/07/03 14:21:39, kalman wrote:
> Same fix here as above (using json.get('parameters', []))?
> 
> I checked other places in the file where this might be a problem and found
only
> one, kinda. That's what the seemingly unrelated choices comment is.
> 
> Thanks for fixing this.

Done.

https://chromiumcodereview.appspot.com/10701012/diff/10002/tools/json_schema_...
tools/json_schema_compiler/model.py:229: raise ParseException(self, 'Choices has
no choices')
On 2012/07/03 14:21:39, kalman wrote:
> also add check that there is >0 choices.

Done.

https://chromiumcodereview.appspot.com/10701012/diff/10002/tools/json_schema_...
tools/json_schema_compiler/model.py:354: event = Function(model, event_json,
from_client=True)
Went with removing the Callback object from the chat in IRC.
Done.

https://chromiumcodereview.appspot.com/10701012/diff/10002/tools/json_schema_...
File tools/json_schema_compiler/schema_bundle_generator.py (right):

https://chromiumcodereview.appspot.com/10701012/diff/10002/tools/json_schema_...
tools/json_schema_compiler/schema_bundle_generator.py:88:
c.Append("registry->RegisterFunction<%sFunction>();" % (function_name))
On 2012/07/03 14:21:39, kalman wrote:
> You can leave out the surrounding () in %(function_name) ?

Done.

https://chromiumcodereview.appspot.com/10701012/diff/10002/tools/json_schema_...
File tools/json_schema_compiler/test/enums.json (right):

https://chromiumcodereview.appspot.com/10701012/diff/10002/tools/json_schema_...
tools/json_schema_compiler/test/enums.json:116: }
On 2012/07/03 14:21:39, kalman wrote:
> Also test events with multiple parameters.

Done.

[not at google - send to devlin, 2012-07-11 07:22:06]

Cool, looking good. Really looking forward to this change going in (and events
being rendered in the docs server).

Those are some epic tests. The comments I made in there apply to the other test
files...

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

http://codereview.chromium.org/10701012/diff/6011/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:654: c.Sblock('ListValue*
%(function_scope)s::Create(%(declaration_list)s) {')
return a scoped_ptr<ListValue> rather than a ListValue*?

http://codereview.chromium.org/10701012/diff/6011/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:667: c.Append('return
create_results;')
.Pass()

http://codereview.chromium.org/10701012/diff/6011/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:671: 'declaration_list': '%s' % ',
'.join(declaration_list)
Is this some kind of Python idiom/requirement I'm not aware of, or is the '%s' %
unnecessary and that can be just ', '.join(declaration_list) ?

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

http://codereview.chromium.org/10701012/diff/6011/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:39: def
GetAllPossibleParameters(self, params):
I was confused in the other class what the difference is between this function
and GetAllPossibleParameterLists. I preferred the old name for this, though
"ExpandChoicesInParams" or even just "ExpandParams" might be simpler to read.

http://codereview.chromium.org/10701012/diff/6011/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:58: return [[]]
Why this check here? Can it be handled outside this function?

http://codereview.chromium.org/10701012/diff/6011/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator.py:62: for partial_list in
partial_parameter_lists]
took me a little while to parse that, but nice.

http://codereview.chromium.org/10701012/diff/6011/tools/json_schema_compiler/...
File tools/json_schema_compiler/cpp_type_generator_test.py (right):

http://codereview.chromium.org/10701012/diff/6011/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator_test.py:225: self.tabs.unix_name)
nit: parameters should all be on 1 line or vertically aligned

http://codereview.chromium.org/10701012/diff/6011/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator_test.py:229: expected_lists =
[['move_properties', 'tab_ids_array'],
Not being used?

Looks like this was half transitioned to doing a direct comparison rather than
based on length (which is the right change)?

tabs.move isn't the most interesting choice, though. Ideally we should use a
function with at least 2 choices in it, and at least 2 non-choices, to test the
permutations. The other tests you've added in this change are pretty epic
though...

http://codereview.chromium.org/10701012/diff/6011/tools/json_schema_compiler/...
tools/json_schema_compiler/cpp_type_generator_test.py:231: param_lists =
manager.GetAllPossibleParameterLists(props)
looks like this is being evaluated twice? move this line up?

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

http://codereview.chromium.org/10701012/diff/6011/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:232: c.Eblock('};')
nit: style elsewhere in the file is to chain together these statements:

return (Code()
  .Sblock(...)
  .Concat(...)
  .Eblock(...))

http://codereview.chromium.org/10701012/diff/6011/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:324: ', '.join(declaration_list))
scoped_ptr, can fit on 1 line?

http://codereview.chromium.org/10701012/diff/6011/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:333: c.Eblock('};')
ditto with
return (Code()
  ...)

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

http://codereview.chromium.org/10701012/diff/6011/tools/json_schema_compiler/...
tools/json_schema_compiler/test/arrays_unittest.cc:224:
EXPECT_TRUE(results->Equals(&expected));
Thanks for doing this cleanup.

Check out chrome/common/extensions/value_builder.h -- it would simplify this
even more.

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

http://codereview.chromium.org/10701012/diff/6011/tools/json_schema_compiler/...
tools/json_schema_compiler/test/crossref_unittest.cc:66:
results->GetDictionary(0, &result);
ASSERT_TRUE around this, otherwise result will be NULL.

[Matt Tytel, 2012-07-12 03:07:56]

Got a little carried away with the value_builders..

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

https://chromiumcodereview.appspot.com/10701012/diff/6011/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:654: c.Sblock('ListValue*
%(function_scope)s::Create(%(declaration_list)s) {')
This CL is getting a bit heavy. Can I do this separate?

https://chromiumcodereview.appspot.com/10701012/diff/6011/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:671: 'declaration_list': '%s' % ',
'.join(declaration_list)
... % '%s' % '%s' % '%s' % ', '.join(declaration_list).
(Done.)

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

https://chromiumcodereview.appspot.com/10701012/diff/6011/tools/json_schema_c...
tools/json_schema_compiler/cpp_type_generator.py:39: def
GetAllPossibleParameters(self, params):
Done.

https://chromiumcodereview.appspot.com/10701012/diff/6011/tools/json_schema_c...
tools/json_schema_compiler/cpp_type_generator.py:58: return [[]]
This function is recursive, this is the base case.

https://chromiumcodereview.appspot.com/10701012/diff/6011/tools/json_schema_c...
tools/json_schema_compiler/cpp_type_generator.py:62: for partial_list in
partial_parameter_lists]
I was nervous putting this in, some people don't like python list
comprehensions.

https://chromiumcodereview.appspot.com/10701012/diff/6011/tools/json_schema_c...
File tools/json_schema_compiler/cpp_type_generator_test.py (right):

https://chromiumcodereview.appspot.com/10701012/diff/6011/tools/json_schema_c...
tools/json_schema_compiler/cpp_type_generator_test.py:225: self.tabs.unix_name)
On 2012/07/11 07:22:06, kalman wrote:
> nit: parameters should all be on 1 line or vertically aligned
Ah copied this from the function above.
Done.

https://chromiumcodereview.appspot.com/10701012/diff/6011/tools/json_schema_c...
tools/json_schema_compiler/cpp_type_generator_test.py:229: expected_lists =
[['move_properties', 'tab_ids_array'],
On 2012/07/11 07:22:06, kalman wrote:
> Not being used?
> 
> Looks like this was half transitioned to doing a direct comparison rather than
> based on length (which is the right change)?
> 
> tabs.move isn't the most interesting choice, though. Ideally we should use a
> function with at least 2 choices in it, and at least 2 non-choices, to test
the
> permutations. The other tests you've added in this change are pretty epic
> though...

Dang rushed through this test. Somehow deleted the comparison at the bottom.
Done.

https://chromiumcodereview.appspot.com/10701012/diff/6011/tools/json_schema_c...
tools/json_schema_compiler/cpp_type_generator_test.py:231: param_lists =
manager.GetAllPossibleParameterLists(props)
On 2012/07/11 07:22:06, kalman wrote:
> looks like this is being evaluated twice? move this line up?

Done.

https://chromiumcodereview.appspot.com/10701012/diff/6011/tools/json_schema_c...
File tools/json_schema_compiler/h_generator.py (right):

https://chromiumcodereview.appspot.com/10701012/diff/6011/tools/json_schema_c...
tools/json_schema_compiler/h_generator.py:232: c.Eblock('};')
On 2012/07/11 07:22:06, kalman wrote:
> nit: style elsewhere in the file is to chain together these statements:
> 
> return (Code()
>   .Sblock(...)
>   .Concat(...)
>   .Eblock(...))

Done.

https://chromiumcodereview.appspot.com/10701012/diff/6011/tools/json_schema_c...
tools/json_schema_compiler/h_generator.py:324: ', '.join(declaration_list))
On 2012/07/11 07:22:06, kalman wrote:
> scoped_ptr, can fit on 1 line?

Done.

https://chromiumcodereview.appspot.com/10701012/diff/6011/tools/json_schema_c...
tools/json_schema_compiler/h_generator.py:333: c.Eblock('};')
On 2012/07/11 07:22:06, kalman wrote:
> ditto with
> return (Code()
>   ...)

Done.

https://chromiumcodereview.appspot.com/10701012/diff/6011/tools/json_schema_c...
File tools/json_schema_compiler/test/arrays_unittest.cc (right):

https://chromiumcodereview.appspot.com/10701012/diff/6011/tools/json_schema_c...
tools/json_schema_compiler/test/arrays_unittest.cc:224:
EXPECT_TRUE(results->Equals(&expected));
On 2012/07/11 07:22:06, kalman wrote:
> Thanks for doing this cleanup.
> 
> Check out chrome/common/extensions/value_builder.h -- it would simplify this
> even more.

I might have gotten carried away.
Done.

https://chromiumcodereview.appspot.com/10701012/diff/6011/tools/json_schema_c...
File tools/json_schema_compiler/test/crossref_unittest.cc (right):

https://chromiumcodereview.appspot.com/10701012/diff/6011/tools/json_schema_c...
tools/json_schema_compiler/test/crossref_unittest.cc:66:
results->GetDictionary(0, &result);
On 2012/07/11 07:22:06, kalman wrote:
> ASSERT_TRUE around this, otherwise result will be NULL.

Done.

[Matt Tytel, 2012-07-12 03:31:28]

https://chromiumcodereview.appspot.com/10701012/diff/8015/tools/json_schema_c...
File tools/json_schema_compiler/test/forbidden.json (right):

https://chromiumcodereview.appspot.com/10701012/diff/8015/tools/json_schema_c...
tools/json_schema_compiler/test/forbidden.json:1: [
Didn't want people to think this was a good idea.

[not at google - send to devlin, 2012-07-12 03:36:49]

lgtm

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

https://chromiumcodereview.appspot.com/10701012/diff/6011/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:654: c.Sblock('ListValue*
%(function_scope)s::Create(%(declaration_list)s) {')
On 2012/07/12 03:07:56, Matt Tytel wrote:
> This CL is getting a bit heavy. Can I do this separate?

sounds good.

https://chromiumcodereview.appspot.com/10701012/diff/2006/tools/json_schema_c...
File tools/json_schema_compiler/cpp_type_generator_test.py (right):

https://chromiumcodereview.appspot.com/10701012/diff/2006/tools/json_schema_c...
tools/json_schema_compiler/cpp_type_generator_test.py:231: self.assertEquals(2,
len(props))
this test is kind of unnecessary, but ok.

https://chromiumcodereview.appspot.com/10701012/diff/2006/tools/json_schema_c...
tools/json_schema_compiler/cpp_type_generator_test.py:233: self.assertEquals(2,
len(param_lists))
This is a bit unnecessary, the assertEquals a couple of lines down tests a
superset?

https://chromiumcodereview.appspot.com/10701012/diff/2006/tools/json_schema_c...
tools/json_schema_compiler/cpp_type_generator_test.py:237: for param_list in
param_lists])
nit: one extra space indent to line up with the inside of the [

https://chromiumcodereview.appspot.com/10701012/diff/8015/tools/json_schema_c...
File tools/json_schema_compiler/test/forbidden.json (right):

https://chromiumcodereview.appspot.com/10701012/diff/8015/tools/json_schema_c...
tools/json_schema_compiler/test/forbidden.json:1: [
On 2012/07/12 03:31:28, Matt Tytel wrote:
> Didn't want people to think this was a good idea.

why not?

[Matt Tytel, 2012-07-13 02:34:43]

Should have just done the scoped_ptr<ListValue> switch earlier..

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

https://chromiumcodereview.appspot.com/10701012/diff/6011/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:654: c.Sblock('ListValue*
%(function_scope)s::Create(%(declaration_list)s) {')
On 2012/07/12 03:36:49, kalman wrote:
> On 2012/07/12 03:07:56, Matt Tytel wrote:
> > This CL is getting a bit heavy. Can I do this separate?
> 
> sounds good.

Zoop. Might as well do it in this CL since I changed the return type and I'll
have to change a bunch of files because of it anyway.

https://chromiumcodereview.appspot.com/10701012/diff/2006/tools/json_schema_c...
File tools/json_schema_compiler/cpp_type_generator_test.py (right):

https://chromiumcodereview.appspot.com/10701012/diff/2006/tools/json_schema_c...
tools/json_schema_compiler/cpp_type_generator_test.py:233: self.assertEquals(2,
len(param_lists))
Ah yep, I was copying the format from the above test.
Done.

https://chromiumcodereview.appspot.com/10701012/diff/2006/tools/json_schema_c...
tools/json_schema_compiler/cpp_type_generator_test.py:237: for param_list in
param_lists])
On 2012/07/12 03:36:50, kalman wrote:
> nit: one extra space indent to line up with the inside of the [

Done.

https://chromiumcodereview.appspot.com/10701012/diff/8015/tools/json_schema_c...
File tools/json_schema_compiler/test/forbidden.json (right):

https://chromiumcodereview.appspot.com/10701012/diff/8015/tools/json_schema_c...
tools/json_schema_compiler/test/forbidden.json:1: [
I just thought if your parameters are this complex you might be doing something
wrong. But I can rename the file and function if you prefer.

[not at google - send to devlin, 2012-07-13 02:40:32]

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

https://chromiumcodereview.appspot.com/10701012/diff/6011/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:654: c.Sblock('ListValue*
%(function_scope)s::Create(%(declaration_list)s) {')
On 2012/07/13 02:34:43, Matt Tytel wrote:
> On 2012/07/12 03:36:49, kalman wrote:
> > On 2012/07/12 03:07:56, Matt Tytel wrote:
> > > This CL is getting a bit heavy. Can I do this separate?
> > 
> > sounds good.
> 
> Zoop. Might as well do it in this CL since I changed the return type and I'll
> have to change a bunch of files because of it anyway.

Cool sg

https://chromiumcodereview.appspot.com/10701012/diff/8015/tools/json_schema_c...
File tools/json_schema_compiler/test/forbidden.json (right):

https://chromiumcodereview.appspot.com/10701012/diff/8015/tools/json_schema_c...
tools/json_schema_compiler/test/forbidden.json:1: [
On 2012/07/13 02:34:43, Matt Tytel wrote:
> I just thought if your parameters are this complex you might be doing
something
> wrong. But I can rename the file and function if you prefer.

Yeah, hopefully that's the kind of thing that would be picked up in code review.
If you gotta have choices, you gotta have choices.

(or just gotta not use json-schema-compiler)

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtytel@chromium.org/10701012/16006

[commit-bot: I haz the power, 2012-07-13 03:45:18]

Try job failure for 10701012-16006 (retry) on mac_rel for steps "unit_tests,
check_deps".
It's a second try, previously, steps "unit_tests, check_deps" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Matt Tytel, 2012-07-13 23:53:39]

Well I'm not sure how to include the value_builder without the dependencies
freaking out. If it's not possible without moving it to base, I just uploaded a
version without the value_builder in the tests.

[not at google - send to devlin, 2012-07-15 22:58:54]

Could always copy ValueBuilder into json_schema_compiler. Easier than rewriting
all the tests.

Move to base later pending decision on whether it's a good idea...

[not at google - send to devlin, 2012-07-15 22:59:12]

But I see you've already removed it, so never mind :)

[Matt Tytel, 2012-07-16 20:06:55]

Yea, I'll make another CL if/when value_builder goes into base.

[commit-bot: I haz the power, 2012-07-16 20:27:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtytel@chromium.org/10701012/28001

[commit-bot: I haz the power, 2012-07-16 21:47:26]

Change committed as 146899