When restoring a closed tab using either ctrl-shift-T or the context menu, put

it back into the window it came from, at the tabstrip index it occupied before,
and activate (select) both the window and the tab.

Restoring a tab from the New Tab Page replaces the NTP, as before.

If the window the tab was in no longer exists, put the tab at the end of the
current window's tabstrip. This behavior may change in a later patch.

BUG=5278
TEST=Open two windows, with >1 tabs each. Close a tab, not the one at the end,
  in one of the windows. Switch to the other window and choose "Undo Closed
  Tab" from the tabstrip context menu, or type ctrl-shift-T. The tab should
  be restored where it was, and activated (selected and brought to the front).
Review URL: http://codereview.chromium.org/69015

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

8 files changed, 187 insertions, 5 deletions

diff --git a/chrome/browser/automation/automation_provider.cc b/chrome/browser/automation/automation_provider.cc
index 1845fd7..9823840 100644
--- a/chrome/browser/automation/automation_provider.cc
+++ b/chrome/browser/automation/automation_provider.cc
@@ -930,6 +930,7 @@ void AutomationProvider::OnMessageReceived(const IPC::Message& message) {
 #endif  // defined(OS_WIN)
     IPC_MESSAGE_HANDLER(AutomationMsg_TabProcessID, GetTabProcessID)
     IPC_MESSAGE_HANDLER(AutomationMsg_TabTitle, GetTabTitle)
+    IPC_MESSAGE_HANDLER(AutomationMsg_TabIndex, GetTabIndex)
     IPC_MESSAGE_HANDLER(AutomationMsg_TabURL, GetTabURL)
     IPC_MESSAGE_HANDLER(AutomationMsg_ShelfVisibility,
                         GetShelfVisibility)
@@ -1745,6 +1746,16 @@ void AutomationProvider::GetTabTitle(int handle, int* title_string_size,
   }
 }
 
