Commit 18b8640a authored by Yue Zhang's avatar Yue Zhang Committed by Commit Bot

Revert "Skip popup Tabstrip update when opening tabs from background"

This reverts commit f8d430ce.

Reason for revert: This fix didn't eliminate the crash.

Original change's description:
> Skip popup Tabstrip update when opening tabs from background
> 
> Currently, we use AnchoredPopupWindow#onRectChanged to force update
> popup strip layout when new tabs are added. This could lead to a race
> condition where RecyclerView tries to recycle a view that has been
> temporarily detached due to the re-layouting process. This is only
> reproducible when there is recycling happening, and when tab is added
> from background.
> 
> This CL bypasses this crash by disabling the force update for adding
> tabs from background. Note that it is already a known issue that strip
> is not properly updated when tab is added from background, so this
> CL doesn't introduce any UI difference before/after.
> 
> Bug: 1045944, 1050401
> Change-Id: I4d7ba97f53d41042867bea9562ce2e76aade118b
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2048988
> Commit-Queue: Yue Zhang <yuezhanggg@chromium.org>
> Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#741526}

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

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

Bug: 1045944, 1050401
Change-Id: I52434502ea7661361fcac3077869f45e485955be
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2075224Reviewed-by: default avatarYue Zhang <yuezhanggg@chromium.org>
Reviewed-by: default avatarWei-Yin Chen (陳威尹) <wychen@chromium.org>
Commit-Queue: Yue Zhang <yuezhanggg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#751125}
parent 800bc23d
...@@ -111,10 +111,7 @@ public class TabGroupPopupUiMediator { ...@@ -111,10 +111,7 @@ public class TabGroupPopupUiMediator {
@Override @Override
public void didAddTab(Tab tab, int type, @TabCreationState int creationState) { public void didAddTab(Tab tab, int type, @TabCreationState int creationState) {
// TODO(crbug.com/1050846): The popup tab strip doesn't update accordingly when tab if (isTabStripShowing()) {
// is opened in background. This might require us to specify how the view measures
// in this case. Skip it for now to avoid crash crbug.com/1045944.
if (isTabStripShowing() && type != TabLaunchType.FROM_LONGPRESS_BACKGROUND) {
mUiUpdater.updateTabGroupPopUi(); mUiUpdater.updateTabGroupPopUi();
return; return;
} }
......
...@@ -40,7 +40,6 @@ import org.junit.Test; ...@@ -40,7 +40,6 @@ import org.junit.Test;
import org.junit.rules.TestRule; import org.junit.rules.TestRule;
import org.junit.runner.RunWith; import org.junit.runner.RunWith;
import org.chromium.base.test.util.CloseableOnMainThread;
import org.chromium.base.test.util.CommandLineFlags; import org.chromium.base.test.util.CommandLineFlags;
import org.chromium.base.test.util.Restriction; import org.chromium.base.test.util.Restriction;
import org.chromium.chrome.browser.ChromeTabbedActivity; import org.chromium.chrome.browser.ChromeTabbedActivity;
...@@ -57,7 +56,6 @@ import org.chromium.chrome.tab_ui.R; ...@@ -57,7 +56,6 @@ import org.chromium.chrome.tab_ui.R;
import org.chromium.chrome.test.ChromeJUnit4ClassRunner; import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
import org.chromium.chrome.test.ChromeTabbedActivityTestRule; import org.chromium.chrome.test.ChromeTabbedActivityTestRule;
import org.chromium.chrome.test.util.browser.Features; import org.chromium.chrome.test.util.browser.Features;
import org.chromium.chrome.test.util.browser.contextmenu.RevampedContextMenuUtils;
import org.chromium.content_public.browser.test.util.CriteriaHelper; import org.chromium.content_public.browser.test.util.CriteriaHelper;
import org.chromium.content_public.browser.test.util.TestThreadUtils; import org.chromium.content_public.browser.test.util.TestThreadUtils;
import org.chromium.net.test.EmbeddedTestServer; import org.chromium.net.test.EmbeddedTestServer;
...@@ -212,10 +210,16 @@ public class TabGroupPopupUiTest { ...@@ -212,10 +210,16 @@ public class TabGroupPopupUiTest {
// Click on the current tab in strip to close, and the closure should trigger tab strip // Click on the current tab in strip to close, and the closure should trigger tab strip
// update. // update.
clickOnNthItemInStrip(cta, getCurrentTabIndexInGroup(cta)); onView(withId(R.id.tab_list_view))
.inRoot(withDecorView(not(cta.getWindow().getDecorView())))
.perform(RecyclerViewActions.actionOnItemAtPosition(
getCurrentTabIndexInGroup(cta), click()));
verifyShowingTabStrip(cta, 3); verifyShowingTabStrip(cta, 3);
clickOnNthItemInStrip(cta, getCurrentTabIndexInGroup(cta)); onView(withId(R.id.tab_list_view))
.inRoot(withDecorView(not(cta.getWindow().getDecorView())))
.perform(RecyclerViewActions.actionOnItemAtPosition(
getCurrentTabIndexInGroup(cta), click()));
verifyShowingTabStrip(cta, 2); verifyShowingTabStrip(cta, 2);
} }
...@@ -255,44 +259,6 @@ public class TabGroupPopupUiTest { ...@@ -255,44 +259,6 @@ public class TabGroupPopupUiTest {
}); });
} }
@Test
@MediumTest
public void testAddTabFromBackground() throws Throwable {
launchActivity();
final ChromeTabbedActivity cta = mActivityTestRule.getActivity();
EmbeddedTestServer embeddedTestServer =
EmbeddedTestServer.createAndStartServer(InstrumentationRegistry.getContext());
String contextMenuTestUrl = embeddedTestServer.getURL(
"/chrome/test/data/android/contextmenu/context_menu_test.html");
mActivityTestRule.loadUrl(contextMenuTestUrl);
try (CloseableOnMainThread ignored = CloseableOnMainThread.StrictMode.allowDiskWrites()) {
RevampedContextMenuUtils.selectContextMenuItem(
InstrumentationRegistry.getInstrumentation(), cta,
cta.getTabModelSelector().getCurrentTab(), "testLink",
R.id.contextmenu_open_in_new_tab);
}
verifyShowingTabStrip(cta, 2);
// Add enough tabs so that adding new items later requires recyclerView to recycle old
// views.
for (int i = 0; i < 6; i++) {
onView(withId(R.id.toolbar_right_button))
.inRoot(withDecorView(not(cta.getWindow().getDecorView())))
.perform(click());
}
verifyShowingTabStrip(cta, 8);
clickOnNthItemInStrip(cta, 0);
try (CloseableOnMainThread ignored = CloseableOnMainThread.StrictMode.allowDiskWrites()) {
RevampedContextMenuUtils.selectContextMenuItem(
InstrumentationRegistry.getInstrumentation(), cta,
cta.getTabModelSelector().getCurrentTab(), "testLink",
R.id.contextmenu_open_in_new_tab);
}
verifyShowingTabStrip(cta, 9);
}
private void createTabGroupAndEnterTabPage(ChromeTabbedActivity cta, int tabCount, String url) { private void createTabGroupAndEnterTabPage(ChromeTabbedActivity cta, int tabCount, String url) {
if (url == null) { if (url == null) {
createTabs(cta, false, tabCount); createTabs(cta, false, tabCount);
...@@ -384,10 +350,4 @@ public class TabGroupPopupUiTest { ...@@ -384,10 +350,4 @@ public class TabGroupPopupUiTest {
.getTabModelFilterProvider() .getTabModelFilterProvider()
.getCurrentTabModelFilter()::isTabModelRestored); .getCurrentTabModelFilter()::isTabModelRestored);
} }
private void clickOnNthItemInStrip(ChromeTabbedActivity cta, int index) {
onView(withId(R.id.tab_list_view))
.inRoot(withDecorView(not(cta.getWindow().getDecorView())))
.perform(RecyclerViewActions.actionOnItemAtPosition(index, click()));
}
} }
...@@ -311,22 +311,6 @@ public class TabGroupPopupUiMediatorUnitTest { ...@@ -311,22 +311,6 @@ public class TabGroupPopupUiMediatorUnitTest {
verify(mUpdater, never()).updateTabGroupPopUi(); verify(mUpdater, never()).updateTabGroupPopUi();
} }
@Test
public void tabAddition_NotUpdate_Background() {
// Mock that the strip is showing.
mModel.set(TabGroupPopupUiProperties.IS_VISIBLE, true);
// Mock that tab1, tab2 and tab3 are in the same group, and tab3 has just been created from
// background.
List<Tab> tabGroup = new ArrayList<>(Arrays.asList(mTab1, mTab2, mTab3));
createTabGroup(tabGroup, TAB1_ID);
mTabModelObserverCaptor.getValue().didAddTab(mTab3, TabLaunchType.FROM_LONGPRESS_BACKGROUND,
TabCreationState.LIVE_IN_BACKGROUND);
assertThat(mModel.get(TabGroupPopupUiProperties.IS_VISIBLE), equalTo(true));
verify(mUpdater, never()).updateTabGroupPopUi();
}
@Test @Test
public void tabClosureUndone_Show() { public void tabClosureUndone_Show() {
// Mock that the strip is hiding. // Mock that the strip is hiding.
......
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