[Tab Groups] Add tabs to the closest available position within a group.

Previously, tabs would always get inserted to the end of the tab group.
This changes it such that if the tab is currently to the left of the
group, it will be inserted at the front of the tab group respecting the
relative order of the selected tabs. If the tabs are to the right of
the group, it will be inserted to the end of the tab group as before
(no behavior change).

This change also allows tabs in a group to be restored to the location
saved at closing time, or the closest location if the group has moved.

Bug: 1030779, 1038362
Change-Id: Id26cdfa2df769bc738d780e48277e603acef42ae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1977849
Commit-Queue: Charlene Yan <cyan@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#728337}

chrome/browser/ui/tabs/tab_strip_model.cc

@@ -1021,14 +1021,6 @@ void TabStripModel::AddToExistingGroup(const std::vector<int>& indices,
   AddToExistingGroupImpl(indices, group);
 }
 
-void TabStripModel::MoveTabsIntoGroup(const std::vector<int>& indices,
-                                      int destination_index,
-                                      tab_groups::TabGroupId group) {
-  ReentrancyCheck reentrancy_check(&reentrancy_guard_);
-
-  MoveTabsIntoGroupImpl(indices, destination_index, group);
-}
-
 void TabStripModel::AddToGroupForRestore(const std::vector<int>& indices,
                                          tab_groups::TabGroupId group) {
   ReentrancyCheck reentrancy_check(&reentrancy_guard_);
@@ -1775,24 +1767,31 @@ void TabStripModel::AddToExistingGroupImpl(const std::vector<int>& indices,
   if (!group_model_->ContainsTabGroup(group))
     return;
 
-  int destination_index = -1;
-  for (int i = contents_data_.size() - 1; i >= 0; i--) {
-    if (contents_data_[i]->group() == group) {
-      destination_index = i + 1;
-      break;
+  // Unpin tabs when grouping -- the states should be mutually exclusive.
+  std::vector<int> new_indices = SetTabsPinned(indices, false);
+
+  std::vector<int> tabs_in_group = group_model_->GetTabGroup(group)->ListTabs();
+  DCHECK(base::STLIsSorted(tabs_in_group));
+
+  // Split |new_indices| into |tabs_left_of_group| and |tabs_right_of_group| to
+  // be moved to proper destination index. Directly set the group for indices
+  // that are inside the group.
+  std::vector<int> tabs_left_of_group;
+  std::vector<int> tabs_right_of_group;
+  for (int new_index : new_indices) {
+    if (new_index >= tabs_in_group.front() &&
+        new_index <= tabs_in_group.back()) {
+      GroupTab(new_index, group);
+    } else if (new_index < tabs_in_group.front()) {
+      tabs_left_of_group.push_back(new_index);
+    } else {
+      DCHECK(new_index > tabs_in_group.back());
+      tabs_right_of_group.push_back(new_index);
     }
   }
 
-  // Ignore indices that are already in the group.
-  std::vector<int> new_indices;
-  for (int candidate_index : indices) {
-    if (GetTabGroupForTab(candidate_index) != group)
-      new_indices.push_back(candidate_index);
-  }
-  // Unpin tabs when grouping -- the states should be mutually exclusive.
-  new_indices = SetTabsPinned(new_indices, false);
-
-  MoveTabsIntoGroupImpl(new_indices, destination_index, group);
+  MoveTabsIntoGroupImpl(tabs_left_of_group, tabs_in_group.front(), group);
+  MoveTabsIntoGroupImpl(tabs_right_of_group, tabs_in_group.back() + 1, group);
 }
 
 void TabStripModel::MoveTabsIntoGroupImpl(const std::vector<int>& indices,
@@ -1816,6 +1815,7 @@ void TabStripModel::MoveTabsIntoGroupImpl(const std::vector<int>& indices,
   for (size_t i = numTabsMovingRight; i < indices.size(); i++) {
     move_left_indices.push_back(indices[i]);
   }
+
   // Move tabs to the left, starting with the leftmost tab.
   for (size_t i = 0; i < move_left_indices.size(); i++)
     MoveAndSetGroup(move_left_indices[i], destination_index + i, group);

chrome/browser/ui/tabs/tab_strip_model.h

@@ -419,13 +419,6 @@ class TabStripModel : public TabGroupController {
   void AddToExistingGroup(const std::vector<int>& indices,
                           tab_groups::TabGroupId group);
 
-  // Moves the set of tabs indicated by |indices| to precede the tab at index
-  // |destination_index|, maintaining their order and the order of tabs not
-  // being moved, and adds them to the tab group |group|.
-  void MoveTabsIntoGroup(const std::vector<int>& indices,
-                         int destination_index,
-                         tab_groups::TabGroupId group);
-
   // Similar to AddToExistingGroup(), but creates a group with id |group| if it
   // doesn't exist. This is only intended to be called from session restore
   // code.
@@ -656,8 +649,9 @@ class TabStripModel : public TabGroupController {
   void AddToExistingGroupImpl(const std::vector<int>& indices,
                               tab_groups::TabGroupId group);
 
-  // Implementation of MoveTabsIntoGroup. Moves the set of tabs in |indices| to
-  // the |destination_index| and updates the tabs to the appropriate |group|.
+  // Moves the set of tabs indicated by |indices| to precede the tab at index
+  // |destination_index|, maintaining their order and the order of tabs not
+  // being moved, and adds them to the tab group |group|.
   void MoveTabsIntoGroupImpl(const std::vector<int>& indices,
                              int destination_index,
                              tab_groups::TabGroupId group);

chrome/browser/ui/tabs/tab_strip_model_unittest.cc

@@ -3026,23 +3026,23 @@ TEST_F(TabStripModelTest, AddTabToExistingGroupUpdatesObservers) {
   strip.CloseAllTabs();
 }
 
-TEST_F(TabStripModelTest, AddTabToExistingGroupReordersToTheRight) {
+TEST_F(TabStripModelTest, AddTabToLeftOfExistingGroupReorders) {
   TestTabStripModelDelegate delegate;
   TabStripModel strip(&delegate, profile());
-  PrepareTabs(&strip, 2);
+  PrepareTabs(&strip, 3);
 
-  strip.AddToNewGroup({1});
-  base::Optional<tab_groups::TabGroupId> group = strip.GetTabGroupForTab(1);
+  strip.AddToNewGroup({2});
+  base::Optional<tab_groups::TabGroupId> group = strip.GetTabGroupForTab(2);
 
   strip.AddToExistingGroup({0}, group.value());
-  EXPECT_EQ(strip.GetTabGroupForTab(0), group);
   EXPECT_EQ(strip.GetTabGroupForTab(1), group);
-  EXPECT_EQ("1 0", GetTabStripStateString(strip));
+  EXPECT_EQ(strip.GetTabGroupForTab(2), group);
+  EXPECT_EQ("1 0 2", GetTabStripStateString(strip));
 
   strip.CloseAllTabs();
 }
 
-TEST_F(TabStripModelTest, AddTabToExistingGroupReordersToTheLeft) {
+TEST_F(TabStripModelTest, AddTabToRighOfExistingGroupReorders) {
   TestTabStripModelDelegate delegate;
   TabStripModel strip(&delegate, profile());
   PrepareTabs(&strip, 3);
@@ -3072,7 +3072,7 @@ TEST_F(TabStripModelTest, AddTabToExistingGroupReorders) {
   EXPECT_EQ(strip.GetTabGroupForTab(1), group);
   EXPECT_EQ(strip.GetTabGroupForTab(2), group);
   EXPECT_FALSE(strip.GetTabGroupForTab(3).has_value());
-  EXPECT_EQ("1 0 3 2", GetTabStripStateString(strip));
+  EXPECT_EQ("0 1 3 2", GetTabStripStateString(strip));
 
   strip.CloseAllTabs();
 }
@@ -3089,7 +3089,7 @@ TEST_F(TabStripModelTest, AddTabToExistingGroupUnpins) {
   strip.AddToExistingGroup({0}, group.value());
   EXPECT_FALSE(strip.IsTabPinned(0));
   EXPECT_EQ(strip.GetTabGroupForTab(0), group);
-  EXPECT_EQ("1 0", GetTabStripStateString(strip));
+  EXPECT_EQ("0 1", GetTabStripStateString(strip));
 
   strip.CloseAllTabs();
 }

chrome/browser/ui/views/tabs/tab_drag_controller.cc

@@ -2142,8 +2142,7 @@ void TabDragController::UpdateGroupForDraggedTabs() {
   }
 
   if (updated_group.has_value()) {
-    attached_model->MoveTabsIntoGroup(selected, selected[0],
-                                      updated_group.value());
+    attached_model->AddToExistingGroup(selected, updated_group.value());
   } else {
     attached_model->RemoveFromGroup(selected);
     attached_model->MoveSelectedTabsTo(to_index);