+void AutomationProvider::GetTabIndex(int handle, int* tabstrip_index) {
+  *tabstrip_index = -1;  // -1 is the error code
+
+  if (tab_tracker_->ContainsHandle(handle)) {
+    NavigationController* tab = tab_tracker_->GetResource(handle);
+    Browser* browser = Browser::GetBrowserForController(tab, NULL);
+    *tabstrip_index = browser->tabstrip_model()->GetIndexOfController(tab);
+  }
+}
+
 void AutomationProvider::HandleUnused(const IPC::Message& message, int handle) {
   if (window_tracker_->ContainsHandle(handle)) {
     window_tracker_->Remove(window_tracker_->GetResource(handle));
diff --git a/chrome/browser/automation/automation_provider.h b/chrome/browser/automation/automation_provider.h
index bf08286..65f8906 100644
--- a/chrome/browser/automation/automation_provider.h
+++ b/chrome/browser/automation/automation_provider.h
@@ -174,6 +174,7 @@ class AutomationProvider : public base::RefCounted<AutomationProvider>,
 #endif  // defined(OS_WIN)
   void GetTabProcessID(int handle, int* process_id);
   void GetTabTitle(int handle, int* title_string_size, std::wstring* title);
+  void GetTabIndex(int handle, int* tabstrip_index);
   void GetTabURL(int handle, bool* success, GURL* url);
   void HandleUnused(const IPC::Message& message, int handle);
   void NavigateToURL(int handle, const GURL& url, IPC::Message* reply_message);
diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index a8501d9..83cd796 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -554,6 +554,8 @@ TabContents* Browser::AddRestoredTab(
   new_tab->controller().RestoreFromState(navigations, selected_navigation);
 
   tabstrip_model_.InsertTabContentsAt(tab_index, new_tab, select, false);
+  if (select)
+    window_->Activate();
   if (profile_->HasSessionService()) {
     SessionService* session_service = profile_->GetSessionService();
     if (session_service)
diff --git a/chrome/browser/browser_list.cc b/chrome/browser/browser_list.cc
index 6665d15..411583b 100644
--- a/chrome/browser/browser_list.cc
+++ b/chrome/browser/browser_list.cc
@@ -211,6 +211,16 @@ Browser* BrowserList::FindBrowserWithType(Profile* p, Browser::Type t) {
 }
 
 // static
+Browser* BrowserList::FindBrowserWithID(SessionID::id_type desired_id) {
+  BrowserList::const_iterator i;
+  for (i = BrowserList::begin(); i != BrowserList::end(); ++i) {
+    if ((*i)->session_id().id() == desired_id)
+      return *i;
+  }
+  return NULL;
+}
+
+// static
 size_t BrowserList::GetBrowserCountForType(Profile* p, Browser::Type type) {
   BrowserList::const_iterator i;
   size_t result = 0;
diff --git a/chrome/browser/browser_list.h b/chrome/browser/browser_list.h
index 63ea28d..858e97e 100644
--- a/chrome/browser/browser_list.h
+++ b/chrome/browser/browser_list.h
@@ -55,6 +55,10 @@ class BrowserList {
   // is returned. Returns NULL if no such browser currently exists.
   static Browser* FindBrowserWithType(Profile* p, Browser::Type t);
 
+  // Find an existing browser with the provided ID. Returns NULL if no such
+  // browser currently exists.
+  static Browser* FindBrowserWithID(SessionID::id_type desired_id);
+
   // Closes all browsers. If use_post is true the windows are closed by way of
   // posting a WM_CLOSE message, otherwise the windows are closed directly. In
   // almost all cases you'll want to use true, the one exception is ending
diff --git a/chrome/browser/sessions/tab_restore_service.cc b/chrome/browser/sessions/tab_restore_service.cc
index 5892013..d61e42a 100644
--- a/chrome/browser/sessions/tab_restore_service.cc
+++ b/chrome/browser/sessions/tab_restore_service.cc
@@ -130,6 +130,13 @@ void TabRestoreService::CreateHistoricalTab(NavigationController* tab) {
   if (local_tab->navigations.empty())
     return;
 
+  // browser may be NULL when running unit tests.
+  if (browser) {
+    local_tab->browser_id = browser->session_id().id();
+    local_tab->tabstrip_index =
+        browser->tabstrip_model()->GetIndexOfController(tab);
+  }
+
   AddEntry(local_tab.release(), true, true);
 }
 
@@ -221,8 +228,19 @@ void TabRestoreService::RestoreEntryById(Browser* browser,
         browser->ReplaceRestoredTab(tab->navigations,
                                     tab->current_navigation_index);
       } else {
-        browser->AddRestoredTab(tab->navigations, browser->tab_count(),
-                                tab->current_navigation_index, true);
+        // Use the tab's former browser and index, if available.
+        Browser* tab_browser = NULL;
+        int tab_index = -1;
+        if (tab->has_browser())
+          tab_browser = BrowserList::FindBrowserWithID(tab->browser_id);
+        if (tab_browser)
+          tab_index = tab->tabstrip_index;
+        else
+          tab_browser = browser;
+        if (tab_index < 0 || tab_index > browser->tab_count())
+          tab_index = browser->tab_count();
+        tab_browser->AddRestoredTab(tab->navigations, tab_index,
+                                    tab->current_navigation_index, true);
       }
     } else if (entry->type == WINDOW) {
       const Window* window = static_cast<Window*>(entry);
diff --git a/chrome/browser/sessions/tab_restore_service.h b/chrome/browser/sessions/tab_restore_service.h
index b768385..c728652 100644
--- a/chrome/browser/sessions/tab_restore_service.h
+++ b/chrome/browser/sessions/tab_restore_service.h
@@ -63,13 +63,26 @@ class TabRestoreService : public BaseSessionService {
 
   // Represents a previously open tab.
   struct Tab : public Entry {
-    Tab() : Entry(TAB), current_navigation_index(-1) {}
+    Tab()
+        : Entry(TAB),
+          current_navigation_index(-1),
+          browser_id(0),
+          tabstrip_index(-1) {}
+
+    bool has_browser() const { return browser_id > 0; }
 
     // The navigations.
     std::vector<TabNavigation> navigations;
 
     // Index of the selected navigation in navigations.
     int current_navigation_index;
+
+    // The ID of the browser to which this tab belonged, so it can be restored
+    // there. May be 0 (an invalid SessionID) when restoring an entire session.
+    SessionID::id_type browser_id;
+
+    // Index within the tab strip. May be -1 for an unknown index.
+    int tabstrip_index;
   };
 
   // Represents a previously open window.
diff --git a/chrome/browser/tab_restore_uitest.cc b/chrome/browser/tab_restore_uitest.cc
index 6a16c9b..109c768 100644
--- a/chrome/browser/tab_restore_uitest.cc
+++ b/chrome/browser/tab_restore_uitest.cc
@@ -12,6 +12,7 @@
 #include "chrome/common/pref_names.h"
 #include "chrome/test/automation/tab_proxy.h"
 #include "chrome/test/automation/browser_proxy.h"
+#include "chrome/test/automation/window_proxy.h"
 #include "chrome/test/ui/ui_test.h"
 #include "googleurl/src/gurl.h"
 #include "net/base/net_util.h"
@@ -49,6 +50,17 @@ class TabRestoreUITest : public UITest {
         action_max_timeout_ms()));
   }
 
+  // Ensure that the given browser occupies the currently active window.
+  void CheckActiveWindow(const BrowserProxy* browser) {
+    // The EXPECT_TRUE may fail during manual debugging, as well as if a user
+    // does other things while running the tests, because Chromium won't be
+    // the foremost application at all.
+    bool is_active = false;
+    scoped_ptr<WindowProxy> window_proxy(browser->GetWindow());
+    ASSERT_TRUE(window_proxy->IsActive(&is_active));
+    EXPECT_TRUE(is_active);
+  }
+
   GURL url1_;
   GURL url2_;
 
@@ -56,36 +68,144 @@ class TabRestoreUITest : public UITest {
   DISALLOW_EVIL_CONSTRUCTORS(TabRestoreUITest);
 };
 
+// Close the end tab in the current window, then restore it. The tab should be
+// in its original position, and active.
 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));
-  scoped_ptr<TabProxy> new_tab(browser_proxy->GetTab(tab_count - 1));
+  ASSERT_EQ(starting_tab_count + 1, tab_count);
+
+  int closed_tab_index = 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));
+  ASSERT_EQ(starting_tab_count, tab_count);
 
   RestoreTab();
 
-  // And make sure the URL matches.
+  // 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());
 }
 
