Added ToJson to JSON schema compiler.

Added ToJson to JSON schema compiler.

Also re-factored the recently landed Debugger api (I believe it's the only API to have compiled events, so far)

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

[not at google - send to devlin, 2012-07-24 23:04:29]

http://codereview.chromium.org/10796114/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

http://codereview.chromium.org/10796114/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/debugger/debugger_api.cc:257:
scoped_ptr<std::string> json_args(OnDetach::ToJson(debuggee));
just return string

http://codereview.chromium.org/10796114/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/debugger/debugger_api.cc:311: params));
nit: argument should be on same line or line up vertically. though I think
they'll fit on the same line when scoped_ptr<> goes away.

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

http://codereview.chromium.org/10796114/diff/1/tools/json_schema_compiler/cc_...
tools/json_schema_compiler/cc_generator.py:684: JSON from the Value.
We should only generate these for events, not functions. Perhaps add an optional
parameter to this method.

http://codereview.chromium.org/10796114/diff/1/tools/json_schema_compiler/cc_...
tools/json_schema_compiler/cc_generator.py:719:
(c.Sblock('scoped_ptr<std::string> %(function_scope)s::'
as I mentioned in the bug, only need to return a std::string not a
scoped_ptr<std::string>. The compiler should optimise out any unnecessary copy.

http://codereview.chromium.org/10796114/diff/1/tools/json_schema_compiler/h_g...
File tools/json_schema_compiler/h_generator.py (right):

http://codereview.chromium.org/10796114/diff/1/tools/json_schema_compiler/h_g...
tools/json_schema_compiler/h_generator.py:38: .Append('#include
"base/json/json_writer.h"')
this should go in the cc file not the header file.

http://codereview.chromium.org/10796114/diff/1/tools/json_schema_compiler/h_g...
tools/json_schema_compiler/h_generator.py:344: c.Append('scoped_ptr<std::string>
ToJson(%s);' %
ditto, only return std::string, and only generate for events not functions.

[mitchellwrosen, 2012-07-25 17:08:20]

https://chromiumcodereview.appspot.com/10796114/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://chromiumcodereview.appspot.com/10796114/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/debugger/debugger_api.cc:257:
scoped_ptr<std::string> json_args(OnDetach::ToJson(debuggee));
On 2012/07/24 23:04:29, kalman wrote:
> just return string

Done.

https://chromiumcodereview.appspot.com/10796114/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/debugger/debugger_api.cc:311: params));
On 2012/07/24 23:04:29, kalman wrote:
> nit: argument should be on same line or line up vertically. though I think
> they'll fit on the same line when scoped_ptr<> goes away.

Oh, I've seen it both ways. I thought the style guide made it sound like a
judgement call. Anyways, I hope I don't come off sounding impertinent, I'll
change it! =)

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

https://chromiumcodereview.appspot.com/10796114/diff/1/tools/json_schema_comp...
tools/json_schema_compiler/cc_generator.py:684: JSON from the Value.
On 2012/07/24 23:04:29, kalman wrote:
> We should only generate these for events, not functions. Perhaps add an
optional
> parameter to this method.

Done.

https://chromiumcodereview.appspot.com/10796114/diff/1/tools/json_schema_comp...
tools/json_schema_compiler/cc_generator.py:719:
(c.Sblock('scoped_ptr<std::string> %(function_scope)s::'
On 2012/07/24 23:04:29, kalman wrote:
> as I mentioned in the bug, only need to return a std::string not a
> scoped_ptr<std::string>. The compiler should optimise out any unnecessary
copy.

Done.

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

https://chromiumcodereview.appspot.com/10796114/diff/1/tools/json_schema_comp...
tools/json_schema_compiler/h_generator.py:38: .Append('#include
"base/json/json_writer.h"')
On 2012/07/24 23:04:29, kalman wrote:
> this should go in the cc file not the header file.

Done.

https://chromiumcodereview.appspot.com/10796114/diff/1/tools/json_schema_comp...
tools/json_schema_compiler/h_generator.py:344: c.Append('scoped_ptr<std::string>
ToJson(%s);' %
On 2012/07/24 23:04:29, kalman wrote:
> ditto, only return std::string, and only generate for events not functions.

Done.

[not at google - send to devlin, 2012-07-25 23:28:41]

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

https://chromiumcodereview.appspot.com/10796114/diff/5001/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:91: cpp_util.Classname(event.name),
event, True))
Boolean arguments should be avoided because they make function calls difficult
to understand without lining up the arguments with the method that's being
called. They're the least self-describing type.

If this were C++ we'd define an enum for this, but Python has named arguments so
use those. This method call would be much easier to understand as

self._GenerateCreateCallbackArguments(
    cpp_util.Classname(event.name), event, generate_to_json=True)

https://chromiumcodereview.appspot.com/10796114/diff/5001/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:294: "%s::Results" % cpp_namespace,
function.callback, False))
Named arguments also let you specify a default value, so you could leave out
this change altogether.

https://chromiumcodereview.appspot.com/10796114/diff/5001/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:685: is_event):
prefer generate_to_json over is_event.

Also, like I said above, generate_to_json=False here.

https://chromiumcodereview.appspot.com/10796114/diff/5001/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:695: generated for events.
need to update comment.

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

https://chromiumcodereview.appspot.com/10796114/diff/5001/tools/json_schema_c...
tools/json_schema_compiler/h_generator.py:38: .Append('#include
"base/json/json_writer.h"')
you can remove this

https://chromiumcodereview.appspot.com/10796114/diff/5001/tools/json_schema_c...
tools/json_schema_compiler/h_generator.py:233:
.Concat(self._GenerateCreateCallbackArguments(event, True))
same comment throughout this file as in cc file

[mitchellwrosen, 2012-07-26 02:01:17]

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

https://chromiumcodereview.appspot.com/10796114/diff/5001/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:91: cpp_util.Classname(event.name),
event, True))
On 2012/07/25 23:28:41, kalman wrote:
> Boolean arguments should be avoided because they make function calls difficult
> to understand without lining up the arguments with the method that's being
> called. They're the least self-describing type.
> 
> If this were C++ we'd define an enum for this, but Python has named arguments
so
> use those. This method call would be much easier to understand as
> 
> self._GenerateCreateCallbackArguments(
>     cpp_util.Classname(event.name), event, generate_to_json=True)

Done.

https://chromiumcodereview.appspot.com/10796114/diff/5001/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:294: "%s::Results" % cpp_namespace,
function.callback, False))
On 2012/07/25 23:28:41, kalman wrote:
> Named arguments also let you specify a default value, so you could leave out
> this change altogether.

Done.

https://chromiumcodereview.appspot.com/10796114/diff/5001/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:685: is_event):
On 2012/07/25 23:28:41, kalman wrote:
> prefer generate_to_json over is_event.
> 
> Also, like I said above, generate_to_json=False here.

Done.

https://chromiumcodereview.appspot.com/10796114/diff/5001/tools/json_schema_c...
tools/json_schema_compiler/cc_generator.py:695: generated for events.
On 2012/07/25 23:28:41, kalman wrote:
> need to update comment.

Done.

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

https://chromiumcodereview.appspot.com/10796114/diff/5001/tools/json_schema_c...
tools/json_schema_compiler/h_generator.py:38: .Append('#include
"base/json/json_writer.h"')
On 2012/07/25 23:28:41, kalman wrote:
> you can remove this

Done.

https://chromiumcodereview.appspot.com/10796114/diff/5001/tools/json_schema_c...
tools/json_schema_compiler/h_generator.py:233:
.Concat(self._GenerateCreateCallbackArguments(event, True))
On 2012/07/25 23:28:41, kalman wrote:
> same comment throughout this file as in cc file

Done.

[not at google - send to devlin, 2012-07-26 02:39:58]

Apologies for not picking up on the test earlier.

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

http://codereview.chromium.org/10796114/diff/7005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:736: 'param_list': ',
'.join([param.unix_name for param in param_list])
I think if you leave out the [] it becomes a generator and still works, i.e.

join(param.unix_name for param in param_list)

and is probably more efficient because it doesn't involve an intermediate list.

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

http://codereview.chromium.org/10796114/diff/7005/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:344: if is_event:
I think this needs to be generate_to_json...

http://codereview.chromium.org/10796114/diff/7005/tools/json_schema_compiler/...
File tools/json_schema_compiler/test/content_settings.json (right):

http://codereview.chromium.org/10796114/diff/7005/tools/json_schema_compiler/...
tools/json_schema_compiler/test/content_settings.json:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
Does this file need to be included in here? I don't like copying big files like
this for testing, better to make small targeted JSON files. I also don't see it
used anywhere.

But yeah that reminds me, you should add some kind of test for this to exercise
the code paths. E.g. emulate OnObjectFiredCreate in objects_unittest.cc.

[mitchellwrosen, 2012-07-26 17:38:58]

Pushing this non-compiling patch, so please don't LGTM it =). 

Problem: Since any::Any has a private copy and assign, the line generated at
cc_generator.py:735-736 won't compile, because Create takes a const object and
not a const ref. I'm sure there's a reason behind this.

I'm not familiar with Any. Would this problem occur outside of testing?
(test/any.json defines an Any with an Event).

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

http://codereview.chromium.org/10796114/diff/7005/tools/json_schema_compiler/...
tools/json_schema_compiler/cc_generator.py:736: 'param_list': ',
'.join([param.unix_name for param in param_list])
On 2012/07/26 02:39:58, kalman wrote:
> I think if you leave out the [] it becomes a generator and still works, i.e.
> 
> join(param.unix_name for param in param_list)
> 
> and is probably more efficient because it doesn't involve an intermediate
list.

Done.

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

http://codereview.chromium.org/10796114/diff/7005/tools/json_schema_compiler/...
tools/json_schema_compiler/h_generator.py:344: if is_event:
On 2012/07/26 02:39:58, kalman wrote:
> I think this needs to be generate_to_json...

Oops! Done

http://codereview.chromium.org/10796114/diff/7005/tools/json_schema_compiler/...
File tools/json_schema_compiler/test/content_settings.json (right):

http://codereview.chromium.org/10796114/diff/7005/tools/json_schema_compiler/...
tools/json_schema_compiler/test/content_settings.json:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2012/07/26 02:39:58, kalman wrote:
> Does this file need to be included in here? I don't like copying big files
like
> this for testing, better to make small targeted JSON files. I also don't see
it
> used anywhere.
> 
> But yeah that reminds me, you should add some kind of test for this to
exercise
> the code paths. E.g. emulate OnObjectFiredCreate in objects_unittest.cc.

It's used in the setup of CppTypeGeneratorTest. I included it basically just to
test json/json_writer.h not being included in files that don't have events (see
testGenerateIncludesAndForwardDeclarations) I can take it out if you'd like.

I augmented OnObjectFiredCreate a little bit, not sure if that's what you meant.
Since C++ doesn't have reflection I couldn't think of a way to test that
functions *don't* have a ToJson function (testing that events do is easy enough
-- it compiles).

