[FirstRun] Allow crash upload should not depending on visibility

Fix the issue when accepting ToS before native initialized caused UMA
reporting to be disabled.

Change-Id: Ifc05b8acbdf2da88952297f191f3afb2545374be
Bug: 1162308
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2606602
Commit-Queue: Wenyu Fu <wenyufu@chromium.org>
Reviewed-by: Sky Malice <skym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#839795}

chrome/android/java/src/org/chromium/chrome/browser/firstrun/ToSAndUMAFirstRunFragment.java

@@ -81,7 +81,6 @@ public class ToSAndUMAFirstRunFragment extends Fragment implements FirstRunFragm
         });
 
         mSendReportCheckBox.setChecked(FirstRunActivity.DEFAULT_METRICS_AND_CRASH_REPORTING);
-
         if (!canShowUmaCheckBox()) {
             mSendReportCheckBox.setVisibility(View.GONE);
         }
@@ -177,8 +176,7 @@ public class ToSAndUMAFirstRunFragment extends Fragment implements FirstRunFragm
         }
 
         mTriggerAcceptAfterNativeInit = false;
-        boolean allowCrashUpload = (mSendReportCheckBox.getVisibility() == View.VISIBLE)
-                && mSendReportCheckBox.isChecked();
+        boolean allowCrashUpload = canShowUmaCheckBox() && mSendReportCheckBox.isChecked();
         getPageDelegate().acceptTermsOfService(allowCrashUpload);
     }
 

chrome/android/javatests/src/org/chromium/chrome/browser/firstrun/TosAndUmaFirstRunFragmentWithEnterpriseSupportTest.java

@@ -7,6 +7,8 @@ package org.chromium.chrome.browser.firstrun;
 import static org.hamcrest.Matchers.is;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.ArgumentMatchers.eq;
 
 import android.app.Activity;
 import android.app.Instrumentation;
@@ -31,9 +33,12 @@ import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Captor;
 import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.mockito.MockitoAnnotations;
+import org.mockito.Spy;
 
 import org.chromium.base.Callback;
 import org.chromium.base.CommandLine;
@@ -45,6 +50,8 @@ import org.chromium.base.test.util.Feature;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.customtabs.CustomTabsTestUtils;
 import org.chromium.chrome.browser.flags.ChromeSwitches;
+import org.chromium.chrome.browser.init.BrowserParts;
+import org.chromium.chrome.browser.init.ChromeBrowserInitializer;
 import org.chromium.chrome.browser.policy.EnterpriseInfo;
 import org.chromium.chrome.browser.policy.PolicyServiceFactory;
 import org.chromium.chrome.browser.preferences.ChromePreferenceKeys;
