[ TabRestore ] Serialize Saved Group Id for saved groups / tabs

Update the tab restore Tab and Group pickles to keep track of the saved group id. This must be done to prevent duplicating a saved group when we restore an entry after the browser has been restarted.

Change-Id: I90a6724e7890bb8a8b1502e95e5d2adb427f1f3b
Bug: 330769691, 330769723
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5498867
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Eshwar Stalin <estalin@chromium.org>
Commit-Queue: Darryl James <dljames@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1294912}

chrome/browser/sessions/tab_restore_browsertest.cc

@@ -2746,3 +2746,104 @@ IN_PROC_BROWSER_TEST_F(TabRestoreSavedGroupsTest,
   EXPECT_EQ(saved_group_id, saved_group.saved_guid());
   EXPECT_EQ(2u, saved_group.saved_tabs().size());
 }
+
+IN_PROC_BROWSER_TEST_F(TabRestoreSavedGroupsTest,
+                       PRE_SavedGroupNotDuplicatedAfterRestart) {
+  // Enable session service in default mode.
+  EnableSessionService();
+
+  // Navigate to url1 in the current tab.
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), url1_, WindowOpenDisposition::CURRENT_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP);
+
+  // Add the tab to a group.
+  tab_groups::TabGroupId group =
+      browser()->tab_strip_model()->AddToNewGroup({0});
+
+  // Save it.
+  tab_groups::SavedTabGroupKeyedService* service =
+      tab_groups::SavedTabGroupServiceFactory::GetForProfile(
+          browser()->profile());
+  CHECK(service);
+  service->SaveGroup(group);
+}
+
+IN_PROC_BROWSER_TEST_F(TabRestoreSavedGroupsTest,
+                       SavedGroupNotDuplicatedAfterRestart) {
+  // Enable session service in default mode.
+  EnableSessionService();
+
+  tab_groups::SavedTabGroupKeyedService* service =
+      tab_groups::SavedTabGroupServiceFactory::GetForProfile(
+          browser()->profile());
+  CHECK(service);
+
+  // Verify there is only 1 saved group in the model and it is not open.
+  ASSERT_EQ(1, service->model()->Count());
+  tab_groups::SavedTabGroup saved_group =
+      service->model()->saved_tab_groups()[0];
+  EXPECT_EQ(std::nullopt, saved_group.local_group_id());
+
+  const base::Uuid& saved_id = saved_group.saved_guid();
+
+  // Restore the group.
+  // We use this over RestoreGroup() since we don't have reference to the
+  // previous group id defined in the PRE step to this test.
+  chrome::RestoreTab(browser());
+
+  // Verify the browser has a single tab group.
+  TabGroupModel* group_model = browser()->tab_strip_model()->group_model();
+  EXPECT_EQ(1u, group_model->ListTabGroups().size());
+
+  // Verify there is still only 1 saved group and that it is open now.
+  EXPECT_EQ(1, service->model()->Count());
+  saved_group = *service->model()->Get(saved_id);
+  EXPECT_TRUE(saved_group.local_group_id().has_value());
+
+  // Verify the local group id exists in the TabGroupModel.
+  EXPECT_TRUE(
+      group_model->ContainsTabGroup(saved_group.local_group_id().value()));
+}
+
+IN_PROC_BROWSER_TEST_F(TabRestoreSavedGroupsTest,
+                       PRE_UnsavedGroupSavedAfterRestart) {
+  // Enable session service in default mode.
+  EnableSessionService();
+
+  // Navigate to url1 in the current tab.
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), url1_, WindowOpenDisposition::CURRENT_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP);
+
+  // Add the tab to a group.
+  browser()->tab_strip_model()->AddToNewGroup({0});
+}
+
+IN_PROC_BROWSER_TEST_F(TabRestoreSavedGroupsTest,
+                       UnsavedGroupSavedAfterRestart) {
+  // Enable session service in default mode.
+  EnableSessionService();
+
+  tab_groups::SavedTabGroupKeyedService* service =
+      tab_groups::SavedTabGroupServiceFactory::GetForProfile(
+          browser()->profile());
+  CHECK(service);
+
+  // Verify no groups were added to the model.
+  EXPECT_TRUE(service->model()->IsEmpty());
+
+  // Restore the group.
+  // We use this over RestoreGroup() since we don't have reference to the
+  // previous group id defined in the PRE step to this test.
+  chrome::RestoreTab(browser());
+
+  // Verify the browser has a single tab group.
+  TabGroupModel* group_model = browser()->tab_strip_model()->group_model();
+  ASSERT_EQ(1u, group_model->ListTabGroups().size());
+  tab_groups::TabGroupId id = group_model->ListTabGroups()[0];
+
+  // The tab group in the browser is linked to the saved group in the model.
+  EXPECT_EQ(1, service->model()->Count());
+  EXPECT_TRUE(service->model()->Contains(id));
+}

components/sessions/core/tab_restore_service_impl.cc

@@ -7,6 +7,7 @@
 #include <stddef.h>
 #include <stdint.h>
 #include <string.h>
+
 #include <map>
 #include <utility>
 #include <vector>
@@ -22,6 +23,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/notreached.h"
 #include "base/time/time.h"
