Restoring a session should restore window minimization state

chrome/browser/sessions/session_service.cc

Index: chrome/browser/sessions/session_service.cc
diff --git a/chrome/browser/sessions/session_service.cc b/chrome/browser/sessions/session_service.cc
index ba4a4e0c8875c6b0657a3339da93fff1fc606493..afbe23a948d125958cdff194a29736e290933bdb 100644
--- a/chrome/browser/sessions/session_service.cc
+++ b/chrome/browser/sessions/session_service.cc
@@ -44,8 +44,7 @@ using base::Time;
 
 // Identifier for commands written to file.
 static const SessionCommand::id_type kCommandSetTabWindow = 0;
-// kCommandSetWindowBounds is no longer used (it's superseded by
-// kCommandSetWindowBounds2). I leave it here to document what it was.
+// OBSOLETE Superseded by kCommandSetWindowBounds3.
 // static const SessionCommand::id_type kCommandSetWindowBounds = 1;
 static const SessionCommand::id_type kCommandSetTabIndexInWindow = 2;
 static const SessionCommand::id_type kCommandTabClosed = 3;
@@ -56,11 +55,13 @@ static const SessionCommand::id_type kCommandUpdateTabNavigation = 6;
 static const SessionCommand::id_type kCommandSetSelectedNavigationIndex = 7;
 static const SessionCommand::id_type kCommandSetSelectedTabInIndex = 8;
 static const SessionCommand::id_type kCommandSetWindowType = 9;
-static const SessionCommand::id_type kCommandSetWindowBounds2 = 10;
+// OBSOLETE Superseded by kCommandSetWindowBounds3. Except for data migration.
+// static const SessionCommand::id_type kCommandSetWindowBounds2 = 10;
 static const SessionCommand::id_type
     kCommandTabNavigationPathPrunedFromFront = 11;
 static const SessionCommand::id_type kCommandSetPinnedState = 12;
 static const SessionCommand::id_type kCommandSetExtensionAppID = 13;
+static const SessionCommand::id_type kCommandSetWindowBounds3 = 14;
 
 // Every kWritesPerReset commands triggers recreating the file.
 static const int kWritesPerReset = 250;
@@ -105,6 +106,15 @@ struct WindowBoundsPayload2 {
   bool is_maximized;
 };
 
