Refactor chrome.debugger api to use the JSON schema compiler. Also modified the

Refactor chrome.debugger api to use the JSON schema compiler. 

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

[not at google - send to devlin, 2012-07-19 23:53:47]

lgtm

https://chromiumcodereview.appspot.com/10803043/diff/6002/chrome/browser/exte...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://chromiumcodereview.appspot.com/10803043/diff/6002/chrome/browser/exte...
chrome/browser/extensions/api/debugger/debugger_api.cc:243: if (params)
params can't be NULL, it's validated to not be, so this check is unnecessary.

I'd make "params" a const& in the constructor.

[Matt Tytel, 2012-07-20 00:41:15]

https://chromiumcodereview.appspot.com/10803043/diff/6002/chrome/browser/exte...
File chrome/browser/extensions/api/debugger/debugger_api.cc (left):

https://chromiumcodereview.appspot.com/10803043/diff/6002/chrome/browser/exte...
chrome/browser/extensions/api/debugger/debugger_api.cc:300: ListValue args;
You can use the the json schema compiler here too since we can compile events
now.

https://chromiumcodereview.appspot.com/10803043/diff/6002/chrome/browser/exte...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://chromiumcodereview.appspot.com/10803043/diff/6002/chrome/browser/exte...
chrome/browser/extensions/api/debugger/debugger_api.cc:19: #include
"chrome/browser/extensions/api/debugger/debugger_api_constants.h"
After we use the json schema compiler for events, i think we can remove all the
Keys from this constants file.

[not at google - send to devlin, 2012-07-20 00:45:44]

