[Unity][Android] Move mSigninAccessPoint to SigninFragment

This CL moves mSigninAccessPoint from SigninFragmentBase to
SigninFragment. SigninFragmentBase will be used for as the base class
for the consent bump fragment and for the consent bump there's no notion
of the access point. This CL also reimplements how SigninFragmentBase
determines the text for the cancel button, because it's been using
mSigninAccessPoint.

Bug: 869426
Change-Id: I8361965a9c9df62d86e058b01f4a06f88e7bfbda
Reviewed-on: https://chromium-review.googlesource.com/1156701Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Commit-Queue: Boris Sazonov <bsazonov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579485}

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

@@ -9,6 +9,7 @@ import android.content.Context;
 import android.os.Bundle;
 
 import org.chromium.base.metrics.RecordUserAction;
+import org.chromium.chrome.R;
 import org.chromium.chrome.browser.ntp.cards.SignInPromo;
 import org.chromium.chrome.browser.signin.SigninAccessPoint;
 import org.chromium.chrome.browser.signin.SigninFragmentBase;
@@ -30,12 +31,11 @@ public class SigninFirstRunFragment extends SigninFragmentBase implements FirstR
         String forceAccountTo =
                 freProperties.getString(AccountFirstRunFragment.FORCE_SIGNIN_ACCOUNT_TO);
         if (forceAccountTo == null) {
-            mArguments = createArguments(SigninAccessPoint.START_PAGE, null);
+            mArguments = createArguments(null);
         } else {
             @ChildAccountStatus.Status int childAccountStatus =
                     freProperties.getInt(AccountFirstRunFragment.CHILD_ACCOUNT_STATUS);
-            mArguments = createArgumentsForForcedSigninFlow(
-                    SigninAccessPoint.START_PAGE, forceAccountTo, childAccountStatus);
+            mArguments = createArgumentsForForcedSigninFlow(forceAccountTo, childAccountStatus);
         }
 
         RecordUserAction.record("MobileFre.SignInShown");
@@ -71,4 +71,9 @@ public class SigninFirstRunFragment extends SigninFragmentBase implements FirstR
         getPageDelegate().advanceToNextPage();
         callback.run();
     }
+
+    @Override
+    protected int getNegativeButtonTextId() {
+        return R.string.no_thanks;
+    }
 }

chrome/android/java/src/org/chromium/chrome/browser/signin/SigninFragment.java

@@ -11,6 +11,7 @@ import android.support.annotation.Nullable;
 
 import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.base.metrics.RecordUserAction;
+import org.chromium.chrome.R;
 import org.chromium.chrome.browser.preferences.PrefServiceBridge;
 import org.chromium.chrome.browser.preferences.PreferencesLauncher;
 
