BookmarkClient can add extra nodes to BookmarkModel.

BookmarkClient can add extra nodes to BookmarkModel.

This change extends the BookmarkModel to optionally load additional root nodes from the BookmarkClient. The BookmarClient interface has also been expanded to make additional checks on those nodes.

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

[Joao da Silva, 2014-05-30 20:26:11]

Please have a look:

@sky: bookmarks changes

@nyquist: android changes

@atwilson: sync changes

Thanks!

[sky, 2014-05-30 22:31:36]

I didn't make it all the way through this. I have to think on this a bit more.
It would be nice if BookmarkModel didn't need all the managed stuff, but instead
offered a way for the client to provide this functionality. I'm going to think
on this.

https://codereview.chromium.org/305973004/diff/20001/chrome/browser/ui/bookma...
File chrome/browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc
(right):

https://codereview.chromium.org/305973004/diff/20001/chrome/browser/ui/bookma...
chrome/browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc:84:
void AddTestData(BookmarkModel* model) {
Move this above members (as style guide says). Also, since this doesn't touch
local state make it static, or even more to a non-member function.

https://codereview.chromium.org/305973004/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc (right):

https://codereview.chromium.org/305973004/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc:498:
DCHECK(root_node->child_count() >= 2 && root_node->child_count() <= 3);
Don't you need to adjust this?

https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.cc:230: // The managed bookmarks
can't be removed.
Not removing managed bookmarks makes this function name and the observer name
confusing. I think you should rename RemoveAll to RemoveAllUserEditableNodes and
update the observer functions similarly. That'll force you to make sure doing
this doesn't cause any problems. If you could do the rename in a separate patch,
that would be great.

https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.cc:341: // The managed node can
change its title.
Why do we want to allow the managed nodes title to change?

https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.cc:646: if (IsManaged(parent)) {
Should this be a DCHECK? Same with 673 and 454.

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

https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.h:115: // are never editable, and
managed folders can't get bookmarks added, removed
Do we not allow applying policy changes during runtime?

https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.h:117: bool IsManaged(const
BookmarkNode* node) const;
This name is mildly confusing. It sort of implies this is equivalent to node ==
managed_node_.

https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
File components/bookmarks/browser/managed_bookmarks_tracker.h (right):

https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/managed_bookmarks_tracker.h:29: static const char
kName[];
nit: constants before constructor.

[Joao da Silva, 2014-06-01 13:32:07]

Addressed the first round of comments and fixed the android build.

Scott, what part of the managed stuff would you like to move out of the model?

Moving the loading bits out of the model and the storage is straightforward, and
it can be all encapsulated in the managed_bookmark_tracker. This initial
implementation tried to keep all bookmark loading done through one code path.

Are you also considering to have the managed_node out of the model and
root_node?

https://codereview.chromium.org/305973004/diff/20001/chrome/browser/ui/bookma...
File chrome/browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc
(right):

https://codereview.chromium.org/305973004/diff/20001/chrome/browser/ui/bookma...
chrome/browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc:84:
void AddTestData(BookmarkModel* model) {
On 2014/05/30 22:31:37, sky wrote:
> Move this above members (as style guide says). Also, since this doesn't touch
> local state make it static, or even more to a non-member function.

Done.

https://codereview.chromium.org/305973004/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc (right):

https://codereview.chromium.org/305973004/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc:498:
DCHECK(root_node->child_count() >= 2 && root_node->child_count() <= 3);
On 2014/05/30 22:31:37, sky wrote:
> Don't you need to adjust this?

Yes, done.

https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.cc:230: // The managed bookmarks
can't be removed.
On 2014/05/30 22:31:37, sky wrote:
> Not removing managed bookmarks makes this function name and the observer name
> confusing. I think you should rename RemoveAll to RemoveAllUserEditableNodes
and
> update the observer functions similarly. That'll force you to make sure doing
> this doesn't cause any problems. If you could do the rename in a separate
patch,
> that would be great.

Agreed, I'll rename in a subsequent CL.

https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.cc:341: // The managed node can
change its title.
On 2014/05/30 22:31:37, sky wrote:
> Why do we want to allow the managed nodes title to change?

This was the UX decision when we first introduced managed bookmarks on the
mobile platforms: if the bookmarks come from cloud policy then the folder name
is "$DOMAIN bookmarks", otherwise it's "Managed bookmarks" (e.g. the policy came
via MDM).

The title can change at runtime if the user signs in or out of a managed
account.

https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.cc:646: if (IsManaged(parent)) {
On 2014/05/30 22:31:37, sky wrote:
> Should this be a DCHECK? Same with 673 and 454.

Done.

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

https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.h:115: // are never editable, and
managed folders can't get bookmarks added, removed
On 2014/05/30 22:31:37, sky wrote:
> Do we not allow applying policy changes during runtime?

We do, and the managed bookmarks will update when that happens. Clarified that
these bookmarks are not editable by the user.

https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.h:117: bool IsManaged(const
BookmarkNode* node) const;
On 2014/05/30 22:31:37, sky wrote:
> This name is mildly confusing. It sort of implies this is equivalent to node
==
> managed_node_.

Right, naming is hard :-) How about "IsInManagedTree" or "InManagedTree"?

https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
File components/bookmarks/browser/managed_bookmarks_tracker.h (right):

https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/managed_bookmarks_tracker.h:29: static const char
kName[];
On 2014/05/30 22:31:37, sky wrote:
> nit: constants before constructor.

Done.

[sky, 2014-06-02 23:08:40]

On Sun, Jun 1, 2014 at 6:32 AM,  <joaodasilva@chromium.org> wrote:
> Addressed the first round of comments and fixed the android build.
>
> Scott, what part of the managed stuff would you like to move out of the
> model?

What you basically want is the ability to expose an additional root
node and impose some limitations on that node. I would rather expose
that API via the BookmarkClient and minimize what the code in
bookmarks does. That results in a smaller API footprint for bookmarks.
I believe this would be possible via a couple of methods in
BookmarkClient. Does that make sense?

  -Scott


>
> Moving the loading bits out of the model and the storage is straightforward,
> and
> it can be all encapsulated in the managed_bookmark_tracker. This initial
> implementation tried to keep all bookmark loading done through one code
> path.
>
> Are you also considering to have the managed_node out of the model and
> root_node?
>
>
>
>
https://codereview.chromium.org/305973004/diff/20001/chrome/browser/ui/bookma...
> File
> chrome/browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc
> (right):
>
>
https://codereview.chromium.org/305973004/diff/20001/chrome/browser/ui/bookma...
> chrome/browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc:84:
> void AddTestData(BookmarkModel* model) {
> On 2014/05/30 22:31:37, sky wrote:
>>
>> Move this above members (as style guide says). Also, since this
>
> doesn't touch
>>
>> local state make it static, or even more to a non-member function.
>
>
> Done.
>
>
>
https://codereview.chromium.org/305973004/diff/20001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc (right):
>
>
https://codereview.chromium.org/305973004/diff/20001/chrome/browser/ui/views/...
> chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc:498:
> DCHECK(root_node->child_count() >= 2 && root_node->child_count() <= 3);
> On 2014/05/30 22:31:37, sky wrote:
>>
>> Don't you need to adjust this?
>
>
> Yes, done.
>
>
>
https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
> File components/bookmarks/browser/bookmark_model.cc (right):
>
>
https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
> components/bookmarks/browser/bookmark_model.cc:230: // The managed
> bookmarks can't be removed.
> On 2014/05/30 22:31:37, sky wrote:
>>
>> Not removing managed bookmarks makes this function name and the
>
> observer name
>>
>> confusing. I think you should rename RemoveAll to
>
> RemoveAllUserEditableNodes and
>>
>> update the observer functions similarly. That'll force you to make
>
> sure doing
>>
>> this doesn't cause any problems. If you could do the rename in a
>
> separate patch,
>>
>> that would be great.
>
>
> Agreed, I'll rename in a subsequent CL.
>
>
>
https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
> components/bookmarks/browser/bookmark_model.cc:341: // The managed node
> can change its title.
> On 2014/05/30 22:31:37, sky wrote:
>>
>> Why do we want to allow the managed nodes title to change?
>
>
> This was the UX decision when we first introduced managed bookmarks on
> the mobile platforms: if the bookmarks come from cloud policy then the
> folder name is "$DOMAIN bookmarks", otherwise it's "Managed bookmarks"
> (e.g. the policy came via MDM).
>
> The title can change at runtime if the user signs in or out of a managed
> account.
>
>
>
https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
> components/bookmarks/browser/bookmark_model.cc:646: if
> (IsManaged(parent)) {
> On 2014/05/30 22:31:37, sky wrote:
>>
>> Should this be a DCHECK? Same with 673 and 454.
>
>
> Done.
>
>
>
https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
> File components/bookmarks/browser/bookmark_model.h (right):
>
>
https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
> components/bookmarks/browser/bookmark_model.h:115: // are never
> editable, and managed folders can't get bookmarks added, removed
> On 2014/05/30 22:31:37, sky wrote:
>>
>> Do we not allow applying policy changes during runtime?
>
>
> We do, and the managed bookmarks will update when that happens.
> Clarified that these bookmarks are not editable by the user.
>
>
>
https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
> components/bookmarks/browser/bookmark_model.h:117: bool IsManaged(const
> BookmarkNode* node) const;
> On 2014/05/30 22:31:37, sky wrote:
>>
>> This name is mildly confusing. It sort of implies this is equivalent
>
> to node ==
>>
>> managed_node_.
>
>
> Right, naming is hard :-) How about "IsInManagedTree" or
> "InManagedTree"?
>
>
>
https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
> File components/bookmarks/browser/managed_bookmarks_tracker.h (right):
>
>
https://codereview.chromium.org/305973004/diff/20001/components/bookmarks/bro...
> components/bookmarks/browser/managed_bookmarks_tracker.h:29: static
> const char kName[];
> On 2014/05/30 22:31:37, sky wrote:
>>
>> nit: constants before constructor.
>
>
> Done.
>
> https://codereview.chromium.org/305973004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Joao da Silva, 2014-06-04 17:05:23]

Scott, can you have a first look at this?

The BookmarkModel can now append additional nodes to the root that are supplied
by the BookmarkClient. The model is completely unaware of managed bookmarks, and
only the ChromeBookmarkClient knows about that.

So other places that need to check if a bookmark is managed need to make that
test using the ChromeBookmarkClient; that's why the BookmarkModelFactory got the
new acessor.

It all seems to work so far, the remaining issue is some sync tests failing; I'm
looking into whether this is some serious issue due to the node IDs getting out
of sync.

(This could probably be split into 3 or so CLs but having all these changes in
one CL makes it easier to see how it all comes together, IMO)

[sky, 2014-06-04 20:39:51]

Nice! this is what I was thinking of.

I would prefer splitting this up. Should be easy to land the
components/bookmarks changes and then finish the others.

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

https://codereview.chromium.org/305973004/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:64: while (node && node !=
managed_node_)
return node->HasAncestor(managed_node_);

https://codereview.chromium.org/305973004/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:219: // Create the managed
node now, and load its initial contents.
DCHECK not on UI thread.

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

https://codereview.chromium.org/305973004/diff/80001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_client.h:61: virtual bool
IsPermanentNodeVisible(const BookmarkNode* node) = 0;
Shouldn't this continue to take a permanentnode?

https://codereview.chromium.org/305973004/diff/80001/components/bookmarks/bro...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/305973004/diff/80001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.cc:776:
client_->ExtraNodesLoaded(extra_nodes);
Can this be nuked and instead use BookmarkModelLoaded and dig out the nodes?
Alternatively can't the callback track the created nodes if it wants to?

[Joao da Silva, 2014-06-04 22:35:57]

Cool, I've updated this issue to just make the bookmarks changes & addressed the
pending issues. Please have another look; thanks!

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

https://codereview.chromium.org/305973004/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:64: while (node && node !=
managed_node_)
On 2014/06/04 20:39:52, sky wrote:
> return node->HasAncestor(managed_node_);

Done.

https://codereview.chromium.org/305973004/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:219: // Create the managed
node now, and load its initial contents.
On 2014/06/04 20:39:52, sky wrote:
> DCHECK not on UI thread.

That fails in around 100 unit tests in ~10 suites, because they map all
BrowserThread::IDs to the same loop as UI.

I've made it explicit that this uses the Profile::GetIOTaskRunner and DCHECK
that that's the current runner; WDYT?

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

https://codereview.chromium.org/305973004/diff/80001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_client.h:61: virtual bool
IsPermanentNodeVisible(const BookmarkNode* node) = 0;
On 2014/06/04 20:39:52, sky wrote:
> Shouldn't this continue to take a permanentnode?

Done.

https://codereview.chromium.org/305973004/diff/80001/components/bookmarks/bro...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/305973004/diff/80001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.cc:776:
client_->ExtraNodesLoaded(extra_nodes);
On 2014/06/04 20:39:52, sky wrote:
> Can this be nuked and instead use BookmarkModelLoaded and dig out the nodes?
> Alternatively can't the callback track the created nodes if it wants to?

BookmarkModelLoaded() comes a tad too late, because the stable_sort below relies
on IsPermanentNodeVisible(), which requires the client to have handles to the
extra nodes. The nodes can also be out-of-order by then, again due to the sort.

But the callback can indeed track the extra nodes and this call is redundant, so
Done!

[sky, 2014-06-04 23:19:07]

LGTM with a couple of name changes.

https://codereview.chromium.org/305973004/diff/120001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/305973004/diff/120001/components/bookmarks/br...
components/bookmarks/browser/bookmark_model.cc:216: if
(!client_->CanRemoveNode(permanent_node))
The name here implies if you return true the node is removed, but that isn't the
case. This is more of CanRemovePermanentNodeChildren().

https://codereview.chromium.org/305973004/diff/120001/components/bookmarks/br...
components/bookmarks/browser/bookmark_model.cc:326: if (is_permanent_node(node)
&& !client_->CanSetTitle(node)) {
Same comment here, since only invoked for permanent nodes it should be:
CanSetPermanentNodeTitle.

[Joao da Silva, 2014-06-05 12:14:53]

https://codereview.chromium.org/305973004/diff/120001/components/bookmarks/br...
File components/bookmarks/browser/bookmark_model.cc (right):

https://codereview.chromium.org/305973004/diff/120001/components/bookmarks/br...
components/bookmarks/browser/bookmark_model.cc:216: if
(!client_->CanRemoveNode(permanent_node))
On 2014/06/04 23:19:07, sky wrote:
> The name here implies if you return true the node is removed, but that isn't
the
> case. This is more of CanRemovePermanentNodeChildren().

Done.

https://codereview.chromium.org/305973004/diff/120001/components/bookmarks/br...
components/bookmarks/browser/bookmark_model.cc:326: if (is_permanent_node(node)
&& !client_->CanSetTitle(node)) {
On 2014/06/04 23:19:07, sky wrote:
> Same comment here, since only invoked for permanent nodes it should be:
> CanSetPermanentNodeTitle.

Done.

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

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-06-05 12:15:55]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/305973004/140001

[commit-bot: I haz the power, 2014-06-05 16:36:26]

Change committed as 275156