Refactor chrome.bookmarks API to use JSON schema compiler.

Refactor chrome.bookmarks API to use JSON schema compiler.

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

[Aaron Boodman, 2012-07-23 23:37:57]

http://codereview.chromium.org/10694062/diff/2001/chrome/browser/bookmarks/bo...
File chrome/browser/bookmarks/bookmark_extension_api.cc (right):

http://codereview.chromium.org/10694062/diff/2001/chrome/browser/bookmarks/bo...
chrome/browser/bookmarks/bookmark_extension_api.cc:309: if
(params->id_or_id_list_type == Get::Params::ID_OR_ID_LIST_ARRAY) {
I see this pattern a lot. Can we teach the JSON Schema Compiler about it so we
don't have to handle it manually each time? Like:

choice: [
  {type: "string", serialize_to: "list_of_type"}
  {type: "array", items: {type: "string"}}
]

JSON Schema Compiler would treat this as if the type was just array-of-string.

http://codereview.chromium.org/10694062/diff/2001/chrome/browser/bookmarks/bo...
chrome/browser/bookmarks/bookmark_extension_api.cc:459: if
(!base::StringToInt64(params->id, &id)) {
Could add a feature to JSON Schema Compiler to know about this int64-as-string
convention, and to be able to handle it internally. That would eliminate a lot
of manual code.

Something like:

{type:"string", serialized_type:"int64"}

[mitchellwrosen, 2012-07-24 17:13:38]

http://codereview.chromium.org/10694062/diff/2001/chrome/browser/bookmarks/bo...
File chrome/browser/bookmarks/bookmark_extension_api.cc (right):

http://codereview.chromium.org/10694062/diff/2001/chrome/browser/bookmarks/bo...
chrome/browser/bookmarks/bookmark_extension_api.cc:309: if
(params->id_or_id_list_type == Get::Params::ID_OR_ID_LIST_ARRAY) {
On 2012/07/23 23:37:57, Aaron Boodman wrote:
> I see this pattern a lot. Can we teach the JSON Schema Compiler about it so we
> don't have to handle it manually each time? Like:
> 
> choice: [
>   {type: "string", serialize_to: "list_of_type"}
>   {type: "array", items: {type: "string"}}
> ]
> 
> JSON Schema Compiler would treat this as if the type was just array-of-string.

If I understand correctly: the end result would be an enum (ex. IdOrIdListType)
to differentiate between the choices, and one member variable (ex. IdOrIdList)
rather than one per choice.

http://codereview.chromium.org/10694062/diff/2001/chrome/browser/bookmarks/bo...
chrome/browser/bookmarks/bookmark_extension_api.cc:459: if
(!base::StringToInt64(params->id, &id)) {
On 2012/07/23 23:37:57, Aaron Boodman wrote:
> Could add a feature to JSON Schema Compiler to know about this int64-as-string
> convention, and to be able to handle it internally. That would eliminate a lot
> of manual code.
> 
> Something like:
> 
> {type:"string", serialized_type:"int64"}

Sounds good. Separate CL though right?

[Aaron Boodman, 2012-07-24 21:25:10]

On 2012/07/24 17:13:38, mitchellwrosen wrote:
>
http://codereview.chromium.org/10694062/diff/2001/chrome/browser/bookmarks/bo...
> File chrome/browser/bookmarks/bookmark_extension_api.cc (right):
> 
>
http://codereview.chromium.org/10694062/diff/2001/chrome/browser/bookmarks/bo...
> chrome/browser/bookmarks/bookmark_extension_api.cc:309: if
> (params->id_or_id_list_type == Get::Params::ID_OR_ID_LIST_ARRAY) {
> On 2012/07/23 23:37:57, Aaron Boodman wrote:
> > I see this pattern a lot. Can we teach the JSON Schema Compiler about it so
we
> > don't have to handle it manually each time? Like:
> > 
> > choice: [
> >   {type: "string", serialize_to: "list_of_type"}
> >   {type: "array", items: {type: "string"}}
> > ]
> > 
> > JSON Schema Compiler would treat this as if the type was just
array-of-string.
> 
> If I understand correctly: the end result would be an enum (ex.
IdOrIdListType)
> to differentiate between the choices, and one member variable (ex. IdOrIdList)
> rather than one per choice.

You probably need to coordinate with someone who knows more about the compiler
than me, because it seems like the naive implementation would end up with two
choices that are exactly the same. What does the compiler do in this case? If it
collapses them, hooray.

It would also be nice if the compiler could recognize the case where there is
only one choice (after removing dupes). In that case, it could simplify the
generated C++ type.

Does this make sense?

http://codereview.chromium.org/10694062/diff/2001/chrome/browser/bookmarks/bo...
> chrome/browser/bookmarks/bookmark_extension_api.cc:459: if
> (!base::StringToInt64(params->id, &id)) {
> On 2012/07/23 23:37:57, Aaron Boodman wrote:
> > Could add a feature to JSON Schema Compiler to know about this
int64-as-string
> > convention, and to be able to handle it internally. That would eliminate a
lot
> > of manual code.
> > 
> > Something like:
> > 
> > {type:"string", serialized_type:"int64"}
> 
> Sounds good. Separate CL though right?

This one seems easier than the above. Can it be done in this CL?

[mitchellwrosen, 2012-07-24 21:32:44]

On 2012/07/24 21:25:10, Aaron Boodman wrote:
> On 2012/07/24 17:13:38, mitchellwrosen wrote:
> >
>
http://codereview.chromium.org/10694062/diff/2001/chrome/browser/bookmarks/bo...
> > File chrome/browser/bookmarks/bookmark_extension_api.cc (right):
> > 
> >
>
http://codereview.chromium.org/10694062/diff/2001/chrome/browser/bookmarks/bo...
> > chrome/browser/bookmarks/bookmark_extension_api.cc:309: if
> > (params->id_or_id_list_type == Get::Params::ID_OR_ID_LIST_ARRAY) {
> > On 2012/07/23 23:37:57, Aaron Boodman wrote:
> > > I see this pattern a lot. Can we teach the JSON Schema Compiler about it
so
> we
> > > don't have to handle it manually each time? Like:
> > > 
> > > choice: [
> > >   {type: "string", serialize_to: "list_of_type"}
> > >   {type: "array", items: {type: "string"}}
> > > ]
> > > 
> > > JSON Schema Compiler would treat this as if the type was just
> array-of-string.
> > 
> > If I understand correctly: the end result would be an enum (ex.
> IdOrIdListType)
> > to differentiate between the choices, and one member variable (ex.
IdOrIdList)
> > rather than one per choice.
> 
> You probably need to coordinate with someone who knows more about the compiler
> than me, because it seems like the naive implementation would end up with two
> choices that are exactly the same. What does the compiler do in this case? If
it
> collapses them, hooray.
> 
> It would also be nice if the compiler could recognize the case where there is
> only one choice (after removing dupes). In that case, it could simplify the
> generated C++ type.
> 
> Does this make sense?

It does make sense. I'll talk to cheebs about it.

http://codereview.chromium.org/10694062/diff/2001/chrome/browser/bookmarks/bo...
> > chrome/browser/bookmarks/bookmark_extension_api.cc:459: if
> > (!base::StringToInt64(params->id, &id)) {
> > On 2012/07/23 23:37:57, Aaron Boodman wrote:
> > > Could add a feature to JSON Schema Compiler to know about this
> int64-as-string
> > > convention, and to be able to handle it internally. That would eliminate a
> lot
> > > of manual code.
> > > 
> > > Something like:
> > > 
> > > {type:"string", serialized_type:"int64"}
> > 
> > Sounds good. Separate CL though right?
> 
> This one seems easier than the above. Can it be done in this CL?

Sure thing. I'll get on it once I get a windows build up and running to debug a
performance monitor related issue. I'm also on the lookout for some deeper bugs
as per your/Clint's recommendation =)

[Matt Tytel, 2012-07-24 22:24:28]

http://codereview.chromium.org/10694062/diff/2001/chrome/browser/bookmarks/bo...
File chrome/browser/bookmarks/bookmark_extension_api.cc (right):

http://codereview.chromium.org/10694062/diff/2001/chrome/browser/bookmarks/bo...
chrome/browser/bookmarks/bookmark_extension_api.cc:309: if
(params->id_or_id_list_type == Get::Params::ID_OR_ID_LIST_ARRAY) {
Json schema compiler drive-by:
Yea this looks implementable by collapsing choices that have the same serialized
type.
An alternative that would be more concise, easier to implement, but maybe too
confusing is:
{type: "array", serialize_singles: true, items:{type: "string"}}

[Aaron Boodman, 2012-07-25 21:40:52]

LGTM for now, the JSON schema proposals can be split out.

Matt: Do you mean that you'd put the following in the schema:

{ type: "array", some_special_marker: true, items: { type: "string" }}

... and then we'd change all the code that interprets schemas everywhere to know
that:

foo("monkey")

should be interpreted as:

foo(["monkey"])

?

That would also be fine by me. I'm not sure which one is easier to implement :)
Your idea requires changes to several places that interpret schemas.

[commit-bot: I haz the power, 2012-07-26 01:25:23]

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

[commit-bot: I haz the power, 2012-07-26 01:25:28]

Presubmit check for 10694062-14001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/bookmarks

Presubmit checks took 1.1s to calculate.

[mitchellwrosen, 2012-07-26 01:39:01]

+brettw for chrome/browser/bookmarks

[brettw, 2012-07-26 18:03:15]

https://chromiumcodereview.appspot.com/10694062/diff/14001/chrome/browser/boo...
File chrome/browser/bookmarks/bookmark_extension_api.cc (right):

https://chromiumcodereview.appspot.com/10694062/diff/14001/chrome/browser/boo...
chrome/browser/bookmarks/bookmark_extension_api.cc:45: namespace Create =
extensions::api::bookmarks::Create;
These namespaces seem really weird to me. It's confusing later on when you
write, for example, Get::Params. I would never expect "Get" to be a namespace.
Do you have any ideas for making this more clear?

[mitchellwrosen, 2012-07-26 18:05:14]

https://chromiumcodereview.appspot.com/10694062/diff/14001/chrome/browser/boo...
File chrome/browser/bookmarks/bookmark_extension_api.cc (right):

https://chromiumcodereview.appspot.com/10694062/diff/14001/chrome/browser/boo...
chrome/browser/bookmarks/bookmark_extension_api.cc:45: namespace Create =
extensions::api::bookmarks::Create;
On 2012/07/26 18:03:15, brettw wrote:
> These namespaces seem really weird to me. It's confusing later on when you
> write, for example, Get::Params. I would never expect "Get" to be a namespace.
> Do you have any ideas for making this more clear?

It's the only way of using the JSON schema compiler I've ever seen. I agree that
it looks weird.

[brettw, 2012-07-27 20:05:10]

https://chromiumcodereview.appspot.com/10694062/diff/14001/chrome/browser/boo...
File chrome/browser/bookmarks/bookmark_extension_api.cc (right):

https://chromiumcodereview.appspot.com/10694062/diff/14001/chrome/browser/boo...
chrome/browser/bookmarks/bookmark_extension_api.cc:45: namespace Create =
extensions::api::bookmarks::Create;
It wouldn't look so weird if the users of this code wrote bookmarks::Create and
bookmarks::Get since then it falls under my expectations of what namespaces look
like.

So what do you think about doing "namespace bookmarks =
extensions::api::bookmarks" and then using that below?

[mitchellwrosen, 2012-07-27 22:53:21]

http://codereview.chromium.org/10694062/diff/14001/chrome/browser/bookmarks/b...
File chrome/browser/bookmarks/bookmark_extension_api.cc (right):

http://codereview.chromium.org/10694062/diff/14001/chrome/browser/bookmarks/b...
chrome/browser/bookmarks/bookmark_extension_api.cc:45: namespace Create =
extensions::api::bookmarks::Create;
On 2012/07/27 20:05:10, brettw wrote:
> It wouldn't look so weird if the users of this code wrote bookmarks::Create
and
> bookmarks::Get since then it falls under my expectations of what namespaces
look
> like.
> 
> So what do you think about doing "namespace bookmarks =
> extensions::api::bookmarks" and then using that below?

Done.

[Matt Tytel, 2012-07-31 17:33:54]

Changes lgtm but think I'd prefer waiting for the bookmark manager to avoid the
extra functions defined.

https://chromiumcodereview.appspot.com/10694062/diff/16003/chrome/browser/boo...
File chrome/browser/bookmarks/bookmark_extension_helpers.cc (right):

https://chromiumcodereview.appspot.com/10694062/diff/16003/chrome/browser/boo...
chrome/browser/bookmarks/bookmark_extension_helpers.cc:54: bool recurse,
nit: indents off.

[brettw, 2012-07-31 21:45:13]

Owners LGTM. I didn't check the details.

http://codereview.chromium.org/10694062/diff/25003/chrome/browser/bookmarks/b...
File chrome/browser/bookmarks/bookmark_extension_helpers.cc (right):

http://codereview.chromium.org/10694062/diff/25003/chrome/browser/bookmarks/b...
chrome/browser/bookmarks/bookmark_extension_helpers.cc:7: #include <vector>
Style nit: C++ includes go after C ones.

http://codereview.chromium.org/10694062/diff/25003/chrome/browser/bookmarks/b...
chrome/browser/bookmarks/bookmark_extension_helpers.cc:72: if (!t.is_null())
Need {} since it's multiple lines.

[mitchellwrosen, 2012-07-31 21:54:26]

http://codereview.chromium.org/10694062/diff/16003/chrome/browser/bookmarks/b...
File chrome/browser/bookmarks/bookmark_extension_helpers.cc (right):

http://codereview.chromium.org/10694062/diff/16003/chrome/browser/bookmarks/b...
chrome/browser/bookmarks/bookmark_extension_helpers.cc:54: bool recurse,
On 2012/07/31 17:33:54, Matt Tytel wrote:
> nit: indents off.

Done.

http://codereview.chromium.org/10694062/diff/25003/chrome/browser/bookmarks/b...
File chrome/browser/bookmarks/bookmark_extension_helpers.cc (right):

http://codereview.chromium.org/10694062/diff/25003/chrome/browser/bookmarks/b...
chrome/browser/bookmarks/bookmark_extension_helpers.cc:7: #include <vector>
On 2012/07/31 21:45:13, brettw wrote:
> Style nit: C++ includes go after C ones.

Done.

http://codereview.chromium.org/10694062/diff/25003/chrome/browser/bookmarks/b...
chrome/browser/bookmarks/bookmark_extension_helpers.cc:72: if (!t.is_null())
On 2012/07/31 21:45:13, brettw wrote:
> Need {} since it's multiple lines.

Done.

[commit-bot: I haz the power, 2012-07-31 21:54:39]

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

[commit-bot: I haz the power, 2012-07-31 23:01:14]

Change committed as 149307