+struct WindowBoundsPayload3 {
+  SessionID::id_type window_id;
+  int32 x;
+  int32 y;
+  int32 w;
+  int32 h;
+  int32 show_state;
+};
+
 struct IDAndIndexPayload {
   SessionID::id_type id;
   int32 index;
@@ -188,12 +198,11 @@ void SessionService::SetTabWindow(const SessionID& window_id,
 
 void SessionService::SetWindowBounds(const SessionID& window_id,
                                      const gfx::Rect& bounds,
-                                     bool is_maximized) {
+                                     ui::WindowShowState show_state) {
   if (!ShouldTrackChangesToWindow(window_id))
     return;
 
-  ScheduleCommand(CreateSetWindowBoundsCommand(window_id, bounds,
-                                               is_maximized));
+  ScheduleCommand(CreateSetWindowBoundsCommand(window_id, bounds, show_state));
 }
 
 void SessionService::SetTabIndexInWindow(const SessionID& window_id,
@@ -667,15 +676,15 @@ SessionCommand* SessionService::CreateSetTabWindowCommand(
 SessionCommand* SessionService::CreateSetWindowBoundsCommand(
     const SessionID& window_id,
     const gfx::Rect& bounds,
-    bool is_maximized) {
-  WindowBoundsPayload2 payload = { 0 };
+    ui::WindowShowState show_state) {
+  WindowBoundsPayload3 payload = { 0 };
   payload.window_id = window_id.id();
   payload.x = bounds.x();
   payload.y = bounds.y();
   payload.w = bounds.width();
   payload.h = bounds.height();
-  payload.is_maximized = is_maximized;
-  SessionCommand* command = new SessionCommand(kCommandSetWindowBounds2,
+  payload.show_state = show_state;
+  SessionCommand* command = new SessionCommand(kCommandSetWindowBounds3,
                                                sizeof(payload));
   memcpy(command->contents(), &payload, sizeof(payload));
   return command;
@@ -925,6 +934,7 @@ bool SessionService::CreateTabsAndWindows(
 
   for (std::vector<SessionCommand*>::const_iterator i = data.begin();
        i != data.end(); ++i) {
+    const SessionCommand::id_type kCommandSetWindowBounds2 = 10;
     const SessionCommand* command = *i;
 
     switch (command->id()) {
@@ -936,6 +946,8 @@ bool SessionService::CreateTabsAndWindows(
         break;
       }
 
+      // This is here for forward migration only.  New data is saved with
+      // |kCommandSetWindowBounds3|.
       case kCommandSetWindowBounds2: {
         WindowBoundsPayload2 payload;
         if (!command->GetPayload(&payload, sizeof(payload)))
@@ -944,8 +956,37 @@ bool SessionService::CreateTabsAndWindows(
                                                               payload.y,
                                                               payload.w,
                                                               payload.h);
-        GetWindow(payload.window_id, windows)->is_maximized =
-            payload.is_maximized;
+        GetWindow(payload.window_id, windows)->show_state =
+            payload.is_maximized ?
+                ui::SHOW_STATE_MAXIMIZED : ui::SHOW_STATE_NORMAL;

[sky, 2011/08/19 16:59:54]

Should this be _DEFAULT?

[dhollowa, 2011/08/19 17:58:53]

No, the DEFAULT state is an indeterminate state, used by the clients for
managing restoration logic.  The saved state here is explicit.

On 2011/08/19 16:59:54, sky wrote:

+        break;
+      }
+
+      case kCommandSetWindowBounds3: {
+        WindowBoundsPayload3 payload;
+        if (!command->GetPayload(&payload, sizeof(payload)))
+          return true;
+        GetWindow(payload.window_id, windows)->bounds.SetRect(payload.x,
+                                                              payload.y,
+                                                              payload.w,
+                                                              payload.h);
+        ui::WindowShowState show_state = ui::SHOW_STATE_DEFAULT;
+        switch (payload.show_state) {

[sky, 2011/08/19 16:59:54]

How about defining a min/max value and making sure payload.show_state is within
that range?

[dhollowa, 2011/08/19 17:58:53]

Done.

+          case 1:
+            show_state = ui::SHOW_STATE_NORMAL;
+            break;
+          case 2:
+            show_state = ui::SHOW_STATE_MINIMIZED;
+            break;
+          case 3:
+            show_state = ui::SHOW_STATE_MAXIMIZED;
+            break;
+          default:
+            show_state = ui::SHOW_STATE_NORMAL;
+            NOTREACHED();
+            break;
+        }
+        GetWindow(payload.window_id, windows)->show_state = show_state;
         break;
       }
 
@@ -1137,10 +1178,18 @@ void SessionService::BuildCommandsForBrowser(
   DCHECK(browser && commands);
   DCHECK(browser->session_id().id());
 
+  ui::WindowShowState show_state = ui::SHOW_STATE_DEFAULT;

[sky, 2011/08/19 16:59:54]

nit: make this normal and remove last else clause.

[dhollowa, 2011/08/19 17:58:53]

Done.

+  if (browser->window()->IsMaximized())
+    show_state = ui::SHOW_STATE_MAXIMIZED;
+  else if (browser->window()->IsMinimized())
+    show_state = ui::SHOW_STATE_MINIMIZED;
+  else
+    show_state = ui::SHOW_STATE_NORMAL;
+
   commands->push_back(
       CreateSetWindowBoundsCommand(browser->session_id(),
                                    browser->window()->GetRestoredBounds(),
-                                   browser->window()->IsMaximized()));
+                                   show_state));
 
   commands->push_back(CreateSetWindowTypeCommand(
       browser->session_id(), WindowTypeForBrowserType(browser->type())));