Commit 64c520f4 authored by Gang Wu's avatar Gang Wu Committed by Commit Bot

Revert "Make LayoutManager observer notification async"

This reverts commit 9183c032.

Reason for revert: 
https://ci.chromium.org/p/chromium/builders/ci/Lollipop%20Phone%20Tester

Original change's description:
> Make LayoutManager observer notification async
> 
> The process of supplying OverviewModeBehavior is now async to prevent re-entrancy in OneshotSupplier (as of https://crrev.com/c/2321067). Since observer notification in LayoutManager is synchronous, this creates a race condition where observers miss an update because an event (e.g. a touch event, as in the linked bug) queued before the call to set() can trigger synchronous observer notification before observers can add themselves.
> 
> Bug: 1129783
> Change-Id: Ia482a109576539d54f171950468e490972e72bc1
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2418981
> Reviewed-by: David Trainor <dtrainor@chromium.org>
> Reviewed-by: Xi Han <hanxi@chromium.org>
> Commit-Queue: Patrick Noland <pnoland@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#809386}

TBR=dtrainor@chromium.org,hanxi@chromium.org,pnoland@chromium.org

Change-Id: Ic1028b99d727da8bf2b85955745068de1eaa45bd
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1129783
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2426724Reviewed-by: default avatarGang Wu <gangwu@chromium.org>
Commit-Queue: Gang Wu <gangwu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809787}
parent 3cc69c75
......@@ -561,8 +561,9 @@ public class ChromeTabbedActivity extends ChromeActivity<ChromeActivityComponent
}
}
mLayoutManager = new LayoutManagerChromePhone(compositorViewHolder, mContentContainer,
mStartSurface, getTabContentManagerSupplier(), mOverviewModeBehaviorSupplier);
mStartSurface, getTabContentManagerSupplier());
mOverviewModeController = mLayoutManager;
mOverviewModeBehaviorSupplier.set(mOverviewModeController);
}
}
......@@ -571,10 +572,10 @@ public class ChromeTabbedActivity extends ChromeActivity<ChromeActivityComponent
try (TraceEvent e = TraceEvent.scoped(
"ChromeTabbedActivity.setupCompositorContentPreNativeForTablet")) {
mLayoutManager =
new LayoutManagerChromeTablet(getCompositorViewHolder(), mContentContainer,
getTabContentManagerSupplier(), mOverviewModeBehaviorSupplier);
mLayoutManager = new LayoutManagerChromeTablet(
getCompositorViewHolder(), mContentContainer, getTabContentManagerSupplier());
mOverviewModeController = mLayoutManager;
mOverviewModeBehaviorSupplier.set(mOverviewModeController);
}
}
......@@ -838,7 +839,7 @@ public class ChromeTabbedActivity extends ChromeActivity<ChromeActivityComponent
if (!mPendingInitialTabCreation
&& !(TabUiFeatureUtilities.supportInstantStart(isTablet())
&& shouldShowTabSwitcherOnStart())) {
setInitialOverviewState();
setInitialOverviewStateAsync();
}
if (TabUiFeatureUtilities.isConditionalTabStripEnabled()
......@@ -915,7 +916,14 @@ public class ChromeTabbedActivity extends ChromeActivity<ChromeActivityComponent
}
}
private void setInitialOverviewState() {
// Posts a call to setInitialOverviewStateSync that will trigger after observers of
// mOverviewModeBehaviorSupplier have a chance to process the supplied OverviewModeBehavior and
// add themselves as observers.
private void setInitialOverviewStateAsync() {
mOverviewModeBehaviorSupplier.onAvailable((unused) -> setInitalOverviewStateSync());
}
private void setInitalOverviewStateSync() {
boolean isOverviewVisible = mOverviewModeController.overviewVisible();
if (shouldShowTabSwitcherOnStart() && !isOverviewVisible) {
......@@ -1127,7 +1135,7 @@ public class ChromeTabbedActivity extends ChromeActivity<ChromeActivityComponent
if (hasStartWithNativeBeenCalled()
&& !(TabUiFeatureUtilities.supportInstantStart(isTablet())
&& shouldShowTabSwitcherOnStart())) {
setInitialOverviewState();
setInitialOverviewStateAsync();
}
}
......@@ -1512,7 +1520,7 @@ public class ChromeTabbedActivity extends ChromeActivity<ChromeActivityComponent
if (shouldShowTabSwitcherOnStart()) {
mLayoutManager.setTabModelSelector(mTabModelSelectorImpl);
mIsAccessibilityTabSwitcherEnabled = DeviceClassManager.enableAccessibilityLayout();
setInitialOverviewState();
setInitialOverviewStateAsync();
}
}
}
......
......@@ -14,7 +14,6 @@ import org.chromium.base.Callback;
import org.chromium.base.ObserverList;
import org.chromium.base.metrics.RecordUserAction;
import org.chromium.base.supplier.ObservableSupplier;
import org.chromium.base.supplier.OneshotSupplierImpl;
import org.chromium.chrome.browser.ActivityTabProvider;
import org.chromium.chrome.browser.accessibility_tab_switcher.OverviewListLayout;
import org.chromium.chrome.browser.browser_controls.BrowserControlsStateProvider;
......@@ -77,7 +76,6 @@ public class LayoutManagerChrome
private boolean mCreateOverviewLayout;
protected ObservableSupplier<TabContentManager> mTabContentManagerSupplier;
private final OneshotSupplierImpl<OverviewModeBehavior> mOverviewModeBehaviorSupplier;
/**
* Creates the {@link LayoutManagerChrome} instance.
......@@ -89,8 +87,7 @@ public class LayoutManagerChrome
*/
public LayoutManagerChrome(LayoutManagerHost host, ViewGroup contentContainer,
boolean createOverviewLayout, @Nullable StartSurface startSurface,
ObservableSupplier<TabContentManager> tabContentManagerSupplier,
OneshotSupplierImpl<OverviewModeBehavior> overviewModeBehaviorSupplier) {
ObservableSupplier<TabContentManager> tabContentManagerSupplier) {
super(host, contentContainer, tabContentManagerSupplier);
Context context = host.getContext();
LayoutRenderHost renderHost = host.getLayoutRenderHost();
......@@ -122,8 +119,10 @@ public class LayoutManagerChrome
@Override
public void onStateChanged(@OverviewModeState int overviewModeState,
boolean shouldShowTabSwitcherToolbar) {
notifyObserversStateChanged(
overviewModeState, shouldShowTabSwitcherToolbar);
for (OverviewModeObserver observer : mOverviewModeObservers) {
observer.onOverviewModeStateChanged(
overviewModeState, shouldShowTabSwitcherToolbar);
}
}
});
final ObservableSupplier<? extends BrowserControlsStateProvider>
......@@ -135,9 +134,6 @@ public class LayoutManagerChrome
mCreateOverviewLayout = true;
}
}
mOverviewModeBehaviorSupplier = overviewModeBehaviorSupplier;
mOverviewModeBehaviorSupplier.set(this);
}
/**
......@@ -282,7 +278,9 @@ public class LayoutManagerChrome
if (isOverviewLayout(layoutBeingShown)) {
boolean showToolbar = animate && (!mEnableAnimations
|| getTabModelSelector().getCurrentModel().getCount() <= 0);
notifyObserversStartedShowing(showToolbar);
for (OverviewModeObserver observer : mOverviewModeObservers) {
observer.onOverviewModeStartedShowing(showToolbar);
}
}
}
......@@ -300,7 +298,9 @@ public class LayoutManagerChrome
boolean creatingNtp = layoutBeingHidden == mOverviewLayout && mCreatingNtp;
notifyObserversStartedHiding(showToolbar, creatingNtp);
for (OverviewModeObserver observer : mOverviewModeObservers) {
observer.onOverviewModeStartedHiding(showToolbar, creatingNtp);
}
}
}
......@@ -309,7 +309,9 @@ public class LayoutManagerChrome
super.doneShowing();
if (isOverviewLayout(getActiveLayout())) {
notifyObserversFinishedShowing();
for (OverviewModeObserver observer : mOverviewModeObservers) {
observer.onOverviewModeFinishedShowing();
}
}
}
......@@ -325,7 +327,9 @@ public class LayoutManagerChrome
super.doneHiding();
if (isOverviewLayout(layoutBeingHidden)) {
notifyObserversFinishedHiding();
for (OverviewModeObserver observer : mOverviewModeObservers) {
observer.onOverviewModeFinishedHiding();
}
}
}
......@@ -592,46 +596,4 @@ public class LayoutManagerChrome
.closeTab(lastTab, tab, false, false, false);
}
}
private void notifyObserversStartedShowing(boolean showToolbar) {
mOverviewModeBehaviorSupplier.onAvailable((unused) -> {
for (OverviewModeObserver overviewModeObserver : mOverviewModeObservers) {
overviewModeObserver.onOverviewModeStartedShowing(showToolbar);
}
});
}
private void notifyObserversFinishedShowing() {
mOverviewModeBehaviorSupplier.onAvailable((unused) -> {
for (OverviewModeObserver overviewModeObserver : mOverviewModeObservers) {
overviewModeObserver.onOverviewModeFinishedShowing();
}
});
}
private void notifyObserversStartedHiding(boolean showToolbar, boolean creatingNtp) {
mOverviewModeBehaviorSupplier.onAvailable((unused) -> {
for (OverviewModeObserver overviewModeObserver : mOverviewModeObservers) {
overviewModeObserver.onOverviewModeStartedHiding(showToolbar, creatingNtp);
}
});
}
private void notifyObserversFinishedHiding() {
mOverviewModeBehaviorSupplier.onAvailable((unused) -> {
for (OverviewModeObserver overviewModeObserver : mOverviewModeObservers) {
overviewModeObserver.onOverviewModeFinishedHiding();
}
});
}
private void notifyObserversStateChanged(
@OverviewModeState int overviewModeState, boolean shouldShowTabSwitcherToolbar) {
mOverviewModeBehaviorSupplier.onAvailable((unused) -> {
for (OverviewModeObserver overviewModeObserver : mOverviewModeObservers) {
overviewModeObserver.onOverviewModeStateChanged(
overviewModeState, shouldShowTabSwitcherToolbar);
}
});
}
}
......@@ -8,7 +8,6 @@ import android.content.Context;
import android.view.ViewGroup;
import org.chromium.base.supplier.ObservableSupplier;
import org.chromium.base.supplier.OneshotSupplierImpl;
import org.chromium.chrome.browser.ActivityTabProvider;
import org.chromium.chrome.browser.compositor.layouts.content.TabContentManager;
import org.chromium.chrome.browser.compositor.layouts.phone.SimpleAnimationLayout;
......@@ -42,10 +41,8 @@ public class LayoutManagerChromePhone extends LayoutManagerChrome {
*/
public LayoutManagerChromePhone(LayoutManagerHost host, ViewGroup contentContainer,
StartSurface startSurface,
ObservableSupplier<TabContentManager> tabContentManagerSupplier,
OneshotSupplierImpl<OverviewModeBehavior> overviewModeBehaviorSupplier) {
super(host, contentContainer, true, startSurface, tabContentManagerSupplier,
overviewModeBehaviorSupplier);
ObservableSupplier<TabContentManager> tabContentManagerSupplier) {
super(host, contentContainer, true, startSurface, tabContentManagerSupplier);
}
@Override
......
......@@ -7,7 +7,6 @@ package org.chromium.chrome.browser.compositor.layouts;
import android.view.ViewGroup;
import org.chromium.base.supplier.ObservableSupplier;
import org.chromium.base.supplier.OneshotSupplierImpl;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.ActivityTabProvider;
import org.chromium.chrome.browser.compositor.layouts.content.TabContentManager;
......@@ -38,10 +37,8 @@ public class LayoutManagerChromeTablet extends LayoutManagerChrome {
* @param tabContentManagerSupplier Supplier of the {@link TabContentManager} instance.
*/
public LayoutManagerChromeTablet(LayoutManagerHost host, ViewGroup contentContainer,
ObservableSupplier<TabContentManager> tabContentManagerSupplier,
OneshotSupplierImpl<OverviewModeBehavior> overviewModeBehaviorSupplier) {
super(host, contentContainer, false, null, tabContentManagerSupplier,
overviewModeBehaviorSupplier);
ObservableSupplier<TabContentManager> tabContentManagerSupplier) {
super(host, contentContainer, false, null, tabContentManagerSupplier);
mTabStripLayoutHelperManager = new StripLayoutHelperManager(
host.getContext(), this, mHost.getLayoutRenderHost(), () -> mTitleCache);
......
......@@ -237,8 +237,7 @@ public class TabbedAppMenuTest {
() -> mActivityTestRule.getActivity().getLayoutManager().hideOverview(false));
Assert.assertFalse("Overview shouldn't be showing.",
mActivityTestRule.getActivity().getOverviewModeBehavior().overviewVisible());
CriteriaHelper.pollUiThread(
() -> !mAppMenuHandler.isAppMenuShowing(), "App menu shouldn't be showing.");
Assert.assertFalse("App menu shouldn't be showing.", mAppMenuHandler.isAppMenuShowing());
}
@Test
......
......@@ -30,7 +30,6 @@ import org.mockito.MockitoAnnotations;
import org.chromium.base.MathUtils;
import org.chromium.base.supplier.ObservableSupplierImpl;
import org.chromium.base.supplier.OneshotSupplierImpl;
import org.chromium.base.test.UiThreadTest;
import org.chromium.base.test.util.CommandLineFlags;
import org.chromium.base.test.util.DisabledTest;
......@@ -167,10 +166,8 @@ public class LayoutManagerTest implements MockTabModelDelegate {
ObservableSupplierImpl<TabContentManager> tabContentManagerSupplier =
new ObservableSupplierImpl<>();
OneshotSupplierImpl<OverviewModeBehavior> overviewModeBehaviorSupplier =
new OneshotSupplierImpl<>();
mManagerPhone = new LayoutManagerChromePhone(layoutManagerHost, container, null,
tabContentManagerSupplier, overviewModeBehaviorSupplier);
mManagerPhone = new LayoutManagerChromePhone(
layoutManagerHost, container, null, tabContentManagerSupplier);
tabContentManagerSupplier.set(tabContentManager);
mManager = mManagerPhone;
CompositorAnimationHandler.setTestingMode(true);
......
......@@ -4,7 +4,6 @@
package org.chromium.chrome.browser.ui.system;
import android.app.Activity;
import android.content.res.Resources;
import android.graphics.Color;
import android.os.Build;
......@@ -12,7 +11,6 @@ import android.os.Build;
import androidx.annotation.ColorInt;
import androidx.test.filters.LargeTest;
import org.hamcrest.Matchers;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Rule;
......@@ -36,15 +34,10 @@ import org.chromium.chrome.test.ChromeTabbedActivityTestRule;
import org.chromium.chrome.test.util.browser.Features;
import org.chromium.chrome.test.util.browser.ThemeTestUtils;
import org.chromium.components.browser_ui.styles.ChromeColors;
import org.chromium.content_public.browser.test.util.Criteria;
import org.chromium.content_public.browser.test.util.CriteriaHelper;
import org.chromium.content_public.browser.test.util.TestThreadUtils;
import org.chromium.ui.test.util.UiRestriction;
import org.chromium.ui.util.ColorUtils;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeoutException;
/**
* {@link StatusBarColorController} tests.
* There are additional status bar color tests in {@link BrandColorTest}.
......@@ -75,7 +68,7 @@ public class StatusBarColorControllerTest {
@Feature({"StatusBar"})
@MinAndroidSdkLevel(Build.VERSION_CODES.LOLLIPOP_MR1)
@Restriction({UiRestriction.RESTRICTION_TYPE_PHONE}) // Status bar is always black on tablets
public void testColorToggleIncongitoInOverview() throws Exception {
public void testColorToggleIncongitoInOverview() {
ChromeTabbedActivity activity = mActivityTestRule.getActivity();
Resources resources = activity.getResources();
final int expectedOverviewStandardColor = defaultColorFallbackToBlack(
......@@ -91,7 +84,8 @@ public class StatusBarColorControllerTest {
TestThreadUtils.runOnUiThreadBlocking(
() -> { activity.getLayoutManager().showOverview(false /* animate */); });
waitForStatusBarColor(activity, expectedOverviewIncognitoColor);
ThemeTestUtils.assertStatusBarColor(activity, expectedOverviewIncognitoColor);
TestThreadUtils.runOnUiThreadBlocking(
() -> { tabModelSelector.selectModel(false /* incongito */); });
ThemeTestUtils.assertStatusBarColor(activity, expectedOverviewStandardColor);
......@@ -115,11 +109,11 @@ public class StatusBarColorControllerTest {
"/chrome/test/data/android/theme_color_test.html");
mActivityTestRule.loadUrl(pageWithBrandColorUrl);
ThemeTestUtils.waitForThemeColor(activity, Color.RED);
waitForStatusBarColor(activity, Color.RED);
ThemeTestUtils.assertStatusBarColor(activity, Color.RED);
TestThreadUtils.runOnUiThreadBlocking(
() -> { activity.getLayoutManager().showOverview(false /* animate */); });
waitForStatusBarColor(activity, expectedDefaultStandardColor);
ThemeTestUtils.assertStatusBarColor(activity, expectedDefaultStandardColor);
}
/**
......@@ -197,12 +191,4 @@ public class StatusBarColorControllerTest {
final int scrimColorOpaque = mScrimColor & 0xFF000000;
return ColorUtils.getColorWithOverlay(color, scrimColorOpaque, fraction * scrimColorAlpha);
}
private void waitForStatusBarColor(Activity activity, int expectedColor)
throws ExecutionException, TimeoutException {
CriteriaHelper.pollUiThread(() -> {
Criteria.checkThat(
activity.getWindow().getStatusBarColor(), Matchers.is(expectedColor));
}, CriteriaHelper.DEFAULT_MAX_TIME_TO_POLL, CriteriaHelper.DEFAULT_POLLING_INTERVAL);
}
}
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