Show the Managed Bookmarks folder in the views UI.

Show the Managed Bookmarks folder in the views UI.

This change makes the Managed Bookmarks folder visible in the bookmarks
bar if it contains at least one bookmark:

- this folder is always shown floating to the left, next to the Apps button;
- this folder also appears in the bookmarks menu from the hotdog menu, as if
  it were part of the bookmarks bar;
- bookmarks can't be dragged into this folder or any of its subfolders;
- dragging bookmarks out of this folder always creates a new copy and never
  moves (the mouse cursor is decorated with the + "copy" indicator);
- the bookmark editor can't select managed folders for new bookmarks;
- bookmarks a page that already has a managed bookmarks creates a new
  bookmark instead.

BUG=49598
R=sky@chromium.org

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

[Joao da Silva, 2014-06-02 21:06:09]

Hey Scott, here's an initial implementation of the managed bookmarks folder
based on your feedback & a UX review from earlier today.

This depends on https://codereview.chromium.org/305973004 and may change
depending on where that CL goes, but you can see how the rest of the changes
look like. Let me know if this is reasonable or not quite what you had in mind
:-)

[sky, 2014-06-02 23:09:28]

I'm going to wait until we resolve the other patch before reviewing this.

  -Scott

On Mon, Jun 2, 2014 at 2:06 PM,  <joaodasilva@chromium.org> wrote:
> Reviewers: sky,
>
> Message:
> Hey Scott, here's an initial implementation of the managed bookmarks folder
> based on your feedback & a UX review from earlier today.
>
> This depends on https://codereview.chromium.org/305973004 and may change
> depending on where that CL goes, but you can see how the rest of the changes
> look like. Let me know if this is reasonable or not quite what you had in
> mind
> :-)
>
> Description:
> Show the Managed Bookmarks folder in the views UI.
>
> This change makes the Managed Bookmarks folder visible in the bookmarks
> bar if it contains at least one bookmark:
>
> - this folder is always shown floating to the left, next to the Apps button;
> - this folder also appears in the bookmarks menu from the hotdog menu, as if
>   it were part of the bookmarks bar;
> - bookmarks can't be dragged into this folder or any of its subfolders;
> - dragging bookmarks out of this folder always creates a new copy and never
>   moves (the mouse cursor is decorated with the + "copy" indicator);
> - the bookmark editor can't select managed folders for new bookmarks;
> - bookmarks a page that already has a managed bookmarks creates a new
>   bookmark instead.
>
> BUG=49598
>
> Please review this at https://codereview.chromium.org/302313005/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+312, -131 lines):
>   M chrome/browser/android/provider/chrome_browser_provider.cc
>   M
>
chrome/browser/extensions/api/bookmark_manager_private/bookmark_manager_private_api.cc
>   M chrome/browser/ui/bookmarks/bookmark_context_menu_controller.cc
>   M chrome/browser/ui/bookmarks/bookmark_drag_drop.h
>   M chrome/browser/ui/bookmarks/bookmark_drag_drop.cc
>   M chrome/browser/ui/bookmarks/recently_used_folders_combo_model.cc
>   M chrome/browser/ui/browser_commands.cc
>   M chrome/browser/ui/cocoa/browser_window_controller.mm
>   M chrome/browser/ui/view_ids.h
>   M chrome/browser/ui/views/bookmarks/bookmark_bar_view.h
>   M chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
>   M chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
>   M chrome/browser/ui/views/bookmarks/bookmark_drag_drop_views.cc
>   M chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc
>   M chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc
>   M chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.h
>   M chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc
>   M components/bookmarks/browser/bookmark_model.h
>   M components/bookmarks/browser/bookmark_model.cc
>   M components/bookmarks/browser/bookmark_model_unittest.cc
>   M components/bookmarks/browser/bookmark_utils.h
>   M components/bookmarks/browser/bookmark_utils.cc
>   M components/bookmarks/browser/bookmark_utils_unittest.cc
>
>

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-05 23:04:51]

Scott, this CL is ready for review and should be the last big change for this
feature.

I plan to add a browser_test that verifies this end-to-end: sets the policy,
looks for the visibility of the button, looks at the hotdog menus, and tries
drag&drop. But before spending the time on that I'd like a first pass on the
direction this is going :-)

