Commit 9183c032 authored by Patrick Noland's avatar Patrick Noland Committed by Commit Bot

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/+/2418981Reviewed-by: default avatarDavid Trainor <dtrainor@chromium.org>
Reviewed-by: default avatarXi Han <hanxi@chromium.org>
Commit-Queue: Patrick Noland <pnoland@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809386}
parent 7298c218
......@@ -561,9 +561,8 @@ public class ChromeTabbedActivity extends ChromeActivity<ChromeActivityComponent
}
}
mLayoutManager = new LayoutManagerChromePhone(compositorViewHolder, mContentContainer,
mStartSurface, getTabContentManagerSupplier());
mStartSurface, getTabContentManagerSupplier(), mOverviewModeBehaviorSupplier);
mOverviewModeController = mLayoutManager;
mOverviewModeBehaviorSupplier.set(mOverviewModeController);
}
}
......@@ -572,10 +571,10 @@ public class ChromeTabbedActivity extends ChromeActivity<ChromeActivityComponent
try (TraceEvent e = TraceEvent.scoped(
"ChromeTabbedActivity.setupCompositorContentPreNativeForTablet")) {
mLayoutManager = new LayoutManagerChromeTablet(
getCompositorViewHolder(), mContentContainer, getTabContentManagerSupplier());
mLayoutManager =
new LayoutManagerChromeTablet(getCompositorViewHolder(), mContentContainer,
getTabContentManagerSupplier(), mOverviewModeBehaviorSupplier);
mOverviewModeController = mLayoutManager;
mOverviewModeBehaviorSupplier.set(mOverviewModeController);
}
}
......@@ -839,7 +838,7 @@ public class ChromeTabbedActivity extends ChromeActivity<ChromeActivityComponent
if (!mPendingInitialTabCreation
&& !(TabUiFeatureUtilities.supportInstantStart(isTablet())
&& shouldShowTabSwitcherOnStart())) {
setInitialOverviewStateAsync();
setInitialOverviewState();
}
if (TabUiFeatureUtilities.isConditionalTabStripEnabled()
......@@ -916,14 +915,7 @@ public class ChromeTabbedActivity extends ChromeActivity<ChromeActivityComponent
}
}
// 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() {
private void setInitialOverviewState() {
boolean isOverviewVisible = mOverviewModeController.overviewVisible();
if (shouldShowTabSwitcherOnStart() && !isOverviewVisible) {
......@@ -1135,7 +1127,7 @@ public class ChromeTabbedActivity extends ChromeActivity<ChromeActivityComponent
if (hasStartWithNativeBeenCalled()
&& !(TabUiFeatureUtilities.supportInstantStart(isTablet())
&& shouldShowTabSwitcherOnStart())) {
setInitialOverviewStateAsync();
setInitialOverviewState();
}
}
......@@ -1520,7 +1512,7 @@ public class ChromeTabbedActivity extends ChromeActivity<ChromeActivityComponent
if (shouldShowTabSwitcherOnStart()) {
mLayoutManager.setTabModelSelector(mTabModelSelectorImpl);
mIsAccessibilityTabSwitcherEnabled = DeviceClassManager.enableAccessibilityLayout();
setInitialOverviewStateAsync();
setInitialOverviewState();
}
}
}
......
......@@ -14,6 +14,7 @@ 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;
......@@ -76,6 +77,7 @@ public class LayoutManagerChrome
private boolean mCreateOverviewLayout;
protected ObservableSupplier<TabContentManager> mTabContentManagerSupplier;
private final OneshotSupplierImpl<OverviewModeBehavior> mOverviewModeBehaviorSupplier;
/**
* Creates the {@link LayoutManagerChrome} instance.
......@@ -87,7 +89,8 @@ public class LayoutManagerChrome
*/
public LayoutManagerChrome(LayoutManagerHost host, ViewGroup contentContainer,
boolean createOverviewLayout, @Nullable StartSurface startSurface,
ObservableSupplier<TabContentManager> tabContentManagerSupplier) {
ObservableSupplier<TabContentManager> tabContentManagerSupplier,
OneshotSupplierImpl<OverviewModeBehavior> overviewModeBehaviorSupplier) {
super(host, contentContainer, tabContentManagerSupplier);
Context context = host.getContext();
LayoutRenderHost renderHost = host.getLayoutRenderHost();
......@@ -119,10 +122,8 @@ public class LayoutManagerChrome
@Override
public void onStateChanged(@OverviewModeState int overviewModeState,
boolean shouldShowTabSwitcherToolbar) {
for (OverviewModeObserver observer : mOverviewModeObservers) {
observer.onOverviewModeStateChanged(
overviewModeState, shouldShowTabSwitcherToolbar);
}
notifyObserversStateChanged(
overviewModeState, shouldShowTabSwitcherToolbar);
}
});
final ObservableSupplier<? extends BrowserControlsStateProvider>
......@@ -134,6 +135,9 @@ public class LayoutManagerChrome
mCreateOverviewLayout = true;
}
}
mOverviewModeBehaviorSupplier = overviewModeBehaviorSupplier;
mOverviewModeBehaviorSupplier.set(this);
}
/**
......@@ -278,9 +282,7 @@ public class LayoutManagerChrome
if (isOverviewLayout(layoutBeingShown)) {
boolean showToolbar = animate && (!mEnableAnimations
|| getTabModelSelector().getCurrentModel().getCount() <= 0);
for (OverviewModeObserver observer : mOverviewModeObservers) {
observer.onOverviewModeStartedShowing(showToolbar);
}
notifyObserversStartedShowing(showToolbar);
}
}
......@@ -298,9 +300,7 @@ public class LayoutManagerChrome
boolean creatingNtp = layoutBeingHidden == mOverviewLayout && mCreatingNtp;
for (OverviewModeObserver observer : mOverviewModeObservers) {
observer.onOverviewModeStartedHiding(showToolbar, creatingNtp);
}
notifyObserversStartedHiding(showToolbar, creatingNtp);
}
}
......@@ -309,9 +309,7 @@ public class LayoutManagerChrome
super.doneShowing();
if (isOverviewLayout(getActiveLayout())) {
for (OverviewModeObserver observer : mOverviewModeObservers) {
observer.onOverviewModeFinishedShowing();
}
notifyObserversFinishedShowing();
}
}
......@@ -327,9 +325,7 @@ public class LayoutManagerChrome
super.doneHiding();
if (isOverviewLayout(layoutBeingHidden)) {
for (OverviewModeObserver observer : mOverviewModeObservers) {
observer.onOverviewModeFinishedHiding();
}
notifyObserversFinishedHiding();
}
}
......@@ -596,4 +592,46 @@ 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,6 +8,7 @@ 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;
......@@ -41,8 +42,10 @@ public class LayoutManagerChromePhone extends LayoutManagerChrome {
*/
public LayoutManagerChromePhone(LayoutManagerHost host, ViewGroup contentContainer,
StartSurface startSurface,
ObservableSupplier<TabContentManager> tabContentManagerSupplier) {
super(host, contentContainer, true, startSurface, tabContentManagerSupplier);
ObservableSupplier<TabContentManager> tabContentManagerSupplier,
OneshotSupplierImpl<OverviewModeBehavior> overviewModeBehaviorSupplier) {
super(host, contentContainer, true, startSurface, tabContentManagerSupplier,
overviewModeBehaviorSupplier);
}
@Override
......
......@@ -7,6 +7,7 @@ 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;
......@@ -37,8 +38,10 @@ public class LayoutManagerChromeTablet extends LayoutManagerChrome {
* @param tabContentManagerSupplier Supplier of the {@link TabContentManager} instance.
*/
public LayoutManagerChromeTablet(LayoutManagerHost host, ViewGroup contentContainer,
ObservableSupplier<TabContentManager> tabContentManagerSupplier) {
super(host, contentContainer, false, null, tabContentManagerSupplier);
ObservableSupplier<TabContentManager> tabContentManagerSupplier,
OneshotSupplierImpl<OverviewModeBehavior> overviewModeBehaviorSupplier) {
super(host, contentContainer, false, null, tabContentManagerSupplier,
overviewModeBehaviorSupplier);
mTabStripLayoutHelperManager = new StripLayoutHelperManager(
host.getContext(), this, mHost.getLayoutRenderHost(), () -> mTitleCache);
......
......@@ -237,7 +237,8 @@ public class TabbedAppMenuTest {
() -> mActivityTestRule.getActivity().getLayoutManager().hideOverview(false));
Assert.assertFalse("Overview shouldn't be showing.",
mActivityTestRule.getActivity().getOverviewModeBehavior().overviewVisible());
Assert.assertFalse("App menu shouldn't be showing.", mAppMenuHandler.isAppMenuShowing());
CriteriaHelper.pollUiThread(
() -> !mAppMenuHandler.isAppMenuShowing(), "App menu shouldn't be showing.");
}
@Test
......
......@@ -30,6 +30,7 @@ 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;
......@@ -166,8 +167,10 @@ public class LayoutManagerTest implements MockTabModelDelegate {
ObservableSupplierImpl<TabContentManager> tabContentManagerSupplier =
new ObservableSupplierImpl<>();
mManagerPhone = new LayoutManagerChromePhone(
layoutManagerHost, container, null, tabContentManagerSupplier);
OneshotSupplierImpl<OverviewModeBehavior> overviewModeBehaviorSupplier =
new OneshotSupplierImpl<>();
mManagerPhone = new LayoutManagerChromePhone(layoutManagerHost, container, null,
tabContentManagerSupplier, overviewModeBehaviorSupplier);
tabContentManagerSupplier.set(tabContentManager);
mManager = mManagerPhone;
CompositorAnimationHandler.setTestingMode(true);
......
......@@ -4,6 +4,7 @@
package org.chromium.chrome.browser.ui.system;
import android.app.Activity;
import android.content.res.Resources;
import android.graphics.Color;
import android.os.Build;
......@@ -11,6 +12,7 @@ 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;
......@@ -34,10 +36,15 @@ 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}.
......@@ -68,7 +75,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() {
public void testColorToggleIncongitoInOverview() throws Exception {
ChromeTabbedActivity activity = mActivityTestRule.getActivity();
Resources resources = activity.getResources();
final int expectedOverviewStandardColor = defaultColorFallbackToBlack(
......@@ -84,8 +91,7 @@ public class StatusBarColorControllerTest {
TestThreadUtils.runOnUiThreadBlocking(
() -> { activity.getLayoutManager().showOverview(false /* animate */); });
ThemeTestUtils.assertStatusBarColor(activity, expectedOverviewIncognitoColor);
waitForStatusBarColor(activity, expectedOverviewIncognitoColor);
TestThreadUtils.runOnUiThreadBlocking(
() -> { tabModelSelector.selectModel(false /* incongito */); });
ThemeTestUtils.assertStatusBarColor(activity, expectedOverviewStandardColor);
......@@ -109,11 +115,11 @@ public class StatusBarColorControllerTest {
"/chrome/test/data/android/theme_color_test.html");
mActivityTestRule.loadUrl(pageWithBrandColorUrl);
ThemeTestUtils.waitForThemeColor(activity, Color.RED);
ThemeTestUtils.assertStatusBarColor(activity, Color.RED);
waitForStatusBarColor(activity, Color.RED);
TestThreadUtils.runOnUiThreadBlocking(
() -> { activity.getLayoutManager().showOverview(false /* animate */); });
ThemeTestUtils.assertStatusBarColor(activity, expectedDefaultStandardColor);
waitForStatusBarColor(activity, expectedDefaultStandardColor);
}
/**
......@@ -191,4 +197,12 @@ 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