Fix bug in not exiting menu when choosing 'delete'

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.h"
 
 #include "base/prefs/pref_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
@@ skipping 295 matching lines @@
   bool copy = event.source_operations() == ui::DragDropTypes::DRAG_COPY;
   return chrome::DropBookmarks(profile_, drop_data_,
                                drop_parent, index_to_drop_at, copy);
 }
 
 bool BookmarkMenuDelegate::ShowContextMenu(MenuItemView* source,
                                            int id,
                                            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;
 }
 
 bool BookmarkMenuDelegate::CanDrag(MenuItemView* menu) {
   const BookmarkNode* node = menu_id_to_node_map_[menu->GetCommand()];
   // Don't let users drag the other folder.
   return node->parent() != GetBookmarkModel()->root_node();
 }
 
 void BookmarkMenuDelegate::WriteDragData(MenuItemView* sender,
@@ skipping 188 matching lines @@
     }
     AddMenuToMaps(child_menu_item, node);
   }
 }
 
 void BookmarkMenuDelegate::AddMenuToMaps(MenuItemView* menu,
                                          const BookmarkNode* node) {
   menu_id_to_node_map_[menu->GetCommand()] = node;
   node_to_menu_map_[node] = menu;
 }