I also made some changes to the drag&drop code to support drag-to-copy in
addition to drag-to-move; from my testing this seems correct (and the mouse
cursor shows the "+" indicator while dragging) but please let me know what you
think.

[sky, 2014-06-05 23:46:48]

Oy, this is a mega patch. Can you please split it into at least two changes to
make it easier to review?

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

https://codereview.chromium.org/302313005/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:725: gfx::Size size;
Make this local to each block.

https://codereview.chromium.org/302313005/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1829: gfx::Size
managed_bookmarks_pref = managed_bookmarks_button_->visible() ?
Move to 1848.

https://codereview.chromium.org/302313005/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right):

https://codereview.chromium.org/302313005/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:393: ChromeBookmarkClient*
client_;
Seems unnecessary to have both this and model_. In a cleanup cl nuke model_ and
replace with an inlined model() member that does 'return client_->model()'.

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

https://codereview.chromium.org/302313005/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_client.h:86: virtual bool
CanBeEditedByUser(const BookmarkNode* node) = 0;
Can this and CanReorderChildren be merged? Maybe even
CanRemovePermanentNodeChildren too?

https://codereview.chromium.org/302313005/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_client.h:88: // Returns true all the nodes
in the |list| can be edited by the user.
all -> if all

Also, move this to bookmark_utils and name CanAllBe...

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

https://codereview.chromium.org/302313005/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.cc:514: if
(client_->CanBeEditedByUser(nodes[i]))
Add test coverage for this.

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

https://codereview.chromium.org/302313005/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.h:181: bool IsBookmarkedByUser(const
GURL& url);
Move this to bookmark_utils.

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

https://codereview.chromium.org/302313005/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model_unittest.cc:1107:
TEST_F(BookmarkModelTest, IsBookmarked) {
Please add descriptions for what you are trying to verify.

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

https://codereview.chromium.org/302313005/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_utils.cc:397: if (index > -1 &&
model->client()->CanBeEditedByUser(node))
And coverage of this.

[sky, 2014-06-05 23:58:48]

One more

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

https://codereview.chromium.org/302313005/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_drag_drop_views.cc:66:
!model->client()->CanBeEditedByUser(node)) {
Can we fold the pref into CanBeEditedByUser?

[Joao da Silva, 2014-06-06 15:41:03]

The changes to the model and the helpers in components/bookmarks/ have been
split into https://codereview.chromium.org/317333004. This CL is now
concentrated in the bookmark_bar_view changes, and also includes smaller updates
to the bookmark editor and the bookmark folder view. I hope it's a more wieldy
patch to review :-)

PTAL, thanks!

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

https://codereview.chromium.org/302313005/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:725: gfx::Size size;
On 2014/06/05 23:46:47, sky wrote:
> Make this local to each block.

Done.

https://codereview.chromium.org/302313005/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1829: gfx::Size
managed_bookmarks_pref = managed_bookmarks_button_->visible() ?
On 2014/06/05 23:46:47, sky wrote:
> Move to 1848.

Done.

https://codereview.chromium.org/302313005/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right):

https://codereview.chromium.org/302313005/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:393: ChromeBookmarkClient*
client_;
On 2014/06/05 23:46:47, sky wrote:
> Seems unnecessary to have both this and model_. In a cleanup cl nuke model_
and
> replace with an inlined model() member that does 'return client_->model()'.

Will do.

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