@@ -68,12 +75,14 @@ import java.util.List;
  */
 @RunWith(ChromeJUnit4ClassRunner.class)
 public class TosAndUmaFirstRunFragmentWithEnterpriseSupportTest {
-    @IntDef({FragmentState.LOADING, FragmentState.NO_POLICY, FragmentState.HAS_POLICY})
+    @IntDef({FragmentState.LOADING, FragmentState.NO_POLICY, FragmentState.HAS_POLICY,
+            FragmentState.WAITING_UNTIL_NEXT_PAGE})
     @Retention(RetentionPolicy.SOURCE)
     @interface FragmentState {
         int LOADING = 0;
         int NO_POLICY = 1;
         int HAS_POLICY = 2;
+        int WAITING_UNTIL_NEXT_PAGE = 3;
     }
 
     @IntDef({SpeedComparedToInflation.NOT_RECORDED, SpeedComparedToInflation.FASTER,
@@ -101,6 +110,11 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupportTest {
     @Mock
     public EnterpriseInfo mMockEnterpriseInfo;
 
+    @Spy
+    public ChromeBrowserInitializer mInitializer;
+    @Captor
+    public ArgumentCaptor<BrowserParts> mBrowserParts;
+
     private FirstRunActivity mActivity;
     private final List<PolicyService.Observer> mPolicyServiceObservers = new ArrayList<>();
     private final List<Callback<Boolean>> mAppRestrictionsCallbacks = new ArrayList<>();
@@ -112,7 +126,8 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupportTest {
 
     private View mTosText;
     private View mAcceptButton;
-    private View mLargeSpinner;
+    private View mLowerSpinner;
+    private View mCenterSpinner;
     private View mPrivacyDisclaimer;
     private CheckBox mUmaCheckBox;
 
@@ -123,6 +138,11 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupportTest {
         Assert.assertFalse(
                 CommandLine.getInstance().hasSwitch(ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE));
 
+        Mockito.doNothing()
+                .when(mInitializer)
+                .handlePostNativeStartup(anyBoolean(), any(BrowserParts.class));
+        ChromeBrowserInitializer.setForTesting(mInitializer);
+
         FirstRunAppRestrictionInfo.setInitializedInstanceForTest(mMockAppRestrictionInfo);
         ToSAndUMAFirstRunFragment.setShowUmaCheckBoxForTesting(true);
         PolicyServiceFactory.setPolicyServiceForTest(mPolicyService);
@@ -188,6 +208,12 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupportTest {
         assertHistograms(true, SpeedComparedToInflation.SLOWER,
                 SpeedComparedToInflation.NOT_RECORDED, SpeedComparedToInflation.NOT_RECORDED);
 
+        SharedPreferencesManager.getInstance().writeBoolean(
+                ChromePreferenceKeys.PRIVACY_METRICS_REPORTING, false);
+        Assert.assertFalse("Crash report should be disabled by shared preference.",
+                PrivacyPreferencesManagerImpl.getInstance()
+                        .isUsageAndCrashReportingPermittedByUser());
+
         // Try to accept ToS.
         TestThreadUtils.runOnUiThreadBlocking((Runnable) mAcceptButton::performClick);
         Assert.assertTrue("Crash report should be enabled.",
@@ -195,6 +221,38 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupportTest {
                         .isUsageAndCrashReportingPermittedByUser());
     }
 
+    @Test
+    @SmallTest
+    public void testNoRestriction_AcceptBeforeNative() {
+        launchFirstRunThroughCustomTabPreNative();
+        assertUIState(FragmentState.LOADING);
+
+        setAppRestrictionsMockInitialized(false);
+        assertUIState(FragmentState.NO_POLICY);
+
+        assertHistograms(true, SpeedComparedToInflation.SLOWER,
+                SpeedComparedToInflation.NOT_RECORDED, SpeedComparedToInflation.NOT_RECORDED);
+
+        SharedPreferencesManager.getInstance().writeBoolean(
+                ChromePreferenceKeys.PRIVACY_METRICS_REPORTING, false);
+        Assert.assertFalse("Crash report should be disabled by shared preference.",
+                PrivacyPreferencesManagerImpl.getInstance()
+                        .isUsageAndCrashReportingPermittedByUser());
+
+        // Try to accept ToS.
+        TestThreadUtils.runOnUiThreadBlocking((Runnable) mAcceptButton::performClick);
+        assertUIState(FragmentState.WAITING_UNTIL_NEXT_PAGE);
+        Assert.assertFalse("Crash report should not be enabled before native initialized.",
+                PrivacyPreferencesManagerImpl.getInstance()
+                        .isUsageAndCrashReportingPermittedByUser());
+
+        // ToS should be accepted when native is initialized.
+        startNativeInitializationAndWait();
+        Assert.assertTrue("Crash report should be enabled.",
+                PrivacyPreferencesManagerImpl.getInstance()
+                        .isUsageAndCrashReportingPermittedByUser());
+    }
+
     @Test
     @SmallTest
     public void testNoRestriction_BeforeInflation() {
@@ -456,10 +514,15 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupportTest {
         renderWithPortraitAndLandscape(tosAndUmaFragment, "fre_tosanduma_withpolicy");
     }
 
+    private void launchFirstRunThroughCustomTab() {
+        launchFirstRunThroughCustomTabPreNative();
+        startNativeInitializationAndWait();
+    }
+
     /**
      * Launch chrome through custom tab and trigger first run.
      */
-    private void launchFirstRunThroughCustomTab() {
+    private void launchFirstRunThroughCustomTabPreNative() {
         final Instrumentation instrumentation = InstrumentationRegistry.getInstrumentation();
         final Context context = instrumentation.getTargetContext();
 
@@ -485,14 +548,11 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupportTest {
         CriteriaHelper.pollUiThread(
                 () -> mActivity.getSupportFragmentManager().getFragments().size() > 0);
 
-        // Force this to happen now to try to make the tests more deterministic. Ideally the tests
-        // could control when this happens and test for difference sequences.
-        waitUntilNativeLoaded();
-
         mTosText = mActivity.findViewById(R.id.tos_and_privacy);
         mUmaCheckBox = mActivity.findViewById(R.id.send_report_checkbox);
         mAcceptButton = mActivity.findViewById(R.id.terms_accept);
-        mLargeSpinner = mActivity.findViewById(R.id.progress_spinner_large);
+        mLowerSpinner = mActivity.findViewById(R.id.progress_spinner);
+        mCenterSpinner = mActivity.findViewById(R.id.progress_spinner_large);
         mPrivacyDisclaimer = mActivity.findViewById(R.id.privacy_disclaimer);
     }
 
@@ -501,12 +561,14 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupportTest {
         int spinnerVisibility = (fragmentState == FragmentState.LOADING) ? View.VISIBLE : View.GONE;
         int privacyVisibility =
                 (fragmentState == FragmentState.HAS_POLICY) ? View.VISIBLE : View.GONE;
+        int lowerSpinnerVisibility =
+                (fragmentState == FragmentState.WAITING_UNTIL_NEXT_PAGE) ? View.VISIBLE : View.GONE;
 
         CriteriaHelper.pollUiThread(
                 ()
                         -> Criteria.checkThat(
                                 "Visibility of Loading spinner never reached test setting.",
-                                mLargeSpinner.getVisibility(), Matchers.is(spinnerVisibility)));
+                                mCenterSpinner.getVisibility(), Matchers.is(spinnerVisibility)));
 
         Assert.assertEquals("Visibility of ToS text is different than the test setting.",
                 tosVisibility, mTosText.getVisibility());
@@ -514,6 +576,8 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupportTest {
                 tosVisibility, mUmaCheckBox.getVisibility());
         Assert.assertEquals("Visibility of accept button is different than the test setting.",
                 tosVisibility, mAcceptButton.getVisibility());
+        Assert.assertEquals("Visibility of lower spinner is different than the test setting.",
+                lowerSpinnerVisibility, mLowerSpinner.getVisibility());
         Assert.assertEquals("Visibility of privacy disclaimer is different than the test setting.",
                 privacyVisibility, mPrivacyDisclaimer.getVisibility());
 
@@ -524,6 +588,21 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupportTest {
                 () -> Criteria.checkThat(mExitCount, Matchers.is(expectedExitCount)));
     }
 
+    private void startNativeInitializationAndWait() {
+        Mockito.verify(mInitializer, Mockito.timeout(3000L))
+                .handlePostNativeStartup(eq(true), mBrowserParts.capture());
+        Mockito.doCallRealMethod()
+                .when(mInitializer)
+                .handlePostNativeStartup(anyBoolean(), any(BrowserParts.class));
+
+        TestThreadUtils.runOnUiThreadBlocking(
+                ()
+                        -> mInitializer.handlePostNativeStartup(
+                                /*isAsync*/ false, mBrowserParts.getValue()));
+        CriteriaHelper.pollUiThread(
+                (() -> mActivity.isNativeSideIsInitializedForTest()), "native never initialized.");
+    }
+
     /**
      * Asserts the speed histograms related to FirstRunAppRestrictions and TosAndUmaFirstRunFragment
      * are recorded correctly. Noting that with the current test setup, it is possible that the
@@ -568,11 +647,6 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupportTest {
                 recorded ? 1 : 0, RecordHistogram.getHistogramTotalCountForTesting(histogram));
     }
 
-    private void waitUntilNativeLoaded() {
-        CriteriaHelper.pollUiThread(
-                (() -> mActivity.isNativeSideIsInitializedForTest()), "native never initialized.");
-    }
-
     private void setAppRestrictionsMockNotInitialized() {
         Mockito.doAnswer(invocation -> {
                    Callback<Boolean> callback = invocation.getArgument(0);