Commit 23abc6ee authored by Yue Zhang's avatar Yue Zhang Committed by Commit Bot

Revert "Fix a crash in pop-up tab strip"

This reverts commit fb08e564.

Reason for revert: Workaround introduced didn't fix the crash.

Original change's description:
> Fix a crash in pop-up tab strip
> 
> In the popup TabStrip used by Duet-TabStrip integration, there is a
> crash in RecycleView when user very intensively clicks on the TabStrip
> favicon to close multiple tabs. The stack indicates that the
> recyclerView is trying to remove the same ViewHolder in two consecutive
> remove operations, and the second call would crash. The fix introduced
> in this CL is to early end the animation if we know for sure that this
> item is going to be removed. This should help with better identifying
> the ViewHolder to remove.
> 
> This fix is inspired by the discussion here:
> https://b.corp.google.com/issues/34184109
> and the CL here:
> https://critique.corp.google.com/#review/188055979/depot/google3/java/com/google/android/apps/play/music/app/ui/common/MediaListRecyclerFragment.java
> Even though we early end the animation, there should be no visible
> animation change from the UI perspective.
> 
> Bug: 1045944
> Change-Id: I0d8a7b098e17cdc86c4ae3227cd9dc1ba37b2adc
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2036874
> Commit-Queue: Yue Zhang <yuezhanggg@chromium.org>
> Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#738392}

