Move opener/group relationship forgetting on navigation into TabStripModel from browser, and write unit tests for it.

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

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

4 files changed, 166 insertions, 19 deletions

diff --git a/chrome/browser/tabs/tab_strip_model.cc b/chrome/browser/tabs/tab_strip_model.cc
index 327047e..46d56fb 100644
--- a/chrome/browser/tabs/tab_strip_model.cc
+++ b/chrome/browser/tabs/tab_strip_model.cc
@@ -20,6 +20,22 @@
 #include "chrome/common/notification_service.h"
 #include "chrome/common/stl_util-inl.h"
 
+namespace {
+
+// Returns true if the specified transition is one of the types that cause the
+// opener relationships for the tab in which the transition occured to be
+// forgotten. This is generally any navigation that isn't a link click (i.e.
+// any navigation that can be considered to be the start of a new task distinct
+// from what had previously occurred in that tab).
+bool ShouldForgetOpenersForTransition(PageTransition::Type transition) {
+  return transition == PageTransition::TYPED ||
+      transition == PageTransition::AUTO_BOOKMARK ||
+      transition == PageTransition::GENERATED ||
+      transition == PageTransition::START_PAGE;
+}
+
+}  // namespace
+
 ///////////////////////////////////////////////////////////////////////////////
 // TabStripModel, public:
 
@@ -33,7 +49,7 @@ TabStripModel::TabStripModel(TabStripModelDelegate* delegate, Profile* profile)
   NotificationService::current()->AddObserver(this,
       NotificationType::TAB_CONTENTS_DESTROYED,
       NotificationService::AllSources());
