Fix bug in not exiting menu when choosing 'delete'

chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc

Index: chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc b/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc
index befbe1f32335ae0b98da288613040c0017db15f8..f0d1fc6aa90f4ba919fd020d2d73182fc9a83eb2 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc
@@ -313,23 +313,25 @@ bool BookmarkMenuDelegate::ShowContextMenu(MenuItemView* source,
                                            const gfx::Point& p,
                                            ui::MenuSourceType source_type) {
   DCHECK(menu_id_to_node_map_.find(id) != menu_id_to_node_map_.end());
+  const BookmarkNode* node = menu_id_to_node_map_[id];
   std::vector<const BookmarkNode*> nodes;
-  nodes.push_back(menu_id_to_node_map_[id]);
-  bool close_on_delete = !parent_menu_item_ &&
-      (nodes[0]->parent() == GetBookmarkModel()->other_node() &&
-       nodes[0]->parent()->child_count() == 1);
-  context_menu_.reset(
-      new BookmarkContextMenu(
-          parent_,
-          browser_,
-          profile_,
-          page_navigator_,
-          nodes[0]->parent(),
-          nodes,
-          close_on_delete));
+  nodes.push_back(node);

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

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};

[sky, 2015/12/29 17:11:27]

I changed it to your first suggestion.

+  // The 'other' bookmarks folder hides when it has no more items, so we need
+  // to exit the menu when the last node is removed.
+  // If the parent is the bookmark bar, then the menu is showing for an item on
+  // the bookmark bar. When removing this item we need to close the menu (as
+  // there is no longer anything to anchor the menu too).

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

s/too/to

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

Done.

+  const bool close_on_remove =
+      !parent_menu_item_ &&
+      ((node->parent() == GetBookmarkModel()->other_node() &&
+        node->parent()->child_count() == 1) ||
+       node->parent() == GetBookmarkModel()->bookmark_bar_node());

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

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:27]

As part of adding test coverage I did this.

+  context_menu_.reset(new BookmarkContextMenu(parent_, browser_, profile_,
+                                              page_navigator_, node->parent(),
+                                              nodes, close_on_remove));
   context_menu_->set_observer(this);
   context_menu_->RunMenuAt(p, source_type);
-  context_menu_.reset(NULL);
+  context_menu_.reset(nullptr);
   return true;
 }