Take 2 at this. The only change between this and the first is to add the GetType message to the end of the list and to keep the old message for

creating a new browser as well as adding one that takes the type. To change the params of the message requires updating the reference build. That's best done on a weekend when I'm bored.

Makes session restore on Chrome OS restore popups.

BUG=18862
TEST=none

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

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

4 files changed, 130 insertions, 23 deletions

diff --git a/chrome/browser/sessions/session_restore_uitest.cc b/chrome/browser/sessions/session_restore_uitest.cc
index 5c8157b..fd25362 100644
--- a/chrome/browser/sessions/session_restore_uitest.cc
+++ b/chrome/browser/sessions/session_restore_uitest.cc
@@ -7,6 +7,7 @@
 #include "base/scoped_ptr.h"
 #include "base/string_util.h"
 #include "chrome/app/chrome_dll_resource.h"
+#include "chrome/browser/defaults.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/automation/tab_proxy.h"
@@ -253,6 +254,54 @@ TEST_F(SessionRestoreUITest, ClosedTabStaysClosed) {
   ASSERT_TRUE(GetActiveTabURL() == url1_);
 }
 
+// Creates a tabbed browser and popup and makes sure we restore both.
+TEST_F(SessionRestoreUITest, NormalAndPopup) {
+  if (!browser_defaults::kRestorePopups)
+    return;  // Test only applicable if restoring popups.
+
+  NavigateToURL(url1_);
+
+  // Make sure we have one window.
+  int window_count;
+  ASSERT_TRUE(automation()->GetBrowserWindowCount(&window_count));
+  ASSERT_EQ(1, window_count);
+
+  // Open a popup.
+  ASSERT_TRUE(automation()->OpenNewBrowserWindow(BrowserProxy::TYPE_POPUP,
+                                                 true));
+  ASSERT_TRUE(automation()->GetBrowserWindowCount(&window_count));
+  ASSERT_EQ(2, window_count);
+
+  // Restart and make sure we have only one window with one tab and the url
+  // is url1_.
+  QuitBrowserAndRestore(1);
+
+  ASSERT_TRUE(automation()->GetBrowserWindowCount(&window_count));
+  ASSERT_EQ(2, window_count);
+
+  scoped_refptr<BrowserProxy> browser_proxy1(
+      automation()->GetBrowserWindow(0));
+  ASSERT_TRUE(browser_proxy1.get());
+
+  scoped_refptr<BrowserProxy> browser_proxy2(
+      automation()->GetBrowserWindow(1));
+  ASSERT_TRUE(browser_proxy2.get());
+
+  BrowserProxy::Type type1, type2;
+  ASSERT_TRUE(browser_proxy1->GetType(&type1));
+  ASSERT_TRUE(browser_proxy2->GetType(&type2));
+
+  // The order of whether the normal window or popup is first depends upon
+  // activation order, which is not necessarily consistant across runs.
+  if (type1 == BrowserProxy::TYPE_NORMAL) {
+    EXPECT_EQ(type2, BrowserProxy::TYPE_POPUP);
+  } else {
+    EXPECT_EQ(type1, BrowserProxy::TYPE_POPUP);
+    EXPECT_EQ(type2, BrowserProxy::TYPE_NORMAL);
+  }
+}
+
+
 #if defined(OS_WIN)
 // Creates a browser, goes incognito, closes browser, launches and make sure
 // we don't restore.
