[TabRestore] Rewrite BrowserLiveTabContext::AddRestoredTab

AddRestoredTab does not take into account the context for which we are
restoring a tab. This change makes it so we know if the tab being
restored is by itself, or restored alongside other tabs within its
group or window.

This change is necessary to prevent tab duplication for saved groups as
we want SavedTabGroups to be the source of truth for restored groups.

Tab duplication was happening anytime there was a conflict between the
SavedTabGroup and the tab group persisted in TabRestore.

Now we only rely on the SavedTabGroup if we have one.

Change-Id: I6266e63f281832341a2b2cdc96de727f20b22882
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6381720
Reviewed-by: Eshwar Stalin <estalin@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Commit-Queue: Darryl James <dljames@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1438261}

chrome/browser/sessions/tab_restore_browsertest.cc

@@ -2559,9 +2559,9 @@ IN_PROC_BROWSER_TEST_P(TabRestoreSavedGroupsTest,
 }
 
 // Verify that any tabs that exist in the restored group but not the saved group
-// are added to the saved group.
+// are not added to the saved group.
 IN_PROC_BROWSER_TEST_P(TabRestoreSavedGroupsTest,
-                       RestoreSavedGroupAddsUniqueTabs) {
+                       RestoreSavedGroupDropsUniqueTabs) {
   if (tab_groups::IsTabGroupSyncServiceDesktopMigrationEnabled()) {
     // TODO(crbug.com/366267926): Fix test after migration.
     GTEST_SKIP()
@@ -2625,9 +2625,9 @@ IN_PROC_BROWSER_TEST_P(TabRestoreSavedGroupsTest,
   const std::optional<tab_groups::SavedTabGroup> restored_saved_group =
       service->GetGroup(saved_group_id);
 
-  // There are 2 www.youtube.com urls, only 1 should be added.
+  // There should still only be 2 tabs in the saved group.
   EXPECT_TRUE(restored_saved_group);
-  EXPECT_EQ(3u, restored_saved_group->saved_tabs().size());
+  EXPECT_EQ(2u, restored_saved_group->saved_tabs().size());
 }
 
 // Verify that a restored group which is already open does not open a new group
@@ -3188,8 +3188,9 @@ IN_PROC_BROWSER_TEST_P(TabRestoreSavedGroupsTest,
   EXPECT_EQ(1u, service->GetAllGroups().size());
   saved_group = *service->GetGroup(saved_id);
   EXPECT_TRUE(saved_group.local_group_id().has_value());
+  EXPECT_EQ(1u, group_model->ListTabGroups().size());
 
-  // Verify the local group id exists in the TabGroupModel.
+  // Verify the local group id exists in the TabGroupModel of the new browser.
   EXPECT_TRUE(
       group_model->ContainsTabGroup(saved_group.local_group_id().value()));
 }

chrome/browser/sessions/tab_restore_service_unittest.cc

@@ -123,6 +123,7 @@ class MockLiveTabContext : public sessions::LiveTabContext {
               ((const sessions::tab_restore::Tab&),
                int,
                bool,
+               bool,
                sessions::tab_restore::Type),
               (override));
   MOCK_METHOD(sessions::LiveTab*,

chrome/browser/ui/android/tab_model/android_live_tab_context.cc

@@ -144,6 +144,7 @@ sessions::LiveTab* AndroidLiveTabContext::AddRestoredTab(
     const sessions::tab_restore::Tab& tab,
     int tab_index,
     bool select,
+    bool restored_from_group_or_window_context,
     sessions::tab_restore::Type original_session_type) {
   Profile* profile = tab_model_->GetProfile();
 

chrome/browser/ui/android/tab_model/android_live_tab_context.h

@@ -66,6 +66,7 @@ class AndroidLiveTabContext : public sessions::LiveTabContext {
       const sessions::tab_restore::Tab& tab,
       int tab_index,
       bool select,
+      bool restored_from_group_or_window_context,
       sessions::tab_restore::Type original_session_type) override;
   sessions::LiveTab* ReplaceRestoredTab(
       const sessions::tab_restore::Tab&) override;

chrome/browser/ui/android/tab_model/android_live_tab_context_wrapper.cc

@@ -104,9 +104,11 @@ sessions::LiveTab* AndroidLiveTabContextRestoreWrapper::AddRestoredTab(
     const sessions::tab_restore::Tab& tab,
     int tab_index,
     bool select,
+    bool restored_from_group_or_window_context,
     sessions::tab_restore::Type original_session_type) {
-  auto* live_tab = AndroidLiveTabContext::AddRestoredTab(tab, tab_index, select,
-                                                         original_session_type);
+  auto* live_tab = AndroidLiveTabContext::AddRestoredTab(
+      tab, tab_index, select, restored_from_group_or_window_context,
+      original_session_type);
   if (tab.group) {
     TabAndroid* restored_tab = TabAndroid::FromWebContents(
         static_cast<sessions::ContentLiveTab*>(live_tab)->web_contents());

chrome/browser/ui/android/tab_model/android_live_tab_context_wrapper.h

@@ -149,6 +149,7 @@ class AndroidLiveTabContextRestoreWrapper : public AndroidLiveTabContext {
       const sessions::tab_restore::Tab& tab,
       int tab_index,
       bool select,
+      bool restored_from_group_or_window_context,
       sessions::tab_restore::Type original_session_type) override;
 
   // Returns the TabGroup data aggregated via AddRestoredTab.

chrome/browser/ui/browser_live_tab_context.cc

@@ -195,6 +195,7 @@ sessions::LiveTab* BrowserLiveTabContext::AddRestoredTab(
     const sessions::tab_restore::Tab& tab,
     int tab_index,
     bool select,
+    bool restored_from_group_or_window_context,
     sessions::tab_restore::Type original_session_type) {
   tab_groups::TabGroupSyncService* tab_group_service =
       tab_groups::SavedTabGroupUtils::GetServiceForProfile(browser_->profile());
@@ -222,7 +223,7 @@ sessions::LiveTab* BrowserLiveTabContext::AddRestoredTab(
       group_id.has_value() && saved_group_id.has_value() &&
       !tab_group_service->GetGroup(saved_group_id.value()).has_value();
   if (is_normal_tab || is_grouped_tab_unsaved || group_deleted_from_model) {
-    // This is either a normal tab or tab in an unsaved group.
+    // Add the tab to the browser.
     web_contents = chrome::AddRestoredTab(
         browser_, tab.navigations, tab_index, tab.normalized_navigation_index(),
         tab.extension_app_id, group_id, select, tab.pinned, base::TimeTicks(),
@@ -230,64 +231,61 @@ sessions::LiveTab* BrowserLiveTabContext::AddRestoredTab(
         tab.extra_data,
         /*from_session_restore=*/false, /*is_active_browser=*/std::nullopt);
 
-    if (is_grouped_tab_unsaved || group_deleted_from_model) {
+    if (group_id.has_value() &&
+        !tab_group_service->GetGroup(group_id.value()).has_value()) {
+      // It's possible a tab's group was deleted or was unsaved before this tab
+      // was restored. In that case, if the local group didn't become saved add
+      // the visual metadata and save it manually.
       browser_->live_tab_context()->SetVisualDataForGroup(
           group_id.value(), tab.group_visual_data.value());
-
-      // Save the group if it was not saved.
-      if (!tab_group_service->GetGroup(group_id.value()).has_value()) {
-        tab_group_service->SaveGroup(
-            tab_groups::SavedTabGroupUtils::CreateSavedTabGroupFromLocalId(
-                tab.group.value()));
-      }
+      tab_group_service->SaveGroup(
+          tab_groups::SavedTabGroupUtils::CreateSavedTabGroupFromLocalId(
+              tab.group.value()));
     }
   } else {
-    CHECK(tab_group_service->GetGroup(saved_group_id.value()).has_value());
-
-    const std::optional<tab_groups::SavedTabGroup> saved_group =
+    std::optional<tab_groups::SavedTabGroup> saved_group =
         tab_group_service->GetGroup(saved_group_id.value());
+    CHECK(saved_group);
     group_id = saved_group->local_group_id();
 
-    if (!group_id.has_value()) {
+    if (group_id) {
+      Browser* source_browser =
+          tab_groups::SavedTabGroupUtils::GetBrowserWithTabGroupId(
+              group_id.value());
+      tab_groups::SavedTabGroupUtils::FocusFirstTabOrWindowInOpenGroup(
+          group_id.value());
+
+      // Move the group into `browser_` if it is open in a different browser.
+      if (source_browser != browser_) {
+        tab_groups::SavedTabGroupUtils::MoveGroupToExistingWindow(
+            source_browser, browser_, group_id.value(), saved_group_id.value());
+      }
+    } else {
       // Open the group in this browser if it is closed.
       group_id = tab_group_service->OpenTabGroup(
           saved_group_id.value(),
           std::make_unique<tab_groups::TabGroupActionContextDesktop>(
               browser_, tab_groups::OpeningSource::kOpenedFromTabRestore));
-    } else {
-      Browser* source_browser =
-          tab_groups::SavedTabGroupUtils::GetBrowserWithTabGroupId(
-              group_id.value());
-      CHECK(source_browser);
-      tab_groups::SavedTabGroupUtils::FocusFirstTabOrWindowInOpenGroup(
-          group_id.value());
-      if (source_browser != browser_) {
-        // Move the group to this browser if it was open somewhere else.
-        tab_groups::SavedTabGroupUtils::MoveGroupToExistingWindow(
-            source_browser, browser_, group_id.value(), saved_group_id.value());
-      }
     }
 
-    std::unordered_set<std::string> saved_urls =
-        tab_groups::SavedTabGroupUtils::GetURLsInSavedTabGroup(
-            browser_->profile(), saved_group_id.value());
-    const sessions::SerializedNavigationEntry& entry =
-        tab.navigations.at(tab.normalized_navigation_index());
-    if (!saved_urls.contains(entry.virtual_url().spec())) {
-      // Restore the tab at the end of the group if we don't have it.
-      tab_index = browser_->tab_strip_model()->count();
-      web_contents = chrome::AddRestoredTab(
-          browser_, tab.navigations, tab_index,
-          tab.normalized_navigation_index(), tab.extension_app_id, group_id,
-          select, tab.pinned, base::TimeTicks(), base::Time(),
-          storage_namespace, tab.user_agent_override, tab.extra_data,
-          /*from_session_restore=*/false, /*is_active_browser=*/std::nullopt);
-    } else {
-      // Do nothing if the tab wasn't added to the group.
+    if (restored_from_group_or_window_context) {
+      // Open the saved tab group as-is if the tab is being restored from a
+      // group or window context. This is to enforce that SavedTabGroups are
+      // the source or truth.
       return nullptr;
     }
+
+    // Add the saved tab to the end of group.
+    web_contents = chrome::AddRestoredTab(
+        browser_, tab.navigations, browser_->tab_strip_model()->count(),
+        tab.normalized_navigation_index(), tab.extension_app_id, group_id,
+        select, tab.pinned, base::TimeTicks(), base::Time(), storage_namespace,
+        tab.user_agent_override, tab.extra_data,
+        /*from_session_restore=*/false, /*is_active_browser=*/std::nullopt);
   }
 
+  CHECK(web_contents);
+
 #if BUILDFLAG(ENABLE_SESSION_SERVICE)
   // The tab may have been made active even if `select` is false if it is the
   // only tab in `browser_`.

chrome/browser/ui/browser_live_tab_context.h

@@ -73,6 +73,7 @@ class BrowserLiveTabContext : public sessions::LiveTabContext {
       const sessions::tab_restore::Tab& tab,
       int tab_index,
       bool select,
+      bool restored_from_group_or_window_context,
       sessions::tab_restore::Type original_session_type) override;
   sessions::LiveTab* ReplaceRestoredTab(
       const sessions::tab_restore::Tab& tab) override;

components/sessions/core/live_tab_context.h

@@ -75,9 +75,13 @@ class SESSIONS_EXPORT LiveTabContext {
   // has been created by TabRestoreService.
   // |original_session_type| indicates the type of session entry the tab
   // belongs to.
+  // |restored_from_group_or_window_context| when true indicates if the tab we
+  // are restoring is part of a window or group which is trying to restore all
+  // of its tabs.
   virtual LiveTab* AddRestoredTab(const tab_restore::Tab& tab,
                                   int tab_index,
                                   bool select,
+                                  bool restored_from_group_or_window_context,
                                   tab_restore::Type original_session_type) = 0;
 
   // Note: |tab.platform_data| may be null (e.g., if restoring from last session

components/sessions/core/tab_restore_service_helper.cc

@@ -71,8 +71,9 @@ void AddSerializedNavigationEntries(
   const int delta =
       (behavior == AddBehavior::kCurrentAndPreceedingEntries) ? -1 : 1;
   int current_index = live_tab->GetCurrentEntryIndex();
-  if (behavior == AddBehavior::kEntriesFollowingCurrentEntry)
+  if (behavior == AddBehavior::kEntriesFollowingCurrentEntry) {
     ++current_index;
+  }
   int added_count = 0;
   while (current_index >= 0 && current_index < max_index &&
          added_count <= gMaxPersistNavigationCount) {
@@ -147,8 +148,9 @@ void TabRestoreServiceHelper::SetHelperObserver(Observer* observer) {
 }
 
 TabRestoreServiceHelper::~TabRestoreServiceHelper() {
-  for (auto& observer : observer_list_)
+  for (auto& observer : observer_list_) {
     observer.TabRestoreServiceDestroyed(tab_restore_service_);
+  }
   base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider(
       this);
 }
@@ -193,8 +195,9 @@ std::optional<SessionID> TabRestoreServiceHelper::CreateHistoricalTab(
 
   auto local_tab = std::make_unique<Tab>();
   PopulateTab(local_tab.get(), index, context, live_tab);
-  if (local_tab->navigations.empty())
+  if (local_tab->navigations.empty()) {
     return std::nullopt;
+  }
 
   SessionID id = local_tab->id;
   AddEntry(std::move(local_tab), true, true);
@@ -472,7 +475,8 @@ LiveTabContext* TabRestoreServiceHelper::RestoreTabOrGroupFromWindow(
     restored_tab_browser_id = tab.browser_id;
     LiveTab* restored_tab = nullptr;
     context = RestoreTab(tab, context, disposition,
-                         sessions::tab_restore::WINDOW, &restored_tab);
+                         sessions::tab_restore::WINDOW, &restored_tab,
+                         /*restored_from_group_or_window_context=*/false);
     live_tabs->push_back(restored_tab);
 
     std::optional<tab_groups::TabGroupId> group_id = tab.group;
@@ -521,9 +525,9 @@ LiveTabContext* TabRestoreServiceHelper::RestoreTabOrGroupFromWindow(
 
         // Restore the tab.
         LiveTab* restored_tab = nullptr;
-        LiveTabContext* new_context =
-            RestoreTab(tab, context, disposition, sessions::tab_restore::WINDOW,
-                       &restored_tab);
+        LiveTabContext* new_context = RestoreTab(
+            tab, context, disposition, sessions::tab_restore::WINDOW,
+            &restored_tab, /*restored_from_group_or_window_context=*/true);
         if (tab_i != 0) {
           // CHECK that the context should be the same except for the first tab.
           DCHECK_EQ(new_context, context);
@@ -623,8 +627,8 @@ std::vector<LiveTab*> TabRestoreServiceHelper::RestoreEntryById(
       }
 
       LiveTab* restored_tab = nullptr;
-      context =
-          RestoreTab(tab, context, disposition, entry.type, &restored_tab);
+      context = RestoreTab(tab, context, disposition, entry.type, &restored_tab,
+                           /*restored_from_group_or_window_context=*/false);
       live_tabs.push_back(restored_tab);
       context->ShowBrowserWindow();
       break;
@@ -668,7 +672,8 @@ std::vector<LiveTab*> TabRestoreServiceHelper::RestoreEntryById(
         for (const auto& tab : window.tabs) {
           const bool select_tab = tab->id == selected_tab_id;
           LiveTab* restored_tab = context->AddRestoredTab(
-              *tab.get(), context->GetTabCount(), select_tab, entry.type);
+              *tab.get(), context->GetTabCount(), select_tab,
+              /*restored_from_group_or_window_context=*/true, entry.type);
 
           if (restored_tab) {
             client_->OnTabRestored(
@@ -730,7 +735,7 @@ std::vector<LiveTab*> TabRestoreServiceHelper::RestoreEntryById(
         for (const auto& tab : group.tabs) {
           LiveTab* restored_tab = context->AddRestoredTab(
               *tab.get(), context->GetTabCount(), group.tabs[0]->id == tab->id,
-              entry.type);
+              /*restored_from_group_or_window_context=*/true, entry.type);
           live_tabs.push_back(restored_tab);
         }
       } else {
@@ -740,8 +745,9 @@ std::vector<LiveTab*> TabRestoreServiceHelper::RestoreEntryById(
           const Tab& tab = *group.tabs[i];
           if (tab.id == id) {
             LiveTab* restored_tab = nullptr;
-            context = RestoreTab(tab, context, disposition, entry.type,
-                                 &restored_tab);
+            context =
+                RestoreTab(tab, context, disposition, entry.type, &restored_tab,
+                           /*restored_from_group_or_window_context=*/false);
             live_tabs.push_back(restored_tab);
             CHECK(ValidateGroup(group));
             group.tabs.erase(group.tabs.begin() + i);
@@ -779,13 +785,15 @@ bool TabRestoreServiceHelper::IsRestoring() const {
 }
 
 void TabRestoreServiceHelper::NotifyEntriesChanged() {
-  for (auto& observer : observer_list_)
+  for (auto& observer : observer_list_) {
     observer.TabRestoreServiceChanged(tab_restore_service_);
+  }
 }
 
 void TabRestoreServiceHelper::NotifyLoaded() {
-  for (auto& observer : observer_list_)
+  for (auto& observer : observer_list_) {
     observer.TabRestoreServiceLoaded(tab_restore_service_);
+  }
 }
 
 void TabRestoreServiceHelper::AddEntry(std::unique_ptr<Entry> entry,
@@ -993,7 +1001,8 @@ LiveTabContext* TabRestoreServiceHelper::RestoreTab(
     LiveTabContext* context,
     WindowOpenDisposition disposition,
     sessions::tab_restore::Type session_restore_type,
-    LiveTab** live_tab) {
+    LiveTab** live_tab,
+    bool restored_from_group_or_window_context) {
   LiveTab* restored_tab;
   if (disposition == WindowOpenDisposition::CURRENT_TAB && context) {
     restored_tab = context->ReplaceRestoredTab(tab);
@@ -1043,7 +1052,7 @@ LiveTabContext* TabRestoreServiceHelper::RestoreTab(
     restored_tab = context->AddRestoredTab(
         tab, tab_index,
         /*select=*/disposition != WindowOpenDisposition::NEW_BACKGROUND_TAB,
-        session_restore_type);
+        restored_from_group_or_window_context, session_restore_type);
   }
 
   client_->OnTabRestored(

components/sessions/core/tab_restore_service_helper.h

@@ -173,7 +173,8 @@ class SESSIONS_EXPORT TabRestoreServiceHelper
                              LiveTabContext* context,
                              WindowOpenDisposition disposition,
                              sessions::tab_restore::Type session_restore_type,
-                             LiveTab** live_tab);
+                             LiveTab** live_tab,
+                             bool restored_from_group_or_window_context);
 
   // This is a helper function for RestoreEntryById(). Restores a single entry
   // from the `window`. The entry to restore is denoted by `id` and can either

ios/chrome/browser/sessions/model/live_tab_context_browser_agent.h

@@ -66,6 +66,7 @@ class LiveTabContextBrowserAgent
       const sessions::tab_restore::Tab& tab,
       int tab_index,
       bool select,
+      bool restored_from_group_or_window_context,
       sessions::tab_restore::Type original_session_type) override;
   sessions::LiveTab* ReplaceRestoredTab(
       const sessions::tab_restore::Tab& tab) override;

ios/chrome/browser/sessions/model/live_tab_context_browser_agent.mm

@@ -131,6 +131,7 @@ sessions::LiveTab* LiveTabContextBrowserAgent::AddRestoredTab(
     const sessions::tab_restore::Tab& tab,
     int tab_index,
     bool select,
+    bool restored_from_group_or_window_context,
     sessions::tab_restore::Type original_session_type) {
   // TODO(crbug.com/40491734): Handle tab-switch animation somehow...
   web_state_list_->InsertWebState(