+// Close a tab not at the end of the current window, then restore it. The tab
+// should be in its original position, and active.
+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);
+
+  // 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));
+  ASSERT_EQ(starting_tab_count + 2, tab_count);
+
+  RestoreTab();
+
+  // 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());
+}
+
+// Close a tab, switch windows, then restore the tab. The tab should be in its
+// original window and position, and active.
 TEST_F(TabRestoreUITest, RestoreToDifferentWindow) {
+  scoped_ptr<BrowserProxy> browser_proxy(automation()->GetBrowserWindow(0));
+
+  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);
+
+  // 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));
+  ASSERT_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));
+
+  RestoreTab();
+
+  // 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());
+}
+
+// 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) {
   // This test is disabled on win2k. See bug 1215881.
   if (win_util::GetWinVersion() == win_util::WINVERSION_2000)
     return;
 
   scoped_ptr<BrowserProxy> browser_proxy(automation()->GetBrowserWindow(0));
+  CheckActiveWindow(browser_proxy.get());
 
   int tab_count;
   ASSERT_TRUE(browser_proxy->GetTabCount(&tab_count));
@@ -106,6 +226,7 @@ TEST_F(TabRestoreUITest, RestoreToDifferentWindow) {
   ASSERT_TRUE(automation()->OpenNewBrowserWindow(false));
   ASSERT_TRUE(automation()->WaitForWindowCountToBecome(
       2, action_max_timeout_ms()));
+  CheckActiveWindow(automation()->GetBrowserWindow(1));
 
   // Close the first browser.
   EXPECT_TRUE(tab_proxy->Close(true));
@@ -118,7 +239,9 @@ TEST_F(TabRestoreUITest, RestoreToDifferentWindow) {
 
   RestoreTab();
 
+  // Tab should be at the end of the current (only) window.
   browser_proxy.reset(automation()->GetBrowserWindow(0));
+  CheckActiveWindow(browser_proxy.get());
   tab_proxy.reset(browser_proxy->GetActiveTab());
   // And make sure the URLs matches.
   ASSERT_EQ(url2_, GetActiveTabURL());