[not at google - send to devlin, 2012-07-26 23:33:37]

the Any thing is a bug. It should take a const& as you said... const doesn't
make any sense if it's being copied anyway.

I don't know how Any works at all, even with the events stuff. If you look at
how any.json gets generated,

namespace ReturnAny {

  namespace Results {
    scoped_ptr<base::ListValue> Create(const json_schema_compiler::any::Any
result);
  };

};

which is clearly wrong. Should be Any&, meaning that there aren't any tests
which exercise that code path.

Definitely something to fix. Your changes lgtm, though you'll need to find and
fix that "any" thing before submitting.

http://codereview.chromium.org/10796114/diff/7005/tools/json_schema_compiler/...
File tools/json_schema_compiler/test/content_settings.json (right):

http://codereview.chromium.org/10796114/diff/7005/tools/json_schema_compiler/...
tools/json_schema_compiler/test/content_settings.json:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2012/07/26 17:38:59, mitchellwrosen wrote:
> On 2012/07/26 02:39:58, kalman wrote:
> > Does this file need to be included in here? I don't like copying big files
> like
> > this for testing, better to make small targeted JSON files. I also don't see
> it
> > used anywhere.
> > 
> > But yeah that reminds me, you should add some kind of test for this to
> exercise
> > the code paths. E.g. emulate OnObjectFiredCreate in objects_unittest.cc.
> 
> It's used in the setup of CppTypeGeneratorTest. I included it basically just
to
> test json/json_writer.h not being included in files that don't have events
(see
> testGenerateIncludesAndForwardDeclarations) I can take it out if you'd like.
> 
> I augmented OnObjectFiredCreate a little bit, not sure if that's what you
meant.
> Since C++ doesn't have reflection I couldn't think of a way to test that
> functions *don't* have a ToJson function (testing that events do is easy
enough
> -- it compiles).

I mean to test that ToJson works, like, make some simple test example that would
serialize to something well known, then test the string. Just something simple.

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

http://codereview.chromium.org/10796114/diff/8010/tools/json_schema_compiler/...
tools/json_schema_compiler/test/objects_unittest.cc:75: ASSERT_EQ(json1, json2);
yep that's fine.

[commit-bot: I haz the power, 2012-07-28 00:31:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10796114/1...

[commit-bot: I haz the power, 2012-07-28 00:59:11]

Try job failure for 10796114-16001 (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-07-28 01:21:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10796114/7008

[commit-bot: I haz the power, 2012-07-28 03:59:52]

Change committed as 148878