Commit 086b58b9 authored by Haiyang Pan's avatar Haiyang Pan Committed by Commit Bot

Revert "[Instant Start] Create ToolbarManger before showing overview."

This reverts commit a47390a0.

Reason for revert: "Instant_Return" related tests fail in a couple of android builders. For examples:
https://ci.chromium.org/p/chromium/builders/ci/Lollipop%20Phone%20Tester/25649
https://ci.chromium.org/p/chromium/builders/ci/android-arm64-proguard-rel/1907

Original change's description:
> [Instant Start] Create ToolbarManger before showing overview.
> 
> In this CL, we introduce an additional state in InflationObserver:
> onInflationComplete. This event happens between onPreInflationStartup
> and onPostInflationStartup, thus provides a way to split the
> implementation of onPostInflationStartup into two methods.
> 
> In RootUiCoordinatior, the ToolbarManger is created in
> onInflationComplete, and ChromeTabbedActivity calls showOverview()
> after RootUiCoordinatior#onInflationComplete but before initializing
> other components in onPostInflationStartup. Thus, ToolbarManager setups
> an OverviewModeObserver and won't lose any early events that an
> overview is shown or overview mode is changed.
> 
> Bug: 1077022
> Change-Id: I6bc8b96906a9519a928c6490bec0ca158b3d6241
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2181667
> Reviewed-by: Matthew Jones <mdjones@chromium.org>
> Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
> Reviewed-by: Ender <ender@google.com>
> Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
> Commit-Queue: Xi Han <hanxi@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#776073}

TBR=yfriedman@chromium.org,hanxi@chromium.org,mdjones@chromium.org,wychen@chromium.org,ender@google.com