@@ -308,7 +357,8 @@ TEST_F(SessionRestoreUITest, TwoWindowsCloseOneRestoreOnlyOne) {
   ASSERT_EQ(1, window_count);
 
   // Open a second window.
-  ASSERT_TRUE(automation()->OpenNewBrowserWindow(true));
+  ASSERT_TRUE(automation()->OpenNewBrowserWindow(BrowserProxy::TYPE_NORMAL,
+                                                 true));
   ASSERT_TRUE(automation()->GetBrowserWindowCount(&window_count));
   ASSERT_EQ(2, window_count);
 
diff --git a/chrome/browser/sessions/session_service.cc b/chrome/browser/sessions/session_service.cc
index 9522cee..31c2966 100644
--- a/chrome/browser/sessions/session_service.cc
+++ b/chrome/browser/sessions/session_service.cc
@@ -119,14 +119,14 @@ struct PinnedStatePayload {
 
 SessionService::SessionService(Profile* profile)
     : BaseSessionService(SESSION_RESTORE, profile, FilePath()),
-      has_open_tabbed_browsers_(false),
+      has_open_trackable_browsers_(false),
       move_on_new_browser_(false) {
   Init();
 }
 
 SessionService::SessionService(const FilePath& save_path)
     : BaseSessionService(SESSION_RESTORE, NULL, save_path),
-      has_open_tabbed_browsers_(false),
+      has_open_trackable_browsers_(false),
       move_on_new_browser_(false) {
   Init();
 }
@@ -216,7 +216,7 @@ void SessionService::TabClosed(const SessionID& window_id,
     // User closed the last tab in the last tabbed browser. Don't mark the
     // tab closed.
     pending_tab_close_ids_.insert(tab_id.id());
-    has_open_tabbed_browsers_ = false;
+    has_open_trackable_browsers_ = false;
   }
 }
 
@@ -229,12 +229,12 @@ void SessionService::WindowClosing(const SessionID& window_id) {
   //
   // NOTE: if the user chooses the exit menu item session service is destroyed
   // and this code isn't hit.
-  if (has_open_tabbed_browsers_) {
-    // Closing a window can never make has_open_tabbed_browsers_ go from false
-    // to true, so only update it if already true.
-    has_open_tabbed_browsers_ = HasOpenTabbedBrowsers(window_id);
+  if (has_open_trackable_browsers_) {
+    // Closing a window can never make has_open_trackable_browsers_ go from
+    // false to true, so only update it if already true.
+    has_open_trackable_browsers_ = HasOpenTrackableBrowsers(window_id);
   }
-  if (!has_open_tabbed_browsers_)
+  if (!has_open_trackable_browsers_)
     pending_window_close_ids_.insert(window_id.id());
   else
     window_closing_ids_.insert(window_id.id());
@@ -252,8 +252,8 @@ void SessionService::WindowClosed(const SessionID& window_id) {
   } else if (pending_window_close_ids_.find(window_id.id()) ==
              pending_window_close_ids_.end()) {
     // We'll hit this if user closed the last tab in a window.
-    has_open_tabbed_browsers_ = HasOpenTabbedBrowsers(window_id);
-    if (!has_open_tabbed_browsers_)
+    has_open_trackable_browsers_ = HasOpenTrackableBrowsers(window_id);
+    if (!has_open_trackable_browsers_)
       pending_window_close_ids_.insert(window_id.id());
     else
       ScheduleCommand(CreateWindowClosedCommand(window_id.id()));
@@ -271,7 +271,7 @@ void SessionService::SetWindowType(const SessionID& window_id,
   // pending closes.
   CommitPendingCloses();
 
-  has_open_tabbed_browsers_ = true;
+  has_open_trackable_browsers_ = true;
   move_on_new_browser_ = true;
 
   ScheduleCommand(CreateSetWindowTypeCommand(window_id, type));
@@ -409,7 +409,7 @@ void SessionService::Observe(NotificationType type,
         return;
       }
 
-      if (!has_open_tabbed_browsers_ && !BrowserInit::InProcessStartup()) {
+      if (!has_open_trackable_browsers_ && !BrowserInit::InProcessStartup()) {
         // 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_) {
@@ -1019,7 +1019,7 @@ void SessionService::ScheduleReset() {
   if (!windows_tracking_.empty()) {
     // We're lazily created on startup and won't get an initial batch of
     // SetWindowType messages. Set these here to make sure our state is correct.
-    has_open_tabbed_browsers_ = true;
+    has_open_trackable_browsers_ = true;
     move_on_new_browser_ = true;
   }
   StartSaveTimer();
@@ -1108,7 +1108,7 @@ bool SessionService::IsOnlyOneTabLeft() {
   }
 
   // NOTE: This uses the original profile so that closing the last non-off the
-  // record window while there are open off the record window resets state).
+  // record window while there are open off the record windows resets state).
   int window_count = 0;
   for (BrowserList::const_iterator i = BrowserList::begin();
        i != BrowserList::end(); ++i) {
@@ -1127,7 +1127,7 @@ bool SessionService::IsOnlyOneTabLeft() {
   return true;
 }
 
-bool SessionService::HasOpenTabbedBrowsers(const SessionID& window_id) {
+bool SessionService::HasOpenTrackableBrowsers(const SessionID& window_id) {
   if (!profile()) {
     // We're testing, always return false.
     return true;
diff --git a/chrome/browser/sessions/session_service.h b/chrome/browser/sessions/session_service.h
index b3daf22..7fddaaa 100644
--- a/chrome/browser/sessions/session_service.h
+++ b/chrome/browser/sessions/session_service.h
@@ -9,6 +9,7 @@
 
 #include "base/basictypes.h"
 #include "chrome/browser/browser.h"
+#include "chrome/browser/defaults.h"
 #include "chrome/browser/sessions/base_session_service.h"
 #include "chrome/browser/sessions/session_id.h"
 #include "chrome/common/notification_observer.h"
@@ -304,16 +305,19 @@ class SessionService : public BaseSessionService,
   // our profile.
   bool IsOnlyOneTabLeft();
 
-  // Returns true if there are no open tabbed browser windows with our profile,
-  // or the only tabbed browser open has a session id of window_id.
-  bool HasOpenTabbedBrowsers(const SessionID& window_id);
+  // Returns true if there are open trackable browser windows whose ids do
+  // match |window_id| with our profile. A trackable window is a window from
+  // which |should_track_changes_for_browser_type| returns true. See
+  // |should_track_changes_for_browser_type| for details.
+  bool HasOpenTrackableBrowsers(const SessionID& window_id);
 
   // Returns true if changes to tabs in the specified window should be tracked.
   bool ShouldTrackChangesToWindow(const SessionID& window_id);
 
   // Returns true if we track changes to the specified browser type.
   static bool should_track_changes_for_browser_type(Browser::Type type) {
-    return type == Browser::TYPE_NORMAL;
+    return type == Browser::TYPE_NORMAL ||
+        (type == Browser::TYPE_POPUP && browser_defaults::kRestorePopups);
   }
 
   NotificationRegistrar registrar_;
@@ -347,11 +351,11 @@ class SessionService : public BaseSessionService,
   typedef std::set<SessionID::id_type> WindowsTracking;
   WindowsTracking windows_tracking_;
 
-  // Are there any open open tabbed browsers?
-  bool has_open_tabbed_browsers_;
+  // Are there any open trackable browsers?
+  bool has_open_trackable_browsers_;
 
   // If true and a new tabbed browser is created and there are no opened tabbed
-  // browser (has_open_tabbed_browsers_ is false), then the current session
+  // browser (has_open_trackable_browsers_ is false), then the current session
   // is made the previous session. See description above class for details on
   // current/previou session.
   bool move_on_new_browser_;
diff --git a/chrome/browser/sessions/session_service_unittest.cc b/chrome/browser/sessions/session_service_unittest.cc
index 19997e7..1fec7f3 100644
--- a/chrome/browser/sessions/session_service_unittest.cc
+++ b/chrome/browser/sessions/session_service_unittest.cc
@@ -397,6 +397,9 @@ TEST_F(SessionServiceTest, WindowCloseCommittedAfterNavigate) {
 
 // Makes sure we don't track popups.
 TEST_F(SessionServiceTest, IgnorePopups) {
+  if (browser_defaults::kRestorePopups)
+    return;  // This test is only applicable if popups aren't restored.
+
   SessionID window2_id;
   SessionID tab_id;
   SessionID tab2_id;
@@ -431,6 +434,56 @@ TEST_F(SessionServiceTest, IgnorePopups) {
   helper_.AssertNavigationEquals(nav1, tab->navigations[0]);
 }
 
+// Makes sure we track popups.
+TEST_F(SessionServiceTest, RestorePopup) {
+  if (!browser_defaults::kRestorePopups)
+    return;  // This test is only applicable if popups are restored.
+
+  SessionID window2_id;
+  SessionID tab_id;
+  SessionID tab2_id;
+  ASSERT_NE(window2_id.id(), window_id.id());
+
+  service()->SetWindowType(window2_id, Browser::TYPE_POPUP);
+  service()->SetWindowBounds(window2_id, window_bounds, false);
+
+  TabNavigation nav1(0, GURL("http://google.com"), GURL(),
+                     ASCIIToUTF16("abc"), "def",
+                     PageTransition::QUALIFIER_MASK);
+  TabNavigation nav2(0, GURL("http://google2.com"), GURL(),
+                     ASCIIToUTF16("abcd"), "defg",
+                     PageTransition::AUTO_BOOKMARK);
+
+  helper_.PrepareTabInWindow(window_id, tab_id, 0, true);
+  UpdateNavigation(window_id, tab_id, nav1, 0, true);
+
+  helper_.PrepareTabInWindow(window2_id, tab2_id, 0, false);
+  UpdateNavigation(window2_id, tab2_id, nav2, 0, true);
+
+  ScopedVector<SessionWindow> windows;
+  ReadWindows(&(windows.get()));
+
+  ASSERT_EQ(2U, windows->size());
+  int normal_index = windows[0]->type == Browser::TYPE_NORMAL ?
+      0 : 1;
+  int popup_index = normal_index == 0 ? 1 : 0;
+  ASSERT_EQ(0, windows[normal_index]->selected_tab_index);
+  ASSERT_EQ(window_id.id(), windows[normal_index]->window_id.id());
+  ASSERT_EQ(1U, windows[normal_index]->tabs.size());
+
+  SessionTab* tab = windows[normal_index]->tabs[0];
+  helper_.AssertTabEquals(window_id, tab_id, 0, 0, 1, *tab);
+  helper_.AssertNavigationEquals(nav1, tab->navigations[0]);
+
+  ASSERT_EQ(0, windows[popup_index]->selected_tab_index);
+  ASSERT_EQ(window2_id.id(), windows[popup_index]->window_id.id());
+  ASSERT_EQ(1U, windows[popup_index]->tabs.size());
+
+  tab = windows[popup_index]->tabs[0];
+  helper_.AssertTabEquals(window2_id, tab2_id, 0, 0, 1, *tab);
+  helper_.AssertNavigationEquals(nav2, tab->navigations[0]);
+}
+
 // Tests pruning from the front.
 TEST_F(SessionServiceTest, PruneFromFront) {
   const std::string base_url("http://google.com/");