Couple more wrench menu issues. The patch I sent around a couple of

days back didn't include these:

. If other bookmarks folder is empty it wouldn't show a menu item for
  (empty).
. Deleting the last item in the other folder would prematurely close
  the menu.
. 'always show bookmark bar' wasn't rendering the check.

BUG=84167
TEST=none
R=ben@chromium.org

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

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

7 files changed, 44 insertions, 25 deletions

diff --git a/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc b/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
index e71a5b9..3d418ca6 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
@@ -1147,8 +1147,11 @@ void BookmarkBarView::ShowContextMenuForView(View* source,
   // Browser may be null during testing.
   PageNavigator* navigator =
       browser() ? browser()->GetSelectedTabContents() : NULL;
+  bool close_on_remove =
+      (parent == profile_->GetBookmarkModel()->other_node() &&
+       parent->child_count() == 1);
   BookmarkContextMenu controller(GetWindow()->GetNativeWindow(), GetProfile(),
-                                 navigator, parent, nodes);
+                                 navigator, parent, nodes, close_on_remove);
   controller.RunMenuAt(p);
 }
 
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_context_menu.cc b/chrome/browser/ui/views/bookmarks/bookmark_context_menu.cc
index 1468dfb..8ac9789 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_context_menu.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_context_menu.cc
@@ -22,14 +22,16 @@ BookmarkContextMenu::BookmarkContextMenu(
     Profile* profile,
     PageNavigator* page_navigator,
     const BookmarkNode* parent,
-    const std::vector<const BookmarkNode*>& selection)
+    const std::vector<const BookmarkNode*>& selection,
+    bool close_on_remove)
     : ALLOW_THIS_IN_INITIALIZER_LIST(
           controller_(new BookmarkContextMenuControllerViews(parent_window,
               this, profile, page_navigator, parent, selection))),
       parent_window_(parent_window),
       ALLOW_THIS_IN_INITIALIZER_LIST(menu_(new views::MenuItemView(this))),
       parent_node_(parent),
-      observer_(NULL) {
+      observer_(NULL),
+      close_on_remove_(close_on_remove) {
   controller_->BuildMenu();
 }
 
@@ -64,10 +66,7 @@ bool BookmarkContextMenu::IsCommandEnabled(int command_id) const {
 }
 
 bool BookmarkContextMenu::ShouldCloseAllMenusOnExecute(int id) {
-  return id != IDC_BOOKMARK_BAR_REMOVE ||
-      (parent_node_ ==
-       controller_->profile()->GetBookmarkModel()->other_node() &&
-       parent_node_->child_count() == 1);
+  return (id != IDC_BOOKMARK_BAR_REMOVE) || close_on_remove_;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_context_menu.h b/chrome/browser/ui/views/bookmarks/bookmark_context_menu.h
index e9f0688..5fb95c5 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_context_menu.h
+++ b/chrome/browser/ui/views/bookmarks/bookmark_context_menu.h
@@ -31,7 +31,8 @@ class BookmarkContextMenu : public BookmarkContextMenuControllerViewsDelegate,
       Profile* profile,
       PageNavigator* page_navigator,
       const BookmarkNode* parent,
-      const std::vector<const BookmarkNode*>& selection);
+      const std::vector<const BookmarkNode*>& selection,
+      bool close_on_remove);
   virtual ~BookmarkContextMenu();
 
   // Shows the context menu at the specified point.
@@ -72,6 +73,9 @@ class BookmarkContextMenu : public BookmarkContextMenuControllerViewsDelegate,
 
   BookmarkContextMenuObserver* observer_;
 
