Implement restoring closed tab menu item. Reworked cross-platform code to handle the case of restoring when there are no browsers open by making window restore re-use a given browser if it has no tabs. Removed unit test that assumes it can pass a NULL Browser. Wrote a new UI test to cover the same area, but it's disabled until another bug is fixed, and I didn't want to hold up landing this feature. Added key shortcut for "open window in incognito mode".

BUG=13758
TEST=restoring closed tabs with visible windows and with no windows remaining open.
Review URL: http://codereview.chromium.org/125257

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

8 files changed, 151 insertions, 75 deletions

diff --git a/chrome/browser/app_controller_mac.mm b/chrome/browser/app_controller_mac.mm
index 6110c99..6de60dec 100644
--- a/chrome/browser/app_controller_mac.mm
+++ b/chrome/browser/app_controller_mac.mm
@@ -23,6 +23,7 @@
 #import "chrome/browser/cocoa/tab_strip_controller.h"
 #import "chrome/browser/cocoa/tab_window_controller.h"
 #include "chrome/browser/command_updater.h"
+#include "chrome/browser/sessions/tab_restore_service.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/pref_service.h"
 #include "chrome/browser/profile_manager.h"
@@ -102,6 +103,16 @@
 
 // Called when the app is shutting down. Clean-up as appropriate.
 - (void)applicationWillTerminate:(NSNotification *)aNotification {
+  DCHECK(!BrowserList::HasBrowserWithProfile([self defaultProfile]));
+  if (!BrowserList::HasBrowserWithProfile([self defaultProfile])) {
+    // As we're shutting down, we need to nuke the TabRestoreService, which will
+    // start the shutdown of the NavigationControllers and allow for proper
+    // shutdown. If we don't do this chrome won't shutdown cleanly, and may end
+    // up crashing when some thread tries to use the IO thread (or another
+    // thread) that is no longer valid.
+    [self defaultProfile]->ResetTabRestoreService();
+  }
+
   [[NSNotificationCenter defaultCenter] removeObserver:self];
 }
 
@@ -264,6 +275,14 @@
   g_browser_process->ReleaseModule();
 }
 
+// Called to determine if we should enable the "restore tab" menu item.
+// Checks with the TabRestoreService to see if there's anything there to
+// restore and returns YES if so.
+- (BOOL)canRestoreTab {
+  TabRestoreService* service = [self defaultProfile]->GetTabRestoreService();
+  return service && !service->entries().empty();
+}
+
 // Called to validate menu items when there are no key windows. All the
 // items we care about have been set with the |commandDispatch:| action and
 // a target of FirstResponder in IB. If it's not one of those, let it
