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

12 files changed, 200 insertions, 68 deletions

diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index bde1736..0f7b25f 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -1007,7 +1007,12 @@ void Browser::Stop() {
 
 void Browser::NewWindow() {
   UserMetrics::RecordAction(UserMetricsAction("NewWindow"), profile_);
-  Browser::OpenEmptyWindow(profile_->GetOriginalProfile());
+  SessionService* session_service =
+      profile_->GetOriginalProfile()->GetSessionService();
+  if (!session_service ||
+      !session_service->RestoreIfNecessary(std::vector<GURL>())) {
+    Browser::OpenEmptyWindow(profile_->GetOriginalProfile());
+  }
 }
 
 void Browser::NewIncognitoWindow() {
diff --git a/chrome/browser/browser_browsertest.cc b/chrome/browser/browser_browsertest.cc
index dc0a2cc..0d102fb 100644
--- a/chrome/browser/browser_browsertest.cc
+++ b/chrome/browser/browser_browsertest.cc
@@ -502,7 +502,7 @@ IN_PROC_BROWSER_TEST_F(BrowserTest, RestorePinnedTabs) {
   CommandLine dummy(CommandLine::ARGUMENTS_ONLY);
   BrowserInit::LaunchWithProfile launch(std::wstring(), dummy);
   launch.profile_ = browser()->profile();
-  launch.OpenStartupURLs(std::vector<GURL>());
+  launch.ProcessStartupURLs(std::vector<GURL>());
 
   // The launch should have created a new browser.
   ASSERT_EQ(2u, BrowserList::GetBrowserCount(browser()->profile()));
diff --git a/chrome/browser/browser_init.cc b/chrome/browser/browser_init.cc
index b4518d7..56b296f 100644
--- a/chrome/browser/browser_init.cc
+++ b/chrome/browser/browser_init.cc
@@ -28,6 +28,7 @@
 #include "chrome/browser/search_engines/template_url_model.h"
 #include "chrome/browser/session_startup_pref.h"
 #include "chrome/browser/sessions/session_restore.h"
+#include "chrome/browser/sessions/session_service.h"
 #include "chrome/browser/shell_integration.h"
 #include "chrome/browser/tabs/pinned_tab_codec.h"
 #include "chrome/browser/tab_contents/infobar_delegate.h"
@@ -489,7 +490,6 @@ bool BrowserInit::LaunchWithProfile::Launch(Profile* profile,
     std::vector<GURL> urls_to_open = GetURLsFromCommandLine(profile_);
     RecordLaunchModeHistogram(urls_to_open.empty()?
                               LM_TO_BE_DECIDED : LM_WITH_URLS);
-
     // TODO(viettrungluu): Temporary: Display a EULA before allowing the user to
     // actually enable Flash, unless they've already accepted it. Process this
     // in the same way as command-line URLs.
@@ -506,16 +506,7 @@ bool BrowserInit::LaunchWithProfile::Launch(Profile* profile,
       }
     }
 
-    if (!process_startup || !OpenStartupURLs(urls_to_open)) {
-      // Add the home page and any special first run URLs.
-      Browser* browser = NULL;
-      if (urls_to_open.empty())
-        AddStartupURLs(&urls_to_open);
-      else if (!command_line_.HasSwitch(switches::kOpenInNewWindow))
-        browser = BrowserList::GetLastActive();
-
-      OpenURLsInBrowser(browser, process_startup, urls_to_open);
-    }
+    ProcessLaunchURLs(process_startup, urls_to_open);
 
     // If this is an app launch, but we didn't open an app window, it may
     // be an app tab.
@@ -628,7 +619,36 @@ bool BrowserInit::LaunchWithProfile::OpenApplicationWindow(Profile* profile) {
   return false;
 }
 
-bool BrowserInit::LaunchWithProfile::OpenStartupURLs(
+void BrowserInit::LaunchWithProfile::ProcessLaunchURLs(
+    bool process_startup,
+    const std::vector<GURL>& urls_to_open) {
+  if (process_startup && ProcessStartupURLs(urls_to_open)) {
+    // ProcessStartupURLs processed the urls, nothing else to do.
+    return;
+  }
+
+  if (!process_startup &&
+      (profile_->GetSessionService() &&
+       profile_->GetSessionService()->RestoreIfNecessary(urls_to_open))) {
+    // We're already running and session restore wanted to run. This can happen
+    // at various points, such as if there is only an app window running and the
+    // user double clicked the chrome icon. Return so we don't open the urls.
+    return;
+  }
+
+  // Session restore didn't occur, open the urls.
+
+  Browser* browser = NULL;
+  std::vector<GURL> adjust_urls = urls_to_open;
+  if (adjust_urls.empty())
+    AddStartupURLs(&adjust_urls);
+  else if (!command_line_.HasSwitch(switches::kOpenInNewWindow))
+    browser = BrowserList::GetLastActive();
+
+  OpenURLsInBrowser(browser, process_startup, adjust_urls);
+}
+
+bool BrowserInit::LaunchWithProfile::ProcessStartupURLs(
     const std::vector<GURL>& urls_to_open) {
   SessionStartupPref pref = GetSessionStartupPref(command_line_, profile_);
   if (command_line_.HasSwitch(switches::kTestingChannelID) &&
diff --git a/chrome/browser/browser_init.h b/chrome/browser/browser_init.h
index 65d2ca9..2ed6bc7 100644
--- a/chrome/browser/browser_init.h
+++ b/chrome/browser/browser_init.h
@@ -142,6 +142,14 @@ class BrowserInit {
     // returns false to specify default processing.
     bool OpenApplicationWindow(Profile* profile);
 
+    // Invoked from OpenURLsInBrowser to handle processing of urls. This may
+    // do any of the following:
+    // . Invoke ProcessStartupURLs if |process_startup| is true.
+    // . Restore the last session if necessary.
+    // . Open the urls directly.
+    void ProcessLaunchURLs(bool process_startup,
+                           const std::vector<GURL>& urls_to_open);
+
     // Does the following:
     // . If the user's startup pref is to restore the last session (or the
     //   command line flag is present to force using last session), it is
@@ -153,7 +161,7 @@ class BrowserInit {
     //
     // Otherwise false is returned, which indicates the caller must create a
     // new browser.
-    bool OpenStartupURLs(const std::vector<GURL>& urls_to_open);
+    bool ProcessStartupURLs(const std::vector<GURL>& urls_to_open);
 
     // If the last session didn't exit cleanly and tab is a web contents tab,
     // an infobar is added allowing the user to restore the last session.
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);
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index a85992c..df9a869 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1271,6 +1271,7 @@
         'browser/gtk/bookmark_manager_browsertest.cc',
         'browser/net/cookie_policy_browsertest.cc',
         'browser/net/ftp_browsertest.cc',
+        'browser/sessions/session_restore_browsertest.cc',
         'browser/ssl/ssl_browser_tests.cc',
         'browser/task_manager_browsertest.cc',
         'renderer/form_autocomplete_unittest.cc',
diff --git a/chrome/test/ui_test_utils.cc b/chrome/test/ui_test_utils.cc
index f04b02e..781c918 100644
--- a/chrome/test/ui_test_utils.cc
+++ b/chrome/test/ui_test_utils.cc
@@ -214,60 +214,60 @@ class DownloadsCompleteObserver : public DownloadManager::Observer,
   DISALLOW_COPY_AND_ASSIGN(DownloadsCompleteObserver);
 };
 
-// Used to block until an application modal dialog is shown.
-class AppModalDialogObserver : public NotificationObserver {
+template <class T>
+class SimpleNotificationObserver : public NotificationObserver {
  public:
-  AppModalDialogObserver() : dialog_(NULL) {}
-
-  AppModalDialog* WaitForAppModalDialog() {
-    registrar_.Add(this, NotificationType::APP_MODAL_DIALOG_SHOWN,
-                   NotificationService::AllSources());
-    dialog_ = NULL;
+  SimpleNotificationObserver(NotificationType notification_type,
+                             T* source) {
+    registrar_.Add(this, notification_type, Source<T>(source));
     ui_test_utils::RunMessageLoop();
-    DCHECK(dialog_);
-    return dialog_;
   }
 
   virtual void Observe(NotificationType type,
                        const NotificationSource& source,
                        const NotificationDetails& details) {
-    if (type == NotificationType::APP_MODAL_DIALOG_SHOWN) {
-      registrar_.Remove(this, NotificationType::APP_MODAL_DIALOG_SHOWN,
-                        NotificationService::AllSources());
-      dialog_ = Source<AppModalDialog>(source).ptr();
-      MessageLoopForUI::current()->Quit();
-    } else {
-      NOTREACHED();
-    }
+    MessageLoopForUI::current()->Quit();
   }
 
  private:
   NotificationRegistrar registrar_;
 
-  AppModalDialog* dialog_;
-
-  DISALLOW_COPY_AND_ASSIGN(AppModalDialogObserver);
+  DISALLOW_COPY_AND_ASSIGN(SimpleNotificationObserver);
 };
 
-template <class T>
-class SimpleNotificationObserver : public NotificationObserver {
+// SimpleNotificationObserver that waits for a single notification. When the
+// notification is observer the source (of type S) is recorded and the message
+// loop stopped. Use |source()| to access the source after the constructor
+// returns.
+template <class S>
+class SimpleNotificationObserverWithSource : public NotificationObserver {
  public:
-  SimpleNotificationObserver(NotificationType notification_type,
-                             T* source) {
-    registrar_.Add(this, notification_type, Source<T>(source));
+  SimpleNotificationObserverWithSource(NotificationType notification_type,
+                                       const NotificationSource& source)
+      : source_(NULL) {
+    registrar_.Add(this, notification_type, source);
     ui_test_utils::RunMessageLoop();
   }
 
   virtual void Observe(NotificationType type,
                        const NotificationSource& source,
                        const NotificationDetails& details) {
+    source_ = Source<S>(source).ptr();
+
+    // Remove observer now, so that if there any other notifications we don't
+    // clobber source_.
+    registrar_.RemoveAll();
+
     MessageLoopForUI::current()->Quit();
   }
 
+  S* source() const { return source_; }
+
  private:
+  S* source_;
   NotificationRegistrar registrar_;
 
-  DISALLOW_COPY_AND_ASSIGN(SimpleNotificationObserver);
+  DISALLOW_COPY_AND_ASSIGN(SimpleNotificationObserverWithSource);
 };
 
 class LanguageDetectionNotificationObserver : public NotificationObserver {
@@ -447,6 +447,13 @@ void WaitForLoadStop(NavigationController* controller) {
       new_tab_observer(NotificationType::LOAD_STOP, controller);
 }
 
+Browser* WaitForNewBrowser() {
+  SimpleNotificationObserverWithSource<Browser> observer(
+      NotificationType::BROWSER_WINDOW_READY,
+      NotificationService::AllSources());
+  return observer.source();
+}
+
 void OpenURLOffTheRecord(Profile* profile, const GURL& url) {
   Browser::OpenURLOffTheRecord(profile, url);
   Browser* browser = BrowserList::FindBrowserWithType(
@@ -556,8 +563,10 @@ void WaitForDownloadCount(DownloadManager* download_manager, size_t count) {
 }
 
 AppModalDialog* WaitForAppModalDialog() {
-  AppModalDialogObserver observer;
-  return observer.WaitForAppModalDialog();
+  SimpleNotificationObserverWithSource<AppModalDialog> observer(
+      NotificationType::APP_MODAL_DIALOG_SHOWN,
+      NotificationService::AllSources());
+  return observer.source();
 }
 
 void CrashTab(TabContents* tab) {
diff --git a/chrome/test/ui_test_utils.h b/chrome/test/ui_test_utils.h
index eca6fe5..639acbf 100644
--- a/chrome/test/ui_test_utils.h
+++ b/chrome/test/ui_test_utils.h
@@ -72,6 +72,9 @@ void WaitForBrowserActionUpdated(ExtensionAction* browser_action);
 // Waits for a load stop for the specified |controller|.
 void WaitForLoadStop(NavigationController* controller);
 
+// Waits for a new browser to be created, returning the browser.
+Browser* WaitForNewBrowser();
+
 // Opens |url| in an incognito browser window with the off the record profile of
 // |profile|, blocking until the navigation finishes. This will create a new
 // browser if a browser with the off the record profile does not exist.