https://chromiumcodereview.appspot.com/10803043/diff/6002/chrome/browser/exte...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://chromiumcodereview.appspot.com/10803043/diff/6002/chrome/browser/exte...
chrome/browser/extensions/api/debugger/debugger_api.cc:478: void
SendCommandDebuggerFunction::SendResponseBody(
ah, matt is right. you can use the compiler here too, with Result.

I think basically search through for "Value" in this file and chances are it
should use the compiler.

[mitchellwrosen, 2012-07-20 19:05:17]

https://chromiumcodereview.appspot.com/10803043/diff/6002/chrome/browser/exte...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://chromiumcodereview.appspot.com/10803043/diff/6002/chrome/browser/exte...
chrome/browser/extensions/api/debugger/debugger_api.cc:243: if (params)
On 2012/07/19 23:53:47, kalman wrote:
> params can't be NULL, it's validated to not be, so this check is unnecessary.
> 
> I'd make "params" a const& in the constructor.

Params is optional, so this pointer can be NULL

[Matt Tytel, 2012-07-20 19:09:57]

https://chromiumcodereview.appspot.com/10803043/diff/6002/chrome/browser/exte...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://chromiumcodereview.appspot.com/10803043/diff/6002/chrome/browser/exte...
chrome/browser/extensions/api/debugger/debugger_api.cc:243: if (params)
Maybe rename params to command_params here and other places to avoid the
confusion that I think is happening here.

[commit-bot: I haz the power, 2012-07-20 19:51:56]

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

[mitchellwrosen, 2012-07-20 20:14:33]

On 2012/07/20 19:51:56, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>
https://chromium-status.appspot.com/cq/mitchellwrosen%40chromium.org/10803043...

Unchecked due to trigger finger, sorry about that

[Matt Tytel, 2012-07-20 21:26:53]

https://chromiumcodereview.appspot.com/10803043/diff/3008/chrome/browser/exte...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://chromiumcodereview.appspot.com/10803043/diff/3008/chrome/browser/exte...
chrome/browser/extensions/api/debugger/debugger_api.cc:240: if (command_params)
nit: People prefer wrapping in {} if the whole statement is longer than 2 lines.
Another one below too.

https://chromiumcodereview.appspot.com/10803043/diff/3008/chrome/browser/exte...
chrome/browser/extensions/api/debugger/debugger_api.cc:309: std::string method =
method_name;
Got an extra string here I think.

https://chromiumcodereview.appspot.com/10803043/diff/3008/chrome/browser/exte...
chrome/browser/extensions/api/debugger/debugger_api.cc:493: results_ =
SendCommand::Results::Create(result);
Not positive, but I think you may still need to pull out the results dictionary
from |results|.

[mitchellwrosen, 2012-07-21 00:18:01]

https://chromiumcodereview.appspot.com/10803043/diff/3008/chrome/browser/exte...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://chromiumcodereview.appspot.com/10803043/diff/3008/chrome/browser/exte...
chrome/browser/extensions/api/debugger/debugger_api.cc:240: if (command_params)
On 2012/07/20 21:26:53, Matt Tytel wrote:
> nit: People prefer wrapping in {} if the whole statement is longer than 2
lines.
> Another one below too.

Done.

https://chromiumcodereview.appspot.com/10803043/diff/3008/chrome/browser/exte...
chrome/browser/extensions/api/debugger/debugger_api.cc:309: std::string method =
method_name;
On 2012/07/20 21:26:54, Matt Tytel wrote:
> Got an extra string here I think.

Done.

[Matt Tytel, 2012-07-21 21:56:49]

LGTM.

https://chromiumcodereview.appspot.com/10803043/diff/5003/chrome/browser/exte...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://chromiumcodereview.appspot.com/10803043/diff/5003/chrome/browser/exte...
chrome/browser/extensions/api/debugger/debugger_api.cc:310: if
(dictionary->Get("params", &params_value)) {
Could use GetDictionary here so we don't have to cast below.

[not at google - send to devlin, 2012-07-23 13:56:23]

still lgtm.

https://chromiumcodereview.appspot.com/10803043/diff/6002/chrome/browser/exte...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://chromiumcodereview.appspot.com/10803043/diff/6002/chrome/browser/exte...
chrome/browser/extensions/api/debugger/debugger_api.cc:243: if (params)
On 2012/07/20 19:09:57, Matt Tytel wrote:
> Maybe rename params to command_params here and other places to avoid the
> confusion that I think is happening here.

Yep, I misread that. Thanks.

https://chromiumcodereview.appspot.com/10803043/diff/11/chrome/browser/extens...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://chromiumcodereview.appspot.com/10803043/diff/11/chrome/browser/extens...
chrome/browser/extensions/api/debugger/debugger_api.cc:257:
scoped_ptr<base::ListValue> args(OnDetach::Create(debuggee));
It occurs to me that we could get json schema compiler to generate a CreateJson
method or something, then all this boilerplate would go away...

I mean, every event method basically calls ToJson on the created result
eventually, anyway.

https://chromiumcodereview.appspot.com/10803043/diff/11/chrome/browser/extens...
chrome/browser/extensions/api/debugger/debugger_api.cc:481: DictionaryValue*
dictionary) {
s/dictionary/response/

[mitchellwrosen, 2012-07-23 17:30:20]

https://chromiumcodereview.appspot.com/10803043/diff/5003/chrome/browser/exte...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://chromiumcodereview.appspot.com/10803043/diff/5003/chrome/browser/exte...
chrome/browser/extensions/api/debugger/debugger_api.cc:310: if
(dictionary->Get("params", &params_value)) {
On 2012/07/21 21:56:49, Matt Tytel wrote:
> Could use GetDictionary here so we don't have to cast below.

Done.

https://chromiumcodereview.appspot.com/10803043/diff/11/chrome/browser/extens...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://chromiumcodereview.appspot.com/10803043/diff/11/chrome/browser/extens...
chrome/browser/extensions/api/debugger/debugger_api.cc:257:
scoped_ptr<base::ListValue> args(OnDetach::Create(debuggee));
On 2012/07/23 13:56:23, kalman wrote:
> It occurs to me that we could get json schema compiler to generate a
CreateJson
> method or something, then all this boilerplate would go away...
> 
> I mean, every event method basically calls ToJson on the created result
> eventually, anyway.

Good thought. With events landed perhaps that should be the next major change

https://chromiumcodereview.appspot.com/10803043/diff/11/chrome/browser/extens...
chrome/browser/extensions/api/debugger/debugger_api.cc:481: DictionaryValue*
dictionary) {
On 2012/07/23 13:56:23, kalman wrote:
> s/dictionary/response/

Done.

[commit-bot: I haz the power, 2012-07-23 17:31:48]

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

[commit-bot: I haz the power, 2012-07-23 19:06:13]

Change committed as 147894