@@ -21,6 +22,7 @@ import java.lang.annotation.RetentionPolicy;
 public class SigninFragment extends SigninFragmentBase {
     private static final String TAG = "SigninFragment";
 
+    private static final String ARGUMENT_ACCESS_POINT = "SigninFragment.AccessPoint";
     private static final String ARGUMENT_PERSONALIZED_PROMO_ACTION =
             "SigninFragment.PersonalizedPromoAction";
 
@@ -34,6 +36,7 @@ public class SigninFragment extends SigninFragmentBase {
         int NEW_ACCOUNT = 3;
     }
 
+    private @SigninAccessPoint int mSigninAccessPoint;
     private @PromoAction int mPromoAction;
 
     /**
@@ -41,7 +44,9 @@ public class SigninFragment extends SigninFragmentBase {
      * @param accessPoint The access point for starting sign-in flow.
      */
     public static Bundle createArguments(@SigninAccessPoint int accessPoint) {
-        return SigninFragmentBase.createArguments(accessPoint, null);
+        Bundle result = SigninFragmentBase.createArguments(null);
+        result.putInt(ARGUMENT_ACCESS_POINT, accessPoint);
+        return result;
     }
 
     /**
@@ -51,7 +56,8 @@ public class SigninFragment extends SigninFragmentBase {
      */
     public static Bundle createArgumentsForPromoDefaultFlow(
             @SigninAccessPoint int accessPoint, String accountName) {
-        Bundle result = SigninFragmentBase.createArguments(accessPoint, accountName);
+        Bundle result = SigninFragmentBase.createArguments(accountName);
+        result.putInt(ARGUMENT_ACCESS_POINT, accessPoint);
         result.putInt(ARGUMENT_PERSONALIZED_PROMO_ACTION, PromoAction.WITH_DEFAULT);
         return result;
     }
@@ -64,8 +70,8 @@ public class SigninFragment extends SigninFragmentBase {
      */
     public static Bundle createArgumentsForPromoChooseAccountFlow(
             @SigninAccessPoint int accessPoint, String accountName) {
-        Bundle result =
-                SigninFragmentBase.createArgumentsForChooseAccountFlow(accessPoint, accountName);
+        Bundle result = SigninFragmentBase.createArgumentsForChooseAccountFlow(accountName);
+        result.putInt(ARGUMENT_ACCESS_POINT, accessPoint);
         result.putInt(ARGUMENT_PERSONALIZED_PROMO_ACTION, PromoAction.NOT_DEFAULT);
         return result;
     }
@@ -76,7 +82,8 @@ public class SigninFragment extends SigninFragmentBase {
      * @param accessPoint The access point for starting sign-in flow.
      */
     public static Bundle createArgumentsForPromoAddAccountFlow(@SigninAccessPoint int accessPoint) {
-        Bundle result = SigninFragmentBase.createArgumentsForAddAccountFlow(accessPoint);
+        Bundle result = SigninFragmentBase.createArgumentsForAddAccountFlow();
+        result.putInt(ARGUMENT_ACCESS_POINT, accessPoint);
         result.putInt(ARGUMENT_PERSONALIZED_PROMO_ACTION, PromoAction.NEW_ACCOUNT);
         return result;
     }
@@ -88,10 +95,20 @@ public class SigninFragment extends SigninFragmentBase {
     public void onCreate(@Nullable Bundle savedInstanceState) {
         super.onCreate(savedInstanceState);
 
+        int accessPoint = getSigninArguments().getInt(ARGUMENT_ACCESS_POINT, -1);
+        assert accessPoint == SigninAccessPoint.AUTOFILL_DROPDOWN
+                || accessPoint == SigninAccessPoint.BOOKMARK_MANAGER
+                || accessPoint == SigninAccessPoint.NTP_CONTENT_SUGGESTIONS
+                || accessPoint == SigninAccessPoint.RECENT_TABS
+                || accessPoint == SigninAccessPoint.SETTINGS
+                || accessPoint == SigninAccessPoint.SIGNIN_PROMO
+                || accessPoint
+                        == SigninAccessPoint.START_PAGE : "invalid access point: " + accessPoint;
+        mSigninAccessPoint = accessPoint;
         mPromoAction =
                 getSigninArguments().getInt(ARGUMENT_PERSONALIZED_PROMO_ACTION, PromoAction.NONE);
 
-        SigninManager.logSigninStartAccessPoint(getSigninAccessPoint());
+        SigninManager.logSigninStartAccessPoint(mSigninAccessPoint);
         recordSigninStartedHistogramAccountInfo();
         recordSigninStartedUserAction();
     }
@@ -135,6 +152,12 @@ public class SigninFragment extends SigninFragmentBase {
         });
     }
 
+    @Override
+    protected int getNegativeButtonTextId() {
+        return mSigninAccessPoint == SigninAccessPoint.SIGNIN_PROMO ? R.string.no_thanks
+                                                                    : R.string.cancel;
+    }
+
     private void recordSigninCompletedHistogramAccountInfo() {
         final String histogram;
         switch (mPromoAction) {
@@ -155,7 +178,7 @@ public class SigninFragment extends SigninFragmentBase {
         }
 
         RecordHistogram.recordEnumeratedHistogram(
-                histogram, getSigninAccessPoint(), SigninAccessPoint.MAX);
+                histogram, mSigninAccessPoint, SigninAccessPoint.MAX);
     }
 
     private void recordSigninStartedHistogramAccountInfo() {
@@ -178,11 +201,11 @@ public class SigninFragment extends SigninFragmentBase {
         }
 
         RecordHistogram.recordEnumeratedHistogram(
-                histogram, getSigninAccessPoint(), SigninAccessPoint.MAX);
+                histogram, mSigninAccessPoint, SigninAccessPoint.MAX);
     }
 
     private void recordSigninStartedUserAction() {
-        switch (getSigninAccessPoint()) {
+        switch (mSigninAccessPoint) {
             case SigninAccessPoint.AUTOFILL_DROPDOWN:
                 RecordUserAction.record("Signin_Signin_FromAutofillDropdown");
                 break;

chrome/android/java/src/org/chromium/chrome/browser/signin/SigninFragmentBase.java

@@ -63,7 +63,6 @@ public abstract class SigninFragmentBase
     private static final String SETTINGS_LINK_OPEN = "<LINK1>";
     private static final String SETTINGS_LINK_CLOSE = "</LINK1>";
 
-    private static final String ARGUMENT_ACCESS_POINT = "SigninFragmentBase.AccessPoint";
     private static final String ARGUMENT_ACCOUNT_NAME = "SigninFragmentBase.AccountName";
     private static final String ARGUMENT_CHILD_ACCOUNT_STATUS =
             "SigninFragmentBase.ChildAccountStatus";
@@ -84,13 +83,11 @@ public abstract class SigninFragmentBase
         int ADD_ACCOUNT = 3;
     }
 
-    private @SigninAccessPoint int mSigninAccessPoint;
     private @SigninFlowType int mSigninFlowType;
     private @ChildAccountStatus.Status int mChildAccountStatus;
 
     private SigninView mView;
     private ConsentTextTracker mConsentTextTracker;
-    private @StringRes int mCancelButtonTextId = R.string.cancel;
 
     private boolean mAccountSelectionPending;
     private @Nullable String mRequestedAccountName;
@@ -114,13 +111,10 @@ public abstract class SigninFragmentBase
     /**
      * Creates an argument bundle for the default SigninFragmentBase flow (account selection is
      * enabled, etc.).
-     * @param accessPoint The access point for starting sign-in flow.
      * @param accountName The account to preselect or null to preselect the default account.
      */
-    protected static Bundle createArguments(
-            @SigninAccessPoint int accessPoint, @Nullable String accountName) {
+    protected static Bundle createArguments(@Nullable String accountName) {
         Bundle result = new Bundle();
-        result.putInt(ARGUMENT_ACCESS_POINT, accessPoint);
         result.putInt(ARGUMENT_SIGNIN_FLOW_TYPE, SigninFlowType.DEFAULT);
         result.putString(ARGUMENT_ACCOUNT_NAME, accountName);
         return result;
@@ -129,13 +123,10 @@ public abstract class SigninFragmentBase
     /**
      * Creates an argument bundle for "Choose account" sign-in flow. Account selection dialog will
      * be shown at the start of the sign-in process.
-     * @param accessPoint The access point for starting sign-in flow.
      * @param accountName The account to preselect or null to preselect the default account.
      */
-    protected static Bundle createArgumentsForChooseAccountFlow(
-            @SigninAccessPoint int accessPoint, @Nullable String accountName) {
+    protected static Bundle createArgumentsForChooseAccountFlow(@Nullable String accountName) {
         Bundle result = new Bundle();
-        result.putInt(ARGUMENT_ACCESS_POINT, accessPoint);
         result.putInt(ARGUMENT_SIGNIN_FLOW_TYPE, SigninFlowType.CHOOSE_ACCOUNT);
         result.putString(ARGUMENT_ACCOUNT_NAME, accountName);
         return result;
@@ -144,25 +135,21 @@ public abstract class SigninFragmentBase
     /**
      * Creates an argument bundle for "Add account" sign-in flow. Activity to add an account will be
      * shown at the start of the sign-in process.
-     * @param accessPoint The access point for starting sign-in flow.
      */
-    protected static Bundle createArgumentsForAddAccountFlow(@SigninAccessPoint int accessPoint) {
+    protected static Bundle createArgumentsForAddAccountFlow() {
         Bundle result = new Bundle();
-        result.putInt(ARGUMENT_ACCESS_POINT, accessPoint);
         result.putInt(ARGUMENT_SIGNIN_FLOW_TYPE, SigninFlowType.ADD_ACCOUNT);
         return result;
     }
 
     /**
      * Creates an argument bundle for a custom SigninFragmentBase flow.
-     * @param accessPoint The access point for starting sign-in flow.
      * @param accountName The account to preselect.
      * @param childAccountStatus Whether the selected account is a child one.
      */
-    protected static Bundle createArgumentsForForcedSigninFlow(@SigninAccessPoint int accessPoint,
+    protected static Bundle createArgumentsForForcedSigninFlow(
             String accountName, @ChildAccountStatus.Status int childAccountStatus) {
         Bundle result = new Bundle();
-        result.putInt(ARGUMENT_ACCESS_POINT, accessPoint);
         result.putInt(ARGUMENT_SIGNIN_FLOW_TYPE, SigninFlowType.FORCED);
         result.putString(ARGUMENT_ACCOUNT_NAME, accountName);
         result.putInt(ARGUMENT_CHILD_ACCOUNT_STATUS, childAccountStatus);
@@ -193,10 +180,12 @@ public abstract class SigninFragmentBase
     protected abstract void onSigninAccepted(String accountName, boolean isDefaultAccount,
             boolean settingsClicked, Runnable callback);
 
-    /** Returns the access point that initiated the sign-in flow. */
-    protected @SigninAccessPoint int getSigninAccessPoint() {
-        return mSigninAccessPoint;
-    }
+    /**
+     * Returns the string resource id for the negative button. This is invoked once from
+     * {@link #onCreateView}.
+     */
+    @StringRes
+    protected abstract int getNegativeButtonTextId();
 
     /** Returns whether this fragment is in "force sign-in" mode. */
     protected boolean isForcedSignin() {
@@ -208,7 +197,6 @@ public abstract class SigninFragmentBase
         super.onCreate(savedInstanceState);
 
         Bundle arguments = getSigninArguments();
-        initAccessPoint(arguments.getInt(ARGUMENT_ACCESS_POINT, -1));
         mRequestedAccountName = arguments.getString(ARGUMENT_ACCOUNT_NAME, null);
         mChildAccountStatus =
                 arguments.getInt(ARGUMENT_CHILD_ACCOUNT_STATUS, ChildAccountStatus.NOT_CHILD);
@@ -243,23 +231,6 @@ public abstract class SigninFragmentBase
                 getResources().getDimensionPixelSize(R.dimen.user_picture_size), badgeConfig);
     }
 
-    private void initAccessPoint(@SigninAccessPoint int accessPoint) {
-        assert accessPoint == SigninAccessPoint.AUTOFILL_DROPDOWN
-                || accessPoint == SigninAccessPoint.BOOKMARK_MANAGER
-                || accessPoint == SigninAccessPoint.NTP_CONTENT_SUGGESTIONS
-                || accessPoint == SigninAccessPoint.RECENT_TABS
-                || accessPoint == SigninAccessPoint.SETTINGS
-                || accessPoint == SigninAccessPoint.SIGNIN_PROMO
-                || accessPoint
-                        == SigninAccessPoint.START_PAGE : "invalid access point: " + accessPoint;
-
-        mSigninAccessPoint = accessPoint;
-        if (accessPoint == SigninAccessPoint.START_PAGE
-                || accessPoint == SigninAccessPoint.SIGNIN_PROMO) {
-            mCancelButtonTextId = R.string.no_thanks;
-        }
-    }
-
     @Override
     public void onDestroy() {
         super.onDestroy();
@@ -351,7 +322,7 @@ public abstract class SigninFragmentBase
 
         mConsentTextTracker.setText(mView.getGoogleServicesDescriptionView(),
                 R.string.signin_google_services_description);
-        mConsentTextTracker.setText(mView.getRefuseButton(), mCancelButtonTextId);
+        mConsentTextTracker.setText(mView.getRefuseButton(), getNegativeButtonTextId());
         mConsentTextTracker.setText(mView.getMoreButton(), R.string.more);
     }