Made the bookmarks extension APIs aware of managed bookmarks.

Made the bookmarks extension APIs aware of managed bookmarks.

The mutating APIs now fail with a new error if they're used to modify
managed bookmarks.

BUG=49598
R=kalman@chromium.org, sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275385

[Joao da Silva, 2014-06-01 19:53:15]

Hi Benjamin, can you review this or suggest more appropriate reviewers? Thanks!

[not at google - send to devlin, 2014-06-02 20:08:46]

rfevang@ or wittman@ are either of you familiar enough to review this? Or know
anybody else?

I've been reviewing bookmarks API changes recently but mostly in lieu of having
any bookmarks API owners at all.

[Mike Wittman, 2014-06-02 21:19:47]

On 2014/06/02 20:08:46, kalman wrote:
> rfevang@ or wittman@ are either of you familiar enough to review this? Or know
> anybody else?
> 
> I've been reviewing bookmarks API changes recently but mostly in lieu of
having
> any bookmarks API owners at all.

I can review this if need be, although I'm not incredibly familiar with the code
that this touches. rfevang@ is unavailable for the next couple weeks. Maybe
sky@, but I don't know if he cares to review extension CLs?

[not at google - send to devlin, 2014-06-02 21:29:29]

ok don't worry about it. I will review. thanks.

[not at google - send to devlin, 2014-06-02 21:49:28]

lgtm

so many lines of code... :( I'd feel much more comfortable if this API were
factored with a central place to make these decisions (has parent; is managed;
etc) but oh well.

https://codereview.chromium.org/308273002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/bookmarks/bookmarks_api.cc (right):

https://codereview.chromium.org/308273002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bookmarks/bookmarks_api.cc:639: // TODO(erikkay)
return an error message.
drive-by: remove this TODO?

https://codereview.chromium.org/308273002/diff/1/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/bookmarks/test.js (right):

https://codereview.chromium.org/308273002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/bookmarks/test.js:435: function
removeTree() {
have you tested removeTree? it's the only function I can't find extra tests for.

[Joao da Silva, 2014-06-03 12:54:00]

Benjamin, PTAL. WDYT of the helper function for the common checks?

https://codereview.chromium.org/308273002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/bookmarks/bookmarks_api.cc (right):

https://codereview.chromium.org/308273002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/bookmarks/bookmarks_api.cc:639: // TODO(erikkay)
return an error message.
On 2014/06/02 21:49:29, kalman wrote:
> drive-by: remove this TODO?

Done.

https://codereview.chromium.org/308273002/diff/1/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/bookmarks/test.js (right):

https://codereview.chromium.org/308273002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/bookmarks/test.js:435: function
removeTree() {
On 2014/06/02 21:49:29, kalman wrote:
> have you tested removeTree? it's the only function I can't find extra tests
for.

Added a new test for that.

[not at google - send to devlin, 2014-06-03 18:09:15]

lgtm

https://codereview.chromium.org/308273002/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/bookmark_manager_private/bookmark_manager_private_api.cc
(right):

https://codereview.chromium.org/308273002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/bookmark_manager_private/bookmark_manager_private_api.cc:357:
if (cut && bookmark_utils::HasManaged(model, nodes)) {
i was considering suggesting such a function in the first patchset but didn't
want to hold you up. cool :)

https://codereview.chromium.org/308273002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/bookmarks/bookmarks_api.cc (right):

https://codereview.chromium.org/308273002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/bookmarks/bookmarks_api.cc:646: if
(model->IsManaged(node)) {
why not !CanBeModified(node)?

https://codereview.chromium.org/308273002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/bookmarks/bookmarks_api.h (right):

https://codereview.chromium.org/308273002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/bookmarks/bookmarks_api.h:152: // children can't
be moved or reordered.
I'm a little confused by this comment. Why does, for example, |node| being a
root node prevent it from having a new child appended?

[Joao da Silva, 2014-06-05 17:10:32]

@sky: please review the change to chrome_bookmark_client

Benjamin, this had to change a bit to accommodate for changes in the underlying
code structure. The checks are still the same as before, but the IsManaged()
check is now at the ChromeBookmarkClient instead of the BookmarkModel. Do you
mind have one last look? :-)

Thanks!

https://codereview.chromium.org/308273002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/bookmarks/bookmarks_api.cc (right):

https://codereview.chromium.org/308273002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/bookmarks/bookmarks_api.cc:646: if
(model->IsManaged(node)) {
On 2014/06/03 18:09:16, kalman wrote:
> why not !CanBeModified(node)?

I thought some of the checks in CanBeModified are redundant for |node|, given
that only IsManaged can possibly fail there. But this becomes more readable and
it's still correct, so done.

https://codereview.chromium.org/308273002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/bookmarks/bookmarks_api.h (right):

https://codereview.chromium.org/308273002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/bookmarks/bookmarks_api.h:152: // children can't
be moved or reordered.
On 2014/06/03 18:09:16, kalman wrote:
> I'm a little confused by this comment. Why does, for example, |node| being a
> root node prevent it from having a new child appended?

Note that there is _the_ root node, and there are the permanent "root" nodes,
which are the direct children of the root node. You can add nodes to the
permanent nodes (bookmark_bar, other_node, etc) but not to the root_node.

[not at google - send to devlin, 2014-06-05 17:16:36]

lgtm

https://codereview.chromium.org/308273002/diff/40001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/chrome_bookmark_client.cc (right):

https://codereview.chromium.org/308273002/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:68: bool
ChromeBookmarkClient::IsAManagedNode(const BookmarkNode* node) {
IsA* is a bit odd, Is* is more typical.

https://codereview.chromium.org/308273002/diff/40001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/chrome_bookmark_client.h (right):

https://codereview.chromium.org/308273002/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.h:42: // Returns true if there
is at least one managed node in the |list|.
indent += 2

[sky, 2014-06-05 18:13:20]

LGTM

https://codereview.chromium.org/308273002/diff/40001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/chrome_bookmark_client.cc (right):

https://codereview.chromium.org/308273002/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:68: bool
ChromeBookmarkClient::IsAManagedNode(const BookmarkNode* node) {
On 2014/06/05 17:16:36, kalman wrote:
> IsA* is a bit odd, Is* is more typical.

I still prefer IsDescendantOfManagedNode.

[Joao da Silva, 2014-06-05 18:58:18]

Thanks for the comments!

https://codereview.chromium.org/308273002/diff/40001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/chrome_bookmark_client.cc (right):

https://codereview.chromium.org/308273002/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:68: bool
ChromeBookmarkClient::IsAManagedNode(const BookmarkNode* node) {
On 2014/06/05 18:13:20, sky wrote:
> On 2014/06/05 17:16:36, kalman wrote:
> > IsA* is a bit odd, Is* is more typical.
> 
> I still prefer IsDescendantOfManagedNode.

Renamed at the parent CL (https://codereview.chromium.org/319543003)

https://codereview.chromium.org/308273002/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:68: bool
ChromeBookmarkClient::IsAManagedNode(const BookmarkNode* node) {
On 2014/06/05 17:16:36, kalman wrote:
> IsA* is a bit odd, Is* is more typical.

IsManagedNode() can also be interpreted as node == managed_node_; I've renamed
to the suggestion below.

https://codereview.chromium.org/308273002/diff/40001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/chrome_bookmark_client.h (right):

https://codereview.chromium.org/308273002/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.h:42: // Returns true if there
is at least one managed node in the |list|.
On 2014/06/05 17:16:36, kalman wrote:
> indent += 2

Done.

[Joao da Silva, 2014-06-06 10:48:47]

Committed patchset #5 manually as r275385 (presubmit successful).