Commit 2a52558f authored by Connie Wan's avatar Connie Wan Committed by Chromium LUCI CQ

Simplify group updates during tab dragging

This makes it so that TabDragController::UpdateGroupForDraggedTabs does
only what its name suggests (updates the group membership of the dragged
tabs) without doing anything extra (lots of tab movement).

The assumption is that tabs are moved to their destination before
UpdateGroupForDraggedTabs is called. This assumption is commented here:
https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/tabs/tab_drag_controller.h;l=527
And it still holds because of this call here:
https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/tabs/tab_drag_controller.cc;l=1034

Most of the deleted movements in UpdateGroupForDraggedTabs were added
because functions like RemoveFromGroup() could potentially shift tabs
around. However, by using MoveAndSetGroup instead, the tabs don't
actually move as long as the above assumption holds.

Tested manually and via interactive UI tests:
https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc;l=698

Bug: 1163845
Change-Id: I447c66a10c26a20a10d08979cd47cee4906d9b71
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2622324Reviewed-by: default avatarCharlene Yan <cyan@chromium.org>
Commit-Queue: Connie Wan <connily@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843162}
parent 1fe29595
...@@ -1073,9 +1073,10 @@ void TabStripModel::AddToExistingGroup(const std::vector<int>& indices, ...@@ -1073,9 +1073,10 @@ void TabStripModel::AddToExistingGroup(const std::vector<int>& indices,
AddToExistingGroupImpl(indices, group); AddToExistingGroupImpl(indices, group);
} }
void TabStripModel::MoveTabsAndSetGroup(const std::vector<int>& indices, void TabStripModel::MoveTabsAndSetGroup(
int destination_index, const std::vector<int>& indices,
const tab_groups::TabGroupId& group) { int destination_index,
base::Optional<tab_groups::TabGroupId> group) {
ReentrancyCheck reentrancy_check(&reentrancy_guard_); ReentrancyCheck reentrancy_check(&reentrancy_guard_);
MoveTabsAndSetGroupImpl(indices, destination_index, group); MoveTabsAndSetGroupImpl(indices, destination_index, group);
......
...@@ -440,7 +440,7 @@ class TabStripModel : public TabGroupController { ...@@ -440,7 +440,7 @@ class TabStripModel : public TabGroupController {
// being moved, and adds them to the tab group |group|. // being moved, and adds them to the tab group |group|.
void MoveTabsAndSetGroup(const std::vector<int>& indices, void MoveTabsAndSetGroup(const std::vector<int>& indices,
int destination_index, int destination_index,
const tab_groups::TabGroupId& group); base::Optional<tab_groups::TabGroupId> group);
// Similar to AddToExistingGroup(), but creates a group with id |group| if it // 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 // doesn't exist. This is only intended to be called from session restore
......
...@@ -2167,27 +2167,11 @@ void TabDragController::UpdateGroupForDraggedTabs() { ...@@ -2167,27 +2167,11 @@ void TabDragController::UpdateGroupForDraggedTabs() {
const base::Optional<tab_groups::TabGroupId> updated_group = const base::Optional<tab_groups::TabGroupId> updated_group =
GetTabGroupForTargetIndex(selected_unpinned); GetTabGroupForTargetIndex(selected_unpinned);
// Removing a tab from a group could change the index of the selected tabs.
// Store this to move the tab back to the proper position.
const int to_index = *selected.begin();
// All selected tabs should all be in the same group unless it is the initial
// move.
if (initial_move_) {
attached_model->RemoveFromGroup(selected_unpinned);
attached_model->MoveSelectedTabsTo(to_index);
}
if (updated_group == attached_model->GetTabGroupForTab(selected_unpinned[0])) if (updated_group == attached_model->GetTabGroupForTab(selected_unpinned[0]))
return; return;
if (updated_group.has_value()) { attached_model->MoveTabsAndSetGroup(selected_unpinned, selected_unpinned[0],
attached_model->MoveTabsAndSetGroup(selected_unpinned, selected_unpinned[0], updated_group);
updated_group.value());
} else {
attached_model->RemoveFromGroup(selected_unpinned);
attached_model->MoveSelectedTabsTo(to_index);
}
} }
base::Optional<tab_groups::TabGroupId> base::Optional<tab_groups::TabGroupId>
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment