When a new tab is opened (either the new tab page via Ctrl+T or pressing the new tab button) or an address is opened from the address bar in a new tab (by pressing Alt+Enter), the opener is remembered briefly, allowing quick lookup in a new tab without disrupting the z-order experience.

Full explanation in bug:

B=1266404

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

5 files changed, 145 insertions, 14 deletions

diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index 7a0bbb0..47299ce 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -589,14 +589,30 @@ void Browser::OpenURLFromTab(TabContents* source,
         transition == PageTransition::AUTO_BOOKMARK ||
         transition == PageTransition::GENERATED ||
         transition == PageTransition::START_PAGE) {
-      // 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.
-      tabstrip_model_.ForgetAllOpeners();
-      // In this specific case we also want to reset the group relationship,
-      // since it is now technically invalid.
-      tabstrip_model_.ForgetGroup(current_tab);
+      // 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.
+      // TODO(beng): (http://b/1306495) write unit tests for this once this
+      //             object is unit-testable.
+      int current_tab_index =
+          tabstrip_model_.GetIndexOfTabContents(current_tab);
+      bool forget_openers = 
+          !(current_tab->type() == TAB_CONTENTS_NEW_TAB_UI &&
+          current_tab_index == (tab_count() - 1) &&
+          current_tab->controller()->GetEntryCount() == 1);
+      if (forget_openers) {
+        // 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.
+        tabstrip_model_.ForgetAllOpeners();
+        // In this specific case we also want to reset the group relationship,
+        // since it is now technically invalid.
+        tabstrip_model_.ForgetGroup(current_tab);
+      }
     }
     current_tab->controller()->LoadURL(url, transition);
     // The TabContents might have changed as part of the navigation (ex: new tab
