Restore closed tabs into new windows when necessary, and track the windows they

came from so they're restored together (into the same new window) when
appropriate.

Fix safety check on tab index when restoring: make it check the correct
browser.

Change some ASSERTs to EXPECTs in the unit test for greater coverage.

BUG=5278
TEST=Open a window with two tabs, close both (closing the window), then
  restore both. Make sure both restored tabs are in the same window.  Open
  a window with multiple tabs, close a tab, then close the window using 
  its close box. Restore both and make sure the tab goes back into the window.
Review URL: http://codereview.chromium.org/92001

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

1 files changed, 207 insertions, 85 deletions

diff --git a/chrome/browser/tab_restore_uitest.cc b/chrome/browser/tab_restore_uitest.cc
index 7c7956f..af98b5c 100644
--- a/chrome/browser/tab_restore_uitest.cc
+++ b/chrome/browser/tab_restore_uitest.cc
@@ -28,26 +28,76 @@ class TabRestoreUITest : public UITest {
   }
 
  protected:
-  void RestoreTab() {
-    int tab_count;
-
-    // Reset browser_proxy to new window.
-    scoped_ptr<BrowserProxy> browser_proxy(automation()->GetBrowserWindow(0));
+  // Uses the undo-close-tab accelerator to undo a close-tab or close-window
+  // operation. The newly restored tab is expected to appear in the
+  // window at index |expected_window_index|, at the |expected_tabstrip_index|,
+  // and to be active. If |expected_window_index| is equal to the number of
+  // current windows, the restored tab is expected to be created in a new
+  // window (since the index is 0-based).
+  void RestoreTab(int expected_window_index,
+                  int expected_tabstrip_index) {
+    int tab_count = 0;
+    int window_count = 0;
+
+    ASSERT_TRUE(automation()->GetBrowserWindowCount(&window_count));
+    ASSERT_GT(window_count, 0);
+
+    bool expect_new_window = (expected_window_index == window_count);
+    scoped_ptr<BrowserProxy> browser_proxy;
+    if (expect_new_window) {
+      browser_proxy.reset(automation()->GetBrowserWindow(0));
+    } else {
+      ASSERT_GT(window_count, expected_window_index);
+      browser_proxy.reset(
+          automation()->GetBrowserWindow(expected_window_index));
+    }
     ASSERT_TRUE(browser_proxy->GetTabCount(&tab_count));
     ASSERT_GT(tab_count, 0);
 
     // Restore the tab.
     ASSERT_TRUE(browser_proxy->ApplyAccelerator(IDC_RESTORE_TAB));
-    ASSERT_TRUE(browser_proxy->WaitForTabCountToBecome(
-        tab_count + 1, action_max_timeout_ms()));
+
+    if (expect_new_window) {
+      ASSERT_TRUE(automation()->WaitForWindowCountToBecome(
+          ++window_count, action_max_timeout_ms()));
+      browser_proxy.reset(automation()->
+          GetBrowserWindow(expected_window_index));
+    } else {
+      ASSERT_TRUE(browser_proxy->WaitForTabCountToBecome(
+          ++tab_count, action_max_timeout_ms()));
+    }
 
     // Get a handle to the restored tab.
     ASSERT_TRUE(browser_proxy->GetTabCount(&tab_count));
+    ASSERT_GT(tab_count, expected_tabstrip_index);
     scoped_ptr<TabProxy> restored_tab_proxy(
-        browser_proxy->GetTab(tab_count - 1));
+        browser_proxy->GetTab(expected_tabstrip_index));
     // Wait for the restored tab to finish loading.
     ASSERT_TRUE(restored_tab_proxy->WaitForTabToBeRestored(
         action_max_timeout_ms()));
+
+    // Ensure that the tab and window are active.
+    CheckActiveWindow(browser_proxy.get());
+    EXPECT_EQ(expected_tabstrip_index,
+        GetActiveTabIndex(expected_window_index));
+  }
+
+  // Adds tabs to the given browser, all navigated to url1_. Returns
+  // the final number of tabs.
+  int AddSomeTabs(BrowserProxy* browser, int how_many) {
+    int starting_tab_count = -1;
+    // Use EXPECT instead of ASSERT throughout to avoid trying to return void.
+    EXPECT_TRUE(browser->GetTabCount(&starting_tab_count));
+
+    for (int i = 0; i < how_many; ++i) {
+      browser->AppendTab(url1_);
+      EXPECT_TRUE(browser->WaitForTabCountToBecome(starting_tab_count + i + 1,
+                                                   action_max_timeout_ms()));
+    }
+    int tab_count;
+    EXPECT_TRUE(browser->GetTabCount(&tab_count));
+    EXPECT_EQ(starting_tab_count + how_many, tab_count);
+    return tab_count;
   }
 
   // Ensure that the given browser occupies the currently active window.
