Ensure History > Recent Tabs restore preserves window disposition.

components/sessions/core/persistent_tab_restore_service.cc

Index: components/sessions/core/persistent_tab_restore_service.cc
diff --git a/components/sessions/core/persistent_tab_restore_service.cc b/components/sessions/core/persistent_tab_restore_service.cc
index d9016ee0369073fcc59ca8e04381f34fff8eb73c..5dc8fe9624fbbb23ddd913e9bd8325695066edc6 100644
--- a/components/sessions/core/persistent_tab_restore_service.cc
+++ b/components/sessions/core/persistent_tab_restore_service.cc
@@ -51,8 +51,9 @@ struct WindowPayload {
   int32_t num_tabs;
 };
 
-// Payload used for the start of a window close.  This struct must be POD,
-// because we memset the contents.
+// Payload used for the start of a window close. This struct must be POD,
+// because we memset the contents. This is an older version of the struct that
+// is used for backwards compat when it comes to reading the session files.
 struct WindowPayload2 : WindowPayload {

[sky, 2017/06/28 22:38:01]

How about adding Obsolete as a suffix to this and WindowPayload?

[chrisha, 2017/06/30 15:42:12]

Done.

   int64_t timestamp;
 };
@@ -135,6 +136,107 @@ void RemoveEntryByID(
   }
 }
 
+// Superset of WindowPayload/WindowPayload2 and the other fields that can appear
+// in the Pickle version of a Window command.
+struct WindowCommandFields {
+  // Fields in WindowPayload/WindowPayload2/Pickle:
+  SessionID::id_type window_id = 0;
+  int32_t selected_tab_index = 0;
+  int32_t num_tabs = 0;
+
+  // Fields in WindowPayload2/Pickle:
+  int64_t timestamp = 0;
+
+  // Fields in Pickle:
+  // Completely zeroed position/dimensions indicates that defaults should be
+  // used.
+  int32_t window_x = 0;
+  int32_t window_y = 0;
+  int32_t window_width = 0;
+  int32_t window_height = 0;
+  // Use the default show state if none is specified.
+  int32_t window_show_state = static_cast<int32_t>(ui::SHOW_STATE_DEFAULT);
+  std::string workspace;
+};
+
+std::unique_ptr<sessions::TabRestoreService::Window>
+CreateWindowEntryFromCommand(const SessionCommand* command,
+                             SessionID::id_type* window_id,
+                             int32_t* num_tabs) {
+  bool parsed = false;
+
+  WindowCommandFields fields;
+
+  // First try to parse the command as a pickle (the most modern version).
+  std::unique_ptr<base::Pickle> pickle(command->PayloadAsPickle());
+  if (pickle.get()) {
+    base::PickleIterator it(*pickle);
+    WindowCommandFields f;

[sky, 2017/06/28 22:38:00]

Please use a more readable name then 'f'.

[chrisha, 2017/06/30 15:42:12]

Done.

+
+    // The first version of the pickle contains all of the following fields, so
+    // they should all successfully parse if the command is in fact a pickle.
+    if (it.ReadInt(&f.window_id) && it.ReadInt(&f.selected_tab_index) &&
+        it.ReadInt(&f.num_tabs) && it.ReadInt64(&f.timestamp) &&
+        it.ReadInt(&f.window_x) && it.ReadInt(&f.window_y) &&
+        it.ReadInt(&f.window_width) && it.ReadInt(&f.window_height) &&
+        it.ReadInt(&f.window_show_state) && it.ReadString(&f.workspace)) {
+      // New fields added to the pickle in later versions would be parsed here.
+
+      // Copy the parsed data.
+      fields = f;
+      parsed = true;
+    }
+  }
+
+  // Next try to parse the command as a WindowPayload2.
+  if (!parsed) {
+    WindowPayload2 payload2;
+    if (command->GetPayload(&payload2, sizeof(payload2))) {
+      fields.window_id = payload2.window_id;
+      fields.selected_tab_index = payload2.selected_tab_index;
+      fields.num_tabs = payload2.num_tabs;
+      fields.timestamp = payload2.timestamp;
+      parsed = true;
+    }
+  }
+
+  // Finally, try the oldest WindowPayload type.
+  if (!parsed) {
+    WindowPayload payload;
+    if (command->GetPayload(&payload, sizeof(payload))) {
+      fields.window_id = payload.window_id;
+      fields.selected_tab_index = payload.selected_tab_index;
+      fields.num_tabs = payload.num_tabs;
+      parsed = true;
+    }
+  }
+
+  if (!parsed)
+    return nullptr;
+
+  // Create the Window entry.
+  std::unique_ptr<sessions::TabRestoreService::Window> window =
+      base::MakeUnique<sessions::TabRestoreService::Window>();
+  window->selected_tab_index = fields.selected_tab_index;
+  window->timestamp = base::Time::FromInternalValue(fields.timestamp);
+  window->selected_tab_index = fields.selected_tab_index;
+  window->timestamp = base::Time::FromInternalValue(fields.timestamp);
+  *window_id = fields.window_id;
+  *num_tabs = fields.num_tabs;
+
+  // Set the bounds, show state and workspace if valid ones have been provided.
+  if (!(fields.window_x == 0 && fields.window_y == 0 &&
+        fields.window_width == 0 && fields.window_height == 0)) {
+    window->bounds.SetRect(fields.window_x, fields.window_y,
+                           fields.window_width, fields.window_height);
+    window->show_state =
+        static_cast<ui::WindowShowState>(fields.window_show_state);

[sky, 2017/06/28 22:38:00]

You need to validate the enum.

[chrisha, 2017/06/30 15:42:13]

Done.

+    window->workspace = std::move(fields.workspace);
+  }
+
+  return window;
+}
+
 }  // namespace
 
 // PersistentTabRestoreService::Delegate ---------------------------------------
