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

10 files changed, 172 insertions, 29 deletions

diff --git a/chrome/browser/automation/automation_provider.cc b/chrome/browser/automation/automation_provider.cc
index d791afb..fcd8d81 100644
--- a/chrome/browser/automation/automation_provider.cc
+++ b/chrome/browser/automation/automation_provider.cc
@@ -309,6 +309,7 @@ void AutomationProvider::OnMessageReceived(const IPC::Message& message) {
                                     WindowSimulateDrag)
 #endif  // defined(OS_WIN) || defined(OS_LINUX)
     IPC_MESSAGE_HANDLER(AutomationMsg_TabCount, GetTabCount)
+    IPC_MESSAGE_HANDLER(AutomationMsg_Type, GetType)
     IPC_MESSAGE_HANDLER(AutomationMsg_Tab, GetTab)
 #if defined(OS_WIN)
     IPC_MESSAGE_HANDLER(AutomationMsg_TabHWND, GetTabHWND)
@@ -332,6 +333,8 @@ void AutomationProvider::OnMessageReceived(const IPC::Message& message) {
     IPC_MESSAGE_HANDLER(AutomationMsg_SetProxyConfig, SetProxyConfig);
     IPC_MESSAGE_HANDLER_DELAY_REPLY(AutomationMsg_OpenNewBrowserWindow,
                                     OpenNewBrowserWindow)
+    IPC_MESSAGE_HANDLER_DELAY_REPLY(AutomationMsg_OpenNewBrowserWindowOfType,
+                                    OpenNewBrowserWindowOfType)
     IPC_MESSAGE_HANDLER(AutomationMsg_WindowForBrowser, GetWindowForBrowser)
     IPC_MESSAGE_HANDLER(AutomationMsg_AutocompleteEditForBrowser,
                         GetAutocompleteEditForBrowser)
@@ -881,6 +884,15 @@ void AutomationProvider::GetTabCount(int handle, int* tab_count) {
   }
 }
 