https://codereview.chromium.org/302313005/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_drag_drop_views.cc:66:
!model->client()->CanBeEditedByUser(node)) {
On 2014/06/05 23:58:48, sky wrote:
> Can we fold the pref into CanBeEditedByUser?

At first glance I think so, yes. If you agree I'd leave this to another CL, to
reduce the risk of breaking something with this work.

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

https://codereview.chromium.org/302313005/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_client.h:86: virtual bool
CanBeEditedByUser(const BookmarkNode* node) = 0;
On 2014/06/05 23:46:47, sky wrote:
> Can this and CanReorderChildren be merged? Maybe even
> CanRemovePermanentNodeChildren too?

Yep, that's a bit less cruft. Done.

https://codereview.chromium.org/302313005/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_client.h:88: // Returns true all the nodes
in the |list| can be edited by the user.
On 2014/06/05 23:46:47, sky wrote:
> all -> if all
> 
> Also, move this to bookmark_utils and name CanAllBe...

Done.

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

https://codereview.chromium.org/302313005/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.cc:514: if
(client_->CanBeEditedByUser(nodes[i]))
On 2014/06/05 23:46:47, sky wrote:
> Add test coverage for this.

Done.

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

https://codereview.chromium.org/302313005/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model.h:181: bool IsBookmarkedByUser(const
GURL& url);
On 2014/06/05 23:46:47, sky wrote:
> Move this to bookmark_utils.

Done.

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

https://codereview.chromium.org/302313005/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model_unittest.cc:1107:
TEST_F(BookmarkModelTest, IsBookmarked) {
On 2014/06/05 23:46:47, sky wrote:
> Please add descriptions for what you are trying to verify.

Done.

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

https://codereview.chromium.org/302313005/diff/20001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_utils.cc:397: if (index > -1 &&
model->client()->CanBeEditedByUser(node))
On 2014/06/05 23:46:47, sky wrote:
> And coverage of this.

Done.

[sky, 2014-06-06 19:34:04]

LGTM

https://codereview.chromium.org/302313005/diff/100001/chrome/browser/ui/bookm...
File chrome/browser/ui/bookmarks/bookmark_drag_drop.h (right):

https://codereview.chromium.org/302313005/diff/100001/chrome/browser/ui/bookm...
chrome/browser/ui/bookmarks/bookmark_drag_drop.h:33: bool copy);
How about making this take a ui::DragOperation? That's clearer than an opaque
boolean and if we want to support linking at some point we can. Additionally it
matches the return type.

https://codereview.chromium.org/302313005/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/302313005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:541: // Check the buttons
first.
nit: update comment.

https://codereview.chromium.org/302313005/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_drag_drop_views.cc (right):

https://codereview.chromium.org/302313005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_drag_drop_views.cc:65: if
(!prefs->GetBoolean(prefs::kEditBookmarksEnabled) ||
Move the pref lookup to the client.

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

https://codereview.chromium.org/302313005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc:77: DCHECK(profile);
Add a DCHECK here that parent is editable.

[Joao da Silva, 2014-06-06 20:06:12]

https://codereview.chromium.org/302313005/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/302313005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:541: // Check the buttons
first.
On 2014/06/06 19:34:04, sky wrote:
> nit: update comment.

Done.

https://codereview.chromium.org/302313005/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_drag_drop_views.cc (right):

https://codereview.chromium.org/302313005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_drag_drop_views.cc:65: if
(!prefs->GetBoolean(prefs::kEditBookmarksEnabled) ||
On 2014/06/06 19:34:04, sky wrote:
> Move the pref lookup to the client.

I'll do that in another CL, if you don't mind.

I'm not 100% sure that all the checks for CanBeEditedForUser() (current &
future) will do the right thing when kEditBookmarksEnabled is false, so to
reduce that risk in this CL I'd do it separately.

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

https://codereview.chromium.org/302313005/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc:77: DCHECK(profile);
On 2014/06/06 19:34:04, sky wrote:
> Add a DCHECK here that parent is editable.

Done.

[Joao da Silva, 2014-06-06 21:15:44]

The CQ bit was checked by joaodasilva@chromium.org

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

CQ is trying da patch. Follow status at

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

[Joao da Silva, 2014-06-06 22:54:47]

The CQ bit was checked by joaodasilva@chromium.org

[Joao da Silva, 2014-06-06 22:56:01]

Scott, FYI turns out that there are indeed tests that enter the BookmarkBarView
with a NULL model. Patch set 9 fixes a small glitch that made
AutofillDialogViewsTest crash due to that.

[commit-bot: I haz the power, 2014-06-06 22:57:13]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/302313005/160001

[commit-bot: I haz the power, 2014-06-07 03:24:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-07 06:32:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-07 06:32:55]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[Joao da Silva, 2014-06-07 10:45:28]

Committed patchset #10 manually as r275676 (presubmit successful).