Skip to content

Commit

Permalink
[M110][Tab Groups] Fix GroupIdToGroupIndexMap on addTab
Browse files Browse the repository at this point in the history
If a tab is added that is not grouped the current TabGroupModelFilter
implementation assumes that group will be at the end of the TabModel
and sets that group's index to be at the end of the GroupIndexMap.

This is not always the case and as such it is possible to end up with a
situation where the order of tabs in the TabGroupModelFilter disagrees
with the order in TabModel. This results in the GTS behaving incorrectly
when moving or grouping tabs.

Fix this by updating the index dynamically whenever a tab is added that
is not during a reset.

Skip flaky test detection due to Filter initialization error.

(cherry picked from commit c9aa68a)

Validate-Test-Flakiness: skip
Bug: 1382771
Change-Id: I7daaf194f0552055e4720ae8235a6757df9aa608
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4133994
Reviewed-by: Yue Zhang <yuezhanggg@chromium.org>
Commit-Queue: Calder Kitagawa <ckitagawa@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1089476}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4143944
Auto-Submit: Calder Kitagawa <ckitagawa@chromium.org>
Commit-Queue: Yue Zhang <yuezhanggg@chromium.org>
Cr-Commit-Position: refs/branch-heads/5481@{#160}
Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}
  • Loading branch information
ckitagawa-work authored and Chromium LUCI CQ committed Jan 7, 2023
1 parent 2a352cd commit fe260c0
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -666,7 +666,17 @@ protected void addTab(Tab tab) {
TabGroup tabGroup = new TabGroup(getRootId(tab));
tabGroup.addTab(tab.getId());
mGroupIdToGroupMap.put(groupId, tabGroup);
mGroupIdToGroupIndexMap.put(groupId, mGroupIdToGroupIndexMap.size());
if (mIsResetting || getTabModel().indexOf(tab) == getTabModel().getCount() - 1) {
// During a reset tabs are iterated over in TabModel order so it is safe to assume
// group ordering matches tab ordering. Same is true if the new tab is the last tab
// in the model.
mGroupIdToGroupIndexMap.put(groupId, mGroupIdToGroupIndexMap.size());
} else {
// When adding a new tab that isn't at the end of the TabModel the new group's
// index should be based on tab model order. This will offset all other groups
// resulting in the index map needing to be regenerated.
resetGroupIdToGroupIndexMap();
}
}

if (mAbsentSelectedTab != null) {
Expand All @@ -676,6 +686,18 @@ protected void addTab(Tab tab) {
}
}

private void resetGroupIdToGroupIndexMap() {
mGroupIdToGroupIndexMap.clear();
TabModel tabModel = getTabModel();
for (int i = 0; i < tabModel.getCount(); i++) {
Tab tab = tabModel.getTabAt(i);
int groupId = getRootId(tab);
if (!mGroupIdToGroupIndexMap.containsKey(groupId)) {
mGroupIdToGroupIndexMap.put(groupId, mGroupIdToGroupIndexMap.size());
}
}
}

@Override
protected void closeTab(Tab tab) {
int groupId = getRootId(tab);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,30 @@ public void addTab_ToNewGroup() {
assertThat(mTabGroupModelFilter.getCount(), equalTo(5));
}

@Test
public void addTab_ToNewGroup_NotAtEnd() {
Tab newTab = prepareTab(NEW_TAB_ID_0, NEW_TAB_ID_0, Tab.INVALID_TAB_ID);
doReturn(TabLaunchType.FROM_CHROME_UI).when(newTab).getLaunchType();
assertThat(mTabGroupModelFilter.getTabGroupCount(), equalTo(2));
assertThat(mTabGroupModelFilter.getCount(), equalTo(4));

// Add a tab to the model not at the end and ensure the indexes are updated correctly for
// all other tabs and groups.
addTabToTabModel(1, newTab);

assertThat(mTabGroupModelFilter.getTabGroupCount(), equalTo(2));
assertThat(mTabGroupModelFilter.getCount(), equalTo(5));
assertThat(mTabGroupModelFilter.getTotalTabCount(), equalTo(7));

assertThat(mTabGroupModelFilter.indexOf(mTab1), equalTo(0));
assertThat(mTabGroupModelFilter.indexOf(newTab), equalTo(1));
assertThat(mTabGroupModelFilter.indexOf(mTab2), equalTo(2));
assertThat(mTabGroupModelFilter.indexOf(mTab3), equalTo(2));
assertThat(mTabGroupModelFilter.indexOf(mTab4), equalTo(3));
assertThat(mTabGroupModelFilter.indexOf(mTab5), equalTo(4));
assertThat(mTabGroupModelFilter.indexOf(mTab6), equalTo(4));
}

@Test
public void addTab_SetRootId() {
Tab newTab = prepareTab(NEW_TAB_ID_0, NEW_TAB_ID_0, TAB1_ID);
Expand Down

0 comments on commit fe260c0

Please sign in to comment.