+#include "base/uuid.h"
 #include "components/history/core/common/pref_names.h"
 #include "components/prefs/pref_service.h"
 #include "components/sessions/core/base_session_service_commands.h"
@@ -408,6 +410,8 @@ struct GroupCommandFields {
   int browser_id = 0;
   std::u16string title;
   uint32_t color = 0;
+  bool is_saved;
+  std::string saved_id;
 };
 
 std::unique_ptr<sessions::tab_restore::Group> CreateGroupEntryFromCommand(
@@ -432,6 +436,14 @@ std::unique_ptr<sessions::tab_restore::Group> CreateGroupEntryFromCommand(
     return nullptr;
   }
 
+  if (it.ReadBool(&parsed_fields.is_saved) && parsed_fields.is_saved) {
+    if (!it.ReadString(&parsed_fields.saved_id) ||
+        parsed_fields.saved_id.empty()) {
+      // A saved group must have a saved id.
+      return nullptr;
+    }
+  }
+
   // Copy the parsed data.
   GroupCommandFields fields = parsed_fields;
 
@@ -440,6 +452,10 @@ std::unique_ptr<sessions::tab_restore::Group> CreateGroupEntryFromCommand(
       std::make_unique<sessions::tab_restore::Group>();
   group->group_id =
       tab_groups::TabGroupId::FromRawToken(fields.tab_group_token.value());
+  if (fields.is_saved) {
+    group->saved_group_id = base::Uuid::ParseLowercase(fields.saved_id);
+  }
+
   group->browser_id = fields.browser_id;
   group->visual_data =
       tab_groups::TabGroupVisualData(fields.title, fields.color);
@@ -531,6 +547,7 @@ class TabRestoreServiceImpl::PersistenceDelegate
       SessionID session_id,
       size_t num_tabs,
       tab_groups::TabGroupId group_id,
+      std::optional<base::Uuid> saved_group_id,
       SessionID::id_type browser_id,
       tab_groups::TabGroupVisualData visual_data);
 
@@ -820,9 +837,9 @@ void TabRestoreServiceImpl::PersistenceDelegate::ScheduleCommandsForGroup(
     const tab_restore::Group& group) {
   DCHECK(!group.tabs.empty());
 
-  command_storage_manager_->ScheduleCommand(
+  command_storage_manager_->ScheduleCommand(CreateGroupCommand(
-      CreateGroupCommand(group.id, group.tabs.size(), group.group_id,
+      group.id, group.tabs.size(), group.group_id, group.saved_group_id,
-                         group.browser_id, group.visual_data));
+      group.browser_id, group.visual_data));
   ScheduleCommandsForTabs(group.tabs);
 }
 
@@ -874,6 +891,13 @@ void TabRestoreServiceImpl::PersistenceDelegate::ScheduleCommandsForTab(
         &tab.group_visual_data.value();
     pickle.WriteString16(visual_data->title());
     pickle.WriteUInt32(static_cast<int>(visual_data->color()));
+
+    // Added in M126. Write the saved group id to the pickle if there is one.
+    pickle.WriteBool(tab.saved_group_id.has_value());
+    if (tab.saved_group_id.has_value()) {
+      pickle.WriteString(tab.saved_group_id.value().AsLowercaseString());
+    }
+
     std::unique_ptr<SessionCommand> command(
         new SessionCommand(kCommandSetTabGroupData, pickle));
     command_storage_manager_->ScheduleCommand(std::move(command));
@@ -953,6 +977,7 @@ TabRestoreServiceImpl::PersistenceDelegate::CreateGroupCommand(
     SessionID session_id,
     size_t num_tabs,
     tab_groups::TabGroupId tab_group_id,
+    std::optional<base::Uuid> saved_group_id,
     SessionID::id_type browser_id,
     tab_groups::TabGroupVisualData visual_data) {
   static_assert(sizeof(SessionID::id_type) == sizeof(int),
@@ -966,6 +991,12 @@ TabRestoreServiceImpl::PersistenceDelegate::CreateGroupCommand(
   pickle.WriteString16(visual_data.title());
   pickle.WriteUInt32(static_cast<int>(visual_data.color()));
 
+  // Added in M126. Write the saved group id to the pickle if there is one.
+  pickle.WriteBool(saved_group_id.has_value());
+  if (saved_group_id.has_value()) {
+    pickle.WriteString(saved_group_id.value().AsLowercaseString());
+  }
+
   std::unique_ptr<SessionCommand> command(
       new SessionCommand(kCommandCreateGroup, pickle));
   return command;
@@ -1221,11 +1252,16 @@ void TabRestoreServiceImpl::PersistenceDelegate::CreateEntriesFromCommands(
         std::optional<base::Token> group_token = ReadTokenFromPickle(&iter);
         std::u16string title;
         uint32_t color_int;
-        if (!iter.ReadString16(&title)) {
+        bool is_saved;
+        std::string saved_id;
+        if (!iter.ReadString16(&title) || !iter.ReadUInt32(&color_int)) {
           break;
         }
-        if (!iter.ReadUInt32(&color_int)) {
+        if (iter.ReadBool(&is_saved) && is_saved) {
-          break;
+          if (!iter.ReadString(&saved_id) || saved_id.empty()) {
+            break;
+          }
+          current_tab->saved_group_id = base::Uuid::ParseLowercase(saved_id);
         }
 
         current_tab->group =