Commit 3e53cb46 authored by gogerald's avatar gogerald Committed by Commit Bot

[StartSurface] Do not create feed surface when activity is finishing or destroyed

The issue this CL solves should happen very rarely.

Screenshot without feed in single pane:
https://drive.google.com/a/google.com/file/d/10BPinS2HA0ciF6TbAJILNoYK9M-8WHAo/view?usp=sharing

When there is no Tabs:
https://drive.google.com/a/google.com/file/d/1r5_NJO-e1osd2bajyOM24B9ePR_QJI1F/view?usp=sharing

Bug: 1047488
Change-Id: I3844a162e1e3e4206738bc8ca60cffbb01093489
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2031441
Commit-Queue: Ganggui Tang <gogerald@chromium.org>
Reviewed-by: default avatarWei-Yin Chen (陳威尹) <wychen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737824}
parent 628f57c2
......@@ -8,6 +8,8 @@ import static org.chromium.chrome.features.start_surface.StartSurfaceProperties.
import androidx.annotation.Nullable;
import org.chromium.base.ActivityState;
import org.chromium.base.ApplicationStatus;
import org.chromium.chrome.browser.ChromeActivity;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.flags.FeatureUtilities;
......@@ -96,7 +98,8 @@ public class StartSurfaceCoordinator implements StartSurface {
mSurfaceMode != SurfaceMode.NO_START_SURFACE
? mActivity.getToolbarManager().getFakeboxDelegate()
: null,
mActivity.getNightModeStateProvider(), mActivity.getFullscreenManager());
mActivity.getNightModeStateProvider(), mActivity.getFullscreenManager(),
this::isActivityFinishingOrDestroyed);
}
// Implements StartSurface.
......@@ -245,4 +248,19 @@ public class StartSurfaceCoordinator implements StartSurface {
}
return mSecondaryTasksSurface.getController();
}
// TODO(crbug.com/1047488): This is a temporary solution of the issue crbug.com/1047488, which
// has not been reproduced locally. The crash is because we can not find ChromeTabbedActivity's
// ActivityInfo in the ApplicationStatus. However, from the code, ActivityInfo is created in
// ApplicationStatus during AsyncInitializationActivity.onCreate, which happens before
// ChromeTabbedActivity.startNativeInitialization where creates the Start surface. So one
// possible reason is the ChromeTabbedActivity is finishing or destroyed when showing overview.
private boolean isActivityFinishingOrDestroyed() {
boolean finishingOrDestroyed = mActivity.isActivityFinishingOrDestroyed()
|| ApplicationStatus.getStateForActivity(mActivity) == ActivityState.DESTROYED;
// TODO(crbug.com/1047488): Assert false. Do not do that in this CL to keep it small since
// Start surface is eanbled in the fieldtrial_testing_config.json, which requires update of
// the other browser tests.
return finishingOrDestroyed;
}
}
......@@ -76,6 +76,16 @@ class StartSurfaceMediator
TabSwitcher.Controller initialize();
}
/**
* Interface to check the associated activity state.
*/
interface ActivityStateChecker {
/**
* @return Whether the associated activity is finishing or destroyed.
*/
boolean isFinishingOrDestroyed();
}
private final ObserverList<StartSurface.OverviewModeObserver> mObservers = new ObserverList<>();
private final TabSwitcher.Controller mController;
private final TabModelSelector mTabModelSelector;
......@@ -112,6 +122,7 @@ class StartSurfaceMediator
private TabModelSelectorObserver mTabModelSelectorObserver;
private ChromeFullscreenManager mFullScreenManager;
private ChromeFullscreenManager.FullscreenListener mFullScreenListener;
private ActivityStateChecker mActivityStateChecker;
StartSurfaceMediator(TabSwitcher.Controller controller, TabModelSelector tabModelSelector,
@Nullable PropertyModel propertyModel,
......@@ -119,7 +130,7 @@ class StartSurfaceMediator
@Nullable SecondaryTasksSurfaceInitializer secondaryTasksSurfaceInitializer,
@SurfaceMode int surfaceMode, @Nullable FakeboxDelegate fakeboxDelegate,
NightModeStateProvider nightModeStateProvider,
ChromeFullscreenManager fullscreenManager) {
ChromeFullscreenManager fullscreenManager, ActivityStateChecker activityStateChecker) {
mController = controller;
mTabModelSelector = tabModelSelector;
mPropertyModel = propertyModel;
......@@ -129,6 +140,7 @@ class StartSurfaceMediator
mFakeboxDelegate = fakeboxDelegate;
mNightModeStateProvider = nightModeStateProvider;
mFullScreenManager = fullscreenManager;
mActivityStateChecker = activityStateChecker;
if (mPropertyModel != null) {
assert mSurfaceMode == SurfaceMode.SINGLE_PANE || mSurfaceMode == SurfaceMode.TWO_PANES
......@@ -406,7 +418,8 @@ class StartSurfaceMediator
// Make sure FeedSurfaceCoordinator is built before the explore surface is showing by
// default.
if (mPropertyModel.get(IS_EXPLORE_SURFACE_VISIBLE)
&& mPropertyModel.get(FEED_SURFACE_COORDINATOR) == null) {
&& mPropertyModel.get(FEED_SURFACE_COORDINATOR) == null
&& !mActivityStateChecker.isFinishingOrDestroyed()) {
mPropertyModel.set(FEED_SURFACE_COORDINATOR,
mFeedSurfaceCreator.createFeedSurfaceCoordinator(
mNightModeStateProvider.isInNightMode()));
......@@ -524,7 +537,8 @@ class StartSurfaceMediator
if (isVisible == mPropertyModel.get(IS_EXPLORE_SURFACE_VISIBLE)) return;
if (isVisible && mPropertyModel.get(IS_SHOWING_OVERVIEW)
&& mPropertyModel.get(FEED_SURFACE_COORDINATOR) == null) {
&& mPropertyModel.get(FEED_SURFACE_COORDINATOR) == null
&& !mActivityStateChecker.isFinishingOrDestroyed()) {
mPropertyModel.set(FEED_SURFACE_COORDINATOR,
mFeedSurfaceCreator.createFeedSurfaceCoordinator(
mNightModeStateProvider.isInNightMode()));
......
......@@ -23,6 +23,7 @@ import static org.chromium.chrome.browser.tasks.TasksSurfaceProperties.MV_TILES_
import static org.chromium.chrome.features.start_surface.StartSurfaceProperties.BOTTOM_BAR_CLICKLISTENER;
import static org.chromium.chrome.features.start_surface.StartSurfaceProperties.BOTTOM_BAR_HEIGHT;
import static org.chromium.chrome.features.start_surface.StartSurfaceProperties.BOTTOM_BAR_SELECTED_TAB_POSITION;
import static org.chromium.chrome.features.start_surface.StartSurfaceProperties.FEED_SURFACE_COORDINATOR;
import static org.chromium.chrome.features.start_surface.StartSurfaceProperties.IS_BOTTOM_BAR_VISIBLE;
import static org.chromium.chrome.features.start_surface.StartSurfaceProperties.IS_EXPLORE_SURFACE_VISIBLE;
import static org.chromium.chrome.features.start_surface.StartSurfaceProperties.IS_SECONDARY_SURFACE_VISIBLE;
......@@ -88,6 +89,8 @@ public class StartSurfaceMediatorUnitTest {
@Mock
private ChromeFullscreenManager mChromeFullscreenManager;
@Mock
private StartSurfaceMediator.ActivityStateChecker mActivityStateChecker;
@Mock
private LocationBarVoiceRecognitionHandler mLocationBarVoiceRecognitionHandler;
@Mock
private SecondaryTasksSurfaceInitializer mSecondaryTasksSurfaceInitializer;
......@@ -122,6 +125,7 @@ public class StartSurfaceMediatorUnitTest {
doReturn(mSecondaryTasksSurfaceController)
.when(mSecondaryTasksSurfaceInitializer)
.initialize();
doReturn(false).when(mActivityStateChecker).isFinishingOrDestroyed();
}
@After
......@@ -691,6 +695,59 @@ public class StartSurfaceMediatorUnitTest {
assertThat(mPropertyModel.get(IS_SECONDARY_SURFACE_VISIBLE), equalTo(false));
}
@Test
public void activityIsFinishingOrDestroyedSinglePane() {
doReturn(false).when(mTabModelSelector).isIncognitoSelected();
doReturn(mLocationBarVoiceRecognitionHandler)
.when(mFakeBoxDelegate)
.getLocationBarVoiceRecognitionHandler();
doReturn(true).when(mLocationBarVoiceRecognitionHandler).isVoiceSearchEnabled();
StartSurfaceMediator mediator = createStartSurfaceMediator(SurfaceMode.SINGLE_PANE);
assertThat(mediator.getOverviewState(), equalTo(OverviewModeState.NOT_SHOWN));
doReturn(2).when(mNormalTabModel).getCount();
doReturn(true).when(mActivityStateChecker).isFinishingOrDestroyed();
mediator.setOverviewState(OverviewModeState.SHOWING_HOMEPAGE);
mediator.showOverview(false);
assertThat(mediator.getOverviewState(), equalTo(OverviewModeState.SHOWN_HOMEPAGE));
assertThat(mPropertyModel.get(IS_SHOWING_OVERVIEW), equalTo(true));
assertThat(mPropertyModel.get(IS_INCOGNITO), equalTo(false));
assertThat(mPropertyModel.get(IS_EXPLORE_SURFACE_VISIBLE), equalTo(true));
assertThat(mPropertyModel.get(MV_TILES_VISIBLE), equalTo(true));
assertThat(mPropertyModel.get(IS_TAB_CAROUSEL_VISIBLE), equalTo(true));
assertThat(mPropertyModel.get(IS_SECONDARY_SURFACE_VISIBLE), equalTo(false));
assertThat(mPropertyModel.get(FEED_SURFACE_COORDINATOR), equalTo(null));
mediator.setSecondaryTasksSurfacePropertyModel(mSecondaryTasksSurfacePropertyModel);
mediator.onClick(mock(View.class));
assertThat(mediator.getOverviewState(), equalTo(OverviewModeState.SHOWN_TABSWITCHER));
assertThat(mPropertyModel.get(IS_SHOWING_OVERVIEW), equalTo(true));
assertThat(mPropertyModel.get(IS_INCOGNITO), equalTo(false));
assertThat(mPropertyModel.get(IS_EXPLORE_SURFACE_VISIBLE), equalTo(false));
assertThat(mPropertyModel.get(MV_TILES_VISIBLE), equalTo(false));
assertThat(mPropertyModel.get(IS_TAB_CAROUSEL_VISIBLE), equalTo(false));
assertThat(mPropertyModel.get(IS_SECONDARY_SURFACE_VISIBLE), equalTo(true));
assertThat(mSecondaryTasksSurfacePropertyModel.get(IS_FAKE_SEARCH_BOX_VISIBLE),
equalTo(false));
assertThat(mSecondaryTasksSurfacePropertyModel.get(IS_INCOGNITO), equalTo(false));
assertThat(mPropertyModel.get(FEED_SURFACE_COORDINATOR), equalTo(null));
mediator.onBackPressed();
assertThat(mediator.getOverviewState(), equalTo(OverviewModeState.SHOWN_HOMEPAGE));
assertThat(mPropertyModel.get(IS_SHOWING_OVERVIEW), equalTo(true));
assertThat(mPropertyModel.get(IS_INCOGNITO), equalTo(false));
assertThat(mPropertyModel.get(IS_EXPLORE_SURFACE_VISIBLE), equalTo(true));
assertThat(mPropertyModel.get(MV_TILES_VISIBLE), equalTo(true));
assertThat(mPropertyModel.get(IS_TAB_CAROUSEL_VISIBLE), equalTo(true));
assertThat(mPropertyModel.get(IS_SECONDARY_SURFACE_VISIBLE), equalTo(false));
assertThat(mPropertyModel.get(FEED_SURFACE_COORDINATOR), equalTo(null));
mediator.startedHiding();
assertThat(mediator.getOverviewState(), equalTo(OverviewModeState.NOT_SHOWN));
assertThat(mPropertyModel.get(IS_SECONDARY_SURFACE_VISIBLE), equalTo(false));
}
@Test
public void overviewModeSwitchToIncognitoModeAndBackTasksOnly() {
doReturn(false).when(mTabModelSelector).isIncognitoSelected();
......@@ -1124,6 +1181,7 @@ public class StartSurfaceMediatorUnitTest {
? mFeedSurfaceCreator
: null,
mode == SurfaceMode.SINGLE_PANE ? mSecondaryTasksSurfaceInitializer : null, mode,
mFakeBoxDelegate, mNightModeStateProvider, mChromeFullscreenManager);
mFakeBoxDelegate, mNightModeStateProvider, mChromeFullscreenManager,
mActivityStateChecker);
}
}
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