[StartSurface] Use the index in the Tab model when animating Tab group dialog

This CL solves the issue that TabGridDialog uses wrong rect for animation
when the Tabs is sorted in MRU order.

It should be no-op when MRU is disabled.

Bug: 996381
Change-Id: I87d93a0f66199cdef50deaf7826bee77be5f7742
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1764268Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Commit-Queue: Ganggui Tang <gogerald@chromium.org>
Auto-Submit: Ganggui Tang <gogerald@chromium.org>
Cr-Commit-Position: refs/heads/master@{#690116}

chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabGridDialogCoordinator.java

@@ -66,14 +66,6 @@ public class TabGridDialogCoordinator implements TabGridDialogMediator.ResetHand
         mParentLayout.destroy();
     }
 
-    private void updateDialogContent(List<Tab> tabs) {
-        if (tabs != null) {
-            mMediator.onReset(tabs.get(0).getId());
-        } else {
-            mMediator.onReset(null);
-        }
-    }
-
     boolean isVisible() {
         return mMediator.isVisible();
     }
@@ -94,7 +86,7 @@ public class TabGridDialogCoordinator implements TabGridDialogMediator.ResetHand
     @Override
     public void resetWithListOfTabs(@Nullable List<Tab> tabs) {
         mTabListCoordinator.resetWithListOfTabs(tabs);
-        updateDialogContent(tabs);
+        mMediator.onReset(tabs);
     }
 
     @Override

chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabGridDialogMediator.java

@@ -17,12 +17,11 @@ import org.chromium.chrome.browser.tabmodel.EmptyTabModelSelectorObserver;
 import org.chromium.chrome.browser.tabmodel.TabCreatorManager;
 import org.chromium.chrome.browser.tabmodel.TabLaunchType;
 import org.chromium.chrome.browser.tabmodel.TabModel;
+import org.chromium.chrome.browser.tabmodel.TabModelFilter;
 import org.chromium.chrome.browser.tabmodel.TabModelObserver;
 import org.chromium.chrome.browser.tabmodel.TabModelSelector;
 import org.chromium.chrome.browser.tabmodel.TabModelSelectorObserver;
-import org.chromium.chrome.browser.tabmodel.TabModelUtils;
 import org.chromium.chrome.browser.tabmodel.TabSelectionType;
-import org.chromium.chrome.browser.tasks.tab_groups.TabGroupModelFilter;
 import org.chromium.chrome.browser.util.UrlConstants;
 import org.chromium.chrome.browser.widget.ScrimView;
 import org.chromium.chrome.tab_ui.R;
@@ -64,10 +63,10 @@ public class TabGridDialogMediator {
         /**
          * Provide a {@link TabGridDialogParent.AnimationParams} to setup the animation.
          *
-         * @param index Index in GridTabSwitcher of the tab whose position is requested.
+         * @param tabId The id of the tab whose position is requested.
          * @return A {@link TabGridDialogParent.AnimationParams} used to setup the animation.
          */
-        TabGridDialogParent.AnimationParams getAnimationParamsForIndex(int index);
+        TabGridDialogParent.AnimationParams getAnimationParamsForTab(int tabId);
     }
 
     private final Context mContext;
