Fix issues with group drag revert

See bug for details on the issues.

Issue #1: When dragging multiple tabs to the left, reverting doesn't return them to the correct position. Was happening even without groups.
- Fixed by adjusting the target index of revert to account for unreverted tabs.

Issue #2: Intermittent crashes.
- Seems to be fixed by crrev.com/c/2020908

Issue #3: Header is not returned to the initial view position.
- Fixed by adding a call to MoveTabGroup(), which is only needed when dragging within the same tabstrip (otherwise, a new group is being created and positioned correctly).

Issue #4: Selection is not restored correctly.
- Fixed by actively setting the index of group headers to kNoTab, so that checks in RestoreInitialSelection() correctly ignore group headers.

Note: The change at line 657 is to work with line 1656, which is assuming that source_model_index is explicitly set to kNoTab.

Bug: 1047761
Change-Id: I18af6a66d4e7788ed90c9be1f27556fc0018c6c3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2036362
Commit-Queue: Connie Wan <connily@chromium.org>
Reviewed-by: Charlene Yan <cyan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#738296}

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

@@ -654,8 +654,8 @@ void TabDragController::InitDragData(TabSlotView* view,
                                      TabDragData* drag_data) {
   TRACE_EVENT0("views", "TabDragController::InitDragData");
   const int source_model_index = source_context_->GetIndexOf(view);
+  drag_data->source_model_index = source_model_index;
   if (source_model_index != TabStripModel::kNoTab) {
-    drag_data->source_model_index = source_model_index;
     drag_data->contents = source_context_->GetTabStripModel()->GetWebContentsAt(
         drag_data->source_model_index);
     drag_data->pinned = source_context_->IsTabPinned(static_cast<Tab*>(view));
@@ -1605,6 +1605,8 @@ void TabDragController::RevertDrag() {
       source_context_->StoppedDragging(views, initial_tab_positions_,
                                        move_behavior_ == MOVE_VISIBLE_TABS,
                                        false);
+      if (header_drag_)
+        source_context_->GetTabStripModel()->MoveTabGroup(group_.value());
     } else {
       attached_context_->DraggedTabsDetached();
     }
@@ -1676,6 +1678,7 @@ void TabDragController::RevertDragAt(size_t drag_index) {
 
   base::AutoReset<bool> setter(&is_mutating_, true);
   TabDragData* data = &(drag_data_[drag_index]);
+  int target_index = data->source_model_index;
   if (attached_context_) {
     int index = attached_context_->GetTabStripModel()->GetIndexOfWebContents(
         data->contents);
@@ -1687,24 +1690,35 @@ void TabDragController::RevertDragAt(size_t drag_index) {
       // TODO(beng): (Cleanup) seems like we should use Attach() for this
       //             somehow.
       source_context_->GetTabStripModel()->InsertWebContentsAt(
-          data->source_model_index, std::move(detached_web_contents),
+          target_index, std::move(detached_web_contents),
           (data->pinned ? TabStripModel::ADD_PINNED : 0));
     } else {
       // The Tab was moved within the TabDragContext where the drag
       // was initiated. Move it back to the starting location.
+
+      // If the target index is to the right, then other unreverted tabs are
+      // occupying indices between this tab and the target index. Those
+      // unreverted tabs will later be reverted to the right of the target
+      // index, so we skip those indices.
+      if (target_index > index) {
+        for (size_t i = drag_index + 1; i < drag_data_.size(); ++i) {
+          if (drag_data_[i].contents)
+            ++target_index;
+        }
+      }
       source_context_->GetTabStripModel()->MoveWebContentsAt(
-          index, data->source_model_index, false);
+          index, target_index, false);
     }
   } else {
     // The Tab was detached from the TabDragContext where the drag
     // began, and has not been attached to any other TabDragContext.
     // We need to put it back into the source TabDragContext.
     source_context_->GetTabStripModel()->InsertWebContentsAt(
-        data->source_model_index, std::move(data->owned_contents),
+        target_index, std::move(data->owned_contents),
         (data->pinned ? TabStripModel::ADD_PINNED : 0));
   }
   source_context_->GetTabStripModel()->UpdateGroupForDragRevert(
-      data->source_model_index,
+      target_index,
       data->tab_group_data.has_value()
           ? base::Optional<tab_groups::TabGroupId>{data->tab_group_data.value()
                                                        .group_id}

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

@@ -1895,7 +1895,7 @@ void PressEscapeWhileDetachedStep2(const BrowserList* browser_list) {
 
 // Detaches a tab and while detached presses escape to revert the drag.
 IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
-                       PressEscapeWhileDetached) {
+                       RevertDragWhileDetached) {
   AddTabsAndResetBrowser(browser(), 1);
   TabStrip* tab_strip = GetTabStripForBrowser(browser());
 
@@ -2096,20 +2096,20 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTestWithTabGroupsEnabled,
   EXPECT_EQ(tab_strip2->GetGroupColorId(groups2[0]), group_color);
 }
 
-// Drags a tab group by the header to a new position and presses escape to
-// revert the drag.
+// Drags a tab group by the header to a new position toward the right and
+// presses escape to revert the drag.
 IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTestWithTabGroupsEnabled,
-                       PressEscapeDuringHeaderDrag) {
+                       RevertHeaderDragRight) {
   TabStrip* tab_strip = GetTabStripForBrowser(browser());
   TabStripModel* model = browser()->tab_strip_model();
-  AddTabsAndResetBrowser(browser(), 1);
-  tab_groups::TabGroupId group = model->AddToNewGroup({0});
+  AddTabsAndResetBrowser(browser(), 3);
+  tab_groups::TabGroupId group = model->AddToNewGroup({0, 1});
   StopAnimating(tab_strip);
 
   TabGroupHeader* group_header = tab_strip->group_header(group);
   EXPECT_FALSE(group_header->dragging());
   ASSERT_TRUE(PressInput(GetCenterInScreenCoordinates(group_header)));
-  ASSERT_TRUE(DragInputTo(GetCenterInScreenCoordinates(tab_strip->tab_at(1))));
+  ASSERT_TRUE(DragInputTo(GetCenterInScreenCoordinates(tab_strip->tab_at(2))));
   ASSERT_TRUE(group_header->dragging());
 
   ASSERT_TRUE(ui_test_utils::SendKeyPressToWindowSync(
@@ -2118,12 +2118,42 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTestWithTabGroupsEnabled,
 
   EXPECT_EQ(1u, browser_list->size());
   EXPECT_FALSE(group_header->dragging());
-  EXPECT_EQ("0 1", IDString(browser()->tab_strip_model()));
+  EXPECT_EQ("0 1 2 3", IDString(browser()->tab_strip_model()));
   std::vector<tab_groups::TabGroupId> groups =
       model->group_model()->ListTabGroups();
   EXPECT_EQ(1u, groups.size());
   EXPECT_THAT(model->group_model()->GetTabGroup(groups[0])->ListTabs(),
-              testing::ElementsAre(0));
+              testing::ElementsAre(0, 1));
+}
+
+// Drags a tab group by the header to a new position toward the left and presses
+// escape to revert the drag.
+IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTestWithTabGroupsEnabled,
+                       RevertHeaderDragLeft) {
+  TabStrip* tab_strip = GetTabStripForBrowser(browser());
+  TabStripModel* model = browser()->tab_strip_model();
+  AddTabsAndResetBrowser(browser(), 3);
+  tab_groups::TabGroupId group = model->AddToNewGroup({2, 3});
+  StopAnimating(tab_strip);
+
+  TabGroupHeader* group_header = tab_strip->group_header(group);
+  EXPECT_FALSE(group_header->dragging());
+  ASSERT_TRUE(PressInput(GetCenterInScreenCoordinates(group_header)));
+  ASSERT_TRUE(DragInputTo(GetCenterInScreenCoordinates(tab_strip->tab_at(0))));
+  ASSERT_TRUE(group_header->dragging());
+
+  ASSERT_TRUE(ui_test_utils::SendKeyPressToWindowSync(
+      browser()->window()->GetNativeWindow(), ui::VKEY_ESCAPE, false, false,
+      false, false));
+
+  EXPECT_EQ(1u, browser_list->size());
+  EXPECT_FALSE(group_header->dragging());
+  EXPECT_EQ("0 1 2 3", IDString(browser()->tab_strip_model()));
+  std::vector<tab_groups::TabGroupId> groups =
+      model->group_model()->ListTabGroups();
+  EXPECT_EQ(1u, groups.size());
+  EXPECT_THAT(model->group_model()->GetTabGroup(groups[0])->ListTabs(),
+              testing::ElementsAre(2, 3));
 }
 
 namespace {
@@ -2156,7 +2186,7 @@ void PressEscapeWhileDetachedHeaderStep2(
 // Drags a tab group by the header and while detached presses escape to revert
 // the drag.
 IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTestWithTabGroupsEnabled,
-                       PressEscapeDuringHeaderDragWhileDetached) {
+                       RevertHeaderDragWhileDetached) {
   TabStrip* tab_strip = GetTabStripForBrowser(browser());
   TabStripModel* model = browser()->tab_strip_model();
   AddTabsAndResetBrowser(browser(), 1);