Default parentId for bookmarks.create()

Default parentId for bookmarks.create()

Made the parentId argument for bookmarks.create() optional.
The default is the "Other Bookmarks" folder.

BUG=21410
TEST=Some of the bookmarks tests have been reported as flaky, so you will need
to change "DISABLED_Bookmarks" to "Bookmarks" in
chrome/browser/bookmarks/bookmark_extension_apitest.cc to run my test.
My test is called createNoParentId.


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

[cduvall, 2012-02-17 06:16:41]

Just sending out my first CL for you guys to check out.

[Devlin, 2012-02-17 06:45:46]

https://chromiumcodereview.appspot.com/9420041/diff/1/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/bookmarks/test.js (right):

https://chromiumcodereview.appspot.com/9420041/diff/1/chrome/test/data/extens...
chrome/test/data/extensions/api_test/bookmarks/test.js:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
Probably a good idea to update the copyright date.

https://chromiumcodereview.appspot.com/9420041/diff/1/chrome/test/data/extens...
chrome/test/data/extensions/api_test/bookmarks/test.js:177: // Make sure
parentId defaults to Other Bookmarks
nit: comments should be grammatically correct (including punctuation).

https://chromiumcodereview.appspot.com/9420041/diff/1/chrome/test/data/extens...
chrome/test/data/extensions/api_test/bookmarks/test.js:186:
expected[0].children[0].children.push(node);
This seems like a lot of duplicate code....Is there a way we avoid this?

[cduvall, 2012-02-17 07:01:34]

https://chromiumcodereview.appspot.com/9420041/diff/1/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/bookmarks/test.js (right):

https://chromiumcodereview.appspot.com/9420041/diff/1/chrome/test/data/extens...
chrome/test/data/extensions/api_test/bookmarks/test.js:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
On 2012/02/17 06:45:47, D Cronin wrote:
> Probably a good idea to update the copyright date.

Done.

https://chromiumcodereview.appspot.com/9420041/diff/1/chrome/test/data/extens...
chrome/test/data/extensions/api_test/bookmarks/test.js:177: // Make sure
parentId defaults to Other Bookmarks
On 2012/02/17 06:45:47, D Cronin wrote:
> nit: comments should be grammatically correct (including punctuation).

Done.

https://chromiumcodereview.appspot.com/9420041/diff/1/chrome/test/data/extens...
chrome/test/data/extensions/api_test/bookmarks/test.js:186:
expected[0].children[0].children.push(node);
On 2012/02/17 06:45:47, D Cronin wrote:
> This seems like a lot of duplicate code....Is there a way we avoid this?
The other tests (createInRoot(), create()) share almost all the same code, so I
wrote a similar test for consistency.

[Matt Tytel, 2012-02-22 00:43:32]

https://chromiumcodereview.appspot.com/9420041/diff/4001/chrome/test/data/ext...
File chrome/test/data/extensions/api_test/bookmarks/test.js (right):

https://chromiumcodereview.appspot.com/9420041/diff/4001/chrome/test/data/ext...
chrome/test/data/extensions/api_test/bookmarks/test.js:186:
expected[0].children[0].children.push(node);
You pushed here to expected[0].children[0].children but you didn't change how it
was accessed in the functions below. You might want to remove this line or use
the new node in later tests.

[cduvall, 2012-02-22 01:11:48]

https://chromiumcodereview.appspot.com/9420041/diff/4001/chrome/test/data/ext...
File chrome/test/data/extensions/api_test/bookmarks/test.js (right):

https://chromiumcodereview.appspot.com/9420041/diff/4001/chrome/test/data/ext...
chrome/test/data/extensions/api_test/bookmarks/test.js:186:
expected[0].children[0].children.push(node);
On 2012/02/22 00:43:32, mtytel wrote:
> You pushed here to expected[0].children[0].children but you didn't change how
it
> was accessed in the functions below. You might want to remove this line or use
> the new node in later tests.

Done.

[Matt Tytel, 2012-02-22 02:10:43]

lgtm

[cduvall, 2012-02-24 00:34:09]

Small bug, should be quick.

[clintstaley, 2012-02-24 02:16:58]

LGTM

[cduvall, 2012-02-29 02:13:37]

Thanks for your time.

[Aaron Boodman, 2012-02-29 20:56:41]

So are you saying that the C++ already handles this case?

[Aaron Boodman, 2012-02-29 20:56:47]

http://codereview.chromium.org/9420041/diff/6002/chrome/common/extensions/api...
File chrome/common/extensions/api/bookmarks.json (right):

http://codereview.chromium.org/9420041/diff/6002/chrome/common/extensions/api...
chrome/common/extensions/api/bookmarks.json:136: "parentId": {"type": "string",
"optional": true},
Can you add a description of what this will default to?

[cduvall, 2012-03-02 01:31:06]

On 2012/02/29 20:56:41, Aaron Boodman wrote:
> So are you saying that the C++ already handles this case?

Yes, in chrome/browser/bookmarks/bookmark_extension_api.cc line 469 this case is
handled.

[cduvall, 2012-03-02 01:31:25]

http://codereview.chromium.org/9420041/diff/6002/chrome/common/extensions/api...
File chrome/common/extensions/api/bookmarks.json (right):

http://codereview.chromium.org/9420041/diff/6002/chrome/common/extensions/api...
chrome/common/extensions/api/bookmarks.json:136: "parentId": {"type": "string",
"optional": true},
On 2012/02/29 20:56:48, Aaron Boodman wrote:
> Can you add a description of what this will default to?

Done.

[clintstaley, 2012-03-02 05:35:41]

LGTM

[Aaron Boodman, 2012-03-02 23:14:34]

LGTM too

[commit-bot: I haz the power, 2012-03-02 23:18:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/9420041/16001

[commit-bot: I haz the power, 2012-03-03 01:34:36]

Change committed as 124812