Keep tabgroup lastShownTabId unchanged during filter reset

Currently, when resetting filter state in TabGroupModelFilter, we clear
the original mapping and re-add all the tabs. However, this approach
loses the last shown tab id information in a tab group, which leads
to some inconsistency. This CL fixes this issue.

Bug: 997029
Change-Id: I303c2a0db490ecbdd17939b72478fdb197b62736
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1767120
Commit-Queue: Yue Zhang <yuezhanggg@chromium.org>
Reviewed-by: Mei Liang <meiliang@chromium.org>
Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#690173}

chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_groups/TabGroupModelFilter.java

@@ -11,6 +11,7 @@ import android.support.annotation.NonNull;
 import org.chromium.base.ContextUtils;
 import org.chromium.base.ObserverList;
 import org.chromium.base.ThreadUtils;
+import org.chromium.base.VisibleForTesting;
 import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.base.metrics.RecordUserAction;
 import org.chromium.base.task.AsyncTask;
@@ -602,8 +603,26 @@ public class TabGroupModelFilter extends TabModelFilter {
     protected void resetFilterState() {
         mShouldRecordUma = false;
         mIsResetting = true;
+        Map<Integer, Integer> groupIdToGroupLastShownTabId = new HashMap<>();
+        for (int groupId : mGroupIdToGroupMap.keySet()) {
+            groupIdToGroupLastShownTabId.put(
+                    groupId, mGroupIdToGroupMap.get(groupId).getLastShownTabId());
+        }
+
         super.resetFilterState();
 
+        // Restore previous last shown tab ids after resetting filter state.
+        for (int groupId : mGroupIdToGroupMap.keySet()) {
+            // This happens when group with new groupId is formed after resetting filter state, i.e.
+            // when ungroup happens. Restoring last shown id of newly generated group is ignored.
+            if (!groupIdToGroupLastShownTabId.containsKey(groupId)) continue;
+            int lastShownId = groupIdToGroupLastShownTabId.get(groupId);
+            // This happens during continuous resetFilterState() calls caused by merging multiple
+            // tabs. Ignore the calls where the merge is not completed but the last shown tab has
+            // already been merged to new group.
+            if (!mGroupIdToGroupMap.get(groupId).contains(lastShownId)) continue;
+            mGroupIdToGroupMap.get(groupId).setLastShownTabId(lastShownId);
+        }
         TabModel tabModel = getTabModel();
         if (tabModel.index() == TabModel.INVALID_TAB_INDEX) {
             mCurrentGroupIndex = TabModel.INVALID_TAB_INDEX;
@@ -750,4 +769,9 @@ public class TabGroupModelFilter extends TabModelFilter {
     public boolean isClosurePending(int tabId) {
         return getTabModel().isClosurePending(tabId);
     }
+
+    @VisibleForTesting
+    int getGroupLastShownTabIdForTesting(int groupId) {
+        return mGroupIdToGroupMap.get(groupId).getLastShownTabId();
+    }
 }

chrome/android/features/tab_ui/junit/src/org/chromium/chrome/browser/tasks/tab_groups/TabGroupModelFilterUnitTest.java

@@ -383,6 +383,41 @@ public class TabGroupModelFilterUnitTest {
         assertArrayEquals(mTabs.toArray(), expectedTabModel.toArray());
     }
 
+    @Test
+    public void moveTabOutOfGroup_OtherGroupsLastShownIdUnchanged() {
+        List<Tab> expectedTabModel =
+                new ArrayList<>(Arrays.asList(mTab1, mTab2, mTab4, mTab5, mTab6, mTab3));
+        assertThat(mTab3.getRootId(), equalTo(TAB2_ID));
+
+        // By default, the last shown tab is the first tab in group by order in tab model.
+        assertThat(mTabGroupModelFilter.getGroupLastShownTabIdForTesting(TAB5_ROOT_ID),
+                equalTo(TAB5_ID));
+        assertThat(mTabGroupModelFilter.getGroupLastShownTabIdForTesting(TAB6_ROOT_ID),
+                equalTo(TAB5_ID));
+
+        // Specifically select a different tab in (Tab5, Tab6) group to change the last shown id in
+        // that group so that it is different from the default setting.
+        mTabGroupModelFilter.selectTab(mTab6);
+        assertThat(mTabGroupModelFilter.getGroupLastShownTabIdForTesting(TAB5_ROOT_ID),
+                equalTo(TAB6_ID));
+        assertThat(mTabGroupModelFilter.getGroupLastShownTabIdForTesting(TAB6_ROOT_ID),
+                equalTo(TAB6_ID));
+
+        mTabGroupModelFilter.moveTabOutOfGroup(TAB3_ID);
+
+        verify(mTabModel).moveTab(mTab3.getId(), mTabs.size());
+        verify(mTabGroupModelFilterObserver).didMoveTabOutOfGroup(mTab3, POSITION2);
+        assertThat(mTab3.getRootId(), equalTo(TAB3_ID));
+        assertArrayEquals(mTabs.toArray(), expectedTabModel.toArray());
+
+        // After ungroup, last shown ids in groups that are unrelated to this ungroup should remain
+        // unchanged.
+        assertThat(mTabGroupModelFilter.getGroupLastShownTabIdForTesting(TAB5_ROOT_ID),
+                equalTo(TAB6_ID));
+        assertThat(mTabGroupModelFilter.getGroupLastShownTabIdForTesting(TAB6_ROOT_ID),
+                equalTo(TAB6_ID));
+    }
+
     @Test
     public void mergeTabToGroup_NoUpdateTabModel() {
         List<Tab> expectedGroup = new ArrayList<>(Arrays.asList(mTab2, mTab3, mTab4));
@@ -530,6 +565,56 @@ public class TabGroupModelFilterUnitTest {
         assertArrayEquals(mTabs.toArray(), expectedTabModel.toArray());
     }
 
+    @Test
+    public void merge_OtherGroupsLastShownIdUnchanged() {
+        List<Tab> expectedGroup = new ArrayList<>(Arrays.asList(mTab1, mTab4));
+        List<Tab> expectedTabModel =
+                new ArrayList<>(Arrays.asList(mTab1, mTab4, mTab2, mTab3, mTab5, mTab6));
+        int startIndex = POSITION1;
+
+        // By default, the last shown tab is the first tab in group by order in tab model.
+        assertThat(mTabGroupModelFilter.getGroupLastShownTabIdForTesting(TAB2_ROOT_ID),
+                equalTo(TAB2_ID));
+        assertThat(mTabGroupModelFilter.getGroupLastShownTabIdForTesting(TAB3_ROOT_ID),
+                equalTo(TAB2_ID));
+        assertThat(mTabGroupModelFilter.getGroupLastShownTabIdForTesting(TAB5_ROOT_ID),
+                equalTo(TAB5_ID));
+        assertThat(mTabGroupModelFilter.getGroupLastShownTabIdForTesting(TAB6_ROOT_ID),
+                equalTo(TAB5_ID));
+
+        // Specifically select different tabs in (Tab2, Tab3) group and (Tab5, Tab6) group to change
+        // the last shown ids in respective groups so that it is different from the default setting.
+        mTabGroupModelFilter.selectTab(mTab3);
+        mTabGroupModelFilter.selectTab(mTab6);
+        assertThat(mTabGroupModelFilter.getGroupLastShownTabIdForTesting(TAB2_ROOT_ID),
+                equalTo(TAB3_ID));
+        assertThat(mTabGroupModelFilter.getGroupLastShownTabIdForTesting(TAB3_ROOT_ID),
+                equalTo(TAB3_ID));
+        assertThat(mTabGroupModelFilter.getGroupLastShownTabIdForTesting(TAB5_ROOT_ID),
+                equalTo(TAB6_ID));
+        assertThat(mTabGroupModelFilter.getGroupLastShownTabIdForTesting(TAB6_ROOT_ID),
+                equalTo(TAB6_ID));
+
+        mTabGroupModelFilter.mergeTabsToGroup(mTab4.getId(), mTab1.getId());
+
+        verify(mTabModel).moveTab(mTab4.getId(), ++startIndex);
+        verify(mTabGroupModelFilterObserver).didMergeTabToGroup(mTab4, mTab1.getId());
+        assertArrayEquals(mTabGroupModelFilter.getRelatedTabList(mTab4.getId()).toArray(),
+                expectedGroup.toArray());
+        assertArrayEquals(mTabs.toArray(), expectedTabModel.toArray());
+
+        // After merge, last shown ids in groups that are unrelated to this merge should remain
+        // unchanged.
+        assertThat(mTabGroupModelFilter.getGroupLastShownTabIdForTesting(TAB2_ROOT_ID),
+                equalTo(TAB3_ID));
+        assertThat(mTabGroupModelFilter.getGroupLastShownTabIdForTesting(TAB3_ROOT_ID),
+                equalTo(TAB3_ID));
+        assertThat(mTabGroupModelFilter.getGroupLastShownTabIdForTesting(TAB5_ROOT_ID),
+                equalTo(TAB6_ID));
+        assertThat(mTabGroupModelFilter.getGroupLastShownTabIdForTesting(TAB6_ROOT_ID),
+                equalTo(TAB6_ID));
+    }
+
     @Test
     public void moveGroup_Backward() {
         List<Tab> expectedTabModel =