Change-Id: I826c512c34d8e945d9cea6dcada8da023d840d1e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1077022
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2236946Reviewed-by: default avatarHaiyang Pan <hypan@google.com>
Commit-Queue: Haiyang Pan <hypan@google.com>
Cr-Commit-Position: refs/heads/master@{#776309}
parent 3d506040
......@@ -361,7 +361,7 @@ public class InstantStartTest {
@CommandLineFlags.Add({ChromeSwitches.DISABLE_NATIVE_INITIALIZATION,
"force-fieldtrials=Study/Group",
IMMEDIATE_RETURN_PARAMS + "/start_surface_variation/single"})
public void startSurfaceToolbarInflatedPreAndWithNativeTest() {
public void startSurfaceIncognitoSwitchCoordinatorInflatedWithNativeTest() {
// clang-format on
startMainActivityFromLauncher();
Assert.assertFalse(mActivityTestRule.getActivity().isTablet());
......@@ -380,6 +380,11 @@ public class InstantStartTest {
.getToolbarManager()
.getToolbar();
// TODO(https://crbug.com/1077022): Removes the call of
// {@link TopToolbarCoordinator#setTabSwithcherMode()} once ToolbarManager shows
// the StartSurfaceToolbar in instant start.
TestThreadUtils.runOnUiThreadBlocking(
() -> { topToolbarCoordinator.setTabSwitcherMode(true, true, false); });
onViewWaiting(allOf(withId(R.id.tab_switcher_toolbar), isDisplayed()));
StartSurfaceToolbarCoordinator startSurfaceToolbarCoordinator =
......
......@@ -230,6 +230,11 @@ public class StartSurfaceTest {
TabUiTestHelper.createTabs(cta, true, 1);
TabUiTestHelper.verifyTabModelTabCount(cta, 1, 1);
if (isInstantReturn()) {
// TODO(crbug.com/1076274): fix toolbar to avoid wrongly focusing on the toolbar
// omnibox.
return;
}
TabUiTestHelper.enterTabSwitcher(cta);
if (!isInstantReturn()) {
// TODO(crbug.com/1076274): fix toolbar to make incognito switch part of the view.
......@@ -317,6 +322,11 @@ public class StartSurfaceTest {
fail("Failed to tap 'more tabs' " + e.toString());
}
onViewWaiting(withId(R.id.secondary_tasks_surface_view));
if (isInstantReturn()) {
// TODO(crbug.com/1076274): fix toolbar to avoid wrongly focusing on the toolbar
// omnibox.
return;
}
pressBack();
onViewWaiting(withId(R.id.primary_tasks_surface_view));
......@@ -377,6 +387,11 @@ public class StartSurfaceTest {
fail("Failed to tap 'more tabs' " + e.toString());
}
onViewWaiting(withId(R.id.secondary_tasks_surface_view));
if (isInstantReturn()) {
// TODO(crbug.com/1076274): fix toolbar to avoid wrongly focusing on the toolbar
// omnibox.
return;
}
pressBack();
onViewWaiting(withId(R.id.primary_tasks_surface_view));
......@@ -442,6 +457,11 @@ public class StartSurfaceTest {
}
onViewWaiting(withId(R.id.secondary_tasks_surface_view));
if (isInstantReturn()) {
// TODO(crbug.com/1076274): fix toolbar to avoid wrongly focusing on the toolbar
// omnibox.
return;
}
pressBack();
onViewWaiting(withId(R.id.primary_tasks_surface_view));
......@@ -525,6 +545,11 @@ public class StartSurfaceTest {
// Single surface is shown as homepage. Exit in order to get into tab switcher later.
pressBack();
}
if (isInstantReturn()) {
// TODO(crbug.com/1076274): fix toolbar to avoid wrongly focusing on the toolbar
// omnibox.
return;
}
TabUiTestHelper.enterTabSwitcher(mActivityTestRule.getActivity());
onViewWaiting(allOf(withId(R.id.secondary_tasks_surface_view), isDisplayed()));
......@@ -663,6 +688,11 @@ public class StartSurfaceTest {
assertThat(
mActivityTestRule.getActivity().getTabModelSelector().getCurrentModel().getCount(),
equalTo(2));
if (isInstantReturn()) {
// TODO(crbug.com/1076274): fix toolbar to avoid wrongly focusing on the toolbar
// omnibox.
return;
}
// Press back button should close the tab opened from the Start surface.
OverviewModeBehaviorWatcher showWatcher =
TabUiTestHelper.createOverviewShowWatcher(mActivityTestRule.getActivity());
......
......@@ -1558,11 +1558,6 @@ public class ChromeTabbedActivity extends ChromeActivity<ChromeActivityComponent
mInactivityTracker = new ChromeInactivityTracker(
ChromePreferenceKeys.TABBED_ACTIVITY_LAST_BACKGROUNDED_TIME_MS_PREF);
}
@Override
protected final void dispatchOnInflationComplete() {
super.dispatchOnInflationComplete();
// When the feature flag {@link ChromeFeatureList.INSTANT_START} turns on phones (not
// tablet), a view-only start page created on Java will be shown before native is
......
......@@ -135,12 +135,6 @@ public class ActivityLifecycleDispatcherImpl implements ActivityLifecycleDispatc
}
}
void dispatchOnInflationComplete() {
for (InflationObserver observer : mInflationObservers) {
observer.onInflationComplete();
}
}
void dispatchPostInflationStartup() {
for (InflationObserver observer : mInflationObservers) {
observer.onPostInflationStartup();
......
......@@ -193,19 +193,9 @@ public abstract class AsyncInitializationActivity extends ChromeBaseAppCompatAct
@Override
public final void postInflationStartup() {
performPostInflationStartup();
dispatchOnInflationComplete();
mLifecycleDispatcher.dispatchPostInflationStartup();
}
/**
* This function allows subclasses overriding and adding additional tasks between calling
* mLifecycleDispatcher.dispatchOnInflationComplete() and
* mLifecycleDispatcher.dispatchPostInflationStartup().
*/
protected void dispatchOnInflationComplete() {
mLifecycleDispatcher.dispatchOnInflationComplete();
}
/**
* Perform post-inflation startup for the activity. Sub-classes providing custom post-inflation
* startup logic should override this method.
......
......@@ -17,7 +17,6 @@ import androidx.annotation.VisibleForTesting;
import org.chromium.base.Callback;
import org.chromium.base.MathUtils;
import org.chromium.base.library_loader.LibraryLoader;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.omnibox.SearchEngineLogoUtils;
import org.chromium.chrome.browser.omnibox.UrlBarEditingTextStateProvider;
......@@ -464,15 +463,12 @@ class StatusMediator implements IncognitoStateProvider.IncognitoStateObserver {
* - shown only if specified,
* - not shown if URL is focused.
*/
void updateLocationBarIcon() {
private void updateLocationBarIcon() {
// Update the accessibility description before continuing since we need it either way.
mModel.set(StatusProperties.STATUS_ICON_DESCRIPTION_RES, getAccessibilityDescriptionRes());
// No need to proceed further if we've already updated it for the search engine icon.
if (!LibraryLoader.getInstance().isInitialized()
|| maybeUpdateStatusIconForSearchEngineIcon()) {
return;
}
if (maybeUpdateStatusIconForSearchEngineIcon()) return;
int icon = 0;
int tint = 0;
......
......@@ -97,7 +97,6 @@ public class StatusViewCoordinator implements View.OnClickListener, UrlTextChang
* Signals that native initialization has completed.
*/
public void onNativeInitialized() {
mMediator.updateLocationBarIcon();
mMediator.setStatusClickListener(this);
}
......
......@@ -214,8 +214,6 @@ class AutocompleteMediator implements OnSuggestionsReceivedListener, StartStopWi
mOverviewModeObserver = new EmptyOverviewModeObserver() {
@Override
public void onOverviewModeStartedShowing(boolean showToolbar) {
if (!mNativeInitialized) return;
if (mDataProvider.shouldShowLocationBarInOverviewMode()) {
AutocompleteControllerJni.get().prefetchZeroSuggestResults();
}
......
......@@ -69,7 +69,7 @@ public class TabCountProvider {
public void addObserverAndTrigger(TabCountObserver observer) {
addObserver(observer);
if (mTabModelSelector != null && mTabModelSelector.isTabStateInitialized()) {
if (mTabModelSelector != null) {
observer.onTabCountChanged(mTabModelSelector.getTabModelFilterProvider()
.getCurrentTabModelFilter()
.getCount(),
......
......@@ -223,7 +223,6 @@ public class StartSurfaceToolbarCoordinator {
// It is possible that the {@link mIncognitoSwitchCoordinator} isn't created because
// inflate() is called when the native library isn't ready. So create it now.
if (isInflated()) {
assert mTabModelSelector != null;
maybeCreateIncognitoSwitchCoordinator();
}
mToolbarMediator.onNativeLibraryReady();
......@@ -260,9 +259,7 @@ public class StartSurfaceToolbarCoordinator {
}
private void maybeCreateIncognitoSwitchCoordinator() {
if (mIncognitoSwitchCoordinator != null || mTabModelSelector == null) {
return;
}
if (mIncognitoSwitchCoordinator != null) return;
if (IncognitoUtils.isIncognitoModeEnabled()
&& !StartSurfaceConfiguration.START_SURFACE_SHOW_STACK_TAB_SWITCHER.getValue()) {
......
......@@ -132,8 +132,6 @@ class StartSurfaceToolbarMediator {
// TODO(crbug.com/1042997): share with TabSwitcherModeTTPhone.
private boolean hasIncognitoTabs() {
if (mTabModelSelector == null) return false;
// Check if there is no incognito tab, or all the incognito tabs are being closed.
TabModel incognitoTabModel = mTabModelSelector.getModel(true);
for (int i = 0; i < incognitoTabModel.getCount(); i++) {
......
......@@ -2071,12 +2071,10 @@ public class ToolbarPhone extends ToolbarLayout implements Invalidator.Client, O
animators.add(animator);
}
if (mToolbarShadow != null) {
animator = ObjectAnimator.ofFloat(mToolbarShadow, ALPHA, 0);
animator.setDuration(URL_FOCUS_CHANGE_ANIMATION_DURATION_MS);
animator.setInterpolator(BakedBezierInterpolator.TRANSFORM_CURVE);
animators.add(animator);
}
animator = ObjectAnimator.ofFloat(mToolbarShadow, ALPHA, 0);
animator.setDuration(URL_FOCUS_CHANGE_ANIMATION_DURATION_MS);
animator.setInterpolator(BakedBezierInterpolator.TRANSFORM_CURVE);
animators.add(animator);
}
private void populateUrlClearFocusingAnimatorSet(List<Animator> animators) {
......@@ -2136,12 +2134,10 @@ public class ToolbarPhone extends ToolbarLayout implements Invalidator.Client, O
if (isLocationBarShownInNTP() && mNtpSearchBoxScrollPercent == 0f) return;
if (mToolbarShadow != null) {
animator = ObjectAnimator.ofFloat(mToolbarShadow, ALPHA, 1);
animator.setDuration(URL_FOCUS_CHANGE_ANIMATION_DURATION_MS);
animator.setInterpolator(BakedBezierInterpolator.TRANSFORM_CURVE);
animators.add(animator);
}
animator = ObjectAnimator.ofFloat(mToolbarShadow, ALPHA, 1);
animator.setDuration(URL_FOCUS_CHANGE_ANIMATION_DURATION_MS);
animator.setInterpolator(BakedBezierInterpolator.TRANSFORM_CURVE);
animators.add(animator);
}
@Override
......@@ -2402,7 +2398,7 @@ public class ToolbarPhone extends ToolbarLayout implements Invalidator.Client, O
boolean shouldDrawShadow = shouldDrawShadow();
int shadowVisibility = shouldDrawShadow ? View.VISIBLE : View.INVISIBLE;
if (mToolbarShadow != null && mToolbarShadow.getVisibility() != shadowVisibility) {
if (mToolbarShadow.getVisibility() != shadowVisibility) {
mToolbarShadow.setVisibility(shadowVisibility);
}
}
......
......@@ -269,7 +269,7 @@ public class RootUiCoordinator
}
@Override
public void onInflationComplete() {
public void onPostInflationStartup() {
ViewGroup coordinator = mActivity.findViewById(R.id.coordinator);
StatusBarColorController statusBarColorController = mActivity.getStatusBarColorController();
mScrimView = new ScrimView(mActivity,
......@@ -287,10 +287,6 @@ public class RootUiCoordinator
initFindToolbarManager();
initializeToolbar();
}
@Override
public void onPostInflationStartup() {
initAppMenu();
initDirectActionInitializer();
initContextualSearchSuppressor();
......
......@@ -75,8 +75,7 @@ import java.util.concurrent.atomic.AtomicBoolean;
@RunWith(ParameterizedRunner.class)
@UseRunnerDelegate(ChromeJUnit4RunnerDelegate.class)
@EnableFeatures({ChromeFeatureList.TAB_GRID_LAYOUT_ANDROID,
ChromeFeatureList.TAB_SWITCHER_ON_RETURN + "<Study",
ChromeFeatureList.START_SURFACE_ANDROID + "<Study"})
ChromeFeatureList.TAB_SWITCHER_ON_RETURN + "<Study"})
// clang-format off
@CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE,
"force-fieldtrials=Study/Group"})
......@@ -134,6 +133,7 @@ public class ReturnToChromeTest {
@Test
@SmallTest
@Feature({"ReturnToChrome"})
@EnableFeatures({ChromeFeatureList.START_SURFACE_ANDROID + "<Study"})
// clang-format off
@CommandLineFlags.Add({BASE_PARAMS + "/" + TAB_SWITCHER_ON_RETURN_MS_PARAM + "/100000"
+ "/start_surface_variation/single/open_ntp_instead_of_start/true"})
......@@ -168,6 +168,7 @@ public class ReturnToChromeTest {
@Test
@SmallTest
@Feature({"ReturnToChrome"})
@EnableFeatures({ChromeFeatureList.START_SURFACE_ANDROID + "<Study"})
// clang-format off
@CommandLineFlags.Add({BASE_PARAMS + "/" + TAB_SWITCHER_ON_RETURN_MS_PARAM + "/100000"
+ "/start_surface_variation/single/open_ntp_instead_of_start/true"})
......@@ -205,6 +206,7 @@ public class ReturnToChromeTest {
@Test
@SmallTest
@Feature({"ReturnToChrome"})
@EnableFeatures({ChromeFeatureList.START_SURFACE_ANDROID + "<Study"})
// clang-format off
@CommandLineFlags.Add({BASE_PARAMS + "/" + TAB_SWITCHER_ON_RETURN_MS_PARAM + "/100000"
+ "/start_surface_variation/single/open_ntp_instead_of_start/true"})
......@@ -237,6 +239,7 @@ public class ReturnToChromeTest {
@Test
@SmallTest
@Feature({"ReturnToChrome"})
@EnableFeatures({ChromeFeatureList.START_SURFACE_ANDROID + "<Study"})
// clang-format off
@CommandLineFlags.Add({BASE_PARAMS + "/" + TAB_SWITCHER_ON_RETURN_MS_PARAM + "/100000"
+ "/start_surface_variation/single"})
......@@ -272,11 +275,8 @@ public class ReturnToChromeTest {
@Test
@SmallTest
@Feature({"ReturnToChrome"})
// clang-format off
@CommandLineFlags.Add({BASE_PARAMS + "/" + TAB_SWITCHER_ON_RETURN_MS_PARAM + "/0"
+ "/start_surface_variation/omniboxonly"})
@CommandLineFlags.Add({BASE_PARAMS + "/" + TAB_SWITCHER_ON_RETURN_MS_PARAM + "/0"})
public void testTabSwitcherModeTriggeredBeyondThreshold() throws Exception {
// clang-format on
InstantStartTest.createTabStateFile(new int[] {0, 1});
startMainActivityWithURLWithoutCurrentTab(null);
......@@ -299,12 +299,9 @@ public class ReturnToChromeTest {
@SmallTest
@Feature({"ReturnToChrome"})
@Restriction(UiRestriction.RESTRICTION_TYPE_PHONE)
// clang-format off
@CommandLineFlags.Add({BASE_PARAMS + "/" + TAB_SWITCHER_ON_RETURN_MS_PARAM + "/0"
+ "/start_surface_variation/omniboxonly"})
@CommandLineFlags.Add({BASE_PARAMS + "/" + TAB_SWITCHER_ON_RETURN_MS_PARAM + "/0"})
@FlakyTest(message = "crbug.com/1040896")
public void testTabSwitcherModeTriggeredBeyondThreshold_UMA() throws Exception {
// clang-format on
testTabSwitcherModeTriggeredBeyondThreshold();
assertThat(mActivityTestRule.getActivity().isTablet()).isFalse();
......@@ -334,11 +331,8 @@ public class ReturnToChromeTest {
@Test
@MediumTest
@Feature({"ReturnToChrome"})
// clang-format off
@CommandLineFlags.Add({BASE_PARAMS + "/" + TAB_SWITCHER_ON_RETURN_MS_PARAM + "/0"
+ "/start_surface_variation/omniboxonly"})
@CommandLineFlags.Add({BASE_PARAMS + "/" + TAB_SWITCHER_ON_RETURN_MS_PARAM + "/0"})
public void testTabSwitcherModeTriggeredBeyondThreshold_WarmStart() throws Exception {
// clang-format on
testTabSwitcherModeTriggeredBeyondThreshold();
// Redo to trigger warm startup UMA.
......@@ -366,12 +360,9 @@ public class ReturnToChromeTest {
@MediumTest
@Feature({"ReturnToChrome"})
@Restriction(UiRestriction.RESTRICTION_TYPE_PHONE)
// clang-format off
@CommandLineFlags.Add({BASE_PARAMS + "/" + TAB_SWITCHER_ON_RETURN_MS_PARAM + "/0"
+ "/start_surface_variation/omniboxonly"})
@CommandLineFlags.Add({BASE_PARAMS + "/" + TAB_SWITCHER_ON_RETURN_MS_PARAM + "/0"})
@FlakyTest(message = "crbug.com/1040896")
public void testTabSwitcherModeTriggeredBeyondThreshold_WarmStart_UMA() throws Exception {
// clang-format on
testTabSwitcherModeTriggeredBeyondThreshold_WarmStart();
assertThat(mActivityTestRule.getActivity().isTablet()).isFalse();
......@@ -401,11 +392,8 @@ public class ReturnToChromeTest {
@Test
@SmallTest
@Feature({"ReturnToChrome"})
// clang-format off
@CommandLineFlags.Add({BASE_PARAMS + "/" + TAB_SWITCHER_ON_RETURN_MS_PARAM + "/0"
+ "/start_surface_variation/omniboxonly"})
@CommandLineFlags.Add({BASE_PARAMS + "/" + TAB_SWITCHER_ON_RETURN_MS_PARAM + "/0"})
public void testTabSwitcherModeTriggeredBeyondThreshold_NoTabs() {
// clang-format on
// Cannot use ChromeTabbedActivityTestRule.startMainActivityFromLauncher() because
// there's no tab.
startMainActivityWithURLWithoutCurrentTab(null);
......@@ -429,12 +417,9 @@ public class ReturnToChromeTest {
@SmallTest
@Feature({"ReturnToChrome"})
@Restriction(UiRestriction.RESTRICTION_TYPE_PHONE)
// clang-format off
@CommandLineFlags.Add({BASE_PARAMS + "/" + TAB_SWITCHER_ON_RETURN_MS_PARAM + "/0"
+ "/start_surface_variation/omniboxonly"})
@CommandLineFlags.Add({BASE_PARAMS + "/" + TAB_SWITCHER_ON_RETURN_MS_PARAM + "/0"})
@DisabledTest(message = "http://crbug.com/1027315")
public void testTabSwitcherModeTriggeredBeyondThreshold_NoTabs_UMA() {
// clang-format on
testTabSwitcherModeTriggeredBeyondThreshold_NoTabs();
assertThat(mActivityTestRule.getActivity().isTablet()).isFalse();
......@@ -470,8 +455,7 @@ public class ReturnToChromeTest {
@SmallTest
@Feature({"ReturnToChrome", "RenderTest"})
// clang-format off
@CommandLineFlags.Add({BASE_PARAMS + "/" + TAB_SWITCHER_ON_RETURN_MS_PARAM + "/0"
+ "/start_surface_variation/omniboxonly"})
@CommandLineFlags.Add({BASE_PARAMS + "/" + TAB_SWITCHER_ON_RETURN_MS_PARAM + "/0"})
@Restriction(UiRestriction.RESTRICTION_TYPE_PHONE)
public void testInitialScrollIndex() throws Exception {
// clang-format on
......
......@@ -15,17 +15,6 @@ public interface InflationObserver extends LifecycleObserver {
*/
void onPreInflationStartup();
/**
* Called immediately after the view hierarchy is inflated. It allows observers finishing high
* priority tasks that the owner of the {@link ActivityLifecycleDispatcher} is waiting for, and
* the owner can add additional tasks before observers' onPostInflationStartup() is called.
* Note: you shouldn't override this function unless any subclass of
* AsyncInitializationActivity waits for the observer's onInflationComplete() being completed.
* Overriding onPostInflationStartup() is preferred.
* TODO(https://crbug.com/1092421): Removes this state.
*/
default void onInflationComplete(){};
/**
* Called immediately after the view hierarchy is inflated.
* See {@link org.chromium.chrome.browser.init.BrowserParts#postInflationStartup()}.
......
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