@@ -187,9 +289,12 @@ class PersistentTabRestoreService::Delegate
 
   // Creates a window close command.
   static std::unique_ptr<SessionCommand> CreateWindowCommand(
-      SessionID::id_type id,
+      SessionID::id_type window_id,
       int selected_tab_index,
       int num_tabs,
+      const gfx::Rect& bounds,
+      ui::WindowShowState show_state,
+      const std::string& workspace,
       base::Time timestamp);
 
   // Creates a tab close command.
@@ -435,7 +540,8 @@ void PersistentTabRestoreService::Delegate::ScheduleCommandsForWindow(
 
   base_session_service_->ScheduleCommand(CreateWindowCommand(
       window.id, std::min(real_selected_tab, valid_tab_count - 1),
-      valid_tab_count, window.timestamp));
+      valid_tab_count, window.bounds, window.show_state, window.workspace,
+      window.timestamp));
 
   if (!window.app_name.empty()) {
     base_session_service_->ScheduleCommand(CreateSetWindowAppNameCommand(
@@ -504,22 +610,36 @@ void PersistentTabRestoreService::Delegate::ScheduleCommandsForTab(
 // static
 std::unique_ptr<SessionCommand>
 PersistentTabRestoreService::Delegate::CreateWindowCommand(
-    SessionID::id_type id,
+    SessionID::id_type window_id,
     int selected_tab_index,
     int num_tabs,
+    const gfx::Rect& bounds,
+    ui::WindowShowState show_state,
+    const std::string& workspace,
     base::Time timestamp) {
-  WindowPayload2 payload;
-  // |timestamp| is aligned on a 16 byte boundary, leaving 4 bytes of
-  // uninitialized memory in the struct.
-  memset(&payload, 0, sizeof(payload));
-  payload.window_id = id;
-  payload.selected_tab_index = selected_tab_index;
-  payload.num_tabs = num_tabs;
-  payload.timestamp = timestamp.ToInternalValue();
+  // Use a pickle to handle marshaling as this command contains variable-length
+  // content.
+  base::Pickle pickle;
+  pickle.WriteInt(window_id);

[sky, 2017/06/28 22:38:00]

You need to make sure the writing, reading and representation of the types line
up. For example, here you use int, but window_id is an int32_t, not an int.

[chrisha, 2017/06/30 15:42:12]

Done.

+  pickle.WriteInt(selected_tab_index);
+  pickle.WriteInt(num_tabs);
+  pickle.WriteInt64(timestamp.ToInternalValue());
+  pickle.WriteInt(bounds.x());
+  pickle.WriteInt(bounds.y());
+  pickle.WriteInt(bounds.width());
+  pickle.WriteInt(bounds.height());
+  pickle.WriteInt(static_cast<int32_t>(show_state));

[sky, 2017/06/28 22:38:00]

I would prefer you mirror WindowShowState locally. That way if the values of the
WindowShowState enum change this code isn't impacted.

[chrisha, 2017/06/30 15:42:12]

Done.

+
+  // Enforce a maximum length on workspace names. A common size is 32 bytes for
+  // GUIDs.
+  if (workspace.size() <= 128) {

[sky, 2017/06/28 22:38:00]

no {}

[chrisha, 2017/06/30 15:42:13]

Done.

+    pickle.WriteString(workspace);
+  } else {
+    pickle.WriteString(std::string());
+  }
 
   std::unique_ptr<SessionCommand> command(
-      new SessionCommand(kCommandWindow, sizeof(payload)));
-  memcpy(command->contents(), &payload, sizeof(payload));
+      new SessionCommand(kCommandWindow, pickle));
   return command;
 }
 
@@ -625,42 +745,27 @@ void PersistentTabRestoreService::Delegate::CreateEntriesFromCommands(
       }
 
       case kCommandWindow: {
-        WindowPayload2 payload;
-        if (pending_window_tabs > 0) {
-          // Should never receive a window command while waiting for all the
-          // tabs in a window.
+        // Should never receive a window command while waiting for all the
+        // tabs in a window.
+        if (pending_window_tabs > 0)
           return;
-        }
-
-        // Try the new payload first
-        if (!command.GetPayload(&payload, sizeof(payload))) {
-          // then the old payload
-          WindowPayload old_payload;
-          if (!command.GetPayload(&old_payload, sizeof(old_payload)))
-            return;
 
-          // Copy the old payload data to the new payload.
-          payload.window_id = old_payload.window_id;
-          payload.selected_tab_index = old_payload.selected_tab_index;
-          payload.num_tabs = old_payload.num_tabs;
-          // Since we don't have a time use time 0 which is used to mark as an
-          // unknown timestamp.
-          payload.timestamp = 0;
-        }
-
-        pending_window_tabs = payload.num_tabs;
-        if (pending_window_tabs <= 0) {
-          // Should always have at least 1 tab. Likely indicates corruption.
+        // Try to parse the command, and silently skip if it fails.
+        int32_t num_tabs = 0;
+        SessionID::id_type window_id = 0;
+        std::unique_ptr<Window> window =
+            CreateWindowEntryFromCommand(&command, &window_id, &num_tabs);
+        if (!window)
           return;
-        }
 
-        RemoveEntryByID(payload.window_id, &entries);
+        // Should always have at least 1 tab. Likely indicates corruption.
+        pending_window_tabs = num_tabs;
+        if (pending_window_tabs <= 0)
+          return;
 
-        entries.push_back(base::MakeUnique<Window>());
-        current_window = static_cast<Window*>(entries.back().get());
-        current_window->selected_tab_index = payload.selected_tab_index;
-        current_window->timestamp =
-            base::Time::FromInternalValue(payload.timestamp);
+        RemoveEntryByID(window_id, &entries);
+        current_window = window.get();
+        entries.push_back(std::move(window));
         break;
       }
 
@@ -773,14 +878,12 @@ void PersistentTabRestoreService::Delegate::CreateEntriesFromCommands(
       }
 
       default:
-        // Unknown type, usually indicates corruption of file. Ignore it.

[sky, 2017/06/28 22:38:00]

Why are you removing this comment?

[chrisha, 2017/06/30 15:42:12]

Oops, stray line removal. Restored.

         return;
     }
   }
 
   // If there was corruption some of the entries won't be valid.
   ValidateAndDeleteEmptyEntries(&entries);
-
   loaded_entries->swap(entries);
 }
 
@@ -833,6 +936,9 @@ bool PersistentTabRestoreService::Delegate::ConvertSessionWindowToWindow(
       std::min(session_window->selected_tab_index,
                static_cast<int>(window->tabs.size() - 1));
   window->timestamp = base::Time();
+  window->bounds = session_window->bounds;
+  window->show_state = session_window->show_state;
+  window->workspace = session_window->workspace;
   return true;
 }