@@ -275,8 +294,15 @@
   BOOL enable = NO;
   if (action == @selector(commandDispatch:)) {
     NSInteger tag = [item tag];
-    if (menuState_->SupportsCommand(tag))
-      enable = menuState_->IsCommandEnabled(tag) ? YES : NO;
+    if (menuState_->SupportsCommand(tag)) {
+      switch (tag) {
+        case IDC_RESTORE_TAB:
+          enable = [self canRestoreTab];
+          break;
+        default:
+          enable = menuState_->IsCommandEnabled(tag) ? YES : NO;
+      }
+    }
   } else if (action == @selector(quit:)) {
     enable = YES;
   } else if (action == @selector(showPreferences:)) {
@@ -296,12 +322,16 @@
 
   NSInteger tag = [sender tag];
   switch (tag) {
+    case IDC_NEW_TAB:
     case IDC_NEW_WINDOW:
       Browser::OpenEmptyWindow(defaultProfile);
       break;
     case IDC_NEW_INCOGNITO_WINDOW:
       Browser::OpenURLOffTheRecord(defaultProfile, GURL());
       break;
+    case IDC_RESTORE_TAB:
+      Browser::OpenWindowWithRestoredTabs(defaultProfile);
+      break;
     case IDC_OPEN_FILE:
       Browser::OpenEmptyWindow(defaultProfile);
       BrowserList::GetLastActive()->
@@ -337,10 +367,12 @@
 
 - (void)initMenuState {
   menuState_.reset(new CommandUpdater(NULL));
+  menuState_->UpdateCommandEnabled(IDC_NEW_TAB, true);
   menuState_->UpdateCommandEnabled(IDC_NEW_WINDOW, true);
   menuState_->UpdateCommandEnabled(IDC_NEW_INCOGNITO_WINDOW, true);
   menuState_->UpdateCommandEnabled(IDC_OPEN_FILE, true);
   menuState_->UpdateCommandEnabled(IDC_CLEAR_BROWSING_DATA, true);
+  menuState_->UpdateCommandEnabled(IDC_RESTORE_TAB, false);
   // TODO(pinkerton): ...more to come...
 }
 
diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index 5bba572..0fe72e6 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -213,6 +213,7 @@ Browser::~Browser() {
 
   BrowserList::RemoveBrowser(this);
 
+#if defined(OS_WIN) || defined(OS_LINUX)
   if (!BrowserList::HasBrowserWithProfile(profile_)) {
     // We're the last browser window with this profile. We need to nuke the
     // TabRestoreService, which will start the shutdown of the
@@ -220,8 +221,12 @@ Browser::~Browser() {
     // chrome won't shutdown cleanly, and may end up crashing when some
     // thread tries to use the IO thread (or another thread) that is no longer
     // valid.
+    // This isn't a valid assumption for Mac OS, as it stays running after
+    // the last browser has closed. The Mac equivalent is in its app
+    // controller.
     profile_->ResetTabRestoreService();
   }
+#endif
 
   SessionService* session_service = profile_->GetSessionService();
   if (session_service)
@@ -315,6 +320,13 @@ void Browser::OpenEmptyWindow(Profile* profile) {
 }
 
 // static
+void Browser::OpenWindowWithRestoredTabs(Profile* profile) {
+  TabRestoreService* service = profile->GetTabRestoreService();
+  if (service)
+    service->RestoreMostRecentEntry(NULL);
+}
+
+// static
 void Browser::OpenURLOffTheRecord(Profile* profile, const GURL& url) {
   Profile* off_the_record_profile = profile->GetOffTheRecordProfile();
   Browser* browser = BrowserList::FindBrowserWithType(
diff --git a/chrome/browser/browser.h b/chrome/browser/browser.h
index 67aa5425..666cc2cc 100644
--- a/chrome/browser/browser.h
+++ b/chrome/browser/browser.h
@@ -141,6 +141,9 @@ class Browser : public TabStripModelDelegate,
   // Opens a new window with the default blank tab.
   static void OpenEmptyWindow(Profile* profile);
 
+  // Opens a new window with the tabs from |profile|'s TabRestoreService.
+  static void OpenWindowWithRestoredTabs(Profile* profile);
+
   // Opens the specified URL in a new browser window in an incognito session.
   // If there is already an existing active incognito session for the specified
   // |profile|, that session is re-used.
diff --git a/chrome/browser/cocoa/browser_window_controller.mm b/chrome/browser/cocoa/browser_window_controller.mm
index d814c36..0401644 100644
--- a/chrome/browser/cocoa/browser_window_controller.mm
+++ b/chrome/browser/cocoa/browser_window_controller.mm
@@ -331,11 +331,18 @@ willPositionSheet:(NSWindow *)sheet
     if (browser_->command_updater()->SupportsCommand(tag)) {
       // Generate return value (enabled state)
       enable = browser_->command_updater()->IsCommandEnabled(tag) ? YES : NO;
-
-      // Disable "close tab" if we're not the key window or if there's only
-      // one tab.
-      if (tag == IDC_CLOSE_TAB)
-        enable &= [self numberOfTabs] > 1 && [[self window] isKeyWindow];
+      switch (tag) {
+        case IDC_CLOSE_TAB:
+          // Disable "close tab" if we're not the key window or if there's only
+          // one tab.
+          enable &= [self numberOfTabs] > 1 && [[self window] isKeyWindow];
+          break;
+        case IDC_RESTORE_TAB:
+          // We have to ask the Browser manually if we can restore. The
+          // command updater doesn't know.
+          enable &= browser_->CanRestoreTab();
+          break;
+      }
 
       // If the item is toggleable, find it's toggle state and
       // try to update it.  This is a little awkward, but the alternative is
diff --git a/chrome/browser/sessions/tab_restore_service.cc b/chrome/browser/sessions/tab_restore_service.cc
index 2915deb..4bbd660 100644
--- a/chrome/browser/sessions/tab_restore_service.cc
+++ b/chrome/browser/sessions/tab_restore_service.cc
@@ -218,58 +218,61 @@ void TabRestoreService::RestoreEntryById(Browser* browser,
   Entry* entry = *i;
   entries_.erase(i);
   i = entries_.end();
-  if (browser) {  // Browser is null during testing.
-    if (entry->type == TAB) {
-      Tab* tab = static_cast<Tab*>(entry);
-      if (replace_existing_tab) {
-        browser->ReplaceRestoredTab(tab->navigations,
-                                    tab->current_navigation_index);
+
+  // |browser| will be NULL in cases where one isn't already available (eg,
+  // when invoked on Mac OS X with no windows open). In this case, create a
+  // new browser into which we restore the tabs.
+  if (entry->type == TAB) {
+    Tab* tab = static_cast<Tab*>(entry);
+    if (replace_existing_tab && browser) {
+      browser->ReplaceRestoredTab(tab->navigations,
+                                  tab->current_navigation_index);
+    } else {
+      // 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 {
-        // 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::Create(profile());
-          if (tab->has_browser()) {
-            UpdateTabBrowserIDs(tab->browser_id,
-                                tab_browser->session_id().id());
-          }
-          tab_browser->window()->Show();
+        tab_browser = Browser::Create(profile());
+        if (tab->has_browser()) {
+          UpdateTabBrowserIDs(tab->browser_id,
+                              tab_browser->session_id().id());
         }
-
-        if (tab_index < 0 || tab_index > tab_browser->tab_count())
-          tab_index = tab_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);
-      Browser* browser = Browser::Create(profile());
-      for (size_t tab_i = 0; tab_i < window->tabs.size(); ++tab_i) {
-        const Tab& tab = window->tabs[tab_i];
-        TabContents* restored_tab =
-            browser->AddRestoredTab(tab.navigations, browser->tab_count(),
-                                    tab.current_navigation_index,
-                                    (static_cast<int>(tab_i) ==
-                                     window->selected_tab_index));
-        if (restored_tab)
-          restored_tab->controller().LoadIfNecessary();
+        tab_browser->window()->Show();
       }
-      // All the window's tabs had the same former browser_id.
-      if (window->tabs[0].has_browser()) {
-        UpdateTabBrowserIDs(window->tabs[0].browser_id,
-                            browser->session_id().id());
-      }
-      browser->window()->Show();
-    } else {
-      NOTREACHED();
+
+      if (tab_index < 0 || tab_index > tab_browser->tab_count())
+        tab_index = tab_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);
+    browser = Browser::Create(profile());
+    for (size_t tab_i = 0; tab_i < window->tabs.size(); ++tab_i) {
+      const Tab& tab = window->tabs[tab_i];
+      TabContents* restored_tab =
+          browser->AddRestoredTab(tab.navigations, browser->tab_count(),
+                                  tab.current_navigation_index,
+                                  (static_cast<int>(tab_i) ==
+                                   window->selected_tab_index));
+      if (restored_tab)
+        restored_tab->controller().LoadIfNecessary();
     }
+    // All the window's tabs had the same former browser_id.
+    if (window->tabs[0].has_browser()) {
+      UpdateTabBrowserIDs(window->tabs[0].browser_id,
+                          browser->session_id().id());
+    }
+    browser->window()->Show();
+  } else {
+    NOTREACHED();
   }
+
   delete entry;
   restoring_ = false;
   NotifyTabsChanged();
diff --git a/chrome/browser/sessions/tab_restore_service.h b/chrome/browser/sessions/tab_restore_service.h
index 6b1da34..a74f335 100644
--- a/chrome/browser/sessions/tab_restore_service.h
+++ b/chrome/browser/sessions/tab_restore_service.h
@@ -134,7 +134,8 @@ class TabRestoreService : public BaseSessionService {
 
   // Restores an entry by id. If there is no entry with an id matching |id|,
   // this does nothing. If |replace_existing_tab| is true and id identifies a
-  // tab, the newly created tab replaces the selected tab in |browser|.
+  // tab, the newly created tab replaces the selected tab in |browser|. If
+  // |browser| is NULL, this creates a new window for the entry.
   void RestoreEntryById(Browser* browser,
                         SessionID::id_type id,
                         bool replace_existing_tab);
diff --git a/chrome/browser/sessions/tab_restore_service_unittest.cc b/chrome/browser/sessions/tab_restore_service_unittest.cc
index 7a24a91..c1ad7b1 100644
--- a/chrome/browser/sessions/tab_restore_service_unittest.cc
+++ b/chrome/browser/sessions/tab_restore_service_unittest.cc
@@ -157,25 +157,6 @@ TEST_F(TabRestoreServiceTest, Restore) {
   EXPECT_EQ(2, tab->current_navigation_index);
 }
 
-// Make sure a tab that is restored doesn't come back.
-TEST_F(TabRestoreServiceTest, DontLoadRestoredTab) {
-  AddThreeNavigations();
-
-  // Have the service record the tab.
-  service_->CreateHistoricalTab(&controller());
-  ASSERT_EQ(1U, service_->entries().size());
-
-  // Restore the tab.
-  service_->RestoreEntryById(NULL, service_->entries().front()->id, true);
-  ASSERT_TRUE(service_->entries().empty());
-
-  // Recreate the service and have it load the tabs.
-  RecreateService();
-
-  // There should be no entries.
-  ASSERT_EQ(0U, service_->entries().size());
-}
-
 // Make sure we persist entries to disk that have post data.
 TEST_F(TabRestoreServiceTest, DontPersistPostData) {
   AddThreeNavigations();
diff --git a/chrome/browser/tab_restore_uitest.cc b/chrome/browser/tab_restore_uitest.cc
index 5807721..5535124 100644
--- a/chrome/browser/tab_restore_uitest.cc
+++ b/chrome/browser/tab_restore_uitest.cc
@@ -282,9 +282,46 @@ TEST_F(TabRestoreUITest, BasicRestoreFromClosedWindow) {
   EXPECT_EQ(url1_, GetActiveTabURL(1));
 }
 
+// Restore a tab then make sure it doesn't restore again.
+// Disabled because the command updater doesn't know the proper state of
+// the tab restore command. http://crbug.com/14428.
+TEST_F(TabRestoreUITest, DISABLED_DontLoadRestoredTab) {
+  scoped_refptr<BrowserProxy> browser_proxy(automation()->GetBrowserWindow(0));
+  CheckActiveWindow(browser_proxy.get());
+
+  // Add two tabs
+  int starting_tab_count = 0;
+  ASSERT_TRUE(browser_proxy->GetTabCount(&starting_tab_count));
+  AddSomeTabs(browser_proxy.get(), 2);
+  int current_tab_count = 0;
+  ASSERT_TRUE(browser_proxy->GetTabCount(&current_tab_count));
+  ASSERT_EQ(current_tab_count, starting_tab_count + 2);
+
+  // Close one of them.
+  scoped_refptr<TabProxy> tab_to_close(browser_proxy->GetTab(0));
+  tab_to_close->Close(true);
+  ASSERT_TRUE(browser_proxy->GetTabCount(&current_tab_count));
+  ASSERT_EQ(current_tab_count, starting_tab_count + 1);
+
+  // Restore it.
+  RestoreTab(0, 0);
+  ASSERT_TRUE(browser_proxy->GetTabCount(&current_tab_count));
+  ASSERT_EQ(current_tab_count, starting_tab_count + 2);
+
+  // Make sure that there's nothing else to restore.
+  // TODO(pinkerton): This currently fails because the command_updater in the
+  // always says yes. See bug above.
+  bool is_timeout = false;
+  bool enabled =
+    browser_proxy->IsPageMenuCommandEnabledWithTimeout(IDC_RESTORE_TAB,
+        action_max_timeout_ms(), &is_timeout);
+  if (!is_timeout)
+    ASSERT_FALSE(enabled);
+}
+
 // 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.
-// Disabled because flacky. See http://crbug.com/14132
+// Disabled because flakey. See http://crbug.com/14132
 TEST_F(TabRestoreUITest, DISABLED_RestoreWindowAndTab) {
   scoped_refptr<BrowserProxy> browser_proxy(automation()->GetBrowserWindow(0));
   CheckActiveWindow(browser_proxy.get());