WebUI Tab Strip: Optimize group move events

With the introduction of TabGroupChange::kMoved, there were multiple
tab-group-moved events being sent, one for the group moving and one
for the tabs in the group moving. This CL removes one of these events.

This CL also selects a group's tabs when the WebUI tab strip moves
the group to mirror the native tab strip and to allow the
TabStripModelChange::kMoved events of the group's tabs to be
ignored correctly. Previously, tab-moved events were being sent for
each tab when a group was moved using the WebUI tab strip which
caused for multiple unnecessary animations.

Bug: 1082344
Change-Id: I663d46e2e9a9857cdbaba8c17d2ff526f2a2fff8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2246956
Commit-Queue: John Lee <johntlee@chromium.org>
Reviewed-by: dpapad <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#779856}

chrome/browser/ui/webui/tab_strip/tab_strip_ui_handler.cc

@@ -34,6 +34,7 @@
 #include "components/tab_groups/tab_group_visual_data.h"
 #include "third_party/skia/include/core/SkStream.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/base/models/list_selection_model.h"
 #include "ui/base/models/simple_menu_model.h"
 #include "ui/base/theme_provider.h"
 #include "ui/gfx/color_utils.h"
@@ -297,8 +298,6 @@ void TabStripUIHandler::OnTabStripModelChanged(
     case TabStripModelChange::kMoved: {
       auto* move = change.GetMove();
 
-      // TODO(johntlee): Investigate if this is still needed, when
-      // TabGroupChange::kMoved exists.
       base::Optional<tab_groups::TabGroupId> tab_group_id =
           tab_strip_model->GetTabGroupForTab(move->to_index);
       if (tab_group_id.has_value()) {
@@ -309,16 +308,9 @@ void TabStripUIHandler::OnTabStripModelChanged(
         if (tabs_in_group == selection.new_model.selected_indices()) {
           // If the selection includes all the tabs within the changed tab's
           // group, it is an indication that the entire group is being moved.
-          // To prevent sending multiple events for the same batch move, fire a
-          // separate single tab-group-moved event once all tabs have been
-          // moved. All tabs have moved only after all the indices in the group
-          // are in the correct continuous order.
-          if (tabs_in_group.back() - tabs_in_group.front() + 1 ==
-              static_cast<int>(tabs_in_group.size())) {
-            FireWebUIListener("tab-group-moved",
-                              base::Value(tab_group_id.value().ToString()),
-                              base::Value(tabs_in_group[0]));
-          }
+          // To prevent sending multiple events for each tab in the group,
+          // ignore these tabs moving as entire group moves will be handled by
+          // TabGroupChange::kMoved.
           break;
         }
       }
@@ -662,7 +654,17 @@ void TabStripUIHandler::HandleMoveGroup(const base::ListValue* args) {
       return;
     }
 
-    // If moving within the same browser, just do a simple move.
+    // When a group is moved, all the tabs in it need to be selected at the same
+    // time. This mimics the way the native tab strip works and also allows
+    // this handler to ignore the events for each individual tab moving.
+    int active_index =
+        target_browser->tab_strip_model()->selection_model().active();
+    ui::ListSelectionModel group_selection;
+    group_selection.SetSelectedIndex(group->ListTabs().front());
+    group_selection.SetSelectionFromAnchorTo(group->ListTabs().back());
+    group_selection.set_active(active_index);
+    target_browser->tab_strip_model()->SetSelectionFromModel(group_selection);
+
     target_browser->tab_strip_model()->MoveGroupTo(group_id.value(), to_index);
     return;
   }

chrome/browser/ui/webui/tab_strip/tab_strip_ui_handler_unittest.cc

@@ -133,55 +133,6 @@ TEST_F(TabStripUIHandlerTest, GroupStateChangedEvents) {
   EXPECT_EQ(nullptr, ungrouped_call_data.arg4());
 }
 
-TEST_F(TabStripUIHandlerTest, GroupMovedEvents) {
-  // Create a tab group and a few other tabs to allow the group to move.
-  AddTab(browser(), GURL("http://foo/1"));
-  AddTab(browser(), GURL("http://foo/2"));
-  AddTab(browser(), GURL("http://foo/3"));
-  AddTab(browser(), GURL("http://foo/4"));
-  tab_groups::TabGroupId expected_group_id =
-      browser()->tab_strip_model()->AddToNewGroup({0, 1});
-
-  // Select all the tabs in the group.
-  ui::ListSelectionModel selection;
-  selection.AddIndexToSelection(0);
-  selection.AddIndexToSelection(1);
-  selection.set_active(0);
-  browser()->tab_strip_model()->SetSelectionFromModel(selection);
-
-  web_ui()->ClearTrackedCalls();
-
-  // Move the selected tabs to later in the tab strip. This should result in
-  // a single event that is fired to indicate the entire group has moved.
-  int expected_index = 2;
-  browser()->tab_strip_model()->MoveSelectedTabsTo(expected_index);
-
-  EXPECT_EQ(1U, web_ui()->call_data().size());
-
-  const content::TestWebUI::CallData& call_data = *web_ui()->call_data().back();
-  EXPECT_EQ("cr.webUIListenerCallback", call_data.function_name());
-  EXPECT_EQ("tab-group-moved", call_data.arg1()->GetString());
-  EXPECT_EQ(expected_group_id.ToString(), call_data.arg2()->GetString());
-  EXPECT_EQ(expected_index, call_data.arg3()->GetInt());
-
-  web_ui()->ClearTrackedCalls();
-
-  // Move the selected tabs to earlier in the tab strip. This should also
-  // result in a single event that is fired to indicate the entire group has
-  // moved.
-  expected_index = 1;
-  browser()->tab_strip_model()->MoveSelectedTabsTo(expected_index);
-
-  EXPECT_EQ(1U, web_ui()->call_data().size());
-
-  const content::TestWebUI::CallData& call_data2 =
-      *web_ui()->call_data().back();
-  EXPECT_EQ("cr.webUIListenerCallback", call_data2.function_name());
-  EXPECT_EQ("tab-group-moved", call_data2.arg1()->GetString());
-  EXPECT_EQ(expected_group_id.ToString(), call_data2.arg2()->GetString());
-  EXPECT_EQ(expected_index, call_data2.arg3()->GetInt());
-}
-
 TEST_F(TabStripUIHandlerTest, GetGroupVisualData) {
   AddTab(browser(), GURL("http://foo/1"));
   AddTab(browser(), GURL("http://foo/2"));
@@ -269,6 +220,7 @@ TEST_F(TabStripUIHandlerTest, MoveGroup) {
   AddTab(browser(), GURL("http://foo/2"));
   tab_groups::TabGroupId group_id =
       browser()->tab_strip_model()->AddToNewGroup({0});
+  web_ui()->ClearTrackedCalls();
 
   // Move the group to index 1.
   int new_index = 1;
@@ -284,6 +236,13 @@ TEST_F(TabStripUIHandlerTest, MoveGroup) {
                                        ->ListTabs();
   ASSERT_EQ(new_index, tabs_in_group.front());
   ASSERT_EQ(new_index, tabs_in_group.back());
+
+  EXPECT_EQ(1U, web_ui()->call_data().size());
+  const content::TestWebUI::CallData& call_data =
+      *web_ui()->call_data().front();
+  EXPECT_EQ("cr.webUIListenerCallback", call_data.function_name());
+  EXPECT_EQ("tab-group-moved", call_data.arg1()->GetString());
+  EXPECT_EQ(group_id.ToString(), call_data.arg2()->GetString());
 }
 
 TEST_F(TabStripUIHandlerTest, MoveGroupAcrossWindows) {