diff options
author | andybons@chromium.org <andybons@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-10-02 17:29:43 +0000 |
---|---|---|
committer | andybons@chromium.org <andybons@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-10-02 17:29:43 +0000 |
commit | 8a34e660e7b6627b71a3b24e6800090f239075bb (patch) | |
tree | e76c3a12d82cd6e6d8dc467596316f0666fd1296 /chrome/browser/tabs | |
parent | 0b4158e6bc07aea93651c05c3bf26c2152bd9842 (diff) | |
download | chromium_src-8a34e660e7b6627b71a3b24e6800090f239075bb.zip chromium_src-8a34e660e7b6627b71a3b24e6800090f239075bb.tar.gz chromium_src-8a34e660e7b6627b71a3b24e6800090f239075bb.tar.bz2 |
Rip out phantom tabs and corresponding unit tests.
Also get rid of the type enum passed to TabReplacedAt since it can only be one option after phantom tabs are removed.
BUG=none
TEST=compile? good. pass tests? good.
Review URL: http://codereview.chromium.org/3539010
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@61303 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome/browser/tabs')
-rw-r--r-- | chrome/browser/tabs/default_tab_handler.cc | 4 | ||||
-rw-r--r-- | chrome/browser/tabs/pinned_tab_codec.cc | 3 | ||||
-rw-r--r-- | chrome/browser/tabs/tab_strip_model.cc | 166 | ||||
-rw-r--r-- | chrome/browser/tabs/tab_strip_model.h | 59 | ||||
-rw-r--r-- | chrome/browser/tabs/tab_strip_model_observer.cc | 7 | ||||
-rw-r--r-- | chrome/browser/tabs/tab_strip_model_observer.h | 19 | ||||
-rw-r--r-- | chrome/browser/tabs/tab_strip_model_order_controller.cc | 19 | ||||
-rw-r--r-- | chrome/browser/tabs/tab_strip_model_order_controller.h | 14 | ||||
-rw-r--r-- | chrome/browser/tabs/tab_strip_model_unittest.cc | 171 |
9 files changed, 41 insertions, 421 deletions
diff --git a/chrome/browser/tabs/default_tab_handler.cc b/chrome/browser/tabs/default_tab_handler.cc index 5149173..dc3598b 100644 --- a/chrome/browser/tabs/default_tab_handler.cc +++ b/chrome/browser/tabs/default_tab_handler.cc @@ -18,8 +18,8 @@ DefaultTabHandler::DefaultTabHandler(TabHandlerDelegate* delegate) } DefaultTabHandler::~DefaultTabHandler() { - // The tab strip should not have any significant tabs at this point. - DCHECK(!model_->HasNonPhantomTabs()); + // The tab strip should not have any tabs at this point. + DCHECK(model_->empty()); model_->RemoveObserver(this); } diff --git a/chrome/browser/tabs/pinned_tab_codec.cc b/chrome/browser/tabs/pinned_tab_codec.cc index 6920bfa..7b9e0b3 100644 --- a/chrome/browser/tabs/pinned_tab_codec.cc +++ b/chrome/browser/tabs/pinned_tab_codec.cc @@ -46,10 +46,9 @@ static void EncodePinnedTab(TabStripModel* model, Extension* extension = tab_contents->extension_app(); DCHECK(extension); value->SetString(kAppID, extension->id()); - // For apps we use the launch url. We do this for two reasons: + // For apps we use the launch url. We do this for the following reason: // . the user is effectively restarting the app, so that returning them to // the app's launch page seems closest to what they expect. - // . we do the same when restoring a phantom tab. value->SetString(kURL, extension->GetFullLaunchURL().spec()); values->Append(value.release()); } else { diff --git a/chrome/browser/tabs/tab_strip_model.cc b/chrome/browser/tabs/tab_strip_model.cc index f27b5a8..1f7a668 100644 --- a/chrome/browser/tabs/tab_strip_model.cc +++ b/chrome/browser/tabs/tab_strip_model.cc @@ -75,17 +75,6 @@ TabStripModel::TabStripModel(TabStripModelDelegate* delegate, Profile* profile) TabStripModel::~TabStripModel() { FOR_EACH_OBSERVER(TabStripModelObserver, observers_, TabStripModelDeleted()); - - // Before deleting any phantom tabs remove our notification observers so that - // we don't attempt to notify our delegate or do any processing. - registrar_.RemoveAll(); - - // Phantom tabs still have valid TabConents that we own and need to delete. - for (int i = count() - 1; i >= 0; --i) { - if (IsPhantomTab(i)) - delete contents_data_[i]->contents; - } - STLDeleteContainerPointers(contents_data_.begin(), contents_data_.end()); delete order_controller_; } @@ -98,14 +87,6 @@ void TabStripModel::RemoveObserver(TabStripModelObserver* observer) { observers_.RemoveObserver(observer); } -bool TabStripModel::HasNonPhantomTabs() const { - for (int i = 0; i < count(); i++) { - if (!IsPhantomTab(i)) - return true; - } - return false; -} - void TabStripModel::SetInsertionPolicy(InsertionPolicy policy) { order_controller_->set_insertion_policy(policy); } @@ -181,12 +162,9 @@ void TabStripModel::InsertTabContentsAt(int index, ChangeSelectedContentsFrom(selected_contents, index, false); } -void TabStripModel::ReplaceTabContentsAt( - int index, - TabContents* new_contents, - TabStripModelObserver::TabReplaceType type) { +void TabStripModel::ReplaceTabContentsAt(int index, TabContents* new_contents) { scoped_ptr<TabContents> old_contents( - ReplaceTabContentsAtImpl(index, new_contents, type)); + ReplaceTabContentsAtImpl(index, new_contents)); // When the selected tab contents is replaced send out selected notification // too. We do this as nearly all observers need to treat a replace of the @@ -218,20 +196,17 @@ TabContents* TabStripModel::DetachTabContentsAt(int index) { DCHECK(ContainsIndex(index)); TabContents* removed_contents = GetContentsAt(index); - int next_selected_index = - order_controller_->DetermineNewSelectedIndex(index, true); + int next_selected_index = order_controller_->DetermineNewSelectedIndex(index); delete contents_data_.at(index); contents_data_.erase(contents_data_.begin() + index); - next_selected_index = IndexOfNextNonPhantomTab(next_selected_index, -1); - if (!HasNonPhantomTabs()) + if (empty()) closing_all_ = true; FOR_EACH_OBSERVER(TabStripModelObserver, observers_, TabDetachedAt(removed_contents, index)); - if (!HasNonPhantomTabs()) { + if (empty()) { // TabDetachedAt() might unregister observers, so send |TabStripEmtpy()| in // a second pass. - FOR_EACH_OBSERVER(TabStripModelObserver, observers_, - TabStripEmpty()); + FOR_EACH_OBSERVER(TabStripModelObserver, observers_, TabStripEmpty()); } else { if (index == selected_index_) { ChangeSelectedContentsFrom(removed_contents, next_selected_index, false); @@ -343,17 +318,13 @@ int TabStripModel::GetIndexOfNextTabContentsOpenedBy( // Check tabs after start_index first. for (int i = start_index + 1; i < count(); ++i) { - if (OpenerMatches(contents_data_[i], opener, use_group) && - !IsPhantomTab(i)) { + if (OpenerMatches(contents_data_[i], opener, use_group)) return i; - } } // Then check tabs before start_index, iterating backwards. for (int i = start_index - 1; i >= 0; --i) { - if (OpenerMatches(contents_data_[i], opener, use_group) && - !IsPhantomTab(i)) { + if (OpenerMatches(contents_data_[i], opener, use_group)) return i; - } } return kNoTab; } @@ -365,7 +336,7 @@ int TabStripModel::GetIndexOfFirstTabContentsOpenedBy( DCHECK(ContainsIndex(start_index)); for (int i = 0; i < start_index; ++i) { - if (contents_data_[i]->opener == opener && !IsPhantomTab(i)) + if (contents_data_[i]->opener == opener) return i; } return kNoTab; @@ -384,10 +355,8 @@ int TabStripModel::GetIndexOfLastTabContentsOpenedBy( next = iter - 1; if (next == end) break; - if ((*next)->opener == opener && - !IsPhantomTab(static_cast<int>(next - contents_data_.begin()))) { + if ((*next)->opener == opener) return static_cast<int>(next - contents_data_.begin()); - } } return kNoTab; } @@ -497,11 +466,6 @@ bool TabStripModel::IsAppTab(int index) const { return contents && contents->is_app(); } -bool TabStripModel::IsPhantomTab(int index) const { - return IsTabPinned(index) && - GetTabContentsAt(index)->controller().needs_reload(); -} - bool TabStripModel::IsTabBlocked(int index) const { return contents_data_[index]->blocked; } @@ -520,23 +484,6 @@ int TabStripModel::ConstrainInsertionIndex(int index, bool mini_tab) { std::min(count(), std::max(index, IndexOfFirstNonMiniTab())); } -int TabStripModel::IndexOfFirstNonPhantomTab() const { - for (int i = 0; i < count(); ++i) { - if (!IsPhantomTab(i)) - return i; - } - return kNoTab; -} - -int TabStripModel::GetNonPhantomTabCount() const { - int tabs = 0; - for (int i = 0; i < count(); ++i) { - if (!IsPhantomTab(i)) - ++tabs; - } - return tabs; -} - void TabStripModel::AddTabContents(TabContents* contents, int index, PageTransition::Type transition, @@ -734,15 +681,8 @@ void TabStripModel::ExecuteContextMenuCommand( UserMetrics::RecordAction( UserMetricsAction("TabContextMenu_TogglePinned"), profile_); - - if (IsPhantomTab(context_index)) { - // The tab is a phantom tab, close it. - CloseTabContentsAt(context_index, - CLOSE_USER_GESTURE | CLOSE_CREATE_HISTORICAL_TAB); - } else { - SelectTabContentsAt(context_index, true); - SetTabPinned(context_index, !IsTabPinned(context_index)); - } + SelectTabContentsAt(context_index, true); + SetTabPinned(context_index, !IsTabPinned(context_index)); break; } @@ -803,13 +743,7 @@ void TabStripModel::Observe(NotificationType type, if (index != TabStripModel::kNoTab) { // Note that we only detach the contents here, not close it - it's // already been closed. We just want to undo our bookkeeping. - if (ShouldMakePhantomOnClose(index)) { - // We don't actually allow pinned tabs to close. Instead they become - // phantom. - MakePhantom(index); - } else { - DetachTabContentsAt(index); - } + DetachTabContentsAt(index); } break; } @@ -960,79 +894,12 @@ void TabStripModel::SelectRelativeTab(bool next) { if (contents_data_.empty()) return; - // Skip pinned-app-phantom tabs when iterating. int index = selected_index_; int delta = next ? 1 : -1; - do { - index = (index + count() + delta) % count(); - } while (index != selected_index_ && IsPhantomTab(index)); + index = (index + count() + delta) % count(); SelectTabContentsAt(index, true); } -int TabStripModel::IndexOfNextNonPhantomTab(int index, - int ignore_index) { - if (index == kNoTab) - return kNoTab; - - if (empty()) - return index; - - index = std::min(count() - 1, std::max(0, index)); - int start = index; - do { - if (index != ignore_index && !IsPhantomTab(index)) - return index; - index = (index + 1) % count(); - } while (index != start); - - // All phantom tabs. - return start; -} - -bool TabStripModel::ShouldMakePhantomOnClose(int index) { - if (browser_defaults::kPhantomTabsEnabled && IsTabPinned(index) && - !IsPhantomTab(index) && !closing_all_ && profile()) { - if (!IsAppTab(index)) - return true; // Always make non-app tabs go phantom. - - ExtensionsService* extension_service = profile()->GetExtensionsService(); - if (!extension_service) - return false; - - Extension* extension_app = GetTabContentsAt(index)->extension_app(); - DCHECK(extension_app); - - // Only allow the tab to be made phantom if the extension still exists. - return extension_service->GetExtensionById(extension_app->id(), - false) != NULL; - } - return false; -} - -void TabStripModel::MakePhantom(int index) { - // MakePhantom is called when the TabContents is being destroyed so we don't - // need to do anything with the returned value from ReplaceTabContentsAtImpl. - ReplaceTabContentsAtImpl(index, GetContentsAt(index)->CloneAndMakePhantom(), - TabStripModelObserver::REPLACE_MADE_PHANTOM); - - if (selected_index_ == index && HasNonPhantomTabs()) { - // Change the selection, otherwise we're going to force the phantom tab - // to become selected. - // NOTE: we must do this after the call to Replace otherwise browser's - // TabSelectedAt will send out updates for the old TabContents which we've - // already told observers has been closed (we sent out TabClosing at). - int new_selected_index = - order_controller_->DetermineNewSelectedIndex(index, false); - new_selected_index = IndexOfNextNonPhantomTab(new_selected_index, - index); - SelectTabContentsAt(new_selected_index, true); - } - - if (!HasNonPhantomTabs()) - FOR_EACH_OBSERVER(TabStripModelObserver, observers_, TabStripEmpty()); -} - - void TabStripModel::MoveTabContentsAtImpl(int index, int to_position, bool select_after_move) { TabContentsData* moved_data = contents_data_.at(index); @@ -1061,8 +928,7 @@ bool TabStripModel::OpenerMatches(const TabContentsData* data, TabContents* TabStripModel::ReplaceTabContentsAtImpl( int index, - TabContents* new_contents, - TabStripModelObserver::TabReplaceType type) { + TabContents* new_contents) { // TODO: this should reset group/opener of any tabs that point at // old_contents. DCHECK(ContainsIndex(index)); @@ -1072,6 +938,6 @@ TabContents* TabStripModel::ReplaceTabContentsAtImpl( contents_data_[index]->contents = new_contents; FOR_EACH_OBSERVER(TabStripModelObserver, observers_, - TabReplacedAt(old_contents, new_contents, index, type)); + TabReplacedAt(old_contents, new_contents, index)); return old_contents; } diff --git a/chrome/browser/tabs/tab_strip_model.h b/chrome/browser/tabs/tab_strip_model.h index 003f3d6..5b58afc 100644 --- a/chrome/browser/tabs/tab_strip_model.h +++ b/chrome/browser/tabs/tab_strip_model.h @@ -41,18 +41,8 @@ class TabStripModelOrderController; // . App. Corresponds to an extension that wants an app tab. App tabs are // identified by TabContents::is_app(). App tabs are always pinneded (you // can't unpin them). -// . Pinned. Any tab can be pinned. A pinned tab is made phantom when closed. -// Non-app tabs whose pinned state is changed are moved to be with other -// mini-tabs or non-mini tabs. -// . Phantom. Only pinned tabs may be made phantom. When a tab that can be made -// phantom is closed the renderer is shutdown, a new -// TabContents/NavigationController is created that has not yet loaded the -// renderer and observers are notified via the TabReplacedAt method. When a -// phantom tab is selected the renderer is loaded and the tab is no longer -// phantom. -// Phantom tabs do not prevent the tabstrip from closing, for example if the -// tabstrip has one phantom and one non-phantom tab and the non-phantom tab is -// closed, then the tabstrip/browser are closed. +// . Pinned. Any tab can be pinned. Non-app tabs whose pinned state is changed +// are moved to be with other mini-tabs or non-mini tabs. // // A TabStripModel has one delegate that it relies on to perform certain tasks // like creating new TabStripModels (probably hosted in Browser windows) when @@ -137,11 +127,6 @@ class TabStripModel : public NotificationObserver { int count() const { return static_cast<int>(contents_data_.size()); } bool empty() const { return contents_data_.empty(); } - // Returns true if there are any non-phantom tabs. When there are no - // non-phantom tabs the delegate is notified by way of TabStripEmpty and the - // browser closes. - bool HasNonPhantomTabs() const; - // Retrieve the Profile associated with this TabStripModel. Profile* profile() const { return profile_; } @@ -208,11 +193,9 @@ class TabStripModel : public NotificationObserver { void ReplaceNavigationControllerAt(int index, NavigationController* controller); - // Replaces the tab contents at |index| with |new_contents|. |type| is passed - // to the observer. This deletes the TabContents currently at |index|. - void ReplaceTabContentsAt(int index, - TabContents* new_contents, - TabStripModelObserver::TabReplaceType type); + // Replaces the tab contents at |index| with |new_contents|. This deletes the + // TabContents currently at |index|. + void ReplaceTabContentsAt(int index, TabContents* new_contents); // Detaches the TabContents at the specified index from this strip. The // TabContents is not destroyed, just removed from display. The caller is @@ -277,20 +260,17 @@ class TabStripModel : public NotificationObserver { // If |use_group| is true, the group property of the tab is used instead of // the opener to find the next tab. Under some circumstances the group // relationship may exist but the opener may not. - // NOTE: this skips phantom tabs. int GetIndexOfNextTabContentsOpenedBy(const NavigationController* opener, int start_index, bool use_group) const; // Returns the index of the first TabContents in the model opened by the // specified opener. - // NOTE: this skips phantom tabs. int GetIndexOfFirstTabContentsOpenedBy(const NavigationController* opener, int start_index) const; // Returns the index of the last TabContents in the model opened by the // specified opener, starting at |start_index|. - // NOTE: this skips phantom tabs. int GetIndexOfLastTabContentsOpenedBy(const NavigationController* opener, int start_index) const; @@ -337,11 +317,6 @@ class TabStripModel : public NotificationObserver { // See description above class for details on app tabs. bool IsAppTab(int index) const; - // Returns true if the tab is a phantom tab. A phantom tab is one where the - // renderer has not been loaded. - // See description above class for details on phantom tabs. - bool IsPhantomTab(int index) const; - // Returns true if the tab at |index| is blocked by a tab modal dialog. bool IsTabBlocked(int index) const; @@ -357,14 +332,6 @@ class TabStripModel : public NotificationObserver { // is between IndexOfFirstNonMiniTab and count(). int ConstrainInsertionIndex(int index, bool mini_tab); - // Returns the index of the first tab that is not a phantom tab. This returns - // kNoTab if all of the tabs are phantom tabs. - int IndexOfFirstNonPhantomTab() const; - - // Returns the number of non phantom tabs in the TabStripModel. - int GetNonPhantomTabCount() const; - - // Command level API ///////////////////////////////////////////////////////// // Adds a TabContents at the best position in the TabStripModel given the @@ -481,17 +448,6 @@ class TabStripModel : public NotificationObserver { // (|forward| is false). void SelectRelativeTab(bool forward); - // Returns the first non-phantom tab starting at |index|, skipping the tab at - // |ignore_index|. - int IndexOfNextNonPhantomTab(int index, int ignore_index); - - // Returns true if the tab at the specified index should be made phantom when - // the tab is closing. - bool ShouldMakePhantomOnClose(int index); - - // Makes the tab a phantom tab. - void MakePhantom(int index); - // Does the work of MoveTabContentsAt. This has no checks to make sure the // position is valid, those are done in MoveTabContentsAt. void MoveTabContentsAtImpl(int index, @@ -508,10 +464,7 @@ class TabStripModel : public NotificationObserver { // Does the work for ReplaceTabContentsAt returning the old TabContents. // The caller owns the returned TabContents. - TabContents* ReplaceTabContentsAtImpl( - int index, - TabContents* new_contents, - TabStripModelObserver::TabReplaceType type); + TabContents* ReplaceTabContentsAtImpl(int index, TabContents* new_contents); // Our delegate. TabStripModelDelegate* delegate_; diff --git a/chrome/browser/tabs/tab_strip_model_observer.cc b/chrome/browser/tabs/tab_strip_model_observer.cc index c3bfb9b..fa2fe8b 100644 --- a/chrome/browser/tabs/tab_strip_model_observer.cc +++ b/chrome/browser/tabs/tab_strip_model_observer.cc @@ -38,13 +38,6 @@ void TabStripModelObserver::TabReplacedAt(TabContents* old_contents, int index) { } -void TabStripModelObserver::TabReplacedAt(TabContents* old_contents, - TabContents* new_contents, - int index, - TabReplaceType type) { - TabReplacedAt(old_contents, new_contents, index); -} - void TabStripModelObserver::TabPinnedStateChanged(TabContents* contents, int index) { } diff --git a/chrome/browser/tabs/tab_strip_model_observer.h b/chrome/browser/tabs/tab_strip_model_observer.h index cfbf9e8..931497b 100644 --- a/chrome/browser/tabs/tab_strip_model_observer.h +++ b/chrome/browser/tabs/tab_strip_model_observer.h @@ -37,17 +37,6 @@ class TabStripModelObserver { ALL }; - // Enum used by ReplaceTabContentsAt. - // TODO(sky): nuke this, pinned is being removed so there is no point in the - // enum. - enum TabReplaceType { - // The replace is the result of the tab being made phantom. - REPLACE_MADE_PHANTOM, - - // The replace is the result of the match preview being committed. - REPLACE_MATCH_PREVIEW - }; - // A new TabContents was inserted into the TabStripModel at the specified // index. |foreground| is whether or not it was opened in the foreground // (selected). @@ -100,14 +89,6 @@ class TabStripModelObserver { TabContents* new_contents, int index); - // The tab contents was replaced at the specified index. |type| describes - // the type of replace. - // This invokes TabReplacedAt with three args. - virtual void TabReplacedAt(TabContents* old_contents, - TabContents* new_contents, - int index, - TabReplaceType type); - // Invoked when the pinned state of a tab changes. This is not invoked if the // tab ends up moving as a result of the mini state changing. // See note in TabMiniStateChanged as to how this relates to diff --git a/chrome/browser/tabs/tab_strip_model_order_controller.cc b/chrome/browser/tabs/tab_strip_model_order_controller.cc index a967bd9..aa66920 100644 --- a/chrome/browser/tabs/tab_strip_model_order_controller.cc +++ b/chrome/browser/tabs/tab_strip_model_order_controller.cc @@ -64,8 +64,7 @@ int TabStripModelOrderController::DetermineInsertionIndexForAppending() { } int TabStripModelOrderController::DetermineNewSelectedIndex( - int removing_index, - bool is_remove) const { + int removing_index) const { int tab_count = tabstrip_->count(); DCHECK(removing_index >= 0 && removing_index < tab_count); NavigationController* parent_opener = @@ -79,7 +78,7 @@ int TabStripModelOrderController::DetermineNewSelectedIndex( removing_index, false); if (index != TabStripModel::kNoTab) - return GetValidIndex(index, removing_index, is_remove); + return GetValidIndex(index, removing_index); if (parent_opener) { // If the tab was in a group, shift selection to the next tab in the group. @@ -87,20 +86,21 @@ int TabStripModelOrderController::DetermineNewSelectedIndex( removing_index, false); if (index != TabStripModel::kNoTab) - return GetValidIndex(index, removing_index, is_remove); + return GetValidIndex(index, removing_index); // If we can't find a subsequent group member, just fall back to the // parent_opener itself. Note that we use "group" here since opener is // reset by select operations.. index = tabstrip_->GetIndexOfController(parent_opener); if (index != TabStripModel::kNoTab) - return GetValidIndex(index, removing_index, is_remove); + return GetValidIndex(index, removing_index); } // No opener set, fall through to the default handler... int selected_index = tabstrip_->selected_index(); - if (is_remove && selected_index >= (tab_count - 1)) + if (selected_index >= (tab_count - 1)) return selected_index - 1; + return selected_index; } @@ -132,10 +132,9 @@ void TabStripModelOrderController::TabSelectedAt(TabContents* old_contents, /////////////////////////////////////////////////////////////////////////////// // TabStripModelOrderController, private: -int TabStripModelOrderController::GetValidIndex(int index, - int removing_index, - bool is_remove) const { - if (is_remove && removing_index < index) +int TabStripModelOrderController::GetValidIndex( + int index, int removing_index) const { + if (removing_index < index) index = std::max(0, index - 1); return index; } diff --git a/chrome/browser/tabs/tab_strip_model_order_controller.h b/chrome/browser/tabs/tab_strip_model_order_controller.h index b7b579c..f2c466f 100644 --- a/chrome/browser/tabs/tab_strip_model_order_controller.h +++ b/chrome/browser/tabs/tab_strip_model_order_controller.h @@ -39,11 +39,8 @@ class TabStripModelOrderController : public TabStripModelObserver { // Returns the index to append tabs at. int DetermineInsertionIndexForAppending(); - // Determine where to shift selection after a tab is closed is made phantom. - // If |is_remove| is false, the tab is not being removed but rather made - // phantom (see description of phantom tabs in TabStripModel). - int DetermineNewSelectedIndex(int removed_index, - bool is_remove) const; + // Determine where to shift selection after a tab is closed. + int DetermineNewSelectedIndex(int removed_index) const; // Overridden from TabStripModelObserver: virtual void TabSelectedAt(TabContents* old_contents, @@ -53,10 +50,9 @@ class TabStripModelOrderController : public TabStripModelObserver { private: // Returns a valid index to be selected after the tab at |removing_index| is - // closed. If |index| is after |removing_index| and |is_remove| is true, - // |index| is adjusted to reflect the fact that |removing_index| is going - // away. This also skips any phantom tabs. - int GetValidIndex(int index, int removing_index, bool is_remove) const; + // closed. If |index| is after |removing_index|, |index| is adjusted to + // reflect the fact that |removing_index| is going away. + int GetValidIndex(int index, int removing_index) const; TabStripModel* tabstrip_; diff --git a/chrome/browser/tabs/tab_strip_model_unittest.cc b/chrome/browser/tabs/tab_strip_model_unittest.cc index 9fabf5b..6eee910 100644 --- a/chrome/browser/tabs/tab_strip_model_unittest.cc +++ b/chrome/browser/tabs/tab_strip_model_unittest.cc @@ -163,9 +163,6 @@ class TabStripModelTest : public RenderViewHostTestHarness { if (model.IsTabPinned(i)) actual += "p"; - - if (model.IsPhantomTab(i)) - actual += "h"; } return actual; } @@ -818,7 +815,6 @@ TEST_F(TabStripModelTest, TestSelectOnClose) { EXPECT_EQ(1, tabstrip.selected_index()); tabstrip.CloseTabContentsAt(1, TabStripModel::CLOSE_NONE); EXPECT_EQ(0, tabstrip.selected_index()); - // Finally test that when a tab has no "siblings" that the opener is // selected. TabContents* other_contents = CreateTabContents(); @@ -1780,167 +1776,6 @@ TEST_F(TabStripModelTest, Pinning) { tabstrip.CloseAllTabs(); } -// Tests various permutations of making a tab phantom. -TEST_F(TabStripModelTest, Phantom) { - if (!browser_defaults::kPhantomTabsEnabled) - return; - - TabStripDummyDelegate delegate(NULL); - TabStripModel tabstrip(&delegate, profile()); - MockTabStripModelObserver observer; - tabstrip.AddObserver(&observer); - - EXPECT_TRUE(tabstrip.empty()); - - typedef MockTabStripModelObserver::State State; - - TabContents* contents1 = CreateTabContents(); - TabContents* contents2 = CreateTabContents(); - TabContents* contents3 = CreateTabContents(); - - SetID(contents1, 1); - SetID(contents2, 2); - SetID(contents3, 3); - - // Note! The ordering of these tests is important, each subsequent test - // builds on the state established in the previous. This is important if you - // ever insert tests rather than append. - - // Initial state, three tabs, first selected. - tabstrip.AppendTabContents(contents1, true); - tabstrip.AppendTabContents(contents2, false); - tabstrip.AppendTabContents(contents3, false); - - observer.ClearStates(); - - // Pin the first tab, and make it phantom. - { - tabstrip.SetTabPinned(0, true); - - observer.ClearStates(); - - tabstrip.CloseTabContentsAt(0, TabStripModel::CLOSE_NONE); - - // The tabcontents should have changed. - TabContents* old_contents1 = contents1; - TabContents* new_contents1 = tabstrip.GetTabContentsAt(0); - ASSERT_TRUE(new_contents1 != contents1); - contents1 = new_contents1; - SetID(contents1, 1); - - // Verify the state. - EXPECT_EQ("1ph 2 3", GetPinnedState(tabstrip)); - - // We should have gotten notification of the following: - // . tab closing. - // . selection changed. - // . tab replaced. - ASSERT_EQ(3, observer.GetStateCount()); - State state(old_contents1, 0, MockTabStripModelObserver::CLOSE); - EXPECT_TRUE(observer.StateEquals(0, state)); - state = State(contents1, 0, MockTabStripModelObserver::REPLACED); - state.src_contents = old_contents1; - EXPECT_TRUE(observer.StateEquals(1, state)); - state = State(contents2, 1, MockTabStripModelObserver::SELECT); - state.src_contents = contents1; - state.user_gesture = true; - EXPECT_TRUE(observer.StateEquals(2, state)); - - observer.ClearStates(); - } - - { - tabstrip.SetTabPinned(1, true); - observer.ClearStates(); - - // Close the second tab, which should make it phantom. - tabstrip.CloseTabContentsAt(1, TabStripModel::CLOSE_NONE); - - // The tabcontents should have changed. - TabContents* new_contents2 = tabstrip.GetTabContentsAt(1); - ASSERT_TRUE(new_contents2 != contents2); - contents2 = new_contents2; - SetID(contents2, 2); - - EXPECT_EQ("1ph 2ph 3", GetPinnedState(tabstrip)); - - EXPECT_EQ(2, tabstrip.selected_index()); - - contents2 = tabstrip.GetTabContentsAt(1); - - observer.ClearStates(); - } - - { - tabstrip.SetTabPinned(2, true); - observer.ClearStates(); - - // Close the last tab, we should get a tabstrip empty notification. - tabstrip.CloseTabContentsAt(2, TabStripModel::CLOSE_NONE); - - // The tabcontents should have changed. - TabContents* old_contents3 = contents3; - TabContents* new_contents3 = tabstrip.GetTabContentsAt(2); - ASSERT_TRUE(new_contents3 != contents3); - contents3 = new_contents3; - SetID(contents3, 3); - - EXPECT_EQ("1ph 2ph 3ph", GetPinnedState(tabstrip)); - - // We should have gotten notification of the following: - // . tab closing. - // . tab replaced. - // . tabstrip empty. - ASSERT_EQ(2, observer.GetStateCount()); - State state(old_contents3, 2, MockTabStripModelObserver::CLOSE); - EXPECT_TRUE(observer.StateEquals(0, state)); - state = State(contents3, 2, MockTabStripModelObserver::REPLACED); - state.src_contents = old_contents3; - EXPECT_TRUE(observer.StateEquals(1, state)); - EXPECT_TRUE(observer.empty()); - - observer.ClearStates(); - } - - // Clean up the phantom tabs. - tabstrip.CloseAllTabs(); -} - -// Makes sure the TabStripModel deletes phantom TabContents from its destructor. -TEST_F(TabStripModelTest, DeletePhantomTabContents) { - if (!browser_defaults::kPhantomTabsEnabled) - return; - - TabStripDummyDelegate delegate(NULL); - scoped_ptr<TabStripModel> strip(new TabStripModel(&delegate, profile())); - - strip->AddTabContents(CreateTabContents(), -1, PageTransition::TYPED, - TabStripModel::ADD_PINNED); - - // This will make the tab go phantom. - delete strip->GetTabContentsAt(0); - - ASSERT_TRUE(strip->IsPhantomTab(0)); - - NotificationRegistrar registrar; - NotificationObserverMock notification_observer; - MockTabStripModelObserver tabstrip_observer; - registrar.Add(¬ification_observer, - NotificationType::TAB_CONTENTS_DESTROYED, - NotificationService::AllSources()); - strip->AddObserver(&tabstrip_observer); - - // Deleting the strip should delete the phanton tabcontents and result in one - // TAB_CONTENTS_DESTROYED. - EXPECT_CALL(notification_observer, Observe(_, _, _)).Times(1); - - // Delete the tabstrip. - strip.reset(); - - // And there should have been no methods sent to the TabStripModelObserver. - EXPECT_EQ(0, tabstrip_observer.GetStateCount()); -} - // Makes sure the TabStripModel calls the right observer methods during a // replace. TEST_F(TabStripModelTest, ReplaceSendsSelected) { @@ -1957,8 +1792,7 @@ TEST_F(TabStripModelTest, ReplaceSendsSelected) { strip.AddObserver(&tabstrip_observer); TabContents* new_contents = CreateTabContents(); - strip.ReplaceTabContentsAt(0, new_contents, - TabStripModelObserver::REPLACE_MATCH_PREVIEW); + strip.ReplaceTabContentsAt(0, new_contents); ASSERT_EQ(2, tabstrip_observer.GetStateCount()); @@ -1982,8 +1816,7 @@ TEST_F(TabStripModelTest, ReplaceSendsSelected) { // And replace it. new_contents = CreateTabContents(); - strip.ReplaceTabContentsAt(1, new_contents, - TabStripModelObserver::REPLACE_MATCH_PREVIEW); + strip.ReplaceTabContentsAt(1, new_contents); ASSERT_EQ(1, tabstrip_observer.GetStateCount()); |