From 90b1a7a5fc8d2939ab40be1081fdbc55c6461bf2 Mon Sep 17 00:00:00 2001 From: "sky@chromium.org" Date: Thu, 17 Mar 2011 21:28:21 +0000 Subject: Updates TabStripModel context menu commands to deal with multiple selected tabs. A right click on an selected tab makes the action apply to all tabs. A right click on an unselected tab only applies to that tab. BUG=30572 TEST=covered by unit tests. Review URL: http://codereview.chromium.org/6673095 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@78604 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/app/generated_resources.grd | 24 ++- chrome/browser/tabs/tab_strip_model.cc | 177 +++++++++++++---- chrome/browser/tabs/tab_strip_model.h | 26 ++- chrome/browser/tabs/tab_strip_model_unittest.cc | 213 +++++++++++++++++++-- chrome/browser/tabs/tab_strip_selection_model.cc | 4 +- chrome/browser/tabs/tab_strip_selection_model.h | 2 +- chrome/browser/ui/browser.cc | 8 +- chrome/browser/ui/tabs/tab_menu_model.cc | 51 ++++- chrome/browser/ui/tabs/tab_menu_model.h | 17 +- .../ui/views/tabs/browser_tab_strip_controller.cc | 4 +- 10 files changed, 448 insertions(+), 78 deletions(-) (limited to 'chrome') diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd index 612fcf3..5b06db7 100644 --- a/chrome/app/generated_resources.grd +++ b/chrome/app/generated_resources.grd @@ -5639,21 +5639,30 @@ Keep your key file in a safe place. You will need it to create new versions of y Duplicate - + Close tab + + Close tabs + Close other tabs Close tabs to the right - + Pin tab - + + Pin tabs + + Unpin tab + + Unpin tabs + Bookmark all tabs... @@ -5674,6 +5683,9 @@ Keep your key file in a safe place. You will need it to create new versions of y Close Tab + + Close Tabs + Close Other Tabs @@ -5683,9 +5695,15 @@ Keep your key file in a safe place. You will need it to create new versions of y Pin Tab + + Pin Tabs + Unpin Tab + + Unpin Tabs + Bookmark All Tabs... diff --git a/chrome/browser/tabs/tab_strip_model.cc b/chrome/browser/tabs/tab_strip_model.cc index 41232ac..e030167 100644 --- a/chrome/browser/tabs/tab_strip_model.cc +++ b/chrome/browser/tabs/tab_strip_model.cc @@ -585,7 +585,7 @@ void TabStripModel::ToggleSelectionAt(int index) { NotifySelectionChanged(old_selection); } -bool TabStripModel::IsTabSelected(int index) { +bool TabStripModel::IsTabSelected(int index) const { DCHECK(ContainsIndex(index)); return selection_model_.IsSelected(index); } @@ -666,9 +666,9 @@ void TabStripModel::AddTabContents(TabContentsWrapper* contents, } } -void TabStripModel::CloseSelectedTab() { - // TODO: this should close all selected tabs. - CloseTabContentsAt(selected_index(), CLOSE_CREATE_HISTORICAL_TAB); +void TabStripModel::CloseSelectedTabs() { + InternalCloseTabs(selection_model_.selected_indices(), + CLOSE_CREATE_HISTORICAL_TAB | CLOSE_USER_GESTURE); } void TabStripModel::SelectNextTab() { @@ -701,37 +701,55 @@ bool TabStripModel::IsContextMenuCommandEnabled( DCHECK(command_id > CommandFirst && command_id < CommandLast); switch (command_id) { case CommandNewTab: + return true; + case CommandCloseTab: return delegate_->CanCloseTab(); - case CommandReload: - if (TabContentsWrapper* contents = GetTabContentsAt(context_index)) { - return contents->tab_contents()-> - delegate()->CanReloadContents(contents->tab_contents()); - } else { - return false; + + case CommandReload: { + std::vector indices = GetIndicesForCommand(context_index); + for (size_t i = 0; i < indices.size(); ++i) { + TabContentsWrapper* tab = GetTabContentsAt(indices[i]); + if (tab && tab->tab_contents()->delegate()->CanReloadContents( + tab->tab_contents())) { + return true; + } } - case CommandCloseOtherTabs: { - int mini_tab_count = IndexOfFirstNonMiniTab(); - int non_mini_tab_count = count() - mini_tab_count; - // Close other doesn't effect mini-tabs. - return non_mini_tab_count > 1 || - (non_mini_tab_count == 1 && context_index != mini_tab_count); + return false; } + + case CommandCloseOtherTabs: case CommandCloseTabsToRight: - // Close doesn't effect mini-tabs. - return count() != IndexOfFirstNonMiniTab() && - context_index < (count() - 1); - case CommandDuplicate: - return delegate_->CanDuplicateContentsAt(context_index); + return !GetIndicesClosedByCommand(context_index, command_id).empty(); + + case CommandDuplicate: { + std::vector indices = GetIndicesForCommand(context_index); + for (size_t i = 0; i < indices.size(); ++i) { + if (delegate_->CanDuplicateContentsAt(indices[i])) + return true; + } + return false; + } + case CommandRestoreTab: return delegate_->CanRestoreTab(); - case CommandTogglePinned: - return !IsAppTab(context_index); + + case CommandTogglePinned: { + std::vector indices = GetIndicesForCommand(context_index); + for (size_t i = 0; i < indices.size(); ++i) { + if (!IsAppTab(indices[i])) + return true; + } + return false; + } + case CommandBookmarkAllTabs: return browser_defaults::bookmarks_enabled && delegate_->CanBookmarkAllTabs(); + case CommandUseVerticalTabs: return true; + default: NOTREACHED(); } @@ -760,22 +778,57 @@ void TabStripModel::ExecuteContextMenuCommand( profile_); delegate()->AddBlankTabAt(context_index + 1, true); break; - case CommandReload: + + case CommandReload: { UserMetrics::RecordAction(UserMetricsAction("TabContextMenu_Reload"), profile_); - GetContentsAt(context_index)->controller().Reload(true); + std::vector indices = GetIndicesForCommand(context_index); + for (size_t i = 0; i < indices.size(); ++i) { + TabContentsWrapper* tab = GetTabContentsAt(indices[i]); + if (tab && tab->tab_contents()->delegate()->CanReloadContents( + tab->tab_contents())) { + tab->controller().Reload(true); + } + } break; - case CommandDuplicate: + } + + case CommandDuplicate: { UserMetrics::RecordAction(UserMetricsAction("TabContextMenu_Duplicate"), profile_); - delegate_->DuplicateContentsAt(context_index); + std::vector indices = GetIndicesForCommand(context_index); + // Copy the TabContents off as the indices will change as tabs are + // duplicated. + std::vector tabs; + for (size_t i = 0; i < indices.size(); ++i) + tabs.push_back(GetTabContentsAt(indices[i])); + for (size_t i = 0; i < tabs.size(); ++i) { + int index = GetIndexOfTabContents(tabs[i]); + if (index != -1 && delegate_->CanDuplicateContentsAt(index)) + delegate_->DuplicateContentsAt(index); + } break; - case CommandCloseTab: + } + + case CommandCloseTab: { UserMetrics::RecordAction(UserMetricsAction("TabContextMenu_CloseTab"), profile_); - CloseTabContentsAt(context_index, CLOSE_CREATE_HISTORICAL_TAB | - CLOSE_USER_GESTURE); + std::vector indices = GetIndicesForCommand(context_index); + // Copy the TabContents off as the indices will change as we remove + // things. + std::vector tabs; + for (size_t i = 0; i < indices.size(); ++i) + tabs.push_back(GetTabContentsAt(indices[i])); + for (size_t i = 0; i < tabs.size() && delegate_->CanCloseTab(); ++i) { + int index = GetIndexOfTabContents(tabs[i]); + if (index != -1) { + CloseTabContentsAt(index, + CLOSE_CREATE_HISTORICAL_TAB | CLOSE_USER_GESTURE); + } + } break; + } + case CommandCloseOtherTabs: { UserMetrics::RecordAction( UserMetricsAction("TabContextMenu_CloseOtherTabs"), @@ -784,6 +837,7 @@ void TabStripModel::ExecuteContextMenuCommand( CLOSE_CREATE_HISTORICAL_TAB); break; } + case CommandCloseTabsToRight: { UserMetrics::RecordAction( UserMetricsAction("TabContextMenu_CloseTabsToRight"), @@ -792,18 +846,33 @@ void TabStripModel::ExecuteContextMenuCommand( CLOSE_CREATE_HISTORICAL_TAB); break; } + case CommandRestoreTab: { UserMetrics::RecordAction(UserMetricsAction("TabContextMenu_RestoreTab"), profile_); delegate_->RestoreTab(); break; } + case CommandTogglePinned: { UserMetrics::RecordAction( UserMetricsAction("TabContextMenu_TogglePinned"), profile_); - SelectTabContentsAt(context_index, true); - SetTabPinned(context_index, !IsTabPinned(context_index)); + std::vector indices = GetIndicesForCommand(context_index); + bool pin = WillContextMenuPin(context_index); + if (pin) { + for (size_t i = 0; i < indices.size(); ++i) { + if (!IsAppTab(indices[i])) + SetTabPinned(indices[i], true); + } + } else { + // Unpin from the back so that the order is maintained (unpinning can + // trigger moving a tab). + for (size_t i = indices.size(); i > 0; --i) { + if (!IsAppTab(indices[i - 1])) + SetTabPinned(indices[i - 1], false); + } + } break; } @@ -829,26 +898,43 @@ void TabStripModel::ExecuteContextMenuCommand( } } - std::vector TabStripModel::GetIndicesClosedByCommand( int index, ContextMenuCommand id) const { DCHECK(ContainsIndex(index)); - - // NOTE: some callers assume indices are sorted in reverse order. + DCHECK(id == CommandCloseTabsToRight || id == CommandCloseOtherTabs); + bool is_selected = IsTabSelected(index); + int start; + if (id == CommandCloseTabsToRight) { + if (is_selected) { + start = selection_model_.selected_indices()[ + selection_model_.selected_indices().size() - 1] + 1; + } else { + start = index + 1; + } + } else { + start = 0; + } + // NOTE: callers expect the vector to be sorted in descending order. std::vector indices; - - if (id != CommandCloseTabsToRight && id != CommandCloseOtherTabs) - return indices; - - int start = (id == CommandCloseTabsToRight) ? index + 1 : 0; for (int i = count() - 1; i >= start; --i) { - if (i != index && !IsMiniTab(i)) + if (i != index && !IsMiniTab(i) && (!is_selected || !IsTabSelected(i))) indices.push_back(i); } return indices; } +bool TabStripModel::WillContextMenuPin(int index) { + std::vector indices = GetIndicesForCommand(index); + // If all tabs are pinned, then we unpin, otherwise we pin. + bool all_pinned = true; + for (size_t i = 0; i < indices.size() && all_pinned; ++i) { + if (!IsAppTab(index)) // We never change app tabs. + all_pinned = IsTabPinned(indices[i]); + } + return !all_pinned; +} + /////////////////////////////////////////////////////////////////////////////// // TabStripModel, NotificationObserver implementation: @@ -928,6 +1014,15 @@ bool TabStripModel::ContextMenuCommandToBrowserCommand(int cmd_id, /////////////////////////////////////////////////////////////////////////////// // TabStripModel, private: +std::vector TabStripModel::GetIndicesForCommand(int index) const { + if (!IsTabSelected(index)) { + std::vector indices; + indices.push_back(index); + return indices; + } + return selection_model_.selected_indices(); +} + bool TabStripModel::IsNewTabAtEndOfTabStrip( TabContentsWrapper* contents) const { return LowerCaseEqualsASCII(contents->tab_contents()->GetURL().spec(), diff --git a/chrome/browser/tabs/tab_strip_model.h b/chrome/browser/tabs/tab_strip_model.h index 9807a22..2b7fd95 100644 --- a/chrome/browser/tabs/tab_strip_model.h +++ b/chrome/browser/tabs/tab_strip_model.h @@ -245,6 +245,7 @@ class TabStripModel : public NotificationObserver { void MoveSelectedTabsTo(int index); // Returns the currently selected TabContents, or NULL if there is none. + // TODO(sky): rename to GetActiveTabContents. TabContentsWrapper* GetSelectedTabContents() const; // Returns the TabContentsWrapper at the specified index, or NULL if there is @@ -375,11 +376,15 @@ class TabStripModel : public NotificationObserver { void ToggleSelectionAt(int index); // Returns true if the tab at |index| is selected. - bool IsTabSelected(int index); + bool IsTabSelected(int index) const; // Sets the selection to match that of |source|. void SetSelectionFromModel(const TabStripSelectionModel& source); + const TabStripSelectionModel& selection_model() const { + return selection_model_; + } + // Command level API ///////////////////////////////////////////////////////// // Adds a TabContents at the best position in the TabStripModel given the @@ -391,8 +396,8 @@ class TabStripModel : public NotificationObserver { PageTransition::Type transition, int add_types); - // Closes the selected TabContents. - void CloseSelectedTab(); + // Closes the selected tabs. + void CloseSelectedTabs(); // Select adjacent tabs void SelectNextTab(); @@ -423,7 +428,8 @@ class TabStripModel : public NotificationObserver { CommandLast }; - // Returns true if the specified command is enabled. + // Returns true if the specified command is enabled. If |context_index| is + // selected the response applies to all selected tabs. bool IsContextMenuCommandEnabled(int context_index, ContextMenuCommand command_id) const; @@ -432,7 +438,8 @@ class TabStripModel : public NotificationObserver { ContextMenuCommand command_id) const; // Performs the action associated with the specified command for the given - // TabStripModel index |context_index|. + // TabStripModel index |context_index|. If |context_index| is selected the + // command applies to all selected tabs. void ExecuteContextMenuCommand(int context_index, ContextMenuCommand command_id); @@ -442,6 +449,10 @@ class TabStripModel : public NotificationObserver { std::vector GetIndicesClosedByCommand(int index, ContextMenuCommand id) const; + // Returns true if 'CommandTogglePinned' will pin. |index| is the index + // supplied to |ExecuteContextMenuCommand|. + bool WillContextMenuPin(int index); + // Overridden from notificationObserver: virtual void Observe(NotificationType type, const NotificationSource& source, @@ -455,6 +466,11 @@ class TabStripModel : public NotificationObserver { // We cannot be constructed without a delegate. TabStripModel(); + // If |index| is selected all the selected indices are returned, otherwise a + // vector with |index| is returned. This is used when executing commands to + // determine which indices the command applies to. + std::vector GetIndicesForCommand(int index) const; + // Returns true if the specified TabContents is a New Tab at the end of the // TabStrip. We check for this because opener relationships are _not_ // forgotten for the New Tab page opened as a result of a New Tab gesture diff --git a/chrome/browser/tabs/tab_strip_model_unittest.cc b/chrome/browser/tabs/tab_strip_model_unittest.cc index 434ba92..cf734ab 100644 --- a/chrome/browser/tabs/tab_strip_model_unittest.cc +++ b/chrome/browser/tabs/tab_strip_model_unittest.cc @@ -572,10 +572,10 @@ TEST_F(TabStripModelTest, TestBasicAPI) { EXPECT_EQ(0, tabstrip.selected_index()); } - // Test CloseSelectedTab + // Test CloseSelectedTabs { - tabstrip.CloseSelectedTab(); - // |CloseSelectedTab| calls CloseTabContentsAt, we already tested that, now + tabstrip.CloseSelectedTabs(); + // |CloseSelectedTabs| calls CloseTabContentsAt, we already tested that, now // just verify that the count and selected index have changed // appropriately... EXPECT_EQ(1, tabstrip.count()); @@ -790,7 +790,7 @@ TEST_F(TabStripModelTest, TestInsertionIndexDetermination) { EXPECT_EQ(fg_link_contents, tabstrip.GetSelectedTabContents()); // Now close this contents. The selection should move to the opener contents. - tabstrip.CloseSelectedTab(); + tabstrip.CloseSelectedTabs(); EXPECT_EQ(0, tabstrip.selected_index()); // Now open a new empty tab. It should open at the end of the strip. @@ -901,6 +901,177 @@ TEST_F(TabStripModelTest, TestSelectOnClose) { EXPECT_TRUE(tabstrip.empty()); } +// Tests IsContextMenuCommandEnabled and ExecuteContextMenuCommand with +// CommandCloseTab. +TEST_F(TabStripModelTest, CommandCloseTab) { + TabStripDummyDelegate delegate(NULL); + TabStripModel tabstrip(&delegate, profile()); + EXPECT_TRUE(tabstrip.empty()); + + // Make sure can_close is honored. + ASSERT_NO_FATAL_FAILURE( + PrepareTabstripForSelectionTest(&tabstrip, 1, 0, "0")); + EXPECT_TRUE(tabstrip.IsContextMenuCommandEnabled( + 0, TabStripModel::CommandCloseTab)); + delegate.set_can_close(false); + EXPECT_TRUE(tabstrip.IsContextMenuCommandEnabled( + 0, TabStripModel::CommandCloseTab)); + delegate.set_can_close(true); + tabstrip.ExecuteContextMenuCommand(0, TabStripModel::CommandCloseTab); + ASSERT_TRUE(tabstrip.empty()); + + // Make sure close on a tab that is selected effects all the selected tabs. + ASSERT_NO_FATAL_FAILURE( + PrepareTabstripForSelectionTest(&tabstrip, 3, 0, "0 1")); + EXPECT_TRUE(tabstrip.IsContextMenuCommandEnabled( + 0, TabStripModel::CommandCloseTab)); + tabstrip.ExecuteContextMenuCommand(0, TabStripModel::CommandCloseTab); + // Should have closed tabs 0 and 1. + EXPECT_EQ("2", GetPinnedState(tabstrip)); + + tabstrip.CloseAllTabs(); + EXPECT_TRUE(tabstrip.empty()); + + // Select two tabs and make close on a tab that isn't selected doesn't effect + // selected tabs. + ASSERT_NO_FATAL_FAILURE( + PrepareTabstripForSelectionTest(&tabstrip, 3, 0, "0 1")); + EXPECT_TRUE(tabstrip.IsContextMenuCommandEnabled( + 2, TabStripModel::CommandCloseTab)); + tabstrip.ExecuteContextMenuCommand(2, TabStripModel::CommandCloseTab); + // Should have closed tab 2. + EXPECT_EQ("0 1", GetPinnedState(tabstrip)); + tabstrip.CloseAllTabs(); + EXPECT_TRUE(tabstrip.empty()); + + // Tests with 3 tabs, one pinned, two tab selected, one of which is pinned. + ASSERT_NO_FATAL_FAILURE( + PrepareTabstripForSelectionTest(&tabstrip, 3, 1, "0 1")); + EXPECT_TRUE(tabstrip.IsContextMenuCommandEnabled( + 0, TabStripModel::CommandCloseTab)); + tabstrip.ExecuteContextMenuCommand(0, TabStripModel::CommandCloseTab); + // Should have closed tab 2. + EXPECT_EQ("2", GetPinnedState(tabstrip)); + tabstrip.CloseAllTabs(); + EXPECT_TRUE(tabstrip.empty()); +} + +// Tests IsContextMenuCommandEnabled and ExecuteContextMenuCommand with +// CommandCloseTabs. +TEST_F(TabStripModelTest, CommandCloseOtherTabs) { + TabStripDummyDelegate delegate(NULL); + TabStripModel tabstrip(&delegate, profile()); + EXPECT_TRUE(tabstrip.empty()); + + // Create three tabs, select two tabs, CommandCloseOtherTabs should be enabled + // and close two tabs. + ASSERT_NO_FATAL_FAILURE( + PrepareTabstripForSelectionTest(&tabstrip, 3, 0, "0 1")); + EXPECT_TRUE(tabstrip.IsContextMenuCommandEnabled( + 0, TabStripModel::CommandCloseOtherTabs)); + tabstrip.ExecuteContextMenuCommand(0, TabStripModel::CommandCloseOtherTabs); + EXPECT_EQ("0 1", GetPinnedState(tabstrip)); + tabstrip.CloseAllTabs(); + EXPECT_TRUE(tabstrip.empty()); + + // Select two tabs, CommandCloseOtherTabs should be enabled and invoking it + // with a non-selected index should close the two other tabs. + ASSERT_NO_FATAL_FAILURE( + PrepareTabstripForSelectionTest(&tabstrip, 3, 0, "0 1")); + EXPECT_TRUE(tabstrip.IsContextMenuCommandEnabled( + 2, TabStripModel::CommandCloseOtherTabs)); + tabstrip.ExecuteContextMenuCommand(0, TabStripModel::CommandCloseOtherTabs); + EXPECT_EQ("0 1", GetPinnedState(tabstrip)); + tabstrip.CloseAllTabs(); + EXPECT_TRUE(tabstrip.empty()); + + // Select all, CommandCloseOtherTabs should not be enabled. + ASSERT_NO_FATAL_FAILURE( + PrepareTabstripForSelectionTest(&tabstrip, 3, 0, "0 1 2")); + EXPECT_FALSE(tabstrip.IsContextMenuCommandEnabled( + 2, TabStripModel::CommandCloseOtherTabs)); + tabstrip.CloseAllTabs(); + EXPECT_TRUE(tabstrip.empty()); + + // Three tabs, pin one, select the two non-pinned. + ASSERT_NO_FATAL_FAILURE( + PrepareTabstripForSelectionTest(&tabstrip, 3, 1, "1 2")); + EXPECT_FALSE(tabstrip.IsContextMenuCommandEnabled( + 1, TabStripModel::CommandCloseOtherTabs)); + // If we don't pass in the pinned index, the command should be enabled. + EXPECT_TRUE(tabstrip.IsContextMenuCommandEnabled( + 0, TabStripModel::CommandCloseOtherTabs)); + tabstrip.CloseAllTabs(); + EXPECT_TRUE(tabstrip.empty()); + + // 3 tabs, one pinned. + ASSERT_NO_FATAL_FAILURE( + PrepareTabstripForSelectionTest(&tabstrip, 3, 1, "1")); + EXPECT_TRUE(tabstrip.IsContextMenuCommandEnabled( + 1, TabStripModel::CommandCloseOtherTabs)); + EXPECT_TRUE(tabstrip.IsContextMenuCommandEnabled( + 0, TabStripModel::CommandCloseOtherTabs)); + tabstrip.ExecuteContextMenuCommand(1, TabStripModel::CommandCloseOtherTabs); + // The pinned tab shouldn't be closed. + EXPECT_EQ("0p 1", GetPinnedState(tabstrip)); + tabstrip.CloseAllTabs(); + EXPECT_TRUE(tabstrip.empty()); +} + +// Tests IsContextMenuCommandEnabled and ExecuteContextMenuCommand with +// CommandCloseTabsToRight. +TEST_F(TabStripModelTest, CommandCloseTabsToRight) { + TabStripDummyDelegate delegate(NULL); + TabStripModel tabstrip(&delegate, profile()); + EXPECT_TRUE(tabstrip.empty()); + + // Create three tabs, select last two tabs, CommandCloseTabsToRight should + // only be enabled for the first tab. + ASSERT_NO_FATAL_FAILURE( + PrepareTabstripForSelectionTest(&tabstrip, 3, 0, "1 2")); + EXPECT_TRUE(tabstrip.IsContextMenuCommandEnabled( + 0, TabStripModel::CommandCloseTabsToRight)); + EXPECT_FALSE(tabstrip.IsContextMenuCommandEnabled( + 1, TabStripModel::CommandCloseTabsToRight)); + EXPECT_FALSE(tabstrip.IsContextMenuCommandEnabled( + 2, TabStripModel::CommandCloseTabsToRight)); + tabstrip.ExecuteContextMenuCommand(0, TabStripModel::CommandCloseTabsToRight); + EXPECT_EQ("0", GetPinnedState(tabstrip)); + tabstrip.CloseAllTabs(); + EXPECT_TRUE(tabstrip.empty()); +} + +// Tests IsContextMenuCommandEnabled and ExecuteContextMenuCommand with +// CommandTogglePinned. +TEST_F(TabStripModelTest, CommandTogglePinned) { + TabStripDummyDelegate delegate(NULL); + TabStripModel tabstrip(&delegate, profile()); + EXPECT_TRUE(tabstrip.empty()); + + // Create three tabs with one pinned, pin the first two. + ASSERT_NO_FATAL_FAILURE( + PrepareTabstripForSelectionTest(&tabstrip, 3, 1, "0 1")); + EXPECT_TRUE(tabstrip.IsContextMenuCommandEnabled( + 0, TabStripModel::CommandTogglePinned)); + EXPECT_TRUE(tabstrip.IsContextMenuCommandEnabled( + 1, TabStripModel::CommandTogglePinned)); + EXPECT_TRUE(tabstrip.IsContextMenuCommandEnabled( + 2, TabStripModel::CommandTogglePinned)); + tabstrip.ExecuteContextMenuCommand(0, TabStripModel::CommandTogglePinned); + EXPECT_EQ("0p 1p 2", GetPinnedState(tabstrip)); + + // Execute CommandTogglePinned again, this should unpin. + tabstrip.ExecuteContextMenuCommand(0, TabStripModel::CommandTogglePinned); + EXPECT_EQ("0 1 2", GetPinnedState(tabstrip)); + + // Pin the last. + tabstrip.ExecuteContextMenuCommand(2, TabStripModel::CommandTogglePinned); + EXPECT_EQ("2p 0 1", GetPinnedState(tabstrip)); + + tabstrip.CloseAllTabs(); + EXPECT_TRUE(tabstrip.empty()); +} + // Tests the following context menu commands: // - Close Tab // - Close Other Tabs @@ -1051,13 +1222,13 @@ TEST_F(TabStripModelTest, AddTabContents_MiddleClickLinksAndClose) { // TabContents in the group before closing the opener or any other // TabContents. tabstrip.SelectTabContentsAt(2, true); - tabstrip.CloseSelectedTab(); + tabstrip.CloseSelectedTabs(); EXPECT_EQ(middle_click_contents3, tabstrip.GetSelectedTabContents()); - tabstrip.CloseSelectedTab(); + tabstrip.CloseSelectedTabs(); EXPECT_EQ(middle_click_contents1, tabstrip.GetSelectedTabContents()); - tabstrip.CloseSelectedTab(); + tabstrip.CloseSelectedTabs(); EXPECT_EQ(homepage_contents, tabstrip.GetSelectedTabContents()); - tabstrip.CloseSelectedTab(); + tabstrip.CloseSelectedTabs(); EXPECT_EQ(typed_page_contents, tabstrip.GetSelectedTabContents()); EXPECT_EQ(1, tabstrip.count()); @@ -1106,7 +1277,7 @@ TEST_F(TabStripModelTest, AddTabContents_LeftClickPopup) { // After closing the selected tab, the selection should move to the left, to // the opener. - tabstrip.CloseSelectedTab(); + tabstrip.CloseSelectedTabs(); EXPECT_EQ(homepage_contents, tabstrip.GetSelectedTabContents()); EXPECT_EQ(2, tabstrip.count()); @@ -1222,13 +1393,13 @@ TEST_F(TabStripModelTest, AddTabContents_ForgetOpeners) { // Now test that closing tabs selects to the right until there are no more, // then to the left, as if there were no context (context has been // successfully forgotten). - tabstrip.CloseSelectedTab(); + tabstrip.CloseSelectedTabs(); EXPECT_EQ(middle_click_contents3, tabstrip.GetSelectedTabContents()); - tabstrip.CloseSelectedTab(); + tabstrip.CloseSelectedTabs(); EXPECT_EQ(typed_page_contents, tabstrip.GetSelectedTabContents()); - tabstrip.CloseSelectedTab(); + tabstrip.CloseSelectedTabs(); EXPECT_EQ(middle_click_contents1, tabstrip.GetSelectedTabContents()); - tabstrip.CloseSelectedTab(); + tabstrip.CloseSelectedTabs(); EXPECT_EQ(homepage_contents, tabstrip.GetSelectedTabContents()); EXPECT_EQ(1, tabstrip.count()); @@ -1986,3 +2157,19 @@ TEST_F(TabStripModelTest, MoveSelectedTabsTo) { strip.CloseAllTabs(); } } + +TEST_F(TabStripModelTest, CloseSelectedTabs) { + TabStripDummyDelegate delegate(NULL); + TabStripModel strip(&delegate, profile()); + TabContentsWrapper* contents1 = CreateTabContents(); + TabContentsWrapper* contents2 = CreateTabContents(); + TabContentsWrapper* contents3 = CreateTabContents(); + strip.AppendTabContents(contents1, true); + strip.AppendTabContents(contents2, true); + strip.AppendTabContents(contents3, true); + strip.ToggleSelectionAt(1); + strip.CloseSelectedTabs(); + EXPECT_EQ(1, strip.count()); + EXPECT_EQ(0, strip.selected_index()); + strip.CloseAllTabs(); +} diff --git a/chrome/browser/tabs/tab_strip_selection_model.cc b/chrome/browser/tabs/tab_strip_selection_model.cc index b02152f..0437e64 100644 --- a/chrome/browser/tabs/tab_strip_selection_model.cc +++ b/chrome/browser/tabs/tab_strip_selection_model.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2010 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -62,7 +62,7 @@ void TabStripSelectionModel::SetSelectedIndex(int index) { SetSelectionFromAnchorTo(index); } -bool TabStripSelectionModel::IsSelected(int index) { +bool TabStripSelectionModel::IsSelected(int index) const { return std::find(selected_indices_.begin(), selected_indices_.end(), index) != selected_indices_.end(); } diff --git a/chrome/browser/tabs/tab_strip_selection_model.h b/chrome/browser/tabs/tab_strip_selection_model.h index 5963d2a..cb41f74 100644 --- a/chrome/browser/tabs/tab_strip_selection_model.h +++ b/chrome/browser/tabs/tab_strip_selection_model.h @@ -58,7 +58,7 @@ class TabStripSelectionModel { void SetSelectedIndex(int index); // Returns true if |index| is selected. - bool IsSelected(int index); + bool IsSelected(int index) const; // Adds |index| to the selection. This does not change the active or anchor // indices. diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc index cce8fb0..1a71300 100644 --- a/chrome/browser/ui/browser.cc +++ b/chrome/browser/ui/browser.cc @@ -1413,12 +1413,8 @@ void Browser::NewTab() { void Browser::CloseTab() { UserMetrics::RecordAction(UserMetricsAction("CloseTab_Accelerator"), profile_); - if (CanCloseTab()) { - tab_handler_->GetTabStripModel()->CloseTabContentsAt( - tab_handler_->GetTabStripModel()->selected_index(), - TabStripModel::CLOSE_USER_GESTURE | - TabStripModel::CLOSE_CREATE_HISTORICAL_TAB); - } + if (CanCloseTab()) + tab_handler_->GetTabStripModel()->CloseSelectedTabs(); } void Browser::SelectNextTab() { diff --git a/chrome/browser/ui/tabs/tab_menu_model.cc b/chrome/browser/ui/tabs/tab_menu_model.cc index 907969d..38b1e78 100644 --- a/chrome/browser/ui/tabs/tab_menu_model.cc +++ b/chrome/browser/ui/tabs/tab_menu_model.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2010 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -15,6 +15,13 @@ TabMenuModel::TabMenuModel(ui::SimpleMenuModel::Delegate* delegate, Build(is_pinned); } +TabMenuModel::TabMenuModel(ui::SimpleMenuModel::Delegate* delegate, + TabStripModel* tab_strip, + int index) + : ui::SimpleMenuModel(delegate) { + Build(tab_strip, index); +} + // static bool TabMenuModel::AreVerticalTabsEnabled() { #if defined(TOOLKIT_VIEWS) || defined(OS_MACOSX) || defined(OS_CHROMEOS) @@ -51,3 +58,45 @@ void TabMenuModel::Build(bool is_pinned) { IDS_TAB_CXMENU_USE_VERTICAL_TABS); } } + +void TabMenuModel::Build(TabStripModel* tab_strip, int index) { + bool effects_multiple_tabs = + (tab_strip->IsTabSelected(index) && + tab_strip->selection_model().selected_indices().size() > 1); + AddItemWithStringId(TabStripModel::CommandNewTab, IDS_TAB_CXMENU_NEWTAB); + AddSeparator(); + AddItemWithStringId(TabStripModel::CommandReload, IDS_TAB_CXMENU_RELOAD); + AddItemWithStringId(TabStripModel::CommandDuplicate, + IDS_TAB_CXMENU_DUPLICATE); + bool will_pin = tab_strip->WillContextMenuPin(index); + if (effects_multiple_tabs) { + AddItemWithStringId( + TabStripModel::CommandTogglePinned, + will_pin ? IDS_TAB_CXMENU_PIN_TABS : IDS_TAB_CXMENU_UNPIN_TABS); + } else { + AddItemWithStringId( + TabStripModel::CommandTogglePinned, + will_pin ? IDS_TAB_CXMENU_PIN_TAB : IDS_TAB_CXMENU_UNPIN_TAB); + } + AddSeparator(); + if (effects_multiple_tabs) { + AddItemWithStringId(TabStripModel::CommandCloseTab, + IDS_TAB_CXMENU_CLOSETABS); + } else { + AddItemWithStringId(TabStripModel::CommandCloseTab, + IDS_TAB_CXMENU_CLOSETAB); + } + AddItemWithStringId(TabStripModel::CommandCloseOtherTabs, + IDS_TAB_CXMENU_CLOSEOTHERTABS); + AddItemWithStringId(TabStripModel::CommandCloseTabsToRight, + IDS_TAB_CXMENU_CLOSETABSTORIGHT); + AddSeparator(); + AddItemWithStringId(TabStripModel::CommandRestoreTab, IDS_RESTORE_TAB); + AddItemWithStringId(TabStripModel::CommandBookmarkAllTabs, + IDS_TAB_CXMENU_BOOKMARK_ALL_TABS); + if (AreVerticalTabsEnabled()) { + AddSeparator(); + AddCheckItemWithStringId(TabStripModel::CommandUseVerticalTabs, + IDS_TAB_CXMENU_USE_VERTICAL_TABS); + } +} diff --git a/chrome/browser/ui/tabs/tab_menu_model.h b/chrome/browser/ui/tabs/tab_menu_model.h index 61a8e34..7d4d952 100644 --- a/chrome/browser/ui/tabs/tab_menu_model.h +++ b/chrome/browser/ui/tabs/tab_menu_model.h @@ -1,4 +1,4 @@ -// Copyright (c) 2010 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -8,20 +8,27 @@ #include "ui/base/models/simple_menu_model.h" -// A menu model that builds the contents of the tab context menu. This menu has -// only one level (no submenus). TabMenuModel caches local state from the -// tab (such as the pinned state). To make sure the menu reflects the real state -// of the tab a new TabMenuModel should be created each time the menu is shown. +class TabStripModel; + +// A menu model that builds the contents of the tab context menu. To make sure +// the menu reflects the real state of the tab a new TabMenuModel should be +// created each time the menu is shown. class TabMenuModel : public ui::SimpleMenuModel { public: + // TODO: nuke this constructor when callers are updated. TabMenuModel(ui::SimpleMenuModel::Delegate* delegate, bool is_pinned); + TabMenuModel(ui::SimpleMenuModel::Delegate* delegate, + TabStripModel* tab_strip, + int index); virtual ~TabMenuModel() {} // Returns true if vertical tabs are enabled. static bool AreVerticalTabsEnabled(); private: + // TODO: nuke this when first constructor is removed. void Build(bool is_pinned); + void Build(TabStripModel* tab_strip, int index); DISALLOW_COPY_AND_ASSIGN(TabMenuModel); }; diff --git a/chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc b/chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc index 988b582..a662e63 100644 --- a/chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc +++ b/chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc @@ -36,7 +36,9 @@ class BrowserTabStripController::TabContextMenuContents TabContextMenuContents(BaseTab* tab, BrowserTabStripController* controller) : ALLOW_THIS_IN_INITIALIZER_LIST( - model_(this, controller->IsTabPinned(tab))), + model_(this, + controller->model_, + controller->tabstrip_->GetModelIndexOfBaseTab(tab))), tab_(tab), controller_(controller), last_command_(TabStripModel::CommandFirst) { -- cgit v1.1