+void AutomationProvider::GetType(int handle, int* type_as_int) {
+  *type_as_int = -1;  // -1 is the error code
+
+  if (browser_tracker_->ContainsHandle(handle)) {
+    Browser* browser = browser_tracker_->GetResource(handle);
+    *type_as_int = static_cast<int>(browser->type());
+  }
+}
+
 void AutomationProvider::GetTab(int win_handle, int tab_index,
                                 int* tab_handle) {
   *tab_handle = 0;
@@ -1279,10 +1291,17 @@ void AutomationProvider::GetDownloadDirectory(
 
 void AutomationProvider::OpenNewBrowserWindow(bool show,
                                               IPC::Message* reply_message) {
+  OpenNewBrowserWindowOfType(static_cast<int>(Browser::TYPE_NORMAL), show,
+                             reply_message);
+}
+
+void AutomationProvider::OpenNewBrowserWindowOfType(
+    int type, bool show, IPC::Message* reply_message) {
   new BrowserOpenedNotificationObserver(this, reply_message);
   // We may have no current browser windows open so don't rely on
   // asking an existing browser to execute the IDC_NEWWINDOW command
-  Browser* browser = Browser::Create(profile_);
+  Browser* browser = new Browser(static_cast<Browser::Type>(type), profile_);
+  browser->CreateBrowserWindow();
   browser->AddBlankTab(true);
   if (show)
     browser->window()->Show();
diff --git a/chrome/browser/automation/automation_provider.h b/chrome/browser/automation/automation_provider.h
index 44ee790..65b7795 100644
--- a/chrome/browser/automation/automation_provider.h
+++ b/chrome/browser/automation/automation_provider.h
@@ -185,6 +185,7 @@ class AutomationProvider : public base::RefCounted<AutomationProvider>,
   void IsWindowMaximized(int handle, bool* is_maximized, bool* success);
 
   void GetTabCount(int handle, int* tab_count);
+  void GetType(int handle, int* type_as_int);
   void GetTab(int win_handle, int tab_index, int* tab_handle);
 #if defined(OS_WIN)
   // TODO(port): Replace HWND.
@@ -284,6 +285,9 @@ class AutomationProvider : public base::RefCounted<AutomationProvider>,
 
   // If |show| is true, call Show() on the new window after creating it.
   void OpenNewBrowserWindow(bool show, IPC::Message* reply_message);
+  void OpenNewBrowserWindowOfType(int type,
+                                  bool show,
+                                  IPC::Message* reply_message);
 
   void ShowInterstitialPage(int tab_handle,
                             const std::string& html_text,
diff --git a/chrome/browser/browser.h b/chrome/browser/browser.h
index dc465e8..73852e1 100644
--- a/chrome/browser/browser.h
+++ b/chrome/browser/browser.h
@@ -44,6 +44,9 @@ class Browser : public TabStripModelDelegate,
                 public NotificationObserver,
                 public SelectFileDialog::Listener {
  public:
+  // If you change the values in this enum you'll need to update browser_proxy.
+  // TODO(sky): move into a common place that is referenced by both ui_tests
+  // and chrome.
   enum Type {
     TYPE_NORMAL = 0,
     TYPE_POPUP = 1,
diff --git a/chrome/browser/defaults.cc b/chrome/browser/defaults.cc
index 098c6b9..916a50f 100644
--- a/chrome/browser/defaults.cc
+++ b/chrome/browser/defaults.cc
@@ -16,6 +16,7 @@ const SessionStartupPref::Type kDefaultSessionStartupType =
     SessionStartupPref::LAST;
 const int kPinnedTabWidth = 64;
 const bool kCanToggleSystemTitleBar = false;
+const bool kRestorePopups = true;
 
 #elif defined(OS_LINUX)
 
@@ -42,6 +43,7 @@ const bool kCanToggleSystemTitleBar = true;
 const SessionStartupPref::Type kDefaultSessionStartupType =
     SessionStartupPref::DEFAULT;
 const int kPinnedTabWidth = 56;
+const bool kRestorePopups = false;
 
 #endif
 
diff --git a/chrome/browser/defaults.h b/chrome/browser/defaults.h
index 814a616..45862c2 100644
--- a/chrome/browser/defaults.h
+++ b/chrome/browser/defaults.h
@@ -34,6 +34,9 @@ extern const SessionStartupPref::Type kDefaultSessionStartupType;
 // Width of pinned tabs.
 extern const int kPinnedTabWidth;
 
+// Should session restore restore popup windows?
+extern const bool kRestorePopups;
+
 }  // namespace browser_defaults
 
 #endif  // CHROME_BROWSER_DEFAULTS_H_
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/");
diff --git a/chrome/browser/tab_restore_uitest.cc b/chrome/browser/tab_restore_uitest.cc
index 491ba8a..0f99ad5 100644
--- a/chrome/browser/tab_restore_uitest.cc
+++ b/chrome/browser/tab_restore_uitest.cc
@@ -226,7 +226,8 @@ TEST_F(TabRestoreUITest, FLAKY_RestoreToDifferentWindow) {
   EXPECT_EQ(starting_tab_count + 2, tab_count);
 
   // Create a new browser.
-  ASSERT_TRUE(automation()->OpenNewBrowserWindow(false));
+  ASSERT_TRUE(automation()->OpenNewBrowserWindow(BrowserProxy::TYPE_NORMAL,
+                                                 false));
   int window_count;
   ASSERT_TRUE(automation()->GetBrowserWindowCount(&window_count));
   EXPECT_EQ(2, window_count);
@@ -268,7 +269,8 @@ TEST_F(TabRestoreUITest, MAYBE_BasicRestoreFromClosedWindow) {
   tab_proxy->NavigateToURL(url2_);
 
   // Create a new browser.
-  ASSERT_TRUE(automation()->OpenNewBrowserWindow(false));
+  ASSERT_TRUE(automation()->OpenNewBrowserWindow(BrowserProxy::TYPE_NORMAL,
+                                                 false));
   int window_count;
   ASSERT_TRUE(automation()->GetBrowserWindowCount(&window_count));
   EXPECT_EQ(2, window_count);
@@ -358,7 +360,8 @@ TEST_F(TabRestoreUITest, FLAKY_RestoreWindowAndTab) {
   EXPECT_EQ(starting_tab_count + 2, tab_count);
 
   // Create a new browser.
-  ASSERT_TRUE(automation()->OpenNewBrowserWindow(false));
+  ASSERT_TRUE(automation()->OpenNewBrowserWindow(BrowserProxy::TYPE_NORMAL,
+                                                 false));
   int window_count;
   ASSERT_TRUE(automation()->GetBrowserWindowCount(&window_count));
   EXPECT_EQ(2, window_count);
@@ -406,7 +409,8 @@ TEST_F(TabRestoreUITest, RestoreIntoSameWindow) {
   tab_proxy->NavigateToURL(url2_);
 
   // Create a new browser.
-  ASSERT_TRUE(automation()->OpenNewBrowserWindow(false));
+  ASSERT_TRUE(automation()->OpenNewBrowserWindow(BrowserProxy::TYPE_NORMAL,
+                                                 false));
   int window_count;
   ASSERT_TRUE(automation()->GetBrowserWindowCount(&window_count));
   EXPECT_EQ(2, window_count);
@@ -548,7 +552,8 @@ TEST_F(TabRestoreUITest, RestoreWindow) {
   // Create a new window.
   int window_count;
   ASSERT_TRUE(automation()->GetBrowserWindowCount(&window_count));
-  ASSERT_TRUE(automation()->OpenNewBrowserWindow(false));
+  ASSERT_TRUE(automation()->OpenNewBrowserWindow(BrowserProxy::TYPE_NORMAL,
+                                                 false));
   int new_window_count = 0;
   ASSERT_TRUE(automation()->GetBrowserWindowCount(&new_window_count));
   EXPECT_EQ(++window_count, new_window_count);