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

Review URL: https://codereview.chromium.org/1550173002

Cr-Commit-Position: refs/heads/master@{#367100}

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

@@ -1503,7 +1503,7 @@ void BookmarkBarView::ShowContextMenuForView(views::View* source,
     return;
   }
 
-  const BookmarkNode* parent = NULL;
+  const BookmarkNode* parent = nullptr;
   std::vector<const BookmarkNode*> nodes;
   if (source == other_bookmarks_button_) {
     parent = model_->other_node();
@@ -1531,8 +1531,9 @@ void BookmarkBarView::ShowContextMenuForView(views::View* source,
     parent = model_->bookmark_bar_node();
     nodes.push_back(parent);
   }
-  bool close_on_remove =
-      (parent == model_->other_node()) && (parent->child_count() == 1);
+  // |close_on_remove| only matters for nested menus. We're not nested at this
+  // point, so this value has no effect.
+  const bool close_on_remove = true;
 
   context_menu_.reset(new BookmarkContextMenu(
       GetWidget(), browser_, browser_->profile(),

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

@@ -117,7 +117,7 @@ void BookmarkMenuDelegate::SetPageNavigator(PageNavigator* navigator) {
     context_menu_->SetPageNavigator(navigator);
 }
 
-BookmarkModel* BookmarkMenuDelegate::GetBookmarkModel() {
+const BookmarkModel* BookmarkMenuDelegate::GetBookmarkModel() const {
   return BookmarkModelFactory::GetForProfile(profile_);
 }
 
@@ -313,23 +313,14 @@ 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());
-  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));
+  const BookmarkNode* node = menu_id_to_node_map_[id];
+  std::vector<const BookmarkNode*> nodes(1, node);
+  context_menu_.reset(new BookmarkContextMenu(
+      parent_, browser_, profile_, page_navigator_, node->parent(), nodes,
+      ShouldCloseOnRemove(node)));
   context_menu_->set_observer(this);
   context_menu_->RunMenuAt(p, source_type);
-  context_menu_.reset(NULL);
+  context_menu_.reset(nullptr);
   return true;
 }
 
@@ -443,6 +434,25 @@ void BookmarkMenuDelegate::DidRemoveBookmarks() {
   is_mutating_model_ = false;
 }
 
+bool BookmarkMenuDelegate::ShouldCloseOnRemove(const BookmarkNode* node) const {
+  // We never need to close when embedded in the app menu.
+  const bool is_shown_from_app_menu = parent_menu_item_ != nullptr;
+  if (is_shown_from_app_menu)
+    return false;
+
+  const bool is_only_child_of_other_folder =
+      (node->parent() == GetBookmarkModel()->other_node() &&
+       node->parent()->child_count() == 1);
+  const bool is_child_of_bookmark_bar =
+      node->parent() == GetBookmarkModel()->bookmark_bar_node();
+  // 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 to).
+  return is_only_child_of_other_folder || is_child_of_bookmark_bar;
+}
+
 MenuItemView* BookmarkMenuDelegate::CreateMenu(const BookmarkNode* parent,
                                                int start_child_index,
                                                ShowOptions show_options) {

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

@@ -81,7 +81,11 @@ class BookmarkMenuDelegate : public bookmarks::BaseBookmarkModelObserver,
   // the first child of |node| to show in the menu.
   void SetActiveMenu(const bookmarks::BookmarkNode* node, int start_index);
 
-  bookmarks::BookmarkModel* GetBookmarkModel();
+  bookmarks::BookmarkModel* GetBookmarkModel() {
+    return const_cast<bookmarks::BookmarkModel*>(
+        const_cast<const BookmarkMenuDelegate*>(this)->GetBookmarkModel());
+  }
+  const bookmarks::BookmarkModel* GetBookmarkModel() const;
   bookmarks::ManagedBookmarkService* GetManagedBookmarkService();
 
   // Returns the menu.
@@ -144,6 +148,9 @@ class BookmarkMenuDelegate : public bookmarks::BaseBookmarkModelObserver,
   typedef std::map<const bookmarks::BookmarkNode*, views::MenuItemView*>
       NodeToMenuMap;
 
+  // Returns whether the menu should close id 'delete' is selected.
+  bool ShouldCloseOnRemove(const bookmarks::BookmarkNode* node) const;
+
   // Creates a menu. This uses BuildMenu() to recursively populate the menu.
   views::MenuItemView* CreateMenu(const bookmarks::BookmarkNode* parent,
                                   int start_child_index,

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

@@ -37,20 +37,30 @@ class BookmarkMenuDelegateTest : public BrowserWithTestWindowTest {
   }
 
   void TearDown() override {
-    if (bookmark_menu_delegate_.get()) {
-      // Since we never show the menu we need to pass the MenuItemView to
-      // MenuRunner so that the MenuItemView is destroyed.
-      views::MenuRunner menu_runner(bookmark_menu_delegate_->menu(), 0);
-      bookmark_menu_delegate_.reset();
-    }
+    DestroyDelegate();
+
     BrowserWithTestWindowTest::TearDown();
   }
 
  protected:
+  bool ShouldCloseOnRemove(const bookmarks::BookmarkNode* node) const {
+    return bookmark_menu_delegate_->ShouldCloseOnRemove(node);
+  }
+
+  // Destroys the delegate. Do this rather than directly deleting
+  // |bookmark_menu_delegate_| as otherwise the menu is leaked.
+  void DestroyDelegate() {
+    if (!bookmark_menu_delegate_.get())
+      return;
+
+    // Since we never show the menu we need to pass the MenuItemView to
+    // MenuRunner so that the MenuItemView is destroyed.
+    views::MenuRunner menu_runner(bookmark_menu_delegate_->menu(), 0);
+    bookmark_menu_delegate_.reset();
+  }
+
   void NewDelegate() {
-    // Destroy current menu if available, see comments in TearDown().
-    if (bookmark_menu_delegate_.get())
-      views::MenuRunner menu_runner(bookmark_menu_delegate_->menu(), 0);
+    DestroyDelegate();
 
     bookmark_menu_delegate_.reset(
         new BookmarkMenuDelegate(browser(), nullptr, nullptr));
@@ -187,3 +197,36 @@ TEST_F(BookmarkMenuDelegateTest, RemoveBookmarks) {
   nodes_to_remove.clear();
   bookmark_menu_delegate_->DidRemoveBookmarks();
 }
+
+// Verifies WillRemoveBookmarks() doesn't attempt to access MenuItemViews that
+// have since been deleted.
+TEST_F(BookmarkMenuDelegateTest, CloseOnRemove) {
+  views::MenuDelegate test_delegate;
+  const BookmarkNode* node = model_->bookmark_bar_node()->GetChild(1);
+  NewDelegate();
+  bookmark_menu_delegate_->Init(&test_delegate, nullptr, node, 0,
+                                BookmarkMenuDelegate::HIDE_PERMANENT_FOLDERS,
+                                BOOKMARK_LAUNCH_LOCATION_NONE);
+  // Any nodes on the bookmark bar should close on remove.
+  EXPECT_TRUE(ShouldCloseOnRemove(model_->bookmark_bar_node()->GetChild(2)));
+
+  // Descendants of the bookmark should not close on remove.
+  EXPECT_FALSE(ShouldCloseOnRemove(
+      model_->bookmark_bar_node()->GetChild(1)->GetChild(0)));
+
+  EXPECT_FALSE(ShouldCloseOnRemove(model_->other_node()->GetChild(0)));
+
+  // Make it so the other node only has one child.
+  // Destroy the current delegate so that it doesn't have any references to
+  // deleted nodes.
+  DestroyDelegate();
+  while (model_->other_node()->child_count() > 1)
+    model_->Remove(model_->other_node()->GetChild(1));
+
+  NewDelegate();
+  bookmark_menu_delegate_->Init(&test_delegate, nullptr, node, 0,
+                                BookmarkMenuDelegate::HIDE_PERMANENT_FOLDERS,
+                                BOOKMARK_LAUNCH_LOCATION_NONE);
+  // Any nodes on the bookmark bar should close on remove.
+  EXPECT_TRUE(ShouldCloseOnRemove(model_->other_node()->GetChild(0)));
+}