@@ -87,16 +137,9 @@ class TabRestoreUITest : public UITest {
 TEST_F(TabRestoreUITest, Basic) {
   scoped_ptr<BrowserProxy> browser_proxy(automation()->GetBrowserWindow(0));
 
-  int tab_count;
-  ASSERT_TRUE(browser_proxy->GetTabCount(&tab_count));
-  int starting_tab_count = tab_count;
-
-  // Add a tab
-  browser_proxy->AppendTab(url1_);
-  ASSERT_TRUE(browser_proxy->WaitForTabCountToBecome(tab_count + 1,
-                                                     action_max_timeout_ms()));
-  ASSERT_TRUE(browser_proxy->GetTabCount(&tab_count));
-  ASSERT_EQ(starting_tab_count + 1, tab_count);
+  int starting_tab_count;
+  ASSERT_TRUE(browser_proxy->GetTabCount(&starting_tab_count));
+  int tab_count = AddSomeTabs(browser_proxy.get(), 1);
 
   int closed_tab_index = tab_count - 1;
   scoped_ptr<TabProxy> new_tab(browser_proxy->GetTab(closed_tab_index));
@@ -106,15 +149,15 @@ TEST_F(TabRestoreUITest, Basic) {
   new_tab->Close(true);
   new_tab.reset();
   ASSERT_TRUE(browser_proxy->GetTabCount(&tab_count));
-  ASSERT_EQ(starting_tab_count, tab_count);
+  EXPECT_EQ(starting_tab_count, tab_count);
 
-  RestoreTab();
+  RestoreTab(0, closed_tab_index);
 
   // And make sure everything looks right.
   ASSERT_TRUE(browser_proxy->GetTabCount(&tab_count));
-  ASSERT_EQ(starting_tab_count + 1, tab_count);
-  ASSERT_EQ(closed_tab_index, GetActiveTabIndex());
-  ASSERT_EQ(url1_, GetActiveTabURL());
+  EXPECT_EQ(starting_tab_count + 1, tab_count);
+  EXPECT_EQ(closed_tab_index, GetActiveTabIndex());
+  EXPECT_EQ(url1_, GetActiveTabURL());
 }
 
 // Close a tab not at the end of the current window, then restore it. The tab
@@ -122,22 +165,9 @@ TEST_F(TabRestoreUITest, Basic) {
 TEST_F(TabRestoreUITest, MiddleTab) {
   scoped_ptr<BrowserProxy> browser_proxy(automation()->GetBrowserWindow(0));
 
-  int tab_count;
-  ASSERT_TRUE(browser_proxy->GetTabCount(&tab_count));
-  int starting_tab_count = tab_count;
-
-  // Add a couple of tabs
-  browser_proxy->AppendTab(url1_);
-  ASSERT_TRUE(browser_proxy->WaitForTabCountToBecome(tab_count + 1,
-                                                     action_max_timeout_ms()));
-  browser_proxy->AppendTab(url1_);
-  ASSERT_TRUE(browser_proxy->WaitForTabCountToBecome(tab_count + 2,
-                                                     action_max_timeout_ms()));
-  browser_proxy->AppendTab(url1_);
-  ASSERT_TRUE(browser_proxy->WaitForTabCountToBecome(tab_count + 3,
-                                                     action_max_timeout_ms()));
-  ASSERT_TRUE(browser_proxy->GetTabCount(&tab_count));
-  ASSERT_EQ(starting_tab_count + 3, tab_count);
+  int starting_tab_count;
+  ASSERT_TRUE(browser_proxy->GetTabCount(&starting_tab_count));
+  int tab_count = AddSomeTabs(browser_proxy.get(), 3);
 
   // Close one in the middle
   int closed_tab_index = starting_tab_count + 1;
@@ -148,15 +178,15 @@ TEST_F(TabRestoreUITest, MiddleTab) {
   new_tab->Close(true);
   new_tab.reset();
   ASSERT_TRUE(browser_proxy->GetTabCount(&tab_count));
-  ASSERT_EQ(starting_tab_count + 2, tab_count);
+  EXPECT_EQ(starting_tab_count + 2, tab_count);
 
-  RestoreTab();
+  RestoreTab(0, closed_tab_index);
 
   // And make sure everything looks right.
   ASSERT_TRUE(browser_proxy->GetTabCount(&tab_count));
-  ASSERT_EQ(starting_tab_count + 3, tab_count);
-  ASSERT_EQ(closed_tab_index, GetActiveTabIndex());
-  ASSERT_EQ(url1_, GetActiveTabURL());
+  EXPECT_EQ(starting_tab_count + 3, tab_count);
+  EXPECT_EQ(closed_tab_index, GetActiveTabIndex());
+  EXPECT_EQ(url1_, GetActiveTabURL());
 }
 
 // Close a tab, switch windows, then restore the tab. The tab should be in its
@@ -169,22 +199,9 @@ TEST_F(TabRestoreUITest, RestoreToDifferentWindow) {
   // CheckActiveWindow(). See comments in that function.
   CheckActiveWindow(browser_proxy.get());
 
-  int tab_count;
-  ASSERT_TRUE(browser_proxy->GetTabCount(&tab_count));
-  int starting_tab_count = tab_count;
-
-  // Add a couple of tabs
-  browser_proxy->AppendTab(url1_);
-  ASSERT_TRUE(browser_proxy->WaitForTabCountToBecome(tab_count + 1,
-                                                     action_max_timeout_ms()));
-  browser_proxy->AppendTab(url1_);
-  ASSERT_TRUE(browser_proxy->WaitForTabCountToBecome(tab_count + 2,
-                                                     action_max_timeout_ms()));
-  browser_proxy->AppendTab(url1_);
-  ASSERT_TRUE(browser_proxy->WaitForTabCountToBecome(tab_count + 3,
-                                                     action_max_timeout_ms()));
-  ASSERT_TRUE(browser_proxy->GetTabCount(&tab_count));
-  ASSERT_EQ(starting_tab_count + 3, tab_count);
+  int starting_tab_count;
+  ASSERT_TRUE(browser_proxy->GetTabCount(&starting_tab_count));
+  int tab_count = AddSomeTabs(browser_proxy.get(), 3);
 
   // Close one in the middle
   int closed_tab_index = starting_tab_count + 1;
@@ -195,7 +212,7 @@ TEST_F(TabRestoreUITest, RestoreToDifferentWindow) {
   new_tab->Close(true);
   new_tab.reset();
   ASSERT_TRUE(browser_proxy->GetTabCount(&tab_count));
-  ASSERT_EQ(starting_tab_count + 2, tab_count);
+  EXPECT_EQ(starting_tab_count + 2, tab_count);
 
   // Create a new browser.
   ASSERT_TRUE(automation()->OpenNewBrowserWindow(false));
@@ -204,19 +221,20 @@ TEST_F(TabRestoreUITest, RestoreToDifferentWindow) {
 
   CheckActiveWindow(automation()->GetBrowserWindow(1));
 
-  RestoreTab();
+  // Restore tab into original browser.
+  RestoreTab(0, closed_tab_index);
 
   // And make sure everything looks right.
   CheckActiveWindow(browser_proxy.get());
   ASSERT_TRUE(browser_proxy->GetTabCount(&tab_count));
   EXPECT_EQ(starting_tab_count + 3, tab_count);
-  ASSERT_EQ(closed_tab_index, GetActiveTabIndex());
-  ASSERT_EQ(url1_, GetActiveTabURL());
+  EXPECT_EQ(closed_tab_index, GetActiveTabIndex(0));
+  EXPECT_EQ(url1_, GetActiveTabURL(0));
 }
 
 // Close a tab, open a new window, close the first window, then restore the
-// tab. It should be at the end of the current (new) window's tabstrip.
-TEST_F(TabRestoreUITest, RestoreFromClosedWindow) {
+// tab. It should be in a new window.
+TEST_F(TabRestoreUITest, BasicRestoreFromClosedWindow) {
   // This test is disabled on win2k. See bug 1215881.
   if (win_util::GetWinVersion() == win_util::WINVERSION_2000)
     return;
@@ -245,7 +263,7 @@ TEST_F(TabRestoreUITest, RestoreFromClosedWindow) {
       2, action_max_timeout_ms()));
   CheckActiveWindow(automation()->GetBrowserWindow(1));
 
-  // Close the first browser.
+  // Close the final tab in the first browser.
   EXPECT_TRUE(tab_proxy->Close(true));
   ASSERT_TRUE(automation()->WaitForWindowCountToBecome(
       1, action_max_timeout_ms()));
@@ -254,16 +272,120 @@ TEST_F(TabRestoreUITest, RestoreFromClosedWindow) {
   tab_proxy.reset();
   browser_proxy.reset();
 
-  RestoreTab();
+  RestoreTab(1, 0);
 
-  // Tab should be at the end of the current (only) window.
-  browser_proxy.reset(automation()->GetBrowserWindow(0));
+  // Tab should be in a new window.
+  browser_proxy.reset(automation()->GetBrowserWindow(1));
   CheckActiveWindow(browser_proxy.get());
   tab_proxy.reset(browser_proxy->GetActiveTab());
   // And make sure the URLs matches.
-  ASSERT_EQ(url2_, GetActiveTabURL());
-  ASSERT_TRUE(tab_proxy->GoBack());
-  ASSERT_EQ(url1_, GetActiveTabURL());
+  EXPECT_EQ(url2_, GetActiveTabURL(1));
+  EXPECT_TRUE(tab_proxy->GoBack());
+  EXPECT_EQ(url1_, GetActiveTabURL(1));
+}
+
+// Open a window with multiple tabs, close a tab, then close the window.
+// Restore both and make sure the tab goes back into the window.
+TEST_F(TabRestoreUITest, RestoreWindowAndTab) {
+  scoped_ptr<BrowserProxy> browser_proxy(automation()->GetBrowserWindow(0));
+  CheckActiveWindow(browser_proxy.get());
+
+  int starting_tab_count;
+  ASSERT_TRUE(browser_proxy->GetTabCount(&starting_tab_count));
+  int tab_count = AddSomeTabs(browser_proxy.get(), 3);
+
+  // Close one in the middle
+  int closed_tab_index = starting_tab_count + 1;
+  scoped_ptr<TabProxy> new_tab(browser_proxy->GetTab(closed_tab_index));
+  // Make sure we're at url.
+  new_tab->NavigateToURL(url1_);
+  // Close the tab.
+  new_tab->Close(true);
+  new_tab.reset();
+  ASSERT_TRUE(browser_proxy->GetTabCount(&tab_count));
+  EXPECT_EQ(starting_tab_count + 2, tab_count);
+
+  // Create a new browser.
+  ASSERT_TRUE(automation()->OpenNewBrowserWindow(false));
+  ASSERT_TRUE(automation()->WaitForWindowCountToBecome(
+      2, action_max_timeout_ms()));
+  CheckActiveWindow(automation()->GetBrowserWindow(1));
+
+  // Close the first browser.
+  bool application_closing;
+  EXPECT_TRUE(CloseBrowser(browser_proxy.get(), &application_closing));
+  EXPECT_FALSE(application_closing);
+  ASSERT_TRUE(automation()->WaitForWindowCountToBecome(
+      1, action_max_timeout_ms()));
+
+  // Browser is no longer valid.
+  browser_proxy.reset();
+
+  // Restore the first window. The expected_tabstrip_index (second argument)
+  // indicates the expected active tab.
+  RestoreTab(1, starting_tab_count + 1);
+  browser_proxy.reset(automation()->GetBrowserWindow(1));
+  CheckActiveWindow(browser_proxy.get());
+  ASSERT_TRUE(browser_proxy->GetTabCount(&tab_count));
+  EXPECT_EQ(starting_tab_count + 2, tab_count);
+
+  // Restore the closed tab.
+  RestoreTab(1, closed_tab_index);
+  CheckActiveWindow(browser_proxy.get());
+  ASSERT_TRUE(browser_proxy->GetTabCount(&tab_count));
+  EXPECT_EQ(starting_tab_count + 3, tab_count);
+  EXPECT_EQ(url1_, GetActiveTabURL(1));
+}
+
+// Open a window with two tabs, close both (closing the window), then restore
+// both. Make sure both restored tabs are in the same window.
+TEST_F(TabRestoreUITest, RestoreIntoSameWindow) {
+  scoped_ptr<BrowserProxy> browser_proxy(automation()->GetBrowserWindow(0));
+  CheckActiveWindow(browser_proxy.get());
+
+  int starting_tab_count;
+  ASSERT_TRUE(browser_proxy->GetTabCount(&starting_tab_count));
+  int tab_count = AddSomeTabs(browser_proxy.get(), 2);
+
+  // Navigate the rightmost one to url2_ for easier identification.
+  scoped_ptr<TabProxy> tab_proxy(browser_proxy->GetTab(tab_count - 1));
+  tab_proxy->NavigateToURL(url2_);
+
+  // Create a new browser.
+  ASSERT_TRUE(automation()->OpenNewBrowserWindow(false));
+  ASSERT_TRUE(automation()->WaitForWindowCountToBecome(
+      2, action_max_timeout_ms()));
+  CheckActiveWindow(automation()->GetBrowserWindow(1));
+
+  // Close all but one tab in the first browser, left to right.
+  while (tab_count > 1) {
+    scoped_ptr<TabProxy> tab_to_close(browser_proxy->GetTab(0));
+    tab_to_close->Close(true);
+    ASSERT_TRUE(browser_proxy->GetTabCount(&tab_count));
+  }
+
+  // Close the last tab, closing the browser.
+  tab_proxy.reset(browser_proxy->GetTab(0));
+  EXPECT_TRUE(tab_proxy->Close(true));
+  ASSERT_TRUE(automation()->WaitForWindowCountToBecome(
+      1, action_max_timeout_ms()));
+  browser_proxy.reset();
+  tab_proxy.reset();
+
+  // Restore the last-closed tab into a new window.
+  RestoreTab(1, 0);
+  browser_proxy.reset(automation()->GetBrowserWindow(1));
+  CheckActiveWindow(browser_proxy.get());
+  ASSERT_TRUE(browser_proxy->GetTabCount(&tab_count));
+  EXPECT_EQ(1, tab_count);
+  EXPECT_EQ(url2_, GetActiveTabURL(1));
+
+  // Restore the next-to-last-closed tab into the same window.
+  RestoreTab(1, 0);
+  CheckActiveWindow(browser_proxy.get());
+  ASSERT_TRUE(browser_proxy->GetTabCount(&tab_count));
+  EXPECT_EQ(2, tab_count);
+  EXPECT_EQ(url1_, GetActiveTabURL(1));
 }
 
 // Tests that a duplicate history entry is not created when we restore a page
@@ -300,13 +422,13 @@ TEST_F(TabRestoreUITest, RestoreWithExistingSiteInstance) {
   browser_proxy->AppendTab(http_url2);
 
   // Restore the closed tab.
-  RestoreTab();
+  RestoreTab(0, tab_count - 1);
   tab.reset(browser_proxy->GetActiveTab());
 
   // And make sure the URLs match.
-  ASSERT_EQ(http_url2, GetActiveTabURL());
-  ASSERT_TRUE(tab->GoBack());
-  ASSERT_EQ(http_url1, GetActiveTabURL());
+  EXPECT_EQ(http_url2, GetActiveTabURL());
+  EXPECT_TRUE(tab->GoBack());
+  EXPECT_EQ(http_url1, GetActiveTabURL());
 }
 
 // Tests that the SiteInstances used for entries in a restored tab's history
@@ -346,19 +468,19 @@ TEST_F(TabRestoreUITest, RestoreCrossSiteWithExistingSiteInstance) {
   browser_proxy->AppendTab(http_url2);
 
   // Restore the closed tab.
-  RestoreTab();
+  RestoreTab(0, tab_count - 1);
   tab.reset(browser_proxy->GetActiveTab());
 
   // And make sure the URLs match.
-  ASSERT_EQ(url1_, GetActiveTabURL());
-  ASSERT_TRUE(tab->GoBack());
-  ASSERT_EQ(http_url1, GetActiveTabURL());
+  EXPECT_EQ(url1_, GetActiveTabURL());
+  EXPECT_TRUE(tab->GoBack());
+  EXPECT_EQ(http_url1, GetActiveTabURL());
 
   // Navigating to a new URL should clear the forward list, because the max
   // page ID of the renderer should have been updated when we restored the tab.
   tab->NavigateToURL(http_url2);
-  ASSERT_FALSE(tab->GoForward());
-  ASSERT_EQ(http_url2, GetActiveTabURL());
+  EXPECT_FALSE(tab->GoForward());
+  EXPECT_EQ(http_url2, GetActiveTabURL());
 }
 
 TEST_F(TabRestoreUITest, RestoreWindow) {
@@ -406,11 +528,11 @@ TEST_F(TabRestoreUITest, RestoreWindow) {
   ASSERT_TRUE(restored_tab_proxy->WaitForTabToBeRestored(action_timeout_ms()));
   GURL url;
   ASSERT_TRUE(restored_tab_proxy->GetCurrentURL(&url));
-  ASSERT_TRUE(url == url1_);
+  EXPECT_TRUE(url == url1_);
 
   restored_tab_proxy.reset(
         browser_proxy->GetTab(initial_tab_count + 1));
   ASSERT_TRUE(restored_tab_proxy->WaitForTabToBeRestored(action_timeout_ms()));
   ASSERT_TRUE(restored_tab_proxy->GetCurrentURL(&url));
-  ASSERT_TRUE(url == url2_);
+  EXPECT_TRUE(url == url2_);
 }