From c7b6a22a88e1333214f53550558d709abebdef44 Mon Sep 17 00:00:00 2001 From: "sky@chromium.org" Date: Fri, 27 May 2011 16:24:43 +0000 Subject: 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 --- .../ui/views/bookmarks/bookmark_bar_view.cc | 5 +++- .../ui/views/bookmarks/bookmark_context_menu.cc | 11 ++++----- .../ui/views/bookmarks/bookmark_context_menu.h | 6 ++++- .../views/bookmarks/bookmark_context_menu_test.cc | 27 +++++++++++----------- .../ui/views/bookmarks/bookmark_menu_delegate.cc | 6 ++++- chrome/browser/ui/views/wrench_menu.cc | 6 +++-- views/controls/menu/menu_controller.cc | 8 ++++++- 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& selection) + const std::vector& 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& selection); + const std::vector& 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 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()); + std::vector(), 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 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 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 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 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()); + NULL, profile_.get(), NULL, NULL, std::vector(), + 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 nodes; nodes.push_back(model_->GetBookmarkBarNode()->GetChild(0)); scoped_ptr 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 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; -- cgit v1.1