Stop setting PRIVACY_METRICS_REPORTING pref on FRE startup.

This change removes the default [re]seting of

This change is needed because CCTs with policy CCTToSDialogEnabled skips
the FRE. This flow doesn't accept the ToS, and doesn't overwrite to the
PRIVACY_METRICS_REPORTING pref. But very early in the FRE startup/flow,
this pref is blindly written with the default (true), and that value is
persisted forward for the CCT skip, when really we want the value to be
false.

The theory here is that we didn't actually need to set this pref. It
seems to have been initially written to seed the checkbox state, see
https://crrev.com/978653002 , but that changed when the default flipped
in https://crrev.com/2275513003 . See associated bug for more details.

This change also removes the temporary hack added in
https://crrev.com/c/2416885 to support CCTs that skip the ToS.

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

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

@@ -25,7 +25,6 @@ import org.chromium.chrome.browser.datareduction.DataReductionPromoUtils;
 import org.chromium.chrome.browser.datareduction.DataReductionProxyUma;
 import org.chromium.chrome.browser.metrics.UmaUtils;
 import org.chromium.chrome.browser.net.spdyproxy.DataReductionProxySettings;
-import org.chromium.chrome.browser.privacy.settings.PrivacyPreferencesManagerImpl;
 import org.chromium.chrome.browser.search_engines.TemplateUrlServiceFactory;
 import org.chromium.chrome.browser.searchwidget.SearchWidgetProvider;
 import org.chromium.ui.base.LocalizationUtils;
@@ -453,13 +452,6 @@ public class FirstRunActivity extends FirstRunActivityBase implements FirstRunPa
         // intent. The re-launched intent will still need to know to avoid the FRE.
         FirstRunStatus.setFirstRunSkippedByPolicy(true);
 
-        // This pref is written to have a value of true during the FRE's startup. If the user
-        // presses the accept ToS button, this pref's value is overridden with their choice.
-        // However, when the FRE is skipped, that initial value is the opposite of what we want, so
-        // manually set it to false here.
-        // TODO(https://crbug.com/1128955): Remove this once the default is not written on startup.
-        PrivacyPreferencesManagerImpl.getInstance().setUsageAndCrashReporting(false);
-
         launchPendingIntentAndFinish();
     }
 

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

@@ -25,7 +25,6 @@ import org.chromium.chrome.browser.LaunchIntentDispatcher;
 import org.chromium.chrome.browser.flags.ChromeSwitches;
 import org.chromium.chrome.browser.locale.LocaleManager;
 import org.chromium.chrome.browser.net.spdyproxy.DataReductionProxySettings;
-import org.chromium.chrome.browser.privacy.settings.PrivacyPreferencesManagerImpl;
 import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.chrome.browser.services.AndroidChildAccountHelper;
 import org.chromium.chrome.browser.signin.services.IdentityServicesProvider;
@@ -139,12 +138,6 @@ public abstract class FirstRunFlowSequencer  {
                 || searchPromoType == LocaleManager.SearchEnginePromoType.SHOW_EXISTING;
     }
 
-    @VisibleForTesting
-    protected void setDefaultMetricsAndCrashReporting() {
-        PrivacyPreferencesManagerImpl.getInstance().setUsageAndCrashReporting(
-                FirstRunActivity.DEFAULT_METRICS_AND_CRASH_REPORTING);
-    }
-
     @VisibleForTesting
     protected void setFirstRunFlowSignInComplete() {
         FirstRunSignInProcessor.setFirstRunFlowSignInComplete(true);
@@ -169,10 +162,6 @@ public abstract class FirstRunFlowSequencer  {
         Bundle freProperties = new Bundle();
         freProperties.putInt(SigninFirstRunFragment.CHILD_ACCOUNT_STATUS, mChildAccountStatus);
 
-        // Initialize usage and crash reporting according to the default value.
-        // The user can explicitly enable or disable the reporting on the Welcome page.
-        setDefaultMetricsAndCrashReporting();
-
         onFlowIsKnown(freProperties);
         if (ChildAccountStatus.isChild(mChildAccountStatus)) {
             setFirstRunFlowSignInComplete();

chrome/android/junit/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencerTest.java

@@ -52,7 +52,6 @@ public class FirstRunFlowSequencerTest {
     public static class TestFirstRunFlowSequencer extends FirstRunFlowSequencer {
         public Bundle returnedBundle;
         public boolean calledOnFlowIsKnown;
-        public boolean calledSetDefaultMetricsAndCrashReporting;
         public boolean calledSetFirstRunFlowSignInComplete;
 
         public boolean isFirstRunFlowComplete;
@@ -115,11 +114,6 @@ public class FirstRunFlowSequencerTest {
             return shouldShowSearchEnginePage;
         }
 
-        @Override
-        public void setDefaultMetricsAndCrashReporting() {
-            calledSetDefaultMetricsAndCrashReporting = true;
-        }
-
         @Override
         protected void setFirstRunFlowSignInComplete() {
             calledSetFirstRunFlowSignInComplete = true;
@@ -155,7 +149,6 @@ public class FirstRunFlowSequencerTest {
         mSequencer.processFreEnvironmentPreNative();
         assertTrue(mSequencer.calledOnFlowIsKnown);
         assertNull(mSequencer.returnedBundle);
-        assertFalse(mSequencer.calledSetDefaultMetricsAndCrashReporting);
     }
 
     @Test
@@ -171,7 +164,6 @@ public class FirstRunFlowSequencerTest {
 
         mSequencer.processFreEnvironmentPreNative();
         assertTrue(mSequencer.calledOnFlowIsKnown);
-        assertTrue(mSequencer.calledSetDefaultMetricsAndCrashReporting);
         assertFalse(mSequencer.calledSetFirstRunFlowSignInComplete);
 
         Bundle bundle = mSequencer.returnedBundle;
@@ -197,7 +189,6 @@ public class FirstRunFlowSequencerTest {
 
         mSequencer.processFreEnvironmentPreNative();
         assertTrue(mSequencer.calledOnFlowIsKnown);
-        assertTrue(mSequencer.calledSetDefaultMetricsAndCrashReporting);
         assertTrue(mSequencer.calledSetFirstRunFlowSignInComplete);
 
         Bundle bundle = mSequencer.returnedBundle;
@@ -225,7 +216,6 @@ public class FirstRunFlowSequencerTest {
 
         mSequencer.processFreEnvironmentPreNative();
         assertTrue(mSequencer.calledOnFlowIsKnown);
-        assertTrue(mSequencer.calledSetDefaultMetricsAndCrashReporting);
         assertFalse(mSequencer.calledSetFirstRunFlowSignInComplete);
 
         Bundle bundle = mSequencer.returnedBundle;
@@ -251,7 +241,6 @@ public class FirstRunFlowSequencerTest {
 
         mSequencer.processFreEnvironmentPreNative();
         assertTrue(mSequencer.calledOnFlowIsKnown);
-        assertTrue(mSequencer.calledSetDefaultMetricsAndCrashReporting);
         assertFalse(mSequencer.calledSetFirstRunFlowSignInComplete);
 
         Bundle bundle = mSequencer.returnedBundle;