Fixes bug where triggering session restore while the browser was

already running would end up creating an extra tab.

BUG=11594
TEST=open chrome with a single tabbed browser, turn on session
  restore, navigate to a page with a popup, close the tabbed browser,
  create a new window ala control-n (or double click on the desktop),
  and make the restored window doesn't end upw 

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

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

5 files changed, 110 insertions, 24 deletions

diff --git a/chrome/browser/sessions/session_restore.cc b/chrome/browser/sessions/session_restore.cc
index 99451b2..700290a 100644
--- a/chrome/browser/sessions/session_restore.cc
+++ b/chrome/browser/sessions/session_restore.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -26,6 +26,9 @@
 #include "chrome/common/notification_registrar.h"
 #include "chrome/common/notification_service.h"
 
+// Are we in the process of restoring?
+static bool restoring = false;
+
 namespace {
 
 // TabLoader ------------------------------------------------------------------
@@ -211,6 +214,7 @@ class SessionRestoreImpl : public NotificationObserver {
 
   ~SessionRestoreImpl() {
     STLDeleteElements(&windows_);
+    restoring = false;
   }
 
   virtual void Observe(NotificationType type,
@@ -502,6 +506,7 @@ static void Restore(Profile* profile,
     NOTREACHED();
     return;
   }
+  restoring = true;
   profile->set_restored_last_session(true);
   // SessionRestoreImpl takes care of deleting itself when done.
   SessionRestoreImpl* restorer =
@@ -527,3 +532,8 @@ void SessionRestore::RestoreSessionSynchronously(
     const std::vector<GURL>& urls_to_open) {
   Restore(profile, NULL, true, false, true, urls_to_open);
 }
+
+// static
+bool SessionRestore::IsRestoring() {
+  return restoring;
+}
diff --git a/chrome/browser/sessions/session_restore.h b/chrome/browser/sessions/session_restore.h
index eb44d9a..ba09694 100644
--- a/chrome/browser/sessions/session_restore.h
+++ b/chrome/browser/sessions/session_restore.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -42,6 +42,9 @@ class SessionRestore {
       Profile* profile,
       const std::vector<GURL>& urls_to_open);
 
+  // Returns true if we're in the process of restoring.
+  static bool IsRestoring();
+
   // The max number of non-selected tabs SessionRestore loads when restoring
   // a session. A value of 0 indicates all tabs are loaded at once.
   static size_t num_tabs_to_load_;
diff --git a/chrome/browser/sessions/session_restore_browsertest.cc b/chrome/browser/sessions/session_restore_browsertest.cc
new file mode 100644
index 0000000..2df1ad6
--- /dev/null
+++ b/chrome/browser/sessions/session_restore_browsertest.cc
@@ -0,0 +1,49 @@
+// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/defaults.h"
+#include "chrome/browser/browser.h"
+#include "chrome/browser/browser_window.h"
+#include "chrome/browser/tab_contents/tab_contents.h"
+#include "chrome/test/in_process_browser_test.h"
+#include "chrome/test/ui_test_utils.h"
+
+typedef InProcessBrowserTest SessionRestoreTest;
+
+// Makes sure when session restore is triggered in the same process we don't end
+// up with an extra tab.
+IN_PROC_BROWSER_TEST_F(SessionRestoreTest,
+                       RestoreOnNewWindowWithNoTabbedBrowsers) {
+  if (browser_defaults::kRestorePopups)
+    return;
+
+  GURL url(ui_test_utils::GetTestUrl(L".", L"title1.html"));
+  ui_test_utils::NavigateToURL(browser(), url);
+
+  // Turn on session restore.
+  SessionStartupPref::SetStartupPref(
+      browser()->profile(),
+      SessionStartupPref(SessionStartupPref::LAST));
+
+  // Create a new popup.
+  Profile* profile = browser()->profile();
+  Browser* popup = Browser::CreateForPopup(profile);
+  popup->window()->Show();
+
+  // Close the browser.
+  browser()->window()->Close();
+
+  // Create a new window, which should trigger session restore.
+  popup->NewWindow();
+
+  Browser* new_browser = ui_test_utils::WaitForNewBrowser();
+
+  ASSERT_TRUE(new_browser != NULL);
+
+  // The browser should only have one tab.
+  ASSERT_EQ(1, new_browser->tab_count());
+
+  // And the first url should be url.
+  EXPECT_EQ(url, new_browser->GetTabContentsAt(0)->GetURL());
+}
diff --git a/chrome/browser/sessions/session_service.cc b/chrome/browser/sessions/session_service.cc
index 94c67ee..1ef2a07 100644
--- a/chrome/browser/sessions/session_service.cc
+++ b/chrome/browser/sessions/session_service.cc
@@ -145,6 +145,10 @@ SessionService::~SessionService() {
   Save();
 }
 
+bool SessionService::RestoreIfNecessary(const std::vector<GURL>& urls_to_open) {
+  return RestoreIfNecessary(urls_to_open, NULL);
+}
+
 void SessionService::ResetFromCurrentBrowsers() {
   ScheduleReset();
 }
@@ -439,6 +443,34 @@ void SessionService::Init() {
                  NotificationService::AllSources());
 }
 
+bool SessionService::RestoreIfNecessary(const std::vector<GURL>& urls_to_open,
+                                        Browser* browser) {
+  if (!has_open_trackable_browsers_ && !BrowserInit::InProcessStartup() &&
+      !SessionRestore::IsRestoring()
+#if defined(OS_MACOSX)
+      // OSX has a fairly different idea of application lifetime than the
+      // other platforms. We need to check that we aren't opening a window
+      // from the dock or the menubar.
+      && !app_controller_mac::IsOpeningNewWindow()
+#endif
+      ) {
+    // We're going from no tabbed browsers to a tabbed browser (and not in
+    // process startup), restore the last session.
+    if (move_on_new_browser_) {
+      // Make the current session the last.
+      MoveCurrentSessionToLastSession();
+      move_on_new_browser_ = false;
+    }
+    SessionStartupPref pref = SessionStartupPref::GetStartupPref(profile());
+    if (pref.type == SessionStartupPref::LAST) {
+      SessionRestore::RestoreSession(
+          profile(), browser, false, browser ? false : true, urls_to_open);
+      return true;
+    }
+  }
+  return false;
+}
+
 void SessionService::Observe(NotificationType type,
                              const NotificationSource& source,
                              const NotificationDetails& details) {
@@ -451,27 +483,7 @@ void SessionService::Observe(NotificationType type,
         return;
       }
 
-      if (!has_open_trackable_browsers_ && !BrowserInit::InProcessStartup()
-#if defined(OS_MACOSX)
-          // OSX has a fairly different idea of application lifetime than the
-          // other platforms. We need to check that we aren't opening a window
-          // from the dock or the menubar.
-          && !app_controller_mac::IsOpeningNewWindow()
-#endif
-          ) {
-        // We're going from no tabbed browsers to a tabbed browser (and not in
-        // process startup), restore the last session.
-        if (move_on_new_browser_) {
-          // Make the current session the last.
-          MoveCurrentSessionToLastSession();
-          move_on_new_browser_ = false;
-        }
-        SessionStartupPref pref = SessionStartupPref::GetStartupPref(profile());
-        if (pref.type == SessionStartupPref::LAST) {
-          SessionRestore::RestoreSession(
-              profile(), browser, false, false, std::vector<GURL>());
-        }
-      }
+      RestoreIfNecessary(std::vector<GURL>(), browser);
       SetWindowType(browser->session_id(), browser->type());
       break;
     }
diff --git a/chrome/browser/sessions/session_service.h b/chrome/browser/sessions/session_service.h
index a950224..278c94b 100644
--- a/chrome/browser/sessions/session_service.h
+++ b/chrome/browser/sessions/session_service.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -53,6 +53,13 @@ class SessionService : public BaseSessionService,
   // For testing.
   explicit SessionService(const FilePath& save_path);
 
+  // Invoke at a point when you think session restore might occur. For example,
+  // during startup and window creation this is invoked to see if a session
+  // needs to be restored. If a session needs to be restored it is done so
+  // asynchronously and true is returned. If false is returned the session was
+  // not restored and the caller needs to create a new window.
+  bool RestoreIfNecessary(const std::vector<GURL>& urls_to_open);
+
   // Resets the contents of the file from the current state of all open
   // browsers whose profile matches our profile.
   void ResetFromCurrentBrowsers();
@@ -180,6 +187,11 @@ class SessionService : public BaseSessionService,
 
   void Init();
 
+  // Implementation of RestoreIfNecessary. If |browser| is non-null and we need
+  // to restore, the tabs are added to it, otherwise a new browser is created.
+  bool RestoreIfNecessary(const std::vector<GURL>& urls_to_open,
+                          Browser* browser);
+
   virtual void Observe(NotificationType type,
                        const NotificationSource& source,
                        const NotificationDetails& details);