diff --git a/chrome/browser/tabs/tab_strip_model.cc b/chrome/browser/tabs/tab_strip_model.cc
index c2076f1..3b632fe 100644
--- a/chrome/browser/tabs/tab_strip_model.cc
+++ b/chrome/browser/tabs/tab_strip_model.cc
@@ -346,6 +346,13 @@ void TabStripModel::ForgetGroup(TabContents* contents) {
   int index = GetIndexOfTabContents(contents);
   DCHECK(ContainsIndex(index));
   contents_data_.at(index)->SetGroup(NULL);
+  contents_data_.at(index)->ForgetOpener();
+}
+
+bool TabStripModel::ShouldResetGroupOnSelect(TabContents* contents) const {
+  int index = GetIndexOfTabContents(contents);
+  DCHECK(ContainsIndex(index));
+  return contents_data_.at(index)->reset_group_on_select;
 }
 
 TabContents* TabStripModel::AddBlankTab(bool foreground) {
@@ -378,8 +385,22 @@ void TabStripModel::AddTabContents(TabContents* contents,
       index = count();
   }
   TabContents* last_selected_contents = GetSelectedTabContents();
-  InsertTabContentsAt(
-      index, contents, foreground, transition == PageTransition::LINK);
+  // Tabs opened from links inherit the "group" attribute of the Tab from which
+  // they were opened. This means when they're closed, that Tab will be
+  // selected again.
+  bool inherit_group = transition == PageTransition::LINK;
+  if (!inherit_group) {
+    // Also, any tab opened at the end of the TabStrip with a "TYPED"
+    // transition inherit group as well. This covers the cases where the user
+    // creates a New Tab (e.g. Ctrl+T, or clicks the New Tab button), or types
+    // in the address bar and presses Alt+Enter. This allows for opening a new
+    // Tab to quickly look up something. When this Tab is closed, the old one
+    // is re-selected, not the next-adjacent.
+    inherit_group = transition == PageTransition::TYPED && index == count();
+  }
+  InsertTabContentsAt(index, contents, foreground, inherit_group);
+  if (inherit_group && transition == PageTransition::TYPED)
+    contents_data_.at(index)->reset_group_on_select = true;
 }
 
 void TabStripModel::CloseSelectedTab() {
diff --git a/chrome/browser/tabs/tab_strip_model.h b/chrome/browser/tabs/tab_strip_model.h
index 8a235c5..c8e48f6 100644
--- a/chrome/browser/tabs/tab_strip_model.h
+++ b/chrome/browser/tabs/tab_strip_model.h
@@ -353,9 +353,14 @@ class TabStripModel : public NotificationObserver {
   // Forgets the group affiliation of the specified TabContents. This should be
   // called when a TabContents that is part of a logical group of tabs is
   // moved to a new logical context by the user (e.g. by typing a new URL or
-  // selecting a bookmark).
+  // selecting a bookmark). This also forgets the opener, which is considered
+  // a weaker relationship than group.
   void ForgetGroup(TabContents* contents);
 
+  // Returns true if the group/opener relationships present for |contents|
+  // should be reset when _any_ selection change occurs in the model.
+  bool ShouldResetGroupOnSelect(TabContents* contents) const;
+
   // Command level API /////////////////////////////////////////////////////////
 
   // Adds a blank tab to the TabStripModel.
@@ -487,8 +492,17 @@ class TabStripModel : public NotificationObserver {
     // same group. This property is used to determine what tab to select next
     // when one is closed.
     NavigationController* opener;
+    // True if our group should be reset the moment selection moves away from
+    // this Tab. This is the case for tabs opened in the foreground at the end
+    // of the TabStrip while viewing another Tab. If these tabs are closed
+    // before selection moves elsewhere, their opener is selected. But if
+    // selection shifts to _any_ tab (including their opener), the group
+    // relationship is reset to avoid confusing close sequencing.
+    bool reset_group_on_select;
+
     explicit TabContentsData(TabContents* a_contents)
-        : contents(a_contents) {
+        : contents(a_contents),
+          reset_group_on_select(false) {
       SetGroup(NULL);
     }
 
diff --git a/chrome/browser/tabs/tab_strip_model_order_controller.cc b/chrome/browser/tabs/tab_strip_model_order_controller.cc
index 7f96399..f564025 100644
--- a/chrome/browser/tabs/tab_strip_model_order_controller.cc
+++ b/chrome/browser/tabs/tab_strip_model_order_controller.cc
@@ -123,8 +123,14 @@ void TabStripModelOrderController::TabSelectedAt(TabContents* old_contents,
   NavigationController* old_opener = NULL;
   if (old_contents) {
     int index = tabstrip_->GetIndexOfTabContents(old_contents);
-    if (index != TabStripModel::kNoTab)
+    if (index != TabStripModel::kNoTab) {
       old_opener = tabstrip_->GetOpenerOfTabContentsAt(index);
+
+      // Forget any group/opener relationships that need to be reset whenever
+      // selection changes (see comment in TabStripModel::AddTabContentsAt).
+      if (tabstrip_->ShouldResetGroupOnSelect(old_contents))
+        tabstrip_->ForgetGroup(old_contents);
+    }
   }
   NavigationController* new_opener =
       tabstrip_->GetOpenerOfTabContentsAt(index);
diff --git a/chrome/browser/tabs/tab_strip_model_unittest.cc b/chrome/browser/tabs/tab_strip_model_unittest.cc
index 49e64ac..39689c9 100644
--- a/chrome/browser/tabs/tab_strip_model_unittest.cc
+++ b/chrome/browser/tabs/tab_strip_model_unittest.cc
@@ -136,6 +136,12 @@ class TabStripModelTest : public testing::Test {
     contents->SetupController(profile_);
     return contents;
   }
+  TabContents* CreateNewTabTabContents() {
+    TabStripModelTestTabContents* contents =
+        new TabStripModelTestTabContents(TAB_CONTENTS_NEW_TAB_UI);
+    contents->SetupController(profile_);
+    return contents;
+  }
 
   // Forwards a URL "load" request through to our dummy TabContents
   // implementation.
@@ -1072,10 +1078,12 @@ TEST_F(TabStripModelTest, AppendContentsReselectionTest) {
   EXPECT_EQ(0, tabstrip.selected_index());
 
   // Now open a blank tab...
+  // (see also AddTabContents_NewTabAtEndOfStripInheritsGroup for an
+  // explanation of this behavior)
   tabstrip.AddBlankTab(true);
   EXPECT_EQ(2, tabstrip.selected_index());
   tabstrip.CloseTabContentsAt(2);
-  EXPECT_EQ(1, tabstrip.selected_index());
+  EXPECT_EQ(0, tabstrip.selected_index());
 
   // clean up after ourselves
   tabstrip.CloseAllTabs();
@@ -1128,3 +1136,69 @@ TEST_F(TabStripModelTest, ReselectionConsidersChildrenTest) {
   // Clean up.
   strip.CloseAllTabs();
 }
+
+TEST_F(TabStripModelTest, AddTabContents_NewTabAtEndOfStripInheritsGroup) {
+  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);
+
+  // Switch to page B's tab.
+  strip.SelectTabContentsAt(1, true);
+
+  // Open a New Tab at the end of the strip (simulate Ctrl+T)
+  TabContents* new_tab_contents = CreateNewTabTabContents();
+  strip.AddTabContents(new_tab_contents, -1, PageTransition::TYPED, true);
+
+  EXPECT_EQ(4, strip.GetIndexOfTabContents(new_tab_contents));
+  EXPECT_EQ(4, strip.selected_index());
+
+  // Close the New Tab that was just opened. We should be returned to page B's
+  // Tab...
+  strip.CloseTabContentsAt(4);
+
+  EXPECT_EQ(1, strip.selected_index());
+
+  // Open a non-New Tab tab at the end of the strip, with a TYPED transition.
+  // This is like typing a URL in the address bar and pressing Alt+Enter. The
+  // behavior should be the same as above.
+  TabContents* page_e_contents = CreateTabContents();
+  strip.AddTabContents(page_e_contents, -1, PageTransition::TYPED, true);
+
+  EXPECT_EQ(4, strip.GetIndexOfTabContents(page_e_contents));
+  EXPECT_EQ(4, strip.selected_index());
+
+  // Close the Tab. Selection should shift back to page B's Tab.
+  strip.CloseTabContentsAt(4);
+
+  EXPECT_EQ(1, strip.selected_index());
+
+  // Open a non-New Tab tab at the end of the strip, with some other
+  // transition. This is like right clicking on a bookmark and choosing "Open
+  // in New Tab". No opener relationship should be preserved between this Tab
+  // and the one that was active when the gesture was performed.
+  TabContents* page_f_contents = CreateTabContents();
+  strip.AddTabContents(page_f_contents, -1, PageTransition::AUTO_BOOKMARK,
+                       true);
+
+  EXPECT_EQ(4, strip.GetIndexOfTabContents(page_f_contents));
+  EXPECT_EQ(4, strip.selected_index());
+
+  // Close the Tab. The next-adjacent should be selected.
+  strip.CloseTabContentsAt(4);
+
+  EXPECT_EQ(3, strip.selected_index());
+
+  // Clean up.
+  strip.CloseAllTabs();
+}