+  // Should the menu close when a node is removed.
+  bool close_on_remove_;
+
   DISALLOW_COPY_AND_ASSIGN(BookmarkContextMenu);
 };
 
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_context_menu_test.cc b/chrome/browser/ui/views/bookmarks/bookmark_context_menu_test.cc
index 2b4eebc..38c3918 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_context_menu_test.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_context_menu_test.cc
@@ -112,7 +112,7 @@ TEST_F(BookmarkContextMenuTest, DeleteURL) {
   std::vector<const BookmarkNode*> nodes;
   nodes.push_back(model_->GetBookmarkBarNode()->GetChild(0));
   BookmarkContextMenu controller(
-      NULL, profile_.get(), NULL, nodes[0]->parent(), nodes);
+      NULL, profile_.get(), NULL, nodes[0]->parent(), nodes, false);
   GURL url = model_->GetBookmarkBarNode()->GetChild(0)->GetURL();
   ASSERT_TRUE(controller.IsCommandEnabled(IDC_BOOKMARK_BAR_REMOVE));
   // Delete the URL.
@@ -136,7 +136,7 @@ TEST_F(BookmarkContextMenuTest, OpenAll) {
 TEST_F(BookmarkContextMenuTest, EmptyNodes) {
   BookmarkContextMenu controller(
       NULL, profile_.get(), NULL, model_->other_node(),
-      std::vector<const BookmarkNode*>());
+      std::vector<const BookmarkNode*>(), false);
   EXPECT_FALSE(controller.IsCommandEnabled(IDC_BOOKMARK_BAR_OPEN_ALL));
   EXPECT_FALSE(
       controller.IsCommandEnabled(IDC_BOOKMARK_BAR_OPEN_ALL_NEW_WINDOW));
@@ -155,7 +155,7 @@ TEST_F(BookmarkContextMenuTest, SingleURL) {
   std::vector<const BookmarkNode*> nodes;
   nodes.push_back(model_->GetBookmarkBarNode()->GetChild(0));
   BookmarkContextMenu controller(
-      NULL, profile_.get(), NULL, nodes[0]->parent(), nodes);
+      NULL, profile_.get(), NULL, nodes[0]->parent(), nodes, false);
   EXPECT_TRUE(controller.IsCommandEnabled(IDC_BOOKMARK_BAR_OPEN_ALL));
   EXPECT_TRUE(
       controller.IsCommandEnabled(IDC_BOOKMARK_BAR_OPEN_ALL_NEW_WINDOW));
@@ -174,7 +174,7 @@ TEST_F(BookmarkContextMenuTest, MultipleURLs) {
   nodes.push_back(model_->GetBookmarkBarNode()->GetChild(0));
   nodes.push_back(model_->GetBookmarkBarNode()->GetChild(1)->GetChild(0));
   BookmarkContextMenu controller(
-      NULL, profile_.get(), NULL, nodes[0]->parent(), nodes);
+      NULL, profile_.get(), NULL, nodes[0]->parent(), nodes, false);
   EXPECT_TRUE(controller.IsCommandEnabled(IDC_BOOKMARK_BAR_OPEN_ALL));
   EXPECT_TRUE(
       controller.IsCommandEnabled(IDC_BOOKMARK_BAR_OPEN_ALL_NEW_WINDOW));
@@ -192,7 +192,7 @@ TEST_F(BookmarkContextMenuTest, SingleFolder) {
   std::vector<const BookmarkNode*> nodes;
   nodes.push_back(model_->GetBookmarkBarNode()->GetChild(2));
   BookmarkContextMenu controller(
-      NULL, profile_.get(), NULL, nodes[0]->parent(), nodes);
+      NULL, profile_.get(), NULL, nodes[0]->parent(), nodes, false);
   EXPECT_FALSE(controller.IsCommandEnabled(IDC_BOOKMARK_BAR_OPEN_ALL));
   EXPECT_FALSE(
       controller.IsCommandEnabled(IDC_BOOKMARK_BAR_OPEN_ALL_NEW_WINDOW));
@@ -212,7 +212,7 @@ TEST_F(BookmarkContextMenuTest, MultipleEmptyFolders) {
   nodes.push_back(model_->GetBookmarkBarNode()->GetChild(2));
   nodes.push_back(model_->GetBookmarkBarNode()->GetChild(3));
   BookmarkContextMenu controller(
-      NULL, profile_.get(), NULL, nodes[0]->parent(), nodes);
+      NULL, profile_.get(), NULL, nodes[0]->parent(), nodes, false);
   EXPECT_FALSE(controller.IsCommandEnabled(IDC_BOOKMARK_BAR_OPEN_ALL));
   EXPECT_FALSE(
       controller.IsCommandEnabled(IDC_BOOKMARK_BAR_OPEN_ALL_NEW_WINDOW));
@@ -232,7 +232,7 @@ TEST_F(BookmarkContextMenuTest, MultipleFoldersWithURLs) {
   nodes.push_back(model_->GetBookmarkBarNode()->GetChild(3));
   nodes.push_back(model_->GetBookmarkBarNode()->GetChild(4));
   BookmarkContextMenu controller(
-      NULL, profile_.get(), NULL, nodes[0]->parent(), nodes);
+      NULL, profile_.get(), NULL, nodes[0]->parent(), nodes, false);
   EXPECT_TRUE(controller.IsCommandEnabled(IDC_BOOKMARK_BAR_OPEN_ALL));
   EXPECT_TRUE(
       controller.IsCommandEnabled(IDC_BOOKMARK_BAR_OPEN_ALL_NEW_WINDOW));
@@ -249,7 +249,7 @@ TEST_F(BookmarkContextMenuTest, DisableIncognito) {
   std::vector<const BookmarkNode*> nodes;
   nodes.push_back(model_->GetBookmarkBarNode()->GetChild(0));
   BookmarkContextMenu controller(
-      NULL, profile_.get(), NULL, nodes[0]->parent(), nodes);
+      NULL, profile_.get(), NULL, nodes[0]->parent(), nodes, false);
   profile_->set_incognito(true);
   EXPECT_FALSE(controller.IsCommandEnabled(IDC_BOOKMARK_BAR_OPEN_INCOGNITO));
   EXPECT_FALSE(
@@ -261,7 +261,7 @@ TEST_F(BookmarkContextMenuTest, DisabledItemsWithOtherNode) {
   std::vector<const BookmarkNode*> nodes;
   nodes.push_back(model_->other_node());
   BookmarkContextMenu controller(
-      NULL, profile_.get(), NULL, nodes[0], nodes);
+      NULL, profile_.get(), NULL, nodes[0], nodes, false);
   EXPECT_FALSE(controller.IsCommandEnabled(IDC_BOOKMARK_BAR_EDIT));
   EXPECT_FALSE(controller.IsCommandEnabled(IDC_BOOKMARK_BAR_REMOVE));
 }
@@ -270,7 +270,8 @@ TEST_F(BookmarkContextMenuTest, DisabledItemsWithOtherNode) {
 // parent.
 TEST_F(BookmarkContextMenuTest, EmptyNodesNullParent) {
   BookmarkContextMenu controller(
-      NULL, profile_.get(), NULL, NULL, std::vector<const BookmarkNode*>());
+      NULL, profile_.get(), NULL, NULL, std::vector<const BookmarkNode*>(),
+      false);
   EXPECT_FALSE(controller.IsCommandEnabled(IDC_BOOKMARK_BAR_OPEN_ALL));
   EXPECT_FALSE(
       controller.IsCommandEnabled(IDC_BOOKMARK_BAR_OPEN_ALL_NEW_WINDOW));
@@ -287,7 +288,7 @@ TEST_F(BookmarkContextMenuTest, CutCopyPasteNode) {
   std::vector<const BookmarkNode*> nodes;
   nodes.push_back(model_->GetBookmarkBarNode()->GetChild(0));
   scoped_ptr<BookmarkContextMenu> controller(new BookmarkContextMenu(
-      NULL, profile_.get(), NULL, nodes[0]->parent(), nodes));
+      NULL, profile_.get(), NULL, nodes[0]->parent(), nodes, false));
   EXPECT_TRUE(controller->IsCommandEnabled(IDC_COPY));
   EXPECT_TRUE(controller->IsCommandEnabled(IDC_CUT));
 
@@ -295,7 +296,7 @@ TEST_F(BookmarkContextMenuTest, CutCopyPasteNode) {
   controller->ExecuteCommand(IDC_COPY);
 
   controller.reset(new BookmarkContextMenu(
-      NULL, profile_.get(), NULL, nodes[0]->parent(), nodes));
+      NULL, profile_.get(), NULL, nodes[0]->parent(), nodes, false));
   int old_count = model_->GetBookmarkBarNode()->child_count();
   controller->ExecuteCommand(IDC_PASTE);
 
