Added BookmarkClient::CanBeEditedByUser.

Added BookmarkClient::CanBeEditedByUser.

CanBeEditedByUser indicates if a bookmark can be edited by the user or not.
All of the bookmarks in the bookmark_bar_node, the other_node and the
mobile_node are always editable but some of the extra nodes provided by the
client may be unmodifiable.

Also updated other methods to work only with user-editable nodes, and added
utility helpers to detect nodes that can or can't be edited.

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

[Joao da Silva, 2014-06-06 15:27:34]

These are the component/bookmarks/ changes from
https://codereview.chromium.org/302313005, after adderssing your initial
comments. Should be easier to review separated from the bookmark_bar_view
changes :-)

Please have a look. Thanks!

[sky, 2014-06-06 16:29:47]

LGTM

One quirk with what you've added is that the model still lets users edit nodes
(add/remove/whatever) of non-editable nodes. You're relying on this when you
need to reload the managed node. I think the model should always check the
canedit function of the client (for adding/removing and anything) but the client
should be given a handle to an API that allows the client to mutate nodes.
Please add a TODO for this and come back to it later.

https://codereview.chromium.org/317333004/diff/60001/components/bookmarks/bro...
File components/bookmarks/browser/bookmark_client.h (right):

https://codereview.chromium.org/317333004/diff/60001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_client.h:78: // Checks if this node can be
edited by the user.
Checks-> Returns true (bonus points if you update the other Can methods too).

https://codereview.chromium.org/317333004/diff/60001/components/bookmarks/bro...
File components/bookmarks/browser/bookmark_model.h (right):

https://codereview.chromium.org/317333004/diff/60001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.h:137: void RemoveAll();
Update the description of this to indicate it doesn't recurse into permanent
nodes that are not user editable. Also, update description for
BookmarkModelObserver::BookmarkAllNodesRemoved too. You should also rename both
methods to make this clearer, but you can do that separately. At a minimum add a
TODO here.

https://codereview.chromium.org/317333004/diff/60001/components/bookmarks/bro...
File components/bookmarks/browser/bookmark_model_unittest.cc (right):

https://codereview.chromium.org/317333004/diff/60001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model_unittest.cc:270:
BookmarkPermanentNode* ReloadModeWithExtraNode() {
Mode->Model

https://codereview.chromium.org/317333004/diff/60001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model_unittest.cc:1118: // Verifies that
IsBookmarked is true if any bookmark matches the given URL,
nit: move comments like this above the TEST_F. See 951-953 for an example of
what I mean.

https://codereview.chromium.org/317333004/diff/60001/components/bookmarks/bro...
File components/bookmarks/browser/bookmark_utils.h (right):

https://codereview.chromium.org/317333004/diff/60001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_utils.h:140: // Returns true all the nodes
in the |list| can be edited by the user, as
all -> id all

https://codereview.chromium.org/317333004/diff/60001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_utils.h:141: // determined by
BookmarkClient::CanBeEditedByUser.
nit: use () when referring to function names in comments, eg
CanBeEditedByUser().

https://codereview.chromium.org/317333004/diff/60001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_utils.h:143: const std::vector<const
BookmarkNode*>& list);
nit: using the name list for vector is a bit odd. Maybe nodes?

[Joao da Silva, 2014-06-06 17:46:40]

That's a great idea, to check CanBeEditedByUser more aggressively. I also
worried that other code can inadvertently mutate a managed node, and having
those checks would prevent that.

Coming to think a bit more about it, the CanBeEditedByUser call is made to the
ChromeBookmarkClient, which is also where the managed nodes are mutated from; so
the ChromeBookmarkClient can have this temporary "no checks" state while it is
updating the model, and the bookmark_model and bookmark_client don't need
further changes to support that.

This assumes that all operations on the BookmarkModel are synchronous on the UI
thread, which I think is the case... another little issue is observers of the
model doing mutations again during the notification.

I've left a TODO and am going to land this one as-is for now.

https://codereview.chromium.org/317333004/diff/60001/components/bookmarks/bro...
File components/bookmarks/browser/bookmark_client.h (right):

https://codereview.chromium.org/317333004/diff/60001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_client.h:78: // Checks if this node can be
edited by the user.
On 2014/06/06 16:29:47, sky wrote:
> Checks-> Returns true (bonus points if you update the other Can methods too).

Done.

https://codereview.chromium.org/317333004/diff/60001/components/bookmarks/bro...
File components/bookmarks/browser/bookmark_model.h (right):

https://codereview.chromium.org/317333004/diff/60001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.h:137: void RemoveAll();
On 2014/06/06 16:29:47, sky wrote:
> Update the description of this to indicate it doesn't recurse into permanent
> nodes that are not user editable. Also, update description for
> BookmarkModelObserver::BookmarkAllNodesRemoved too. You should also rename
both
> methods to make this clearer, but you can do that separately. At a minimum add
a
> TODO here.

Done. I'll do the renaming in a separate CL.

https://codereview.chromium.org/317333004/diff/60001/components/bookmarks/bro...
File components/bookmarks/browser/bookmark_model_unittest.cc (right):

https://codereview.chromium.org/317333004/diff/60001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model_unittest.cc:270:
BookmarkPermanentNode* ReloadModeWithExtraNode() {
On 2014/06/06 16:29:47, sky wrote:
> Mode->Model

Done.

https://codereview.chromium.org/317333004/diff/60001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model_unittest.cc:1118: // Verifies that
IsBookmarked is true if any bookmark matches the given URL,
On 2014/06/06 16:29:47, sky wrote:
> nit: move comments like this above the TEST_F. See 951-953 for an example of
> what I mean.

Done.

https://codereview.chromium.org/317333004/diff/60001/components/bookmarks/bro...
File components/bookmarks/browser/bookmark_utils.h (right):

https://codereview.chromium.org/317333004/diff/60001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_utils.h:140: // Returns true all the nodes
in the |list| can be edited by the user, as
On 2014/06/06 16:29:47, sky wrote:
> all -> id all

Done.

https://codereview.chromium.org/317333004/diff/60001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_utils.h:141: // determined by
BookmarkClient::CanBeEditedByUser.
On 2014/06/06 16:29:47, sky wrote:
> nit: use () when referring to function names in comments, eg
> CanBeEditedByUser().

Done.

https://codereview.chromium.org/317333004/diff/60001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_utils.h:143: const std::vector<const
BookmarkNode*>& list);
On 2014/06/06 16:29:47, sky wrote:
> nit: using the name list for vector is a bit odd. Maybe nodes?

Done.

[Joao da Silva, 2014-06-06 17:46:46]

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-06-06 17:47:35]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/317333004/100001

[commit-bot: I haz the power, 2014-06-06 21:10:25]

Change committed as 275523

[Kibeom Kim (inactive), 2014-06-06 22:27:24]

A revert of this CL has been created in
https://codereview.chromium.org/312093007/ by kkimlabs@chromium.org.

The reason for reverting is: http://crbug.com/381921.

[Joao da Silva, 2014-06-06 22:41:21]

Kibeom has landed a different fix at
https://chrome-internal-review.googlesource.com/165381 so that this doesn't need
to be reverted.

Meanwhile I'm working on the Android CL for this feature too.

Thanks for holding the revert & getting the fix quickly landing :-)