-  SetOrderController(new TabStripModelOrderController(this));
+  order_controller_ = new TabStripModelOrderController(this);
 }
 
 TabStripModel::~TabStripModel() {
@@ -52,13 +68,6 @@ void TabStripModel::RemoveObserver(TabStripModelObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
-void TabStripModel::SetOrderController(
-    TabStripModelOrderController* order_controller) {
-  if (order_controller_)
-    delete order_controller_;
-  order_controller_ = order_controller;
-}
-
 bool TabStripModel::ContainsIndex(int index) const {
   return index >= 0 && index < count();
 }
@@ -287,6 +296,28 @@ int TabStripModel::GetIndexOfLastTabContentsOpenedBy(
   return kNoTab;
 }
 
+void TabStripModel::TabNavigating(TabContents* contents,
+                                  PageTransition::Type transition) {
+  if (ShouldForgetOpenersForTransition(transition)) {
+    // Don't forget the openers if this tab is a New Tab page opened at the
+    // end of the TabStrip (e.g. by pressing Ctrl+T). Give the user one
+    // navigation of one of these transition types before resetting the
+    // opener relationships (this allows for the use case of opening a new
+    // tab to do a quick look-up of something while viewing a tab earlier in
+    // the strip). We can make this heuristic more permissive if need be.
+    if (!IsNewTabAtEndOfTabStrip(contents)) {
+      // If the user navigates the current tab to another page in any way
+      // other than by clicking a link, we want to pro-actively forget all
+      // TabStrip opener relationships since we assume they're beginning a
+      // different task by reusing the current tab.
+      ForgetAllOpeners();
+      // In this specific case we also want to reset the group relationship,
+      // since it is now technically invalid.
+      ForgetGroup(contents);
+    }
+  }
+}
+
 void TabStripModel::ForgetAllOpeners() {
   // Forget all opener memories so we don't do anything weird with tab
   // re-selection ordering.
@@ -500,6 +531,12 @@ void TabStripModel::Observe(NotificationType type,
 ///////////////////////////////////////////////////////////////////////////////
 // TabStripModel, private:
 
+bool TabStripModel::IsNewTabAtEndOfTabStrip(TabContents* contents) const {
+  return contents->type() == TAB_CONTENTS_NEW_TAB_UI &&
+      contents == GetContentsAt(count() - 1) &&
+      contents->controller()->GetEntryCount() == 1;
+}
+
 bool TabStripModel::InternalCloseTabContentsAt(int index,
                                                bool create_historical_tab) {
   TabContents* detached_contents = GetContentsAt(index);
diff --git a/chrome/browser/tabs/tab_strip_model.h b/chrome/browser/tabs/tab_strip_model.h
index 9183b1b..b12ff80 100644
--- a/chrome/browser/tabs/tab_strip_model.h
+++ b/chrome/browser/tabs/tab_strip_model.h
@@ -185,13 +185,6 @@ class TabStripModel : public NotificationObserver {
   // Retrieve the Profile associated with this TabStripModel.
   Profile* profile() const { return profile_; }
 
-  // Retrieve/set the active TabStripModelOrderController associated with this
-  // TabStripModel
-  TabStripModelOrderController* order_controller() const {
-    return order_controller_;
-  }
-  void SetOrderController(TabStripModelOrderController* order_controller);
-
   // Retrieve the index of the currently selected TabContents.
   int selected_index() const { return selected_index_; }
 
@@ -204,6 +197,11 @@ class TabStripModel : public NotificationObserver {
   // avoid doing meaningless or unhelpful work.
   bool closing_all() const { return closing_all_; }
 
+  // Access the order controller. Exposed only for unit tests.
+  TabStripModelOrderController* order_controller() const {
+    return order_controller_;
+  }
+
   // Basic API /////////////////////////////////////////////////////////////////
 
   static const int kNoTab = -1;
@@ -312,6 +310,12 @@ class TabStripModel : public NotificationObserver {
   int GetIndexOfLastTabContentsOpenedBy(NavigationController* opener,
                                         int start_index);
 
+  // Called by the Browser when a navigation is about to occur in the specified
+  // TabContents. Depending on the tab, and the transition type of the
+  // navigation, the TabStripModel may adjust its selection and grouping
+  // behavior.
+  void TabNavigating(TabContents* contents, PageTransition::Type transition);
+
   // Forget all Opener relationships that are stored (but _not_ group
   // relationships!) This is to reduce unpredictable tab switching behavior
   // in complex session states. The exact circumstances under which this method
@@ -398,6 +402,13 @@ class TabStripModel : public NotificationObserver {
   // We cannot be constructed without a delegate.
   TabStripModel();
 
+  // 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
+  // (e.g. Ctrl+T, etc) since the user may open a tab transiently to look up
+  // something related to their current activity.
+  bool IsNewTabAtEndOfTabStrip(TabContents* contents) const;
+
   // Closes the TabContents at the specified index. This causes the TabContents
   // to be destroyed, but it may not happen immediately (e.g. if it's a
   // WebContents). If the page in question has an unload event the TabContents
diff --git a/chrome/browser/tabs/tab_strip_model_order_controller.h b/chrome/browser/tabs/tab_strip_model_order_controller.h
index 97f32fa..6abe3d7 100644
--- a/chrome/browser/tabs/tab_strip_model_order_controller.h
+++ b/chrome/browser/tabs/tab_strip_model_order_controller.h
@@ -25,12 +25,12 @@ class TabStripModelOrderController : public TabStripModelObserver {
 
   // Determine where to place a newly opened tab by using the supplied
   // transition and foreground flag to figure out how it was opened.
-  virtual int DetermineInsertionIndex(TabContents* new_contents,
-                                      PageTransition::Type transition,
-                                      bool foreground);
+  int DetermineInsertionIndex(TabContents* new_contents,
+                              PageTransition::Type transition,
+                              bool foreground);
 
   // Determine where to shift selection after a tab is closed.
-  virtual int DetermineNewSelectedIndex(int removed_index) const;
+  int DetermineNewSelectedIndex(int removed_index) const;
 
   // Overridden from TabStripModelObserver:
   virtual void TabSelectedAt(TabContents* old_contents,
diff --git a/chrome/browser/tabs/tab_strip_model_unittest.cc b/chrome/browser/tabs/tab_strip_model_unittest.cc
index 38467b0..1f5e8e0 100644
--- a/chrome/browser/tabs/tab_strip_model_unittest.cc
+++ b/chrome/browser/tabs/tab_strip_model_unittest.cc
@@ -1190,3 +1190,102 @@ TEST_F(TabStripModelTest, AddTabContents_NewTabAtEndOfStripInheritsGroup) {
   strip.CloseAllTabs();
 }
 
+// A test of navigations in a tab that is part of a group of opened from some
+// parent tab. If the navigations are link clicks, the group relationship of
+// the tab to its parent are preserved. If they are of any other type, they are
+// not preserved.
+TEST_F(TabStripModelTest, NavigationForgetsOpeners) {
+  TabStripDummyDelegate delegate(NULL);
+  TabStripModel strip(&delegate, profile_);
+
+  // Open page A
+  TabContents* page_a_contents = CreateTabContents();
+  strip.AddTabContents(page_a_contents, -1, PageTransition::START_PAGE, true);
+
+  // Open pages B, C and D in the background from links on page A...
+  TabContents* page_b_contents = CreateTabContents();
+  TabContents* page_c_contents = CreateTabContents();
+  TabContents* page_d_contents = CreateTabContents();
+  strip.AddTabContents(page_b_contents, -1, PageTransition::LINK, false);
+  strip.AddTabContents(page_c_contents, -1, PageTransition::LINK, false);
+  strip.AddTabContents(page_d_contents, -1, PageTransition::LINK, false);
+
+  // Open page E in a different opener group from page A.
+  TabContents* page_e_contents = CreateTabContents();
+  strip.AddTabContents(page_e_contents, -1, PageTransition::START_PAGE, false);
+
+  // Tell the TabStripModel that we are navigating page D via a link click.
+  strip.SelectTabContentsAt(3, true);
+  strip.TabNavigating(page_d_contents, PageTransition::LINK);
+
+  // Close page D, page C should be selected. (part of same group).
+  strip.CloseTabContentsAt(3);
+  EXPECT_EQ(2, strip.selected_index());
+
+  // Tell the TabStripModel that we are navigating in page C via a bookmark.
+  strip.TabNavigating(page_c_contents, PageTransition::AUTO_BOOKMARK);
+
+  // Close page C, page E should be selected. (C is no longer part of the
+  // A-B-C-D group, selection moves to the right).
+  strip.CloseTabContentsAt(2);
+  EXPECT_EQ(page_e_contents, strip.GetTabContentsAt(strip.selected_index()));
+
+  strip.CloseAllTabs();
+}
+
+// A test that the forgetting behavior tested in NavigationForgetsOpeners above
+// doesn't cause the opener relationship for a New Tab opened at the end of the
+// TabStrip to be reset (Test 1 below), unless another any other tab is
+// seelcted (Test 2 below).
+TEST_F(TabStripModelTest, NavigationForgettingDoesntAffectNewTab) {
+  TabStripDummyDelegate delegate(NULL);
+  TabStripModel strip(&delegate, profile_);
+
+  // Open a tab and several tabs from it, then select one of the tabs that was
+  // opened.
+  TabContents* page_a_contents = CreateTabContents();
+  strip.AddTabContents(page_a_contents, -1, PageTransition::START_PAGE, true);
+
+  TabContents* page_b_contents = CreateTabContents();
+  TabContents* page_c_contents = CreateTabContents();
+  TabContents* page_d_contents = CreateTabContents();
+  strip.AddTabContents(page_b_contents, -1, PageTransition::LINK, false);
+  strip.AddTabContents(page_c_contents, -1, PageTransition::LINK, false);
+  strip.AddTabContents(page_d_contents, -1, PageTransition::LINK, false);
+
+  strip.SelectTabContentsAt(2, true);
+
+  // TEST 1: If the user is in a group of tabs and opens a new tab at the end
+  // of the strip, closing that new tab will select the tab that they were
+  // last on.
+
+  // Now simulate opening a new tab at the end of the TabStrip.
+  TabContents* new_tab_contents1 = CreateNewTabTabContents();
+  strip.AddTabContents(new_tab_contents1, -1, PageTransition::TYPED, true);
+
+  // At this point, if we close this tab the last selected one should be
+  // re-selected.
+  strip.CloseTabContentsAt(strip.count() - 1);
+  EXPECT_EQ(page_c_contents, strip.GetTabContentsAt(strip.selected_index()));
+
+  // TEST 2: If the user is in a group of tabs and opens a new tab at the end
+  // of the strip, selecting any other tab in the strip will cause that new
+  // tab's opener relationship to be forgotten.
+
+  // Open a new tab again.
+  TabContents* new_tab_contents2 = CreateNewTabTabContents();
+  strip.AddTabContents(new_tab_contents2, -1, PageTransition::TYPED, true);
+
+  // Now select the first tab.
+  strip.SelectTabContentsAt(0, true);
+
+  // Now select the last tab.
+  strip.SelectTabContentsAt(strip.count() - 1, true);
+
+  // Now close the last tab. The next adjacent should be selected.
+  strip.CloseTabContentsAt(strip.count() - 1);
+  EXPECT_EQ(page_d_contents, strip.GetTabContentsAt(strip.selected_index()));
+
+  strip.CloseAllTabs();
+}
+