Fix bug in not exiting menu when choosing 'delete'

Fix bug in not exiting menu when choosing 'delete'

This case fixes clicking on an item with no children to show the menu,
then right clicking on the (empty) text and choosing 'delete'. In this
case 'delete' removes the item, so the menu needs to close.

BUG=571362
TEST=create folder with no bookmarks on the bookmark bar, click on
folder so it opens, right click in '(empty)' area, and choose
delete. The menu should close.
R=rdevlin.cronin@chromium.org

Committed: https://crrev.com/bdbc8d3f7300ab21ad9c53c94135821558cc7092
Cr-Commit-Position: refs/heads/master@{#367100}

[Devlin, 2015-12-29 00:59:34]

Any chance of there being a test in bookmark_bar_view_test.cc that could be
modified/copied and altered to give us test coverage?

https://codereview.chromium.org/1550173002/diff/1/chrome/browser/ui/views/boo...
File chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc (right):

https://codereview.chromium.org/1550173002/diff/1/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc:318:
nodes.push_back(node);
nit: I'd probably use std::vector<const BookmarkNode*> nodes(1, node); here to
shave a line (could even inline in the constructor if you like).  Or, if the
initializer list discussion wraps up soon, nodes {node};

https://codereview.chromium.org/1550173002/diff/1/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc:323: // there is no
longer anything to anchor the menu too).
s/too/to

https://codereview.chromium.org/1550173002/diff/1/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc:328: node->parent()
== GetBookmarkModel()->bookmark_bar_node());
I think this bool might be more readable if it were broken into multiple bools,
e.g.
bool is_other_only_child = ...
bool is_anchored_to_bar = ...
bool close_on_remove = ...

But the comment above does a pretty good job of explaining it, so I don't feel
strongly.

[sky, 2015-12-29 17:11:28]

I added test coverage of the close_on_remove value, which is what matters here.

https://codereview.chromium.org/1550173002/diff/1/chrome/browser/ui/views/boo...
File chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc (right):

https://codereview.chromium.org/1550173002/diff/1/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc:318:
nodes.push_back(node);
On 2015/12/29 00:59:34, Devlin wrote:
> nit: I'd probably use std::vector<const BookmarkNode*> nodes(1, node); here to
> shave a line (could even inline in the constructor if you like).  Or, if the
> initializer list discussion wraps up soon, nodes {node};

I changed it to your first suggestion.

https://codereview.chromium.org/1550173002/diff/1/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc:323: // there is no
longer anything to anchor the menu too).
On 2015/12/29 00:59:34, Devlin wrote:
> s/too/to

Done.

https://codereview.chromium.org/1550173002/diff/1/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc:328: node->parent()
== GetBookmarkModel()->bookmark_bar_node());
On 2015/12/29 00:59:34, Devlin wrote:
> I think this bool might be more readable if it were broken into multiple
bools,
> e.g.
> bool is_other_only_child = ...
> bool is_anchored_to_bar = ...
> bool close_on_remove = ...
> 
> But the comment above does a pretty good job of explaining it, so I don't feel
> strongly.

As part of adding test coverage I did this.

[Devlin, 2015-12-29 19:30:05]

lgtm, thanks for the tests :)

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

https://codereview.chromium.org/1550173002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.h:151: // Returns the
value to supply to creation of BookmarkContextMenu for
nit: I think it'd be clearer to describe the meaning of the value (i.e., whether
or not all active menus should be closed if 'remove' is selected) rather than
specifying a ctor parameter that it matches up to.  That said, the comments +
method name make it sufficiently obvious that I don't feel too strongly.

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

https://codereview.chromium.org/1550173002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate_unittest.cc:207:
bookmark_menu_delegate_->Init(&test_delegate, NULL, node, 0,
nit: nullptr

[sky, 2015-12-29 19:38:43]

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

https://codereview.chromium.org/1550173002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.h:151: // Returns the
value to supply to creation of BookmarkContextMenu for
On 2015/12/29 19:30:04, Devlin wrote:
> nit: I think it'd be clearer to describe the meaning of the value (i.e.,
whether
> or not all active menus should be closed if 'remove' is selected) rather than
> specifying a ctor parameter that it matches up to.  That said, the comments +
> method name make it sufficiently obvious that I don't feel too strongly.

Done.

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

https://codereview.chromium.org/1550173002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate_unittest.cc:207:
bookmark_menu_delegate_->Init(&test_delegate, NULL, node, 0,
On 2015/12/29 19:30:04, Devlin wrote:
> nit: nullptr

Done.

[sky, 2015-12-29 19:39:03]

The CQ bit was checked by sky@chromium.org

[sky, 2015-12-29 19:39:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1550173002/#ps40001
(title: "feedback")

[commit-bot: I haz the power, 2015-12-29 19:39:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1550173002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1550173002/40001

[commit-bot: I haz the power, 2015-12-29 20:37:25]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-29 20:38:09]

Description was changed from

==========
Fix bug in not exiting menu when choosing 'delete'

This case fixes clicking on an item with no children to show the menu,
then right clicking on the (empty) text and choosing 'delete'. In this
case 'delete' removes the item, so the menu needs to close.

BUG=571362
TEST=create folder with no bookmarks on the bookmark bar, click on
folder so it opens, right click in '(empty)' area, and choose
delete. The menu should close.
R=rdevlin.cronin@chromium.org
==========

to

==========
Fix bug in not exiting menu when choosing 'delete'

This case fixes clicking on an item with no children to show the menu,
then right clicking on the (empty) text and choosing 'delete'. In this
case 'delete' removes the item, so the menu needs to close.

BUG=571362
TEST=create folder with no bookmarks on the bookmark bar, click on
folder so it opens, right click in '(empty)' area, and choose
delete. The menu should close.
R=rdevlin.cronin@chromium.org

Committed: https://crrev.com/bdbc8d3f7300ab21ad9c53c94135821558cc7092
Cr-Commit-Position: refs/heads/master@{#367100}
==========

[commit-bot: I haz the power, 2015-12-29 20:38:10]

Patchset 3 (id:??) landed as
https://crrev.com/bdbc8d3f7300ab21ad9c53c94135821558cc7092
Cr-Commit-Position: refs/heads/master@{#367100}