Makes it so deleting a bookmark from the context menu doesn't close

the bookmark menu.

BUG=2469
TEST=click on a bookmark folder on the bookmark bar, right click on an
 item and chose delete. Make sure the folder stays up and still works
 correctly (and the item you deleted isn't there).

Review URL: http://codereview.chromium.org/551178

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@37424 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 234 insertions, 14 deletions

diff --git a/chrome/browser/bookmarks/bookmark_context_menu_controller.cc b/chrome/browser/bookmarks/bookmark_context_menu_controller.cc
index a46110b..0ce5386 100644
--- a/chrome/browser/bookmarks/bookmark_context_menu_controller.cc
+++ b/chrome/browser/bookmarks/bookmark_context_menu_controller.cc
@@ -278,6 +278,8 @@ void BookmarkContextMenuController::BuildMenu() {
 }
 
 void BookmarkContextMenuController::ExecuteCommand(int id) {
+  BookmarkModel* model = RemoveModelObserver();
+
   switch (id) {
     case IDS_BOOMARK_BAR_OPEN_ALL:
     case IDS_BOOMARK_BAR_OPEN_ALL_INCOGNITO:
@@ -327,12 +329,13 @@ void BookmarkContextMenuController::ExecuteCommand(int id) {
 
     case IDS_BOOKMARK_BAR_REMOVE: {
       UserMetrics::RecordAction("BookmarkBar_ContextMenu_Remove", profile_);
-      BookmarkModel* model = RemoveModelObserver();
 
+      delegate_->WillRemoveBookmarks(selection_);
       for (size_t i = 0; i < selection_.size(); ++i) {
         model->Remove(selection_[i]->GetParent(),
                       selection_[i]->GetParent()->IndexOfChild(selection_[i]));
       }
+      delegate_->DidRemoveBookmarks();
       selection_.clear();
       break;
     }
@@ -387,13 +390,17 @@ void BookmarkContextMenuController::ExecuteCommand(int id) {
 
     case IDS_BOOKMARK_MANAGER_SORT:
       UserMetrics::RecordAction("BookmarkManager_Sort", profile_);
-      model_->SortChildren(parent_);
+      model->SortChildren(parent_);
       break;
 
-    case IDS_COPY:
     case IDS_CUT:
-      bookmark_utils::CopyToClipboard(profile_->GetBookmarkModel(),
-                                      selection_, id == IDS_CUT);
+      delegate_->WillRemoveBookmarks(selection_);
+      bookmark_utils::CopyToClipboard(model, selection_, false);
+      delegate_->DidRemoveBookmarks();
+      break;
+
+    case IDS_COPY:
+      bookmark_utils::CopyToClipboard(model, selection_, false);
       break;
 
     case IDS_PASTE: {
@@ -405,8 +412,7 @@ void BookmarkContextMenuController::ExecuteCommand(int id) {
           parent_->IndexOfChild(selection_[0]) : -1;
       if (index != -1)
         index++;
-      bookmark_utils::PasteFromClipboard(profile_->GetBookmarkModel(),
-                                         parent_, index);
+      bookmark_utils::PasteFromClipboard(model, parent_, index);
       break;
     }
 
diff --git a/chrome/browser/bookmarks/bookmark_context_menu_controller.h b/chrome/browser/bookmarks/bookmark_context_menu_controller.h
index 0934b70..3812e28 100644
--- a/chrome/browser/bookmarks/bookmark_context_menu_controller.h
+++ b/chrome/browser/bookmarks/bookmark_context_menu_controller.h
@@ -29,6 +29,13 @@ class BookmarkContextMenuControllerDelegate {
   virtual void AddItemWithStringId(int command_id, int string_id) = 0;
   virtual void AddSeparator() = 0;
   virtual void AddCheckboxItem(int command_id) = 0;
+
+  // Sent before bookmarks are removed.
+  virtual void WillRemoveBookmarks(
+      const std::vector<const BookmarkNode*>& bookmarks) {}
+
+  // Sent after bookmarks have been removed.
+  virtual void DidRemoveBookmarks() {}
 };
 
 // BookmarkContextMenuController creates and manages state for the context menu
diff --git a/chrome/browser/views/bookmark_bar_view_test.cc b/chrome/browser/views/bookmark_bar_view_test.cc
index 8e24366..3d84ff8 100644
--- a/chrome/browser/views/bookmark_bar_view_test.cc
+++ b/chrome/browser/views/bookmark_bar_view_test.cc
@@ -14,6 +14,7 @@
 #include "chrome/common/pref_service.h"
 #include "chrome/test/testing_profile.h"
 #include "chrome/test/interactive_ui/view_event_test_base.h"
+#include "grit/generated_resources.h"
 #include "views/controls/button/menu_button.h"
 #include "views/controls/button/text_button.h"
 #include "views/controls/menu/menu_controller.h"
@@ -38,8 +39,8 @@ class TestingPageNavigator : public PageNavigator {
 }  // namespace
 
 // Base class for event generating bookmark view tests. These test are intended
-// to exercise ChromeMenus, but that's easier done with BookmarkBarView rather
-// than ChromeMenu itself.
+// to exercise View's menus, but that's easier done with BookmarkBarView rather
+// than View's menu itself.
 //
 // SetUp creates a bookmark model with the following structure.
 // All folders are in upper case, all URLs in lower case.
@@ -1101,3 +1102,76 @@ class BookmarkBarViewTest14 : public BookmarkBarViewEventTestBase {
 };
 
 VIEW_TEST(BookmarkBarViewTest14, ContextMenus2)
+
+// Makes sure deleting from the context menu keeps the bookmark menu showing.
+class BookmarkBarViewTest15 : public BookmarkBarViewEventTestBase {
+ public:
+  BookmarkBarViewTest15() : deleted_menu_id_(0) {}
+
+ protected:
+  virtual void DoTestOnMessageLoop() {
+    // Show the other bookmarks.
+    views::TextButton* button = bb_view_->other_bookmarked_button();
+    ui_controls::MoveMouseToCenterAndPress(button, ui_controls::LEFT,
+        ui_controls::DOWN | ui_controls::UP,
+        CreateEventTask(this, &BookmarkBarViewTest15::Step2));
+  }
+
+ private:
+  void Step2() {
+    // Menu should be showing.
+    views::MenuItemView* menu = bb_view_->GetMenu();
+    ASSERT_TRUE(menu != NULL);
+    ASSERT_TRUE(menu->GetSubmenu()->IsShowing());
+
+    views::MenuItemView* child_menu =
+        menu->GetSubmenu()->GetMenuItemAt(1);
+    ASSERT_TRUE(child_menu != NULL);
+
+    deleted_menu_id_ = child_menu->GetCommand();
+
+    // Right click on the second child to get its context menu.
+    ui_controls::MoveMouseToCenterAndPress(child_menu, ui_controls::RIGHT,
+        ui_controls::DOWN | ui_controls::UP,
+        CreateEventTask(this, &BookmarkBarViewTest15::Step3));
+  }
+
+  void Step3() {
+    // Make sure the context menu is showing.
+    views::MenuItemView* menu = bb_view_->GetContextMenu();
+    ASSERT_TRUE(menu != NULL);
+    ASSERT_TRUE(menu->GetSubmenu());
+    ASSERT_TRUE(menu->GetSubmenu()->IsShowing());
+
+    views::MenuItemView* delete_menu =
+        menu->GetMenuItemByID(IDS_BOOKMARK_BAR_REMOVE);
+    ASSERT_TRUE(delete_menu);
+
+    // Click on the delete button.
+    ui_controls::MoveMouseToCenterAndPress(delete_menu,
+        ui_controls::LEFT, ui_controls::DOWN | ui_controls::UP,
+        CreateEventTask(this, &BookmarkBarViewTest15::Step4));
+  }
+
+  void Step4() {
+    // The context menu should not be showing.
+    views::MenuItemView* context_menu = bb_view_->GetContextMenu();
+    ASSERT_TRUE(context_menu == NULL);
+
+    // But the menu should be showing.
+    views::MenuItemView* menu = bb_view_->GetMenu();
+    ASSERT_TRUE(menu != NULL);
+    ASSERT_TRUE(menu->GetSubmenu()->IsShowing());
+
+    // And the deleted_menu_id_ should have been removed.
+    ASSERT_TRUE(menu->GetMenuItemByID(deleted_menu_id_) == NULL);
+
+    bb_view_->GetMenu()->GetMenuController()->Cancel(true);
+
+    Done();
+  }
+
+  int deleted_menu_id_;
+};
+
+VIEW_TEST(BookmarkBarViewTest15, MenuStaysVisibleAfterDelete)
diff --git a/chrome/browser/views/bookmark_context_menu.cc b/chrome/browser/views/bookmark_context_menu.cc
index a3f3a15..21eb200 100644
--- a/chrome/browser/views/bookmark_context_menu.cc
+++ b/chrome/browser/views/bookmark_context_menu.cc
@@ -25,7 +25,8 @@ BookmarkContextMenu::BookmarkContextMenu(
                                                         parent, selection,
                                                         configuration))),
       parent_window_(parent_window),
-      ALLOW_THIS_IN_INITIALIZER_LIST(menu_(new views::MenuItemView(this))) {
+      ALLOW_THIS_IN_INITIALIZER_LIST(menu_(new views::MenuItemView(this))),
+      observer_(NULL) {
   controller_->BuildMenu();
 }
 
@@ -56,6 +57,10 @@ bool BookmarkContextMenu::IsCommandEnabled(int command_id) const {
   return controller_->IsCommandEnabled(command_id);
 }
 
+bool BookmarkContextMenu::ShouldCloseAllMenusOnExecute(int id) {
+  return id != IDS_BOOKMARK_BAR_REMOVE;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // BookmarkContextMenu, BookmarkContextMenuControllerDelegate implementation:
 
@@ -79,3 +84,14 @@ void BookmarkContextMenu::AddCheckboxItem(int command_id) {
   menu_->AppendMenuItem(command_id, l10n_util::GetString(command_id),
                         views::MenuItemView::CHECKBOX);
 }
+
+void BookmarkContextMenu::WillRemoveBookmarks(
+    const std::vector<const BookmarkNode*>& bookmarks) {
+  if (observer_)
+    observer_->WillRemoveBookmarks(bookmarks);
+}
+
+void BookmarkContextMenu::DidRemoveBookmarks() {
+  if (observer_)
+    observer_->DidRemoveBookmarks();
+}
diff --git a/chrome/browser/views/bookmark_context_menu.h b/chrome/browser/views/bookmark_context_menu.h
index 94c2d97..47b8ade 100644
--- a/chrome/browser/views/bookmark_context_menu.h
+++ b/chrome/browser/views/bookmark_context_menu.h
@@ -8,6 +8,17 @@
 #include "chrome/browser/bookmarks/bookmark_context_menu_controller.h"
 #include "views/controls/menu/menu_delegate.h"
 
+// Observer for the BookmarkContextMenu.
+class BookmarkContextMenuObserver {
+ public:
+  // Invoked before the specified items are removed from the bookmark model.
+  virtual void WillRemoveBookmarks(
+      const std::vector<const BookmarkNode*>& bookmarks) = 0;
+
+  // Invoked after the items have been removed from the model.
+  virtual void DidRemoveBookmarks() = 0;
+};
+
 class BookmarkContextMenu : public BookmarkContextMenuControllerDelegate,
                             public views::MenuDelegate {
  public:
@@ -25,10 +36,15 @@ class BookmarkContextMenu : public BookmarkContextMenuControllerDelegate,
 
   views::MenuItemView* menu() const { return menu_.get(); }
 
+  void set_observer(BookmarkContextMenuObserver* observer) {
+    observer_ = observer;
+  }
+
   // Overridden from views::MenuDelegate:
   virtual void ExecuteCommand(int command_id);
   virtual bool IsItemChecked(int command_id) const;
   virtual bool IsCommandEnabled(int command_id) const;
+  virtual bool ShouldCloseAllMenusOnExecute(int id);
 
   // Overridden from BookmarkContextMenuControllerDelegate:
   virtual void CloseMenu();
@@ -36,6 +52,9 @@ class BookmarkContextMenu : public BookmarkContextMenuControllerDelegate,
   virtual void AddItemWithStringId(int command_id, int string_id);
   virtual void AddSeparator();
   virtual void AddCheckboxItem(int command_id);
+  virtual void WillRemoveBookmarks(
+      const std::vector<const BookmarkNode*>& bookmarks);
+  virtual void DidRemoveBookmarks();
 
  private:
   scoped_ptr<BookmarkContextMenuController> controller_;
@@ -46,6 +65,8 @@ class BookmarkContextMenu : public BookmarkContextMenuControllerDelegate,
   // The menu itself.
   scoped_ptr<views::MenuItemView> menu_;
 
+  BookmarkContextMenuObserver* observer_;
+
   DISALLOW_COPY_AND_ASSIGN(BookmarkContextMenu);
 };
 
diff --git a/chrome/browser/views/bookmark_menu_controller_views.cc b/chrome/browser/views/bookmark_menu_controller_views.cc
index 5a7256b..cb334ed 100644
--- a/chrome/browser/views/bookmark_menu_controller_views.cc
+++ b/chrome/browser/views/bookmark_menu_controller_views.cc
@@ -206,6 +206,7 @@ bool BookmarkMenuController::ShowContextMenu(MenuItemView* source,
                               nodes[0]->GetParent(),
                               nodes,
                               BookmarkContextMenuController::BOOKMARK_BAR));
+  context_menu_->set_observer(this);
   context_menu_->RunMenuAt(gfx::Point(x, y));
   context_menu_.reset(NULL);
   return true;
@@ -271,8 +272,7 @@ void BookmarkMenuController::BookmarkModelChanged() {
 
 void BookmarkMenuController::BookmarkNodeFavIconLoaded(
     BookmarkModel* model, const BookmarkNode* node) {
-  std::map<const BookmarkNode*, int>::iterator menu_pair =
-      node_to_menu_id_map_.find(node);
+  NodeToMenuIDMap::iterator menu_pair = node_to_menu_id_map_.find(node);
   if (menu_pair == node_to_menu_id_map_.end())
     return;  // We're not showing a menu item for the node.
 
@@ -287,6 +287,19 @@ void BookmarkMenuController::BookmarkNodeFavIconLoaded(
   }
 }
 
+void BookmarkMenuController::WillRemoveBookmarks(
+    const std::vector<const BookmarkNode*>& bookmarks) {
+  std::set<MenuItemView*> removed_menus;
+
+  WillRemoveBookmarksImpl(bookmarks, &removed_menus);
+
+  STLDeleteElements(&removed_menus);
+}
+
+void BookmarkMenuController::DidRemoveBookmarks() {
+  profile_->GetBookmarkModel()->AddObserver(this);
+}
+
 MenuItemView* BookmarkMenuController::CreateMenu(const BookmarkNode* parent,
                                                  int start_child_index) {
   MenuItemView* menu = new MenuItemView(this);
@@ -337,6 +350,7 @@ void BookmarkMenuController::BuildMenu(const BookmarkNode* parent,
           GetBitmapNamed(IDR_BOOKMARK_BAR_FOLDER);
       MenuItemView* submenu =
           menu->AppendSubMenuWithIcon(id, node->GetTitle(), *folder_icon);
+      node_to_menu_id_map_[node] = id;
       BuildMenu(node, 0, submenu, next_menu_id);
     } else {
       NOTREACHED();
@@ -351,3 +365,66 @@ BookmarkMenuController::~BookmarkMenuController() {
     observer_->BookmarkMenuDeleted(this);
   STLDeleteValues(&node_to_menu_map_);
 }
+
+MenuItemView* BookmarkMenuController::GetMenuByID(int id) {
+  for (NodeToMenuMap::const_iterator i = node_to_menu_map_.begin();
+       i != node_to_menu_map_.end(); ++i) {
+    MenuItemView* menu = i->second->GetMenuItemByID(id);
+    if (menu)
+      return menu;
+  }
+  return NULL;
+}
+
+void BookmarkMenuController::WillRemoveBookmarksImpl(
+      const std::vector<const BookmarkNode*>& bookmarks,
+      std::set<views::MenuItemView*>* removed_menus) {
+  // Remove the observer so that when the remove happens we don't prematurely
+  // cancel the menu.
+  profile_->GetBookmarkModel()->RemoveObserver(this);
+
+  // Remove the menu items.
+  std::set<MenuItemView*> changed_parent_menus;
+  for (std::vector<const BookmarkNode*>::const_iterator i = bookmarks.begin();
+       i != bookmarks.end(); ++i) {
+    NodeToMenuIDMap::iterator node_to_menu = node_to_menu_id_map_.find(*i);
+    if (node_to_menu != node_to_menu_id_map_.end()) {
+      MenuItemView* menu = GetMenuByID(node_to_menu->second);
+      DCHECK(menu);  // If there an entry in node_to_menu_id_map_, there should
+                     // be a menu.
+      removed_menus->insert(menu);
+      changed_parent_menus.insert(menu->GetParentMenuItem());
+      menu->GetParent()->RemoveChildView(menu);
+      node_to_menu_id_map_.erase(node_to_menu);
+    }
+  }
+
+  // All the bookmarks in |bookmarks| should have the same parent. It's possible
+  // to support different parents, but this would need to prune any nodes whose
+  // parent has been removed. As all nodes currently have the same parent, there
+  // is the DCHECK.
+  DCHECK(changed_parent_menus.size() <= 1);
+
+  for (std::set<MenuItemView*>::const_iterator i = changed_parent_menus.begin();
+       i != changed_parent_menus.end(); ++i) {
+    (*i)->ChildrenChanged();
+  }
+
+  // Remove any descendants of the removed nodes in node_to_menu_id_map_.
+  for (NodeToMenuIDMap::iterator i = node_to_menu_id_map_.begin();
+       i != node_to_menu_id_map_.end(); ) {
+    bool ancestor_removed = false;
+    for (std::vector<const BookmarkNode*>::const_iterator j = bookmarks.begin();
+         j != bookmarks.end(); ++j) {
+      if (i->first->HasAncestor(*j)) {
+        ancestor_removed = true;
+        break;
+      }
+    }
+    if (ancestor_removed) {
+      node_to_menu_id_map_.erase(i++);
+    } else {
+      ++i;
+    }
+  }
+}
diff --git a/chrome/browser/views/bookmark_menu_controller_views.h b/chrome/browser/views/bookmark_menu_controller_views.h
index f19c3cc..13e1295 100644
--- a/chrome/browser/views/bookmark_menu_controller_views.h
+++ b/chrome/browser/views/bookmark_menu_controller_views.h
@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_VIEWS_BOOKMARK_MENU_CONTROLLER_VIEWS_H_
 
 #include <map>
+#include <set>
 
 #include "app/gfx/native_widget_types.h"
 #include "chrome/browser/bookmarks/base_bookmark_model_observer.h"
@@ -35,7 +36,8 @@ class Profile;
 // BookmarkMenuController deletes itself as necessary, although the menu can
 // be explicitly hidden by way of the Cancel method.
 class BookmarkMenuController : public BaseBookmarkModelObserver,
-                               public views::MenuDelegate {
+                               public views::MenuDelegate,
+                               public BookmarkContextMenuObserver {
  public:
   // The observer is notified prior to the menu being deleted.
   class Observer {
@@ -112,7 +114,14 @@ class BookmarkMenuController : public BaseBookmarkModelObserver,
   virtual void BookmarkNodeFavIconLoaded(BookmarkModel* model,
                                          const BookmarkNode* node);
 
+  // BookmarkContextMenu::Observer methods.
+  virtual void WillRemoveBookmarks(
+      const std::vector<const BookmarkNode*>& bookmarks);
+  virtual void DidRemoveBookmarks();
+
  private:
+  typedef std::map<const BookmarkNode*, int> NodeToMenuIDMap;
+
   // BookmarkMenuController deletes itself as necessary.
   ~BookmarkMenuController();
 
@@ -132,6 +141,16 @@ class BookmarkMenuController : public BaseBookmarkModelObserver,
                  views::MenuItemView* menu,
                  int* next_menu_id);
 
+  // Returns the menu whose id is |id|.
+  views::MenuItemView* GetMenuByID(int id);
+
+  // Does the work of processing WillRemoveBookmarks. On exit the set of removed
+  // menus is added to |removed_menus|. It's up to the caller to delete the
+  // the menus added to |removed_menus|.
+  void WillRemoveBookmarksImpl(
+      const std::vector<const BookmarkNode*>& bookmarks,
+      std::set<views::MenuItemView*>* removed_menus);
+
   Browser* browser_;
 
   Profile* profile_;
@@ -149,7 +168,7 @@ class BookmarkMenuController : public BaseBookmarkModelObserver,
 
   // Mapping from node to menu id. This only contains entries for nodes of type
   // URL.
-  std::map<const BookmarkNode*, int> node_to_menu_id_map_;
+  NodeToMenuIDMap node_to_menu_id_map_;
 
   // Current menu.
   views::MenuItemView* menu_;