Skip to content

Commit

Permalink
[FRE] Inflate SigninFirstRunFragment before child account status fetch
Browse files Browse the repository at this point in the history
Currently we wait for accounts and child account status to be fetched
before inflating the FirstRunFragment views. If the child account status
takes a long time to be fetched then the users will see a blank page.

This cl changes this behavior for the new signin FRE and inflates
SigninFirstRunFragment with the chrome logo, title and a progress
spinner.

Also this cl adds unit and integration tests for this flow.

(cherry picked from commit 6ba6223)

Bug: 1307392
Change-Id: I547b9712852dd803b79e31945198196d870384c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3564568
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Commit-Queue: Tanmoy Mollik <triploblastic@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#993030}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3588417
Reviewed-by: Alice Wang <aliceywang@chromium.org>
Commit-Queue: Boris Sazonov <bsazonov@chromium.org>
Cr-Commit-Position: refs/branch-heads/5005@{#28}
Cr-Branched-From: 5b4d945-refs/heads/main@{#992738}
  • Loading branch information
Tanmoy Mollik authored and Chromium LUCI CQ committed Apr 19, 2022
1 parent 2396a9e commit 3a14f23
Show file tree
Hide file tree
Showing 10 changed files with 125 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.chromium.base.ApplicationStatus.ActivityStateListener;
import org.chromium.base.metrics.RecordHistogram;
import org.chromium.base.supplier.BooleanSupplier;
import org.chromium.base.supplier.OneshotSupplier;
import org.chromium.base.supplier.OneshotSupplierImpl;
import org.chromium.base.supplier.Supplier;
import org.chromium.chrome.R;
Expand Down Expand Up @@ -100,6 +101,8 @@ public boolean onPreDraw() {
// Use hasValue() to simplify access. Will be null before initialized.
private final OneshotSupplierImpl<Boolean> mNativeSideIsInitializedSupplier =
new OneshotSupplierImpl<>();
private final OneshotSupplierImpl<Boolean> mChildAccountStatusSupplier =
new OneshotSupplierImpl<>();

private FirstRunFlowSequencer mFirstRunFlowSequencer;

Expand Down Expand Up @@ -254,14 +257,22 @@ public void triggerLayoutInflation() {
setFinishOnTouchOutside(true);

setContentView(createContentView());
ViewDrawBlocker.blockViewDrawUntilReady(
findViewById(android.R.id.content), () -> mPages.size() > 0);
if (FREMobileIdentityConsistencyFieldTrial.isEnabled() && mPagerAdapter == null) {
// SigninFirstRunFragment doesn't use getProperties() and can be shown right away,
// without waiting for FirstRunFlowSequencer.
createFirstPage();
} else {
ViewDrawBlocker.blockViewDrawUntilReady(
findViewById(android.R.id.content), () -> mPages.size() > 0);
}

mFirstRunFlowSequencer = new FirstRunFlowSequencer(this) {
@Override
public void onFlowIsKnown(Bundle freProperties) {
assert freProperties != null;
mFreProperties = freProperties;
mChildAccountStatusSupplier.set(
mFreProperties.getBoolean(SyncConsentFirstRunFragment.IS_CHILD_ACCOUNT));

onInternalStateChanged();

Expand Down Expand Up @@ -590,6 +601,11 @@ public void recordNativeAndPoliciesLoadedHistogram() {
SystemClock.elapsedRealtime() - mIntentCreationElapsedRealtimeMs);
}

@Override
public OneshotSupplier<Boolean> getChildAccountStatusListener() {
return mChildAccountStatusSupplier;
}

@Override
public void showInfoPage(@StringRes int url) {
CustomTabActivity.showInfoPage(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,9 @@ public interface FirstRunPageDelegate {
* See {@link PolicyLoadListener} for details.
*/
OneshotSupplier<Boolean> getPolicyLoadListener();

/**
* Returns the supplier that supplies child account status.
*/
OneshotSupplier<Boolean> getChildAccountStatusListener();
}
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,9 @@ public SigninFirstRunFragment() {}
public void onAttach(Context context) {
super.onAttach(context);
getPageDelegate().getPolicyLoadListener().onAvailable(
hasPolicies -> notifyCoordinatorWhenNativeAndPolicyAreLoaded());
hasPolicies -> notifyCoordinatorWhenNativePolicyAndChildStatusAreLoaded());
getPageDelegate().getChildAccountStatusListener().onAvailable(
ignored -> notifyCoordinatorWhenNativePolicyAndChildStatusAreLoaded());
if (getPageDelegate().isLaunchedFromCct()) {
mSkipTosDialogPolicyListener = new SkipTosDialogPolicyListener(
getPageDelegate().getPolicyLoadListener(), EnterpriseInfo.getInstance(), null);
Expand Down Expand Up @@ -134,7 +136,7 @@ public void setInitialA11yFocus() {
@Override
public void onNativeInitialized() {
mNativeInitialized = true;
notifyCoordinatorWhenNativeAndPolicyAreLoaded();
notifyCoordinatorWhenNativePolicyAndChildStatusAreLoaded();
}

/** Implements {@link FirstRunFragment}. */
Expand Down Expand Up @@ -221,14 +223,18 @@ private void setSigninFirstRunCoordinator(@Nullable SigninFirstRunCoordinator co
mSigninFirstRunCoordinator = coordinator;
}

private void notifyCoordinatorWhenNativeAndPolicyAreLoaded() {
/**
* Notifies the coordinator that native, policies and child account status has been loaded.
*/
private void notifyCoordinatorWhenNativePolicyAndChildStatusAreLoaded() {
// 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;

if (mSigninFirstRunCoordinator != null && mNativeInitialized
&& getPageDelegate().getChildAccountStatusListener().get() != null
&& getPageDelegate().getPolicyLoadListener().get() != null) {
mSigninFirstRunCoordinator.onNativeAndPolicyLoaded(
mSigninFirstRunCoordinator.onNativePolicyAndChildStatusLoaded(
getPageDelegate().getPolicyLoadListener().get());
getPageDelegate().recordNativeAndPoliciesLoadedHistogram();
mAllowCrashUpload = !mSigninFirstRunCoordinator.isMetricsReportingDisabledByPolicy();
Expand All @@ -246,7 +252,7 @@ private View inflateFragmentView(LayoutInflater inflater, Configuration configur
null, false);
setSigninFirstRunCoordinator(new SigninFirstRunCoordinator(requireContext(), view,
mModalDialogManager, this, PrivacyPreferencesManagerImpl.getInstance()));
notifyCoordinatorWhenNativeAndPolicyAreLoaded();
notifyCoordinatorWhenNativePolicyAndChildStatusAreLoaded();
return view;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@

package org.chromium.chrome.browser.firstrun;

import static androidx.test.espresso.Espresso.onView;
import static androidx.test.espresso.assertion.ViewAssertions.matches;
import static androidx.test.espresso.matcher.ViewMatchers.isDisplayed;
import static androidx.test.espresso.matcher.ViewMatchers.withId;

import static org.hamcrest.Matchers.is;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.when;
Expand All @@ -14,12 +19,14 @@
import android.app.Instrumentation.ActivityMonitor;
import android.content.Context;
import android.content.Intent;
import android.graphics.drawable.ColorDrawable;
import android.net.Uri;
import android.os.Bundle;
import android.support.test.InstrumentationRegistry;
import android.view.View;
import android.widget.Button;
import android.widget.CheckBox;
import android.widget.ProgressBar;

import androidx.annotation.IdRes;
import androidx.test.filters.MediumTest;
Expand Down Expand Up @@ -73,6 +80,7 @@
import org.chromium.chrome.browser.signin.SigninFirstRunFragment;
import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
import org.chromium.chrome.test.util.browser.Features;
import org.chromium.components.browser_ui.styles.SemanticColorUtils;
import org.chromium.components.externalauth.ExternalAuthUtils;
import org.chromium.components.policy.AbstractAppRestrictionsProvider;
import org.chromium.components.search_engines.TemplateUrl;
Expand Down Expand Up @@ -898,6 +906,32 @@ public void testNativeInitBeforeFragment() throws Exception {
verifyUrlEquals(TEST_URL, waitAndGetUriFromChromeActivity(ChromeTabbedActivity.class));
}

@Test
@MediumTest
@CommandLineFlags.Remove({ChromeSwitches.FORCE_DISABLE_SIGNIN_FRE})
@CommandLineFlags.Add({ChromeSwitches.FORCE_ENABLE_SIGNIN_FRE})
public void testSigninFirstRunPageShownBeforeChildStatusFetch() throws Exception {
blockOnFlowIsKnown();
initializePreferences(new FirstRunPagesTestCase());

FirstRunActivity firstRunActivity = launchFirstRunActivity();
new FirstRunNavigationHelper(firstRunActivity).ensureTermsOfServiceIsCurrentPage();
TestThreadUtils.runOnUiThreadBlocking(() -> {
ProgressBar progressBar =
((SigninFirstRunFragment) firstRunActivity.getCurrentFragmentForTesting())
.getView()
.findViewById(R.id.fre_native_and_policy_load_progress_spinner);
// Replace the progress bar with a dummy to allow other checks. Currently the
// progress bar cannot be stopped otherwise due to some espresso issues (crbug/1115067).
progressBar.setIndeterminateDrawable(
new ColorDrawable(SemanticColorUtils.getDefaultBgColor(firstRunActivity)));
});

onView(withId(R.id.fre_logo)).check(matches(isDisplayed()));
onView(withId(R.id.fre_native_and_policy_load_progress_spinner))
.check(matches(isDisplayed()));
}

@Test
@MediumTest
public void testNativeInitBeforeFragmentSkip() throws Exception {
Expand Down
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.supplier.OneshotSupplierImpl;
import org.chromium.base.test.params.ParameterAnnotations;
import org.chromium.base.test.params.ParameterProvider;
import org.chromium.base.test.params.ParameterSet;
Expand Down Expand Up @@ -144,7 +145,13 @@ public void setUp() {
when(mFirstRunPageDelegateMock.getPolicyLoadListener()).thenReturn(mPolicyLoadListenerMock);
mChromeActivityTestRule.startMainActivityOnBlankPage();
mFragment = new CustomSigninFirstRunFragment();
TestThreadUtils.runOnUiThreadBlocking(() -> { mFragment.onNativeInitialized(); });
TestThreadUtils.runOnUiThreadBlocking(() -> {
mFragment.onNativeInitialized();
OneshotSupplierImpl<Boolean> childAccountStatusListener = new OneshotSupplierImpl<>();
childAccountStatusListener.set(false);
when(mFirstRunPageDelegateMock.getChildAccountStatusListener())
.thenReturn(childAccountStatusListener);
});
mFragment.setPageDelegate(mFirstRunPageDelegateMock);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
import org.mockito.junit.MockitoRule;

import org.chromium.base.Callback;
import org.chromium.base.supplier.OneshotSupplierImpl;
import org.chromium.base.test.util.ApplicationTestUtils;
import org.chromium.base.test.util.CallbackHelper;
import org.chromium.base.test.util.CommandLineFlags;
Expand Down Expand Up @@ -140,6 +141,8 @@ void setPageDelegate(FirstRunPageDelegate delegate) {
@Mock
private PolicyLoadListener mPolicyLoadListenerMock;
@Mock
private OneshotSupplierImpl<Boolean> mChildAccountStatusListenerMock;
@Mock
private SigninManager mSigninManagerMock;
@Mock
private IdentityManager mIdentityManagerMock;
Expand Down Expand Up @@ -170,6 +173,9 @@ public void setUp() {
VariationsGroup.DEFAULT);
when(mPolicyLoadListenerMock.get()).thenReturn(false);
when(mFirstRunPageDelegateMock.getPolicyLoadListener()).thenReturn(mPolicyLoadListenerMock);
when(mChildAccountStatusListenerMock.get()).thenReturn(false);
when(mFirstRunPageDelegateMock.getChildAccountStatusListener())
.thenReturn(mChildAccountStatusListenerMock);
when(mFirstRunPageDelegateMock.isLaunchedFromCct()).thenReturn(false);
mChromeActivityTestRule.startMainActivityOnBlankPage();
mFragment = new CustomSigninFirstRunFragment();
Expand Down Expand Up @@ -699,7 +705,7 @@ public void testFragmentWhenAddingDefaultAccount() {

@Test
@MediumTest
public void testFragmentWhenPolicyIsLoadedAfterNative() {
public void testFragmentWhenPolicyIsLoadedAfterNativeAndChildStatus() {
TestThreadUtils.runOnUiThreadBlocking(() -> { mFragment.onNativeInitialized(); });
mAccountManagerTestRule.addAccount(TEST_EMAIL1, FULL_NAME1, GIVEN_NAME1, null);
when(mPolicyLoadListenerMock.get()).thenReturn(null);
Expand All @@ -719,7 +725,7 @@ public void testFragmentWhenPolicyIsLoadedAfterNative() {

@Test
@MediumTest
public void testFragmentWhenNativeIsLoadedAfterPolicy() {
public void testFragmentWhenNativeIsLoadedAfterPolicyAndChildStatus() {
mAccountManagerTestRule.addAccount(TEST_EMAIL1, FULL_NAME1, GIVEN_NAME1, null);
launchActivityWithFragment();
checkFragmentWhenLoadingNativeAndPolicy();
Expand All @@ -729,6 +735,26 @@ public void testFragmentWhenNativeIsLoadedAfterPolicy() {
checkFragmentWithSelectedAccount(TEST_EMAIL1, FULL_NAME1, GIVEN_NAME1);
}

@Test
@MediumTest
public void testFragmentWhenChildStatusIsLoadedAfterNativeAndPolicy() {
TestThreadUtils.runOnUiThreadBlocking(() -> { mFragment.onNativeInitialized(); });
mAccountManagerTestRule.addAccount(TEST_EMAIL1, FULL_NAME1, GIVEN_NAME1, null);
when(mChildAccountStatusListenerMock.get()).thenReturn(null);
launchActivityWithFragment();
checkFragmentWhenLoadingNativeAndPolicy();

when(mChildAccountStatusListenerMock.get()).thenReturn(false);
verify(mChildAccountStatusListenerMock, atLeastOnce())
.onAvailable(mCallbackCaptor.capture());
TestThreadUtils.runOnUiThreadBlocking(() -> {
for (Callback<Boolean> callback : mCallbackCaptor.getAllValues()) {
callback.onResult(false);
}
});
checkFragmentWithSelectedAccount(TEST_EMAIL1, FULL_NAME1, GIVEN_NAME1);
}

@Test
@MediumTest
public void testFragmentWithTosDialogBehaviorPolicy() throws Exception {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@
android:layout_height="32dp"
android:layout_marginTop="20dp"
android:layout_marginBottom="50dp"
android:layout_gravity="center_horizontal" />
android:layout_gravity="center_horizontal"
android:alpha="0.0" />
</LinearLayout>

<include layout="@layout/signin_first_run_bottom_group_view" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
android:visibility="invisible" />

<ImageView
android:id="@+id/fre_logo"
android:layout_width="match_parent"
android:layout_height="@dimen/fre_tos_image_height"
android:layout_marginBottom="24dp"
Expand Down Expand Up @@ -91,7 +92,8 @@
style="@style/Widget.AppCompat.ProgressBar"
android:layout_width="32dp"
android:layout_height="32dp"
android:layout_marginBottom="128dp" />
android:layout_marginBottom="128dp"
android:alpha="0.0" />

<include layout="@layout/signin_first_run_bottom_group_view" />
</LinearLayout>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,11 @@ public void reset() {
}

/**
* Notifies that native is loaded, and if policies are available, that they are also available.
* Notifies that native is loaded, policies are available if any exists and child account
* status is fetched.
* @param hasPolicies whether policies are found on device.
*/
public void onNativeAndPolicyLoaded(boolean hasPolicies) {
public void onNativePolicyAndChildStatusLoaded(boolean hasPolicies) {
ThreadUtils.assertOnUiThread();
mMediator.onNativeAndPolicyLoaded(hasPolicies);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import android.transition.AutoTransition;
import android.transition.TransitionManager;
import android.view.View;
import android.widget.ProgressBar;

import androidx.annotation.Nullable;

Expand Down Expand Up @@ -48,8 +49,18 @@ static void bind(PropertyModel model, SigninFirstRunView view, PropertyKey prope
view.getSelectedAccountView().setEnabled(!isSelectedAccountSupervised);
updateVisibility(view, model);
} else if (propertyKey == SigninFirstRunProperties.ARE_NATIVE_AND_POLICY_LOADED) {
// Add a transition animation between the view changes.
TransitionManager.beginDelayedTransition(view);
final boolean areNativeAndPolicyLoaded =
model.get(SigninFirstRunProperties.ARE_NATIVE_AND_POLICY_LOADED);
final ProgressBar initialLoadProgressSpinner = view.getInitialLoadProgressSpinnerView();
if (areNativeAndPolicyLoaded) {
TransitionManager.beginDelayedTransition(view);
initialLoadProgressSpinner.setVisibility(View.GONE);
} else {
// The progress spinner is shown at the beginning when layout inflation may not be
// complete. So it is not possible to use TransitionManager with a startDelay in
// this case.
initialLoadProgressSpinner.animate().alpha(1.0f).setStartDelay(500);
}
updateVisibility(view, model);
} else if (propertyKey == SigninFirstRunProperties.FRE_POLICY) {
view.getBrowserManagedHeaderView().setVisibility(
Expand Down Expand Up @@ -89,8 +100,6 @@ private static void updateSelectedAccount(SigninFirstRunView view, PropertyModel
private static void updateVisibility(SigninFirstRunView view, PropertyModel model) {
final boolean areNativeAndPolicyLoaded =
model.get(SigninFirstRunProperties.ARE_NATIVE_AND_POLICY_LOADED);
view.getInitialLoadProgressSpinnerView().setVisibility(
areNativeAndPolicyLoaded ? View.GONE : View.VISIBLE);
if (areNativeAndPolicyLoaded) view.onNativeAndPoliciesLoaded();

final int selectedAccountVisibility = areNativeAndPolicyLoaded
Expand Down

0 comments on commit 3a14f23

Please sign in to comment.