[SavedTabGroup] Keep active tab if active tab exists in saved tab group

Before clicking the group button would always focus the first tab even if a tab in the group was the active tab.

if the active tab is in the group already, focus only the browser window.

Bug: 1485128
Change-Id: Ia5c513372b24e801083521fd7fdf195b9856b9e8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4885836
Reviewed-by: Darryl James <dljames@chromium.org>
Commit-Queue: jongmok kim <johny.kimc@gmail.com>
Cr-Commit-Position: refs/heads/main@{#1209969}

chrome/browser/ui/tabs/saved_tab_groups/saved_tab_group_keyed_service.cc

@@ -20,6 +20,7 @@
 #include "chrome/browser/ui/bookmarks/bookmark_utils_desktop.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/tabs/saved_tab_groups/saved_tab_group_model_listener.h"
 #include "chrome/browser/ui/tabs/saved_tab_groups/saved_tab_group_utils.h"
 #include "chrome/browser/ui/tabs/tab_group.h"
@@ -111,7 +112,7 @@ void SavedTabGroupKeyedService::OpenSavedTabGroupInBrowser(
 
   // Activate the first tab in a group if it is already open.
   if (saved_group->local_group_id().has_value()) {
-    FocusFirstTabInOpenGroup(saved_group->local_group_id().value());
+    FocusFirstTabOrWindowInOpenGroup(saved_group->local_group_id().value());
     return;
   }
 
@@ -442,20 +443,30 @@ SavedTabGroupKeyedService::GetWebContentsToTabGuidMappingForOpening(
   return web_contents;
 }
 
-void SavedTabGroupKeyedService::FocusFirstTabInOpenGroup(
+void SavedTabGroupKeyedService::FocusFirstTabOrWindowInOpenGroup(
     tab_groups::TabGroupId local_group_id) {
   Browser* browser_for_activation =
       SavedTabGroupUtils::GetBrowserWithTabGroupId(local_group_id);
 
-  // Only activate the tab group's first tab if it exists in any browser's
-  // tabstrip model.
+  // Only activate the tab group's first tab, if it exists in any browser's
+  // tabstrip model and it is not in the active tab in the tab group.
   CHECK(browser_for_activation);
+  TabGroup* tab_group =
+      browser_for_activation->tab_strip_model()->group_model()->GetTabGroup(
+          local_group_id);
 
-  absl::optional<int> first_tab = browser_for_activation->tab_strip_model()
-                                      ->group_model()
-                                      ->GetTabGroup(local_group_id)
-                                      ->GetFirstTab();
+  absl::optional<int> first_tab = tab_group->GetFirstTab();
+  absl::optional<int> last_tab = tab_group->GetLastTab();
+  int active_index = browser_for_activation->tab_strip_model()->active_index();
   DCHECK(first_tab.has_value());
+  DCHECK(last_tab.has_value());
+  DCHECK_GT(active_index, 0);
+
+  if (active_index >= first_tab.value() && active_index <= last_tab) {
+    browser_for_activation->window()->Activate();
+    return;
+  }
+
   browser_for_activation->ActivateContents(
       browser_for_activation->tab_strip_model()->GetWebContentsAt(
           first_tab.value()));

chrome/browser/ui/tabs/saved_tab_groups/saved_tab_group_keyed_service.h

@@ -101,8 +101,9 @@ class SavedTabGroupKeyedService : public KeyedService,
       const base::Uuid& saved_group_guid);
 
   // Activates the first tab in saved group that is already opened when its
-  // button is pressed.
-  void FocusFirstTabInOpenGroup(tab_groups::TabGroupId local_group_id);
+  // button is pressed, If active tab exists in saved group, only activates
+  // window.
+  void FocusFirstTabOrWindowInOpenGroup(tab_groups::TabGroupId local_group_id);
 
   // Returns a pointer to the TabStripModel which contains `local_group_id`.
   const TabStripModel* GetTabStripModelWithTabGroupId(

chrome/browser/ui/tabs/saved_tab_groups/saved_tab_group_keyed_service_unittest.cc

@@ -283,6 +283,63 @@ TEST_F(SavedTabGroupKeyedServiceUnitTest, AlreadyOpenedGroupIsFocused) {
   EXPECT_EQ(0, browser_1->tab_strip_model()->active_index());
 }
 
+TEST_F(SavedTabGroupKeyedServiceUnitTest,
+       ActiveTabInAlreadyOpenedGroupIsFocused) {
+  Browser* browser_1 = AddBrowser();
+  ASSERT_EQ(0, browser_1->tab_strip_model()->count());
+
+  // Add 2 tabs to the browser_1.
+  AddTabToBrowser(browser_1, 0);
+  AddTabToBrowser(browser_1, 0);
+  ASSERT_EQ(2, browser_1->tab_strip_model()->count());
+
+  const tab_groups::TabGroupId tab_group_id_1 =
+      browser_1->tab_strip_model()->AddToNewGroup({0});
+
+  const base::Uuid guid_1 = base::Uuid::GenerateRandomV4();
+
+  // Store the guid to tab_group_id association in the keyed service. We should
+  // expect at the end of the test, `tab_group_id_3` has no association with the
+  // SavedTabGroupModel at all.
+  service()->StoreLocalToSavedId(guid_1, tab_group_id_1);
+
+  // Populate the SavedTabGroupModel with some test data to simulate the browser
+  // loading in persisted data on startup.
+  std::vector<SavedTabGroupTab> group_1_tabs = {
+      SavedTabGroupTab(GURL("https://www.google.com"), u"Google", guid_1,
+                       /*position=*/0),
+      SavedTabGroupTab(GURL("https://www.youtube.com"), u"Youtube", guid_1,
+                       /*position=*/1)};
+  SavedTabGroup saved_group_1(u"Group 1", tab_groups::TabGroupColorId::kGrey,
+                              std::move(group_1_tabs), absl::nullopt, guid_1);
+
+  service()->model()->Add(saved_group_1);
+
+  // Notify the KeyedService that the SavedTabGroupModel has loaded all local
+  // data triggered by the completion of SavedTabGroupModel::LoadStoredEntries.
+  service()->model()->LoadStoredEntries({});
+
+  // Activate the second tab.
+  browser_1->tab_strip_model()->ActivateTabAt(1);
+  EXPECT_EQ(1, browser_1->tab_strip_model()->active_index());
+
+  service()->OpenSavedTabGroupInBrowser(browser_1, guid_1);
+
+  // Ensure the active tab in the saved group is not changed.
+  EXPECT_EQ(1, browser_1->tab_strip_model()->active_index());
+
+  // Activate the third tab.
+  browser_1->tab_strip_model()->ActivateTabAt(2);
+  EXPECT_EQ(2, browser_1->tab_strip_model()->active_index());
+
+  service()->OpenSavedTabGroupInBrowser(browser_1, guid_1);
+
+  // If there is no active tab in the saved tab group, the first tab of the
+  // saved tab group is activated. Ensure the first tab in the saved group is
+  // activated.
+  EXPECT_EQ(0, browser_1->tab_strip_model()->active_index());
+}
+
 TEST_F(SavedTabGroupKeyedServiceUnitTest,
        RestoredGroupWithoutSavedGuidIsDiscarded) {
   Browser* browser_1 = AddBrowser();