Skip to content

Commit

Permalink
[Signin][Android] Introduce promise for native loaded status in FRE
Browse files Browse the repository at this point in the history 
This CL introduces a promise for native loaded status that can be
used by FRE fragments and deprecates onNativeInitialized that was used
for this before. This unifies handling native initialization with
other async processes in the FRE (policies and child account status)
and allows creation of truly independent MVC coordinators in FRE.

Bug: 1343837, 1346301
Change-Id: I690f77adc619a5cd04d5ee1818c6db6582eeddca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3776624
Commit-Queue: Boris Sazonov <bsazonov@chromium.org>
Reviewed-by: Victor Vianna <victorvianna@google.com>
Cr-Commit-Position: refs/heads/main@{#1028205}
  • Loading branch information
Boris Sazonov authored and Chromium LUCI CQ committed Jul 26, 2022
1 parent d262c22 commit 9771d16
Show file tree
Hide file tree
Showing 8 changed files with 100 additions and 95 deletions.
29 changes: 14 additions & 15 deletions chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@
import org.chromium.base.ActivityState;
import org.chromium.base.ApplicationStatus;
import org.chromium.base.ApplicationStatus.ActivityStateListener;
import org.chromium.base.Promise;
import org.chromium.base.metrics.RecordHistogram;
import org.chromium.base.supplier.BooleanSupplier;
import org.chromium.base.supplier.OneshotSupplierImpl;
import org.chromium.base.supplier.Supplier;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.BackPressHelper;
Expand Down Expand Up @@ -97,9 +97,8 @@ public boolean onPreDraw() {
private static FirstRunActivityObserver sObserver;

private boolean mPostNativeAndPolicyPagesCreated;
// Use hasValue() to simplify access. Will be null before initialized.
private final OneshotSupplierImpl<Boolean> mNativeSideIsInitializedSupplier =
new OneshotSupplierImpl<>();
/** Use {@link Promise#isFulfilled()} to verify whether the native has been initialized. */
private final Promise<Void> mNativeInitializationPromise = new Promise<>();

private FirstRunFlowSequencer mFirstRunFlowSequencer;

Expand Down Expand Up @@ -325,7 +324,7 @@ public void run() {
}

private void onNativeDependenciesFullyInitialized() {
mNativeSideIsInitializedSupplier.set(true);
mNativeInitializationPromise.fulfill(null);

onInternalStateChanged();
}
Expand Down Expand Up @@ -358,7 +357,7 @@ private void onInternalStateChanged() {
}

private boolean areNativeAndPoliciesInitialized() {
return mNativeSideIsInitializedSupplier.hasValue() && isFlowKnown()
return mNativeInitializationPromise.isFulfilled() && isFlowKnown()
&& this.getPolicyLoadListener().get() != null;
}

Expand All @@ -370,13 +369,13 @@ public void onAttachFragment(Fragment fragment) {

FirstRunFragment page = (FirstRunFragment) fragment;
// Delay notifying the child page until native and the TemplateUrlService are initialized.
// Tracked by mNativeSideIsInitializedSupplier is ready. Otherwise if the next page handles
// Tracked by mNativeSideIsInitialized is ready. Otherwise if the next page handles
// the default search engine, it will be missing dependencies. See https://crbug.com/1275950
// for when this didn't work.
if (mNativeSideIsInitializedSupplier.hasValue()) {
if (mNativeInitializationPromise.isFulfilled()) {
page.onNativeInitialized();
} else {
mNativeSideIsInitializedSupplier.onAvailable((ignored) -> page.onNativeInitialized());
mNativeInitializationPromise.then((ignored) -> { page.onNativeInitialized(); });
}
}

Expand Down Expand Up @@ -526,7 +525,7 @@ public boolean isLaunchedFromCct() {

@Override
public void acceptTermsOfService(boolean allowCrashUpload) {
assert mNativeSideIsInitializedSupplier.hasValue();
assert mNativeInitializationPromise.isFulfilled();

// If default is true then it corresponds to opt-out and false corresponds to opt-in.
UmaUtils.recordMetricsReportingDefaultOptIn(!DEFAULT_METRICS_AND_CRASH_REPORTING);
Expand Down Expand Up @@ -615,14 +614,14 @@ public void showInfoPage(@StringRes int url) {
this, LocalizationUtils.substituteLocalePlaceholder(getString(url)));
}

@VisibleForTesting
boolean hasPages() {
return mPagerAdapter != null && mPagerAdapter.getItemCount() > 0;
@Override
public Promise<Void> getNativeInitializationPromise() {
return mNativeInitializationPromise;
}

@VisibleForTesting
public boolean isNativeSideIsInitializedForTest() {
return mNativeSideIsInitializedSupplier.hasValue();
boolean hasPages() {
return mPagerAdapter != null && mPagerAdapter.getItemCount() > 0;
}

@VisibleForTesting
Expand Down
4 changes: 4 additions & 0 deletions chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFragment.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@ public interface FirstRunFragment {
/**
* Notifies that the object returned by {@link #getPageDelegate()} and its dependencies have
* been fully initialized, including native initialization.
*
* TODO(https://crbug.com/1346301): Remove this method.
* @deprecated Use {@link FirstRunPageDelegate#getNativeInitializationPromise()} instead.
*/
@Deprecated
default void onNativeInitialized() {}

/**
Expand Down
7 changes: 7 additions & 0 deletions chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunPageDelegate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import android.os.Bundle;

import org.chromium.base.Promise;
import org.chromium.base.supplier.OneshotSupplier;

/**
Expand Down Expand Up @@ -92,4 +93,10 @@ public interface FirstRunPageDelegate {
* Returns the supplier that supplies child account status.
*/
OneshotSupplier<Boolean> getChildAccountStatusSupplier();

/**
* Returns the promise that provides information about native initialization. Callers can use
* {@link Promise#isFulfilled()} to check whether the native has already been initialized.
*/
Promise<Void> getNativeInitializationPromise();
}
53 changes: 24 additions & 29 deletions chrome/android/java/src/org/chromium/chrome/browser/signin/SigninFirstRunFragment.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ public class SigninFirstRunFragment extends Fragment implements FirstRunFragment
private @Nullable SigninFirstRunCoordinator mSigninFirstRunCoordinator;
private @LoadPoint int mSlowestLoadPoint;
private boolean mExitFirstRunCalled;
private boolean mNativeInitialized;
private boolean mNativePolicyAndChildStatusLoaded;
private boolean mAllowCrashUpload;

Expand All @@ -83,6 +82,9 @@ public SigninFirstRunFragment() {}
@Override
public void onAttach(Context context) {
super.onAttach(context);
mModalDialogManager = ((ModalDialogManagerHolder) getActivity()).getModalDialogManager();

getPageDelegate().getNativeInitializationPromise().then(result -> { onNativeLoaded(); });
getPageDelegate().getPolicyLoadListener().onAvailable(hasPolicies -> onPolicyLoad());
getPageDelegate().getChildAccountStatusSupplier().onAvailable(
ignored -> onChildAccountStatusAvailable());
Expand All @@ -93,7 +95,6 @@ public void onAttach(Context context) {
if (skipTos) exitFirstRun();
});
}
mModalDialogManager = ((ModalDialogManagerHolder) getActivity()).getModalDialogManager();
}

@Override
Expand Down Expand Up @@ -153,20 +154,6 @@ public void setInitialA11yFocus() {
title.sendAccessibilityEvent(AccessibilityEvent.TYPE_VIEW_FOCUSED);
}

/** Implements {@link FirstRunFragment}. */
@Override
public void onNativeInitialized() {
if (mNativeInitialized) return;
// This may happen when the native initialized supplier in FirstRunActivity calls back after
// the fragment has been detached from the activity. See https://crbug.com/1294998.
if (getPageDelegate() == null) return;

mNativeInitialized = true;
mSlowestLoadPoint = LoadPoint.NATIVE_INITIALIZATION;
getPageDelegate().recordNativeInitializedHistogram();
notifyCoordinatorWhenNativePolicyAndChildStatusAreLoaded();
}

/** Implements {@link FirstRunFragment}. */
@Override
public void reset() {
Expand Down Expand Up @@ -255,6 +242,16 @@ private void setSigninFirstRunCoordinator(@Nullable SigninFirstRunCoordinator co
}
}

private void onNativeLoaded() {
// This may happen when the native initialized supplier in FirstRunActivity calls back after
// the fragment has been detached from the activity. See https://crbug.com/1294998.
if (getPageDelegate() == null) return;

mSlowestLoadPoint = LoadPoint.NATIVE_INITIALIZATION;
getPageDelegate().recordNativeInitializedHistogram();
notifyCoordinatorWhenNativePolicyAndChildStatusAreLoaded();
}

private void onChildAccountStatusAvailable() {
mSlowestLoadPoint = LoadPoint.CHILD_STATUS_LOAD;
notifyCoordinatorWhenNativePolicyAndChildStatusAreLoaded();
Expand All @@ -274,20 +271,18 @@ private void notifyCoordinatorWhenNativePolicyAndChildStatusAreLoaded() {
// the fragment has been detached from the activity. See https://crbug.com/1294998.
if (getPageDelegate() == null) return;

if (mSigninFirstRunCoordinator != null && mNativeInitialized
if (mSigninFirstRunCoordinator != null
&& getPageDelegate().getNativeInitializationPromise().isFulfilled()
&& getPageDelegate().getChildAccountStatusSupplier().get() != null
&& getPageDelegate().getPolicyLoadListener().get() != null) {
// Only notify once.
if (!mNativePolicyAndChildStatusLoaded) {
mNativePolicyAndChildStatusLoaded = true;
mAllowCrashUpload =
!mSigninFirstRunCoordinator.isMetricsReportingDisabledByPolicy();
mSigninFirstRunCoordinator.onNativePolicyAndChildStatusLoaded(
getPageDelegate().getPolicyLoadListener().get());
getPageDelegate().recordNativePolicyAndChildStatusLoadedHistogram();
RecordHistogram.recordEnumeratedHistogram(
"MobileFre.SlowestLoadPoint", mSlowestLoadPoint, LoadPoint.MAX);
}
&& getPageDelegate().getPolicyLoadListener().get() != null
&& !mNativePolicyAndChildStatusLoaded) {
mNativePolicyAndChildStatusLoaded = true;
mAllowCrashUpload = !mSigninFirstRunCoordinator.isMetricsReportingDisabledByPolicy();
mSigninFirstRunCoordinator.onNativePolicyAndChildStatusLoaded(
getPageDelegate().getPolicyLoadListener().get());
getPageDelegate().recordNativePolicyAndChildStatusLoadedHistogram();
RecordHistogram.recordEnumeratedHistogram(
"MobileFre.SlowestLoadPoint", mSlowestLoadPoint, LoadPoint.MAX);
}
}

Expand Down
3 changes: 2 additions & 1 deletion ...javatests/src/org/chromium/chrome/browser/firstrun/FirstRunActivitySigninAndSyncTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,8 @@ private void launchFirstRunActivity() {
intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
mFirstRunActivity = ApplicationTestUtils.waitForActivityWithClass(
FirstRunActivity.class, Stage.RESUMED, () -> context.startActivity(intent));
CriteriaHelper.pollUiThread(mFirstRunActivity::isNativeSideIsInitializedForTest);
CriteriaHelper.pollUiThread(
() -> mFirstRunActivity.getNativeInitializationPromise().isFulfilled());
}

private static class LinkClick implements ViewAction {
Expand Down
9 changes: 6 additions & 3 deletions ...e/android/javatests/src/org/chromium/chrome/browser/firstrun/FirstRunIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -669,7 +669,8 @@ public void testExitFirstRunWithPolicy() throws Exception {
CriteriaHelper.pollUiThread(
() -> freActivity.getSupportFragmentManager().getFragments().size() > 0);
// Make sure native is initialized so that the subsequent transition is not blocked.
CriteriaHelper.pollUiThread((() -> freActivity.isNativeSideIsInitializedForTest()),
CriteriaHelper.pollUiThread(
(() -> freActivity.getNativeInitializationPromise().isFulfilled()),
"native never initialized.");

waitForActivity(CustomTabActivity.class);
Expand Down Expand Up @@ -923,7 +924,8 @@ public void testNativeInitBeforeFragment() throws Exception {

launchViewIntent(TEST_URL);
FirstRunActivity firstRunActivity = waitForActivity(FirstRunActivity.class);
CriteriaHelper.pollUiThread((() -> firstRunActivity.isNativeSideIsInitializedForTest()),
CriteriaHelper.pollUiThread(
(() -> firstRunActivity.getNativeInitializationPromise().isFulfilled()),
"native never initialized.");

unblockOnFlowIsKnown();
Expand Down Expand Up @@ -987,7 +989,8 @@ public void testNativeInitBeforeFragmentSkip() throws Exception {

launchCustomTabs(TEST_URL);
FirstRunActivity firstRunActivity = waitForActivity(FirstRunActivity.class);
CriteriaHelper.pollUiThread((() -> firstRunActivity.isNativeSideIsInitializedForTest()),
CriteriaHelper.pollUiThread(
(() -> firstRunActivity.getNativeInitializationPromise().isFulfilled()),
"native never initialized.");

unblockOnFlowIsKnown();
Expand Down
8 changes: 6 additions & 2 deletions ...id/javatests/src/org/chromium/chrome/browser/signin/SigninFirstRunFragmentRenderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.mockito.junit.MockitoJUnit;
import org.mockito.junit.MockitoRule;

import org.chromium.base.Promise;
import org.chromium.base.supplier.OneshotSupplierImpl;
import org.chromium.base.test.params.ParameterAnnotations;
import org.chromium.base.test.params.ParameterProvider;
Expand Down Expand Up @@ -148,7 +149,11 @@ public void setUp() {
mFragment = new CustomSigninFirstRunFragment();
mFragment.setPageDelegate(mFirstRunPageDelegateMock);
TestThreadUtils.runOnUiThreadBlocking(() -> {
mFragment.onNativeInitialized();
Promise<Void> nativeSideIsInitialized = new Promise<>();
nativeSideIsInitialized.fulfill(null);
when(mFirstRunPageDelegateMock.getNativeInitializationPromise())
.thenReturn(nativeSideIsInitialized);

OneshotSupplierImpl<Boolean> childAccountStatusListener = new OneshotSupplierImpl<>();
childAccountStatusListener.set(false);
when(mFirstRunPageDelegateMock.getChildAccountStatusSupplier())
Expand Down Expand Up @@ -302,7 +307,6 @@ public void testFragmentWithChildAccount(boolean nightModeEnabled, int orientati
public void testFragmentWhenCannotUseGooglePlayService(
boolean nightModeEnabled, int orientation) throws IOException {
when(mExternalAuthUtilsMock.canUseGooglePlayServices()).thenReturn(false);
TestThreadUtils.runOnUiThreadBlocking(() -> { mFragment.onNativeInitialized(); });

launchActivityWithFragment(orientation);

Expand Down

0 comments on commit 9771d16

Please sign in to comment.