Make the Bookmark Manager aware of managed bookmarks.

Make the Bookmark Manager aware of managed bookmarks.

The managed bookmarks are a new node at the root that are configured via
policy and can't be modified directly by the user. This change makes the
Bookmark Manager aware of those nodes by disabling the mutating options
when working with nodes from the managed tree.

BUG=49598
R=arv@chromium.org

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

[Joao da Silva, 2014-06-01 20:05:19]

Hi Erik, PTAL. Thanks!

[arv (Not doing code reviews), 2014-06-02 23:33:23]

In general I'm a bit concerned about walking the tree too much. Can this be done
in a faster way?

https://codereview.chromium.org/308003013/diff/1/chrome/browser/resources/boo...
File chrome/browser/resources/bookmark_manager/js/bmm.js (right):

https://codereview.chromium.org/308003013/diff/1/chrome/browser/resources/boo...
chrome/browser/resources/bookmark_manager/js/bmm.js:41: * @param
{!BookmarkTreeNode} node The node to test.
The code assumes node can be null but the type annotation contradicts that.
Which one is right?

https://codereview.chromium.org/308003013/diff/1/chrome/browser/resources/boo...
chrome/browser/resources/bookmark_manager/js/bmm.js:41: * @param
{!BookmarkTreeNode} node The node to test.
Remove useless description

https://codereview.chromium.org/308003013/diff/1/chrome/browser/resources/boo...
chrome/browser/resources/bookmark_manager/js/bmm.js:49: return
this.contains(bmm.treeLookup[MANAGED_NODE_ID].bookmarkNode, node);
This is O(depth). Is that acceptable?

https://codereview.chromium.org/308003013/diff/1/chrome/browser/resources/boo...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://codereview.chromium.org/308003013/diff/1/chrome/browser/resources/boo...
chrome/browser/resources/bookmark_manager/js/main.js:803: return
nodes.some(bmm.isManaged.bind(bmm));
no need for bind here since isManaged does not use `this` (bind is slow)

[Joao da Silva, 2014-06-03 14:07:54]

The tree is usually very shallow, and even at the limit of 50 nodes for the
managed tree this is going to be quite fast.

PTAL, thanks!

https://codereview.chromium.org/308003013/diff/1/chrome/browser/resources/boo...
File chrome/browser/resources/bookmark_manager/js/bmm.js (right):

https://codereview.chromium.org/308003013/diff/1/chrome/browser/resources/boo...
chrome/browser/resources/bookmark_manager/js/bmm.js:41: * @param
{!BookmarkTreeNode} node The node to test.
On 2014/06/02 23:33:23, arv wrote:
> The code assumes node can be null but the type annotation contradicts that.
> Which one is right?

The annotation is wrong, fixed.

https://codereview.chromium.org/308003013/diff/1/chrome/browser/resources/boo...
chrome/browser/resources/bookmark_manager/js/bmm.js:41: * @param
{!BookmarkTreeNode} node The node to test.
On 2014/06/02 23:33:23, arv wrote:
> Remove useless description

PRESUBMIT.py runs a javascript linter that fails without it:

  E0214: Missing description in @param tag

https://codereview.chromium.org/308003013/diff/1/chrome/browser/resources/boo...
chrome/browser/resources/bookmark_manager/js/bmm.js:49: return
this.contains(bmm.treeLookup[MANAGED_NODE_ID].bookmarkNode, node);
On 2014/06/02 23:33:23, arv wrote:
> This is O(depth). Is that acceptable?

I think it's fine because the bookmarks tree is rather shallow. Even a tree with
a depth of 1000 nodes would be quickly traversed, and this is performed only
when determining if a command is enabled (e.g. right-clicking a node, clicking
the Organize menu).

Also, these bookmarks are capped to a depth of at most 50, because each
declaration stacks 2 levels in the JSON representation and the JSONReader has a
max depth of 100.

https://codereview.chromium.org/308003013/diff/1/chrome/browser/resources/boo...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://codereview.chromium.org/308003013/diff/1/chrome/browser/resources/boo...
chrome/browser/resources/bookmark_manager/js/main.js:803: return
nodes.some(bmm.isManaged.bind(bmm));
On 2014/06/02 23:33:23, arv wrote:
> no need for bind here since isManaged does not use `this` (bind is slow)

Done.

[arv (Not doing code reviews), 2014-06-03 14:31:38]

LGTM

https://codereview.chromium.org/308003013/diff/1/chrome/browser/resources/boo...
File chrome/browser/resources/bookmark_manager/js/bmm.js (right):

https://codereview.chromium.org/308003013/diff/1/chrome/browser/resources/boo...
chrome/browser/resources/bookmark_manager/js/bmm.js:41: * @param
{!BookmarkTreeNode} node The node to test.
On 2014/06/03 14:07:54, Joao da Silva wrote:
> On 2014/06/02 23:33:23, arv wrote:
> > Remove useless description
> 
> PRESUBMIT.py runs a javascript linter that fails without it:
> 
>   E0214: Missing description in @param tag

Sad. Our linter is out of date.

https://codereview.chromium.org/308003013/diff/20001/chrome/browser/resources...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://codereview.chromium.org/308003013/diff/20001/chrome/browser/resources...
chrome/browser/resources/bookmark_manager/js/main.js:557: e.canExecute =
isFolder && canEdit && !hasManaged(items);
Can't this just check if the list is managed since all the items in the list
have the same logical parent (except for the search result).

[Joao da Silva, 2014-06-05 20:28:39]

Erik, please have another look at this.

The assumption that the top-level nodes have stable IDs was false, so looking
for an ancestor with ID "4" to determine if a node is managed was broken.

I agreed with the bookmarks & extensions owners to introduce a new optional
property to BookmarkTreeNodes that indicates if they're unmodifiable, and this
CL has been updated to use that instead. It's a bit less code now, and as a
bonus the tree traversal is gone :-)

https://codereview.chromium.org/308003013/diff/20001/chrome/browser/resources...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://codereview.chromium.org/308003013/diff/20001/chrome/browser/resources...
chrome/browser/resources/bookmark_manager/js/main.js:557: e.canExecute =
isFolder && canEdit && !hasManaged(items);
On 2014/06/03 14:31:39, arv wrote:
> Can't this just check if the list is managed since all the items in the list
> have the same logical parent (except for the search result).

The isManaged check needs a BookmarkTreeNode, I'm not sure how we can get the
parent of all of the list's elements...

https://codereview.chromium.org/308003013/diff/40001/chrome/browser/resources...
File chrome/browser/resources/bookmark_manager/js/main.js (right):

https://codereview.chromium.org/308003013/diff/40001/chrome/browser/resources...
chrome/browser/resources/bookmark_manager/js/main.js:803: return node &&
node.unmodifiable;
The "unmodifiable" property will be new in 37, see
https://codereview.chromium.org/320473002 for the implementation.

[arv (Not doing code reviews), 2014-06-05 20:55:21]

LGTM

[Joao da Silva, 2014-06-06 10:52:35]

Committed patchset #4 manually as r275388 (presubmit successful).