@@ -305,7 +306,7 @@ TEST_F(BookmarkContextMenuTest, CutCopyPasteNode) {
             model_->GetBookmarkBarNode()->GetChild(1)->GetURL());
 
   controller.reset(new BookmarkContextMenu(
-      NULL, profile_.get(), NULL, nodes[0]->parent(), nodes));
+      NULL, profile_.get(), NULL, nodes[0]->parent(), nodes, false));
   // Cut the URL.
   controller->ExecuteCommand(IDC_CUT);
   ASSERT_TRUE(model_->GetBookmarkBarNode()->GetChild(0)->is_url());
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc b/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc
index 2fa2125..f3438bb 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc
@@ -226,13 +226,17 @@ bool BookmarkMenuDelegate::ShowContextMenu(MenuItemView* source,
   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() == profile()->GetBookmarkModel()->other_node() &&
+       nodes[0]->parent()->child_count() == 1);
   context_menu_.reset(
       new BookmarkContextMenu(
           parent_,
           profile_,
           page_navigator_,
           nodes[0]->parent(),
-          nodes));
+          nodes,
+          close_on_delete));
   context_menu_->set_observer(this);
   context_menu_->RunMenuAt(p);
   context_menu_.reset(NULL);
diff --git a/chrome/browser/ui/views/wrench_menu.cc b/chrome/browser/ui/views/wrench_menu.cc
index b7d453c..7b5e5ce0 100644
--- a/chrome/browser/ui/views/wrench_menu.cc
+++ b/chrome/browser/ui/views/wrench_menu.cc
@@ -693,7 +693,7 @@ int WrenchMenu::GetMaxWidthForMenu(MenuItemView* menu) {
 }
 
 bool WrenchMenu::IsItemChecked(int id) const {
-  if (!is_bookmark_command(id))
+  if (is_bookmark_command(id))
     return false;
 
   const Entry& entry = id_to_entry_.find(id)->second;
@@ -762,7 +762,9 @@ void WrenchMenu::WillShowMenu(MenuItemView* menu) {
 }
 
 void WrenchMenu::BookmarkModelChanged() {
-  root_->Cancel();
+  DCHECK(bookmark_menu_delegate_.get());
+  if (!bookmark_menu_delegate_->is_mutating_model())
+    root_->Cancel();
 }
 
 WrenchMenu::~WrenchMenu() {
diff --git a/views/controls/menu/menu_controller.cc b/views/controls/menu/menu_controller.cc
index a59caac..022f176 100644
--- a/views/controls/menu/menu_controller.cc
+++ b/views/controls/menu/menu_controller.cc
@@ -1325,8 +1325,14 @@ void MenuController::OpenMenu(MenuItemView* item) {
 }
 
 void MenuController::OpenMenuImpl(MenuItemView* item, bool show) {
-  if (show)
+  if (show) {
+    int old_count = item->GetSubmenu()->child_count();
     item->GetDelegate()->WillShowMenu(item);
+    if (old_count != item->GetSubmenu()->child_count()) {
+      // If the number of children changed then we may need to add empty items.
+      item->AddEmptyMenus();
+    }
+  }
   bool prefer_leading =
       state_.open_leading.empty() ? true : state_.open_leading.back();
   bool resulting_direction;