@@ -121,7 +120,6 @@ public class TabGridDialogMediator {
                 List<Tab> relatedTabs = getRelatedTabs(tab.getId());
                 // If the group is empty, update the animation and hide the dialog.
                 if (relatedTabs.size() == 0) {
-                    mCurrentTabId = Tab.INVALID_TAB_ID;
                     hideDialog(false);
                     return;
                 }
@@ -181,30 +179,27 @@ public class TabGridDialogMediator {
         if (!showAnimation) {
             mModel.set(TabGridSheetProperties.ANIMATION_PARAMS, null);
         } else {
-            TabGroupModelFilter filter =
-                    (TabGroupModelFilter) mTabModelSelector.getTabModelFilterProvider()
-                            .getCurrentTabModelFilter();
-            int index = filter.indexOf(
-                    TabModelUtils.getTabById(mTabModelSelector.getCurrentModel(), mCurrentTabId));
-            if (mAnimationParamsProvider != null && index != TabModel.INVALID_TAB_INDEX) {
+            if (mAnimationParamsProvider != null && mCurrentTabId != Tab.INVALID_TAB_ID) {
                 mModel.set(TabGridSheetProperties.ANIMATION_PARAMS,
-                        mAnimationParamsProvider.getAnimationParamsForIndex(index));
+                        mAnimationParamsProvider.getAnimationParamsForTab(mCurrentTabId));
             }
         }
         mDialogResetHandler.resetWithListOfTabs(null);
     }
 
-    void onReset(Integer tabId) {
-        TabGroupModelFilter filter =
-                (TabGroupModelFilter) mTabModelSelector.getTabModelFilterProvider()
-                        .getCurrentTabModelFilter();
-        if (tabId != null) {
-            mCurrentTabId = tabId;
-            int index = filter.indexOf(
-                    TabModelUtils.getTabById(mTabModelSelector.getCurrentModel(), tabId));
+    void onReset(@Nullable List<Tab> tabs) {
+        if (tabs == null) {
+            mCurrentTabId = Tab.INVALID_TAB_ID;
+        } else {
+            TabModelFilter filter =
+                    mTabModelSelector.getTabModelFilterProvider().getCurrentTabModelFilter();
+            mCurrentTabId = filter.getTabAt(filter.indexOf(tabs.get(0))).getId();
+        }
+
+        if (mCurrentTabId != Tab.INVALID_TAB_ID) {
             if (mAnimationParamsProvider != null) {
                 TabGridDialogParent.AnimationParams params =
-                        mAnimationParamsProvider.getAnimationParamsForIndex(index);
+                        mAnimationParamsProvider.getAnimationParamsForTab(mCurrentTabId);
                 mModel.set(TabGridSheetProperties.ANIMATION_PARAMS, params);
             }
             updateDialog();
@@ -275,8 +270,10 @@ public class TabGridDialogMediator {
 
     private View.OnClickListener getAddButtonClickListener() {
         return view -> {
-            hideDialog(false);
+            // Get the current Tab first since hideDialog causes mCurrentTabId to be
+            // Tab.INVALID_TAB_ID.
             Tab currentTab = mTabModelSelector.getTabById(mCurrentTabId);
+            hideDialog(false);
             if (currentTab == null) {
                 mTabCreatorManager.getTabCreator(mTabModelSelector.isIncognitoSelected())
                         .launchNTP();

chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabListCoordinator.java

@@ -213,7 +213,7 @@ public class TabListCoordinator implements Destroyable {
      */
     boolean updateThumbnailLocation() {
         Rect rect = mRecyclerView.getRectOfCurrentThumbnail(
-                mMediator.indexOfSelected(), mMediator.selectedTabId());
+                mMediator.indexOfTab(mMediator.selectedTabId()), mMediator.selectedTabId());
         if (rect == null) return false;
         mThumbnailLocationOfCurrentTab.set(rect);
         return true;
@@ -242,6 +242,10 @@ public class TabListCoordinator implements Destroyable {
         return resetWithListOfTabs(tabs, false, false);
     }
 
+    int indexOfTab(int tabId) {
+        return mMediator.indexOfTab(tabId);
+    }
+
     void softCleanup() {
         mMediator.softCleanup();
     }

chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabListMediator.java

@@ -1005,11 +1005,16 @@ class TabListMediator {
         return mTabModelSelector.getCurrentTabId();
     }
 
-    // Find the index of the currently selected tab in the TabListRecyclerView.
-    // Note that Tabs may have different index in TabListModel/TabListRecyclerView and
-    // mTabModelSelector, like when resetWithListOfTabs is called with 'mruMode = true'.
-    int indexOfSelected() {
-        return mModel.indexFromId(selectedTabId());
+    /**
+     * Find the index of the given tab in the {@link TabListRecyclerView}.
+     * Note that Tabs may have different index in {@link TabListRecyclerView} and {@link
+     * TabModelSelector}, like when {@link resetWithListOfTabs} above is called with MRU mode
+     * enabled.
+     * @param tabId The given Tab id.
+     * @return The index of the Tab in the {@link TabListRecyclerView}.
+     */
+    int indexOfTab(int tabId) {
+        return mModel.indexFromId(tabId);
     }
 
     @VisibleForTesting

chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabSwitcherCoordinator.java

@@ -229,7 +229,8 @@ public class TabSwitcherCoordinator implements Destroyable, TabSwitcher,
         return mTabListCoordinator.resetWithListOfTabs(tabs, quickMode, mruMode);
     }
 
-    private TabGridDialogParent.AnimationParams getTabGridDialogAnimationParams(int index) {
+    private TabGridDialogParent.AnimationParams getTabGridDialogAnimationParams(int tabId) {
+        int index = mTabListCoordinator.indexOfTab(tabId);
         View itemView = mTabListCoordinator.getContainerView()
                                 .findViewHolderForAdapterPosition(index)
                                 .itemView;

chrome/android/features/tab_ui/junit/src/org/chromium/chrome/browser/tasks/tab_management/TabGridDialogMediatorUnitTest.java

@@ -133,6 +133,8 @@ public class TabGridDialogMediatorUnitTest {
         doReturn(mTabGroupModelFilter).when(mTabModelFilterProvider).getCurrentTabModelFilter();
         doReturn(POSITION1).when(mTabGroupModelFilter).indexOf(mTab1);
         doReturn(POSITION2).when(mTabGroupModelFilter).indexOf(mTab2);
+        doReturn(mTab1).when(mTabGroupModelFilter).getTabAt(POSITION1);
+        doReturn(mTab2).when(mTabGroupModelFilter).getTabAt(POSITION2);
         doReturn(tabs1).when(mTabGroupModelFilter).getRelatedTabList(TAB1_ID);
         doReturn(tabs2).when(mTabGroupModelFilter).getRelatedTabList(TAB2_ID);
         doReturn(mTab1).when(mTabModelSelector).getCurrentTab();
@@ -156,7 +158,7 @@ public class TabGridDialogMediatorUnitTest {
                 .getQuantityString(R.plurals.bottom_tab_grid_title_placeholder, 2, 2);
         doReturn(mAnimationParams)
                 .when(mAnimationParamsProvider)
-                .getAnimationParamsForIndex(anyInt());
+                .getAnimationParamsForTab(anyInt());
         doReturn(mTabCreator).when(mTabCreatorManager).getTabCreator(anyBoolean());
 
         mModel = new PropertyModel(TabGridSheetProperties.ALL_KEYS);
@@ -288,11 +290,13 @@ public class TabGridDialogMediatorUnitTest {
 
         mTabModelObserverCaptor.getValue().willCloseTab(mTab1, false);
 
-        assertThat(mMediator.getCurrentTabIdForTest(), equalTo(Tab.INVALID_TAB_ID));
         assertThat(mModel.get(TabGridSheetProperties.ANIMATION_PARAMS), equalTo(null));
         verify(mDialogResetHandler).resetWithListOfTabs(null);
         verify(mTabSwitcherResetHandler, never())
                 .resetWithTabList(mTabGroupModelFilter, false, false);
+
+        mMediator.onReset(null);
+        assertThat(mMediator.getCurrentTabIdForTest(), equalTo(Tab.INVALID_TAB_ID));
     }
 
     @Test
@@ -375,6 +379,7 @@ public class TabGridDialogMediatorUnitTest {
         // Mock that the animation source Rect is null.
         mModel.set(TabGridSheetProperties.ANIMATION_PARAMS, null);
 
+        mMediator.setCurrentTabIdForTest(TAB1_ID);
         mMediator.hideDialog(true);
 
         // Animation params should be specified.
@@ -398,7 +403,7 @@ public class TabGridDialogMediatorUnitTest {
         mModel.set(TabGridSheetProperties.ANIMATION_PARAMS, null);
         mModel.set(TabGridSheetProperties.HEADER_TITLE, null);
 
-        mMediator.onReset(TAB1_ID);
+        mMediator.onReset(Arrays.asList(mTab1));
 
         assertThat(mModel.get(TabGridSheetProperties.IS_DIALOG_VISIBLE), equalTo(true));
         // Animation source Rect should be updated with specific Rect.
@@ -418,7 +423,7 @@ public class TabGridDialogMediatorUnitTest {
         mModel.set(TabGridSheetProperties.ANIMATION_PARAMS, null);
         mModel.set(TabGridSheetProperties.HEADER_TITLE, null);
 
-        mMediator.onReset(TAB1_ID);
+        mMediator.onReset(Arrays.asList(mTab1));
 
         assertThat(mModel.get(TabGridSheetProperties.IS_DIALOG_VISIBLE), equalTo(true));
         // Animation params should not be specified.

chrome/android/features/tab_ui/junit/src/org/chromium/chrome/browser/tasks/tab_management/TabListMediatorUnitTest.java

@@ -1085,6 +1085,37 @@ public class TabListMediatorUnitTest {
                 .setSpanCount(TabListCoordinator.GRID_LAYOUT_SPAN_COUNT_PORTRAIT);
     }
 
+    @Test
+    public void resetWithListOfTabs_MruOrder() {
+        List<Tab> tabs = new ArrayList<>();
+        for (int i = 0; i < mTabModel.getCount(); i++) {
+            tabs.add(mTabModel.getTabAt(i));
+        }
+        assertThat(tabs.size(), equalTo(2));
+
+        long timestamp1 = 1;
+        long timestamp2 = 2;
+        doReturn(timestamp1).when(mTab1).getTimestampMillis();
+        doReturn(timestamp2).when(mTab2).getTimestampMillis();
+        mMediator.resetWithListOfTabs(tabs, /*quickMode =*/false, /*mruMode =*/true);
+
+        assertThat(mModel.size(), equalTo(2));
+        assertThat(mModel.get(0).get(TabProperties.TAB_ID), equalTo(TAB2_ID));
+        assertThat(mModel.get(1).get(TabProperties.TAB_ID), equalTo(TAB1_ID));
+        assertThat(mMediator.indexOfTab(TAB1_ID), equalTo(1));
+        assertThat(mMediator.indexOfTab(TAB2_ID), equalTo(0));
+
+        doReturn(timestamp2).when(mTab1).getTimestampMillis();
+        doReturn(timestamp1).when(mTab2).getTimestampMillis();
+        mMediator.resetWithListOfTabs(tabs, /*quickMode =*/false, /*mruMode =*/true);
+
+        assertThat(mModel.size(), equalTo(2));
+        assertThat(mModel.get(0).get(TabProperties.TAB_ID), equalTo(TAB1_ID));
+        assertThat(mModel.get(1).get(TabProperties.TAB_ID), equalTo(TAB2_ID));
+        assertThat(mMediator.indexOfTab(TAB1_ID), equalTo(0));
+        assertThat(mMediator.indexOfTab(TAB2_ID), equalTo(1));
+    }
+
     private void initAndAssertAllProperties() {
         List<Tab> tabs = new ArrayList<>();
         for (int i = 0; i < mTabModel.getCount(); i++) {