TBR=yusufo@chromium.org,wychen@chromium.org,yuezhanggg@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1045944
Change-Id: I733bc7729f9118808999e59695ae01b7194ef0e6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2107120Reviewed-by: default avatarYue Zhang <yuezhanggg@chromium.org>
Commit-Queue: Yue Zhang <yuezhanggg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#751146}
parent d40b8b90
...@@ -230,7 +230,7 @@ public class TabListCoordinator implements Destroyable { ...@@ -230,7 +230,7 @@ public class TabListCoordinator implements Destroyable {
mMediator = new TabListMediator(context, mModel, tabModelSelector, thumbnailProvider, mMediator = new TabListMediator(context, mModel, tabModelSelector, thumbnailProvider,
titleProvider, tabListFaviconProvider, actionOnRelatedTabs, titleProvider, tabListFaviconProvider, actionOnRelatedTabs,
selectionDelegateProvider, gridCardOnClickListenerProvider, dialogHandler, selectionDelegateProvider, gridCardOnClickListenerProvider, dialogHandler,
this::endItemAnimationForPosition, componentName, itemType); componentName, itemType);
if (mMode == TabListMode.GRID) { if (mMode == TabListMode.GRID) {
GridLayoutManager gridLayoutManager = GridLayoutManager gridLayoutManager =
...@@ -416,12 +416,4 @@ public class TabListCoordinator implements Destroyable { ...@@ -416,12 +416,4 @@ public class TabListCoordinator implements Destroyable {
void removeSpecialListItem(@UiType int uiType, int itemIdentifier) { void removeSpecialListItem(@UiType int uiType, int itemIdentifier) {
mMediator.removeSpecialItemFromModel(uiType, itemIdentifier); mMediator.removeSpecialItemFromModel(uiType, itemIdentifier);
} }
private void endItemAnimationForPosition(int pos) {
RecyclerView.ViewHolder viewHolder = mRecyclerView.findViewHolderForAdapterPosition(pos);
if (viewHolder == null || mRecyclerView.getItemAnimator() == null) {
return;
}
mRecyclerView.getItemAnimator().endAnimation(viewHolder);
}
} }
...@@ -31,7 +31,6 @@ import androidx.annotation.VisibleForTesting; ...@@ -31,7 +31,6 @@ import androidx.annotation.VisibleForTesting;
import androidx.appcompat.content.res.AppCompatResources; import androidx.appcompat.content.res.AppCompatResources;
import androidx.recyclerview.widget.GridLayoutManager; import androidx.recyclerview.widget.GridLayoutManager;
import androidx.recyclerview.widget.ItemTouchHelper; import androidx.recyclerview.widget.ItemTouchHelper;
import androidx.recyclerview.widget.RecyclerView;
import org.chromium.base.Callback; import org.chromium.base.Callback;
import org.chromium.base.Log; import org.chromium.base.Log;
...@@ -202,19 +201,6 @@ class TabListMediator { ...@@ -202,19 +201,6 @@ class TabListMediator {
} }
} }
/**
* An interface used to end current item animation in {@link RecyclerView} at certain position.
*/
interface ItemAnimationStopper {
/**
* This method stops the item animation from {@link RecyclerView.ItemAnimator} for item in
* {@code position}.
*
* @param position The position of the item whose animation will end.
*/
void endItemAnimationForPosition(int position);
}
/** /**
* An interface to show IPH for a tab. * An interface to show IPH for a tab.
*/ */
...@@ -451,8 +437,8 @@ class TabListMediator { ...@@ -451,8 +437,8 @@ class TabListMediator {
TabListFaviconProvider tabListFaviconProvider, boolean actionOnRelatedTabs, TabListFaviconProvider tabListFaviconProvider, boolean actionOnRelatedTabs,
@Nullable SelectionDelegateProvider selectionDelegateProvider, @Nullable SelectionDelegateProvider selectionDelegateProvider,
@Nullable GridCardOnClickListenerProvider gridCardOnClickListenerProvider, @Nullable GridCardOnClickListenerProvider gridCardOnClickListenerProvider,
@Nullable TabGridDialogHandler dialogHandler, ItemAnimationStopper itemAnimationStopper, @Nullable TabGridDialogHandler dialogHandler, String componentName,
String componentName, @UiType int uiType) { @UiType int uiType) {
mContext = context; mContext = context;
mTabModelSelector = tabModelSelector; mTabModelSelector = tabModelSelector;
mThumbnailProvider = thumbnailProvider; mThumbnailProvider = thumbnailProvider;
...@@ -552,14 +538,9 @@ class TabListMediator { ...@@ -552,14 +538,9 @@ class TabListMediator {
@Override @Override
public void willCloseTab(Tab tab, boolean animate) { public void willCloseTab(Tab tab, boolean animate) {
int index = mModel.indexFromId(tab.getId()); if (mModel.indexFromId(tab.getId()) == TabModel.INVALID_TAB_INDEX) return;
if (index == TabModel.INVALID_TAB_INDEX) return;
tab.removeObserver(mTabObserver); tab.removeObserver(mTabObserver);
// TODO(crbug.com/1045944): This line is for a specific crash, it should not be mModel.removeAt(mModel.indexFromId(tab.getId()));
// needed. Investigate why the crash is happening only in pop-up tab strip. Maybe
// related to how RecyclerView layouts in a wrap_content PopupWindow.
if (uiType == UiType.STRIP) itemAnimationStopper.endItemAnimationForPosition(index);
mModel.removeAt(index);
} }
@Override @Override
......
...@@ -215,8 +215,6 @@ public class TabListMediatorUnitTest { ...@@ -215,8 +215,6 @@ public class TabListMediatorUnitTest {
@Mock @Mock
TabListMediator.TabGridAccessibilityHelper mTabGridAccessibilityHelper; TabListMediator.TabGridAccessibilityHelper mTabGridAccessibilityHelper;
@Mock @Mock
TabListMediator.ItemAnimationStopper mItemAnimationStopper;
@Mock
TemplateUrlService mTemplateUrlService; TemplateUrlService mTemplateUrlService;
@Captor @Captor
...@@ -326,7 +324,7 @@ public class TabListMediatorUnitTest { ...@@ -326,7 +324,7 @@ public class TabListMediatorUnitTest {
mMediator = new TabListMediator(mContext, mModel, mTabModelSelector, mMediator = new TabListMediator(mContext, mModel, mTabModelSelector,
mTabContentManager::getTabThumbnailWithCallback, mTitleProvider, mTabContentManager::getTabThumbnailWithCallback, mTitleProvider,
mTabListFaviconProvider, false, null, mGridCardOnClickListenerProvider, null, mTabListFaviconProvider, false, null, mGridCardOnClickListenerProvider, null,
mItemAnimationStopper, getClass().getSimpleName(), UiType.CLOSABLE); getClass().getSimpleName(), UiType.CLOSABLE);
mMediator.registerOrientationListener(mGridLayoutManager); mMediator.registerOrientationListener(mGridLayoutManager);
TrackerFactory.setTrackerForTests(mTracker); TrackerFactory.setTrackerForTests(mTracker);
} }
...@@ -585,16 +583,6 @@ public class TabListMediatorUnitTest { ...@@ -585,16 +583,6 @@ public class TabListMediatorUnitTest {
assertThat(mModel.size(), equalTo(2)); assertThat(mModel.size(), equalTo(2));
} }
@Test
public void tabClosure_ForceStopItemAnimationForStrip() {
setUpForTabGroupOperation(TabListMediatorType.TAB_STRIP);
mMediatorTabModelObserver.willCloseTab(mTab2, false);
assertThat(mModel.size(), equalTo(1));
verify(mItemAnimationStopper).endItemAnimationForPosition(eq(1));
}
@Test @Test
public void tabAddition_RestoreNotComplete() { public void tabAddition_RestoreNotComplete() {
initAndAssertAllProperties(); initAndAssertAllProperties();
...@@ -1934,8 +1922,8 @@ public class TabListMediatorUnitTest { ...@@ -1934,8 +1922,8 @@ public class TabListMediatorUnitTest {
// Re-initialize the mediator to setup TemplateUrlServiceObserver if needed. // Re-initialize the mediator to setup TemplateUrlServiceObserver if needed.
mMediator = new TabListMediator(mContext, mModel, mTabModelSelector, mMediator = new TabListMediator(mContext, mModel, mTabModelSelector,
mTabContentManager::getTabThumbnailWithCallback, mTitleProvider, mTabContentManager::getTabThumbnailWithCallback, mTitleProvider,
mTabListFaviconProvider, true, null, null, null, mItemAnimationStopper, mTabListFaviconProvider, true, null, null, null, getClass().getSimpleName(),
getClass().getSimpleName(), TabProperties.UiType.CLOSABLE); TabProperties.UiType.CLOSABLE);
initAndAssertAllProperties(); initAndAssertAllProperties();
...@@ -1957,8 +1945,8 @@ public class TabListMediatorUnitTest { ...@@ -1957,8 +1945,8 @@ public class TabListMediatorUnitTest {
// Re-initialize the mediator to setup TemplateUrlServiceObserver if needed. // Re-initialize the mediator to setup TemplateUrlServiceObserver if needed.
mMediator = new TabListMediator(mContext, mModel, mTabModelSelector, mMediator = new TabListMediator(mContext, mModel, mTabModelSelector,
mTabContentManager::getTabThumbnailWithCallback, mTitleProvider, mTabContentManager::getTabThumbnailWithCallback, mTitleProvider,
mTabListFaviconProvider, true, null, null, null, mItemAnimationStopper, mTabListFaviconProvider, true, null, null, null, getClass().getSimpleName(),
getClass().getSimpleName(), TabProperties.UiType.CLOSABLE); TabProperties.UiType.CLOSABLE);
initAndAssertAllProperties(); initAndAssertAllProperties();
...@@ -2092,7 +2080,7 @@ public class TabListMediatorUnitTest { ...@@ -2092,7 +2080,7 @@ public class TabListMediatorUnitTest {
mMediator = new TabListMediator(mContext, mModel, mTabModelSelector, mMediator = new TabListMediator(mContext, mModel, mTabModelSelector,
mTabContentManager::getTabThumbnailWithCallback, mTitleProvider, mTabContentManager::getTabThumbnailWithCallback, mTitleProvider,
mTabListFaviconProvider, actionOnRelatedTabs, null, null, handler, mTabListFaviconProvider, actionOnRelatedTabs, null, null, handler,
mItemAnimationStopper, getClass().getSimpleName(), uiType); getClass().getSimpleName(), uiType);
// There are two TabModelObserver and two TabGroupModelFilter.Observer added when // There are two TabModelObserver and two TabGroupModelFilter.Observer added when
// initializing TabListMediator, one set from TabListMediator and the other from // initializing TabListMediator, one set from TabListMediator and the other from
......
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