diff options
Diffstat (limited to 'chrome')
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_; |