[Android] Plumb SignoutReason for SigninManagerAndroid

SigninManagerAndroid::SignOut() has historically hard coded the reason
as signin_metrics::USER_CLICKED_SIGNOUT_SETTINGS. This CL begins to
address the TODO there by plumbing the reason from the java layer and
various callers.

For now the CL preserves the existing "USER_CLICKED" everywhere. Follow
up work should change this reason to reflect the real trigger.

Change-Id: Id225b9bf5e9b9fd5771c2353616c6a1a82e60af8
Bug: 873116
Reviewed-on: https://chromium-review.googlesource.com/1170763Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589607}

chrome/android/java/src/org/chromium/chrome/browser/preferences/SyncAndServicesPreferences.java

@@ -46,6 +46,7 @@ import org.chromium.chrome.browser.metrics.UmaSessionStats;
 import org.chromium.chrome.browser.preferences.privacy.PrivacyPreferencesManager;
 import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.chrome.browser.signin.SigninManager;
+import org.chromium.chrome.browser.signin.SignoutReason;
 import org.chromium.chrome.browser.signin.UnifiedConsentServiceBridge;
 import org.chromium.chrome.browser.sync.GoogleServiceAuthError;
 import org.chromium.chrome.browser.sync.ProfileSyncService;
@@ -811,7 +812,9 @@ public class SyncAndServicesPreferences extends PreferenceFragment
 
         if (mCurrentSyncError == SyncError.OTHER_ERRORS) {
             final Account account = ChromeSigninController.get().getSignedInUser();
-            SigninManager.get().signOut(() -> SigninManager.get().signIn(account, null, null));
+            // TODO(https://crbug.com/873116): Pass the correct reason for the signout.
+            SigninManager.get().signOut(SignoutReason.USER_CLICKED_SIGNOUT_SETTINGS,
+                    () -> SigninManager.get().signIn(account, null, null));
             return;
         }
 

chrome/android/java/src/org/chromium/chrome/browser/services/GoogleServicesManager.java

@@ -16,6 +16,7 @@ import org.chromium.base.TraceEvent;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.chrome.browser.signin.SigninHelper;
 import org.chromium.chrome.browser.signin.SigninManager;
+import org.chromium.chrome.browser.signin.SignoutReason;
 import org.chromium.chrome.browser.sync.SyncController;
 import org.chromium.components.signin.ChromeSigninController;
 
@@ -83,7 +84,8 @@ public class GoogleServicesManager implements ApplicationStateListener {
             SigninManager signinManager = SigninManager.get();
             if (!mChromeSigninController.isSignedIn() && signinManager.isSignedInOnNative()) {
                 Log.w(TAG, "Signed in state got out of sync, forcing native sign out");
-                signinManager.signOut();
+                // TODO(https://crbug.com/873116): Pass the correct reason for the signout.
+                signinManager.signOut(SignoutReason.USER_CLICKED_SIGNOUT_SETTINGS);
             }
 
             // Initialize sync.

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

@@ -442,19 +442,20 @@ public class AccountManagementFragment extends PreferenceFragment
 
         final Activity activity = getActivity();
         final DialogFragment clearDataProgressDialog = new ClearDataProgressDialog();
-        SigninManager.get().signOut(null, new SigninManager.WipeDataHooks() {
-            @Override
-            public void preWipeData() {
-                clearDataProgressDialog.show(
-                        activity.getFragmentManager(), CLEAR_DATA_PROGRESS_DIALOG_TAG);
-            }
-            @Override
-            public void postWipeData() {
-                if (clearDataProgressDialog.isAdded()) {
-                    clearDataProgressDialog.dismissAllowingStateLoss();
-                }
-            }
-        });
+        SigninManager.get().signOut(SignoutReason.USER_CLICKED_SIGNOUT_SETTINGS, null,
+                new SigninManager.WipeDataHooks() {
+                    @Override
+                    public void preWipeData() {
+                        clearDataProgressDialog.show(
+                                activity.getFragmentManager(), CLEAR_DATA_PROGRESS_DIALOG_TAG);
+                    }
+                    @Override
+                    public void postWipeData() {
+                        if (clearDataProgressDialog.isAdded()) {
+                            clearDataProgressDialog.dismissAllowingStateLoss();
+                        }
+                    }
+                });
         AccountManagementScreenHelper.logEvent(
                 ProfileAccountManagementMetrics.SIGNOUT_SIGNOUT,
                 mGaiaServiceType);

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

@@ -165,7 +165,8 @@ public class SigninHelper {
                         // Here we have to sign out though to ensure account consistency,
                         // so override the flag.
                         mSigninManager.prohibitSignout(false);
-                        mSigninManager.signOut();
+                        // TODO(https://crbug.com/873116): Pass the correct reason for the signout.
+                        mSigninManager.signOut(SignoutReason.USER_CLICKED_SIGNOUT_SETTINGS);
                     } else {
                         validateAccountSettings(true);
                     }
@@ -209,7 +210,7 @@ public class SigninHelper {
 
         // TODO(acleung): Deal with passphrase or just prompt user to re-enter it?
         // Perform a sign-out with a callback to sign-in again.
-        mSigninManager.signOut(() -> {
+        mSigninManager.signOut(SignoutReason.USER_CLICKED_SIGNOUT_SETTINGS, () -> {
             // Clear the shared perf only after signOut is successful.
             // If Chrome dies, we can try it again on next run.
             // Otherwise, if re-sign-in fails, we'll just leave chrome

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

@@ -504,12 +504,12 @@ public class SigninManager implements AccountTrackerService.OnSystemAccountsSeed
 
     /**
      * Invokes signOut and returns a {@link Promise} that will be fulfilled on completion.
-     * This is equivalent to calling {@link #signOut(Runnable callback)} with a callback that
-     * fulfills the returned {@link Promise}.
+     * This is equivalent to calling {@link #signOut(@SignoutReason int signoutSource, Runnable
+     * callback)} with a callback that fulfills the returned {@link Promise}.
      */
-    public Promise<Void> signOutPromise() {
+    public Promise<Void> signOutPromise(@SignoutReason int signoutSource) {
         final Promise<Void> promise = new Promise<>();
-        signOut(() -> promise.fulfill(null));
+        signOut(signoutSource, () -> promise.fulfill(null));
 
         return promise;
     }
@@ -517,15 +517,15 @@ public class SigninManager implements AccountTrackerService.OnSystemAccountsSeed
     /**
      * Invokes signOut with no callback or wipeDataHooks.
      */
-    public void signOut() {
-        signOut(null, null);
+    public void signOut(@SignoutReason int signoutSource) {
+        signOut(signoutSource, null, null);
     }
 
     /**
      * Invokes signOut() with no wipeDataHooks.
      */
-    public void signOut(Runnable callback) {
-        signOut(callback, null);
+    public void signOut(@SignoutReason int signoutSource, Runnable callback) {
+        signOut(signoutSource, callback, null);
     }
 
     /**
@@ -534,10 +534,13 @@ public class SigninManager implements AccountTrackerService.OnSystemAccountsSeed
      * This method clears the signed-in username, stops sync and sends out a
      * sign-out notification on the native side.
      *
+     * @param signoutSource describes the event driving the signout (e.g.
+     *         {@link SignoutReason.USER_CLICKED_SIGNOUT_SETTINGS}).
      * @param callback Will be invoked after sign-out completes, if not null.
      * @param wipeDataHooks Hooks to call before/after data wiping phase of sign-out.
      */
-    public void signOut(Runnable callback, WipeDataHooks wipeDataHooks) {
+    public void signOut(
+            @SignoutReason int signoutSource, Runnable callback, WipeDataHooks wipeDataHooks) {
         // Only one signOut at a time!
         assert mSignOutState == null;
 
@@ -547,7 +550,7 @@ public class SigninManager implements AccountTrackerService.OnSystemAccountsSeed
         Log.d(TAG, "Signing out, managementDomain: " + mSignOutState.managementDomain);
 
         // User data will be wiped in resetAccountData(), called from onNativeSignOut().
-        nativeSignOut(mNativeSigninManagerAndroid);
+        nativeSignOut(mNativeSigninManagerAndroid, signoutSource);
     }
 
     /**
@@ -735,7 +738,7 @@ public class SigninManager implements AccountTrackerService.OnSystemAccountsSeed
     @VisibleForTesting
     native void nativeOnSignInCompleted(long nativeSigninManagerAndroid, String username);
     @VisibleForTesting
-    native void nativeSignOut(long nativeSigninManagerAndroid);
+    native void nativeSignOut(long nativeSigninManagerAndroid, @SignoutReason int reason);
     @VisibleForTesting
     native String nativeGetManagementDomain(long nativeSigninManagerAndroid);
     @VisibleForTesting

chrome/android/java/src/org/chromium/chrome/browser/sync/SyncAccountSwitcher.java

@@ -9,8 +9,6 @@ import android.preference.Preference.OnPreferenceChangeListener;
 import android.support.v4.app.FragmentActivity;
 import android.text.TextUtils;
 
-import org.chromium.base.Callback;
-import org.chromium.base.Promise;
 import org.chromium.chrome.browser.preferences.SyncedAccountPreference;
 import org.chromium.chrome.browser.signin.AccountSigninActivity;
 import org.chromium.chrome.browser.signin.ConfirmImportSyncDataDialog;
@@ -18,6 +16,7 @@ import org.chromium.chrome.browser.signin.ConfirmImportSyncDataDialog.ImportSync
 import org.chromium.chrome.browser.signin.ConfirmSyncDataStateMachine;
 import org.chromium.chrome.browser.signin.SigninManager;
 import org.chromium.chrome.browser.signin.SigninManager.SignInCallback;
+import org.chromium.chrome.browser.signin.SignoutReason;
 
 /**
  * A class that encapsulates the control flow of listeners and callbacks when switching sync
@@ -72,21 +71,19 @@ public class SyncAccountSwitcher
         assert mNewAccountName != null;
 
         // Sign out first to ensure we don't wipe the data when sync is still on.
-        SigninManager.get().signOutPromise()
-                .then(new Promise.AsyncFunction<Void, Void>(){
-                    @Override
-                    public Promise<Void> apply(Void argument) {
-                        // Once signed out, clear the last signed in user and wipe data if needed.
-                        SigninManager.get().clearLastSignedInUser();
-                        return SigninManager.wipeSyncUserDataIfRequired(wipeData);
-                    }
-                }).then(new Callback<Void>(){
-                    @Override
-                    public void onResult(Void result) {
-                        // Once the data has been wiped (if needed), sign in to the next account.
-                        SigninManager.get().signIn(
-                                mNewAccountName, mActivity, SyncAccountSwitcher.this);
-                    }
+        SigninManager
+                .get()
+                // TODO(https://crbug.com/873116): Pass the correct reason for the signout.
+                .signOutPromise(SignoutReason.USER_CLICKED_SIGNOUT_SETTINGS)
+                .then((Void argument) -> {
+                    // Once signed out, clear the last signed in user and wipe data if needed.
+                    SigninManager.get().clearLastSignedInUser();
+                    return SigninManager.wipeSyncUserDataIfRequired(wipeData);
+                })
+                .then((Void argument) -> {
+                    // Once the data has been wiped (if needed), sign in to the next account.
+                    SigninManager.get().signIn(
+                            mNewAccountName, mActivity, SyncAccountSwitcher.this);
                 });
 
         AccountSigninActivity.recordSwitchAccountSourceHistogram(
@@ -108,4 +105,4 @@ public class SyncAccountSwitcher
     public void onSignInAborted() {
         // If the user aborted signin, there is nothing to do.
     }
-}
\ No newline at end of file
+}

chrome/android/java/src/org/chromium/chrome/browser/sync/ui/SyncCustomizationFragment.java

@@ -40,6 +40,7 @@ import org.chromium.chrome.browser.preferences.ChromeSwitchPreference;
 import org.chromium.chrome.browser.preferences.SyncedAccountPreference;
 import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.chrome.browser.signin.SigninManager;
+import org.chromium.chrome.browser.signin.SignoutReason;
 import org.chromium.chrome.browser.sync.GoogleServiceAuthError;
 import org.chromium.chrome.browser.sync.ProfileSyncService;
 import org.chromium.chrome.browser.sync.SyncAccountSwitcher;
@@ -677,7 +678,9 @@ public class SyncCustomizationFragment extends PreferenceFragment
 
         if (mCurrentSyncError == SyncError.OTHER_ERRORS) {
             final Account account = ChromeSigninController.get().getSignedInUser();
-            SigninManager.get().signOut(() -> SigninManager.get().signIn(account, null, null));
+            // TODO(https://crbug.com/873116): Pass the correct reason for the signout.
+            SigninManager.get().signOut(SignoutReason.USER_CLICKED_SIGNOUT_SETTINGS,
+                    () -> SigninManager.get().signIn(account, null, null));
             return;
         }
 

chrome/android/javatests/src/org/chromium/chrome/browser/history/HistoryActivityTest.java

@@ -50,6 +50,7 @@ import org.chromium.chrome.browser.preferences.PrefServiceBridge;
 import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.chrome.browser.signin.SigninManager;
 import org.chromium.chrome.browser.signin.SigninManager.SignInStateObserver;
+import org.chromium.chrome.browser.signin.SignoutReason;
 import org.chromium.chrome.browser.widget.DateDividedAdapter;
 import org.chromium.chrome.browser.widget.TintedImageButton;
 import org.chromium.chrome.browser.widget.selection.SelectableItemView;
@@ -741,7 +742,7 @@ public class HistoryActivityTest {
         ThreadUtils.runOnUiThreadBlocking(new Runnable() {
             @Override
             public void run() {
-                SigninManager.get().signOut(null);
+                SigninManager.get().signOut(SignoutReason.SIGNOUT_TEST);
             }
         });
         mTestObserver.onSigninStateChangedCallback.waitForCallback(currentCallCount, 1);

chrome/android/javatests/src/org/chromium/chrome/browser/signin/SigninTest.java

@@ -264,7 +264,7 @@ public class SigninTest {
             mSigninManager.removeSignInStateObserver(mTestSignInObserver);
 
             if (ChromeSigninController.get().isSignedIn()) {
-                mSigninManager.signOut(null, null);
+                mSigninManager.signOut(SignoutReason.SIGNOUT_TEST);
             }
 
             mBookmarks.destroy();

chrome/android/junit/src/org/chromium/chrome/browser/signin/SigninManagerTest.java

@@ -4,9 +4,11 @@
 
 package org.chromium.chrome.browser.signin;
 
+import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.eq;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.times;
@@ -58,18 +60,18 @@ public class SigninManagerTest {
     public void signOutFromJavaWithManagedDomain() {
         // Stub out various native calls. Some of these are verified as never called
         // and those stubs simply allow that verification to catch any issues.
-        doNothing().when(mSigninManager).nativeSignOut(anyLong());
+        doNothing().when(mSigninManager).nativeSignOut(anyLong(), anyInt());
         doNothing().when(mSigninManager).nativeWipeProfileData(anyLong());
         doNothing().when(mSigninManager).nativeWipeGoogleServiceWorkerCaches(anyLong());
         // See verification of nativeWipeProfileData below.
         doReturn("TestDomain").when(mSigninManager).nativeGetManagementDomain(anyLong());
 
         // Trigger the sign out flow!
-        mSigninManager.signOut();
+        mSigninManager.signOut(SignoutReason.SIGNOUT_TEST);
 
         // nativeSignOut should be called *before* clearing any account data.
         // http://crbug.com/589028
-        verify(mSigninManager, times(1)).nativeSignOut(anyLong());
+        verify(mSigninManager, times(1)).nativeSignOut(anyLong(), eq(SignoutReason.SIGNOUT_TEST));
         verify(mSigninManager, never()).nativeWipeGoogleServiceWorkerCaches(anyLong());
         verify(mSigninManager, never()).nativeWipeProfileData(anyLong());
 
@@ -85,18 +87,18 @@ public class SigninManagerTest {
     public void signOutFromJavaWithNullDomain() {
         // Stub out various native calls. Some of these are verified as never called
         // and those stubs simply allow that verification to catch any issues.
-        doNothing().when(mSigninManager).nativeSignOut(anyLong());
+        doNothing().when(mSigninManager).nativeSignOut(anyLong(), anyInt());
         doNothing().when(mSigninManager).nativeWipeProfileData(anyLong());
         doNothing().when(mSigninManager).nativeWipeGoogleServiceWorkerCaches(anyLong());
         // See verification of nativeWipeGoogleServiceWorkerCaches below.
         doReturn(null).when(mSigninManager).nativeGetManagementDomain(anyLong());
 
         // Trigger the sign out flow!
-        mSigninManager.signOut();
+        mSigninManager.signOut(SignoutReason.SIGNOUT_TEST);
 
         // nativeSignOut should be called *before* clearing any account data.
         // http://crbug.com/589028
-        verify(mSigninManager, times(1)).nativeSignOut(anyLong());
+        verify(mSigninManager, times(1)).nativeSignOut(anyLong(), eq(SignoutReason.SIGNOUT_TEST));
         verify(mSigninManager, never()).nativeWipeGoogleServiceWorkerCaches(anyLong());
         verify(mSigninManager, never()).nativeWipeProfileData(anyLong());
 
@@ -112,7 +114,7 @@ public class SigninManagerTest {
     public void signOutFromNativeWithManagedDomain() {
         // Stub out various native calls. Some of these are verified as never called
         // and those stubs simply allow that verification to catch any issues.
-        doNothing().when(mSigninManager).nativeSignOut(anyLong());
+        doNothing().when(mSigninManager).nativeSignOut(anyLong(), anyInt());
         doNothing().when(mSigninManager).nativeWipeProfileData(anyLong());
         doNothing().when(mSigninManager).nativeWipeGoogleServiceWorkerCaches(anyLong());
         // See verification of nativeWipeProfileData below.
@@ -124,7 +126,7 @@ public class SigninManagerTest {
         // nativeSignOut should only be called when signOut() is triggered on
         // the Java side of the JNI boundary. This test instead initiates sign-out
         // from the native side.
-        verify(mSigninManager, never()).nativeSignOut(anyLong());
+        verify(mSigninManager, never()).nativeSignOut(anyLong(), anyInt());
 
         // Sign-out should wipe profile data when user has managed domain.
         verify(mSigninManager, times(1)).nativeWipeProfileData(anyLong());
@@ -135,7 +137,7 @@ public class SigninManagerTest {
     public void signOutFromNativeWithNullDomain() {
         // Stub out various native calls. Some of these are verified as never called
         // and those stubs simply allow that verification to catch any issues.
-        doNothing().when(mSigninManager).nativeSignOut(anyLong());
+        doNothing().when(mSigninManager).nativeSignOut(anyLong(), anyInt());
         doNothing().when(mSigninManager).nativeWipeProfileData(anyLong());
         doNothing().when(mSigninManager).nativeWipeGoogleServiceWorkerCaches(anyLong());
         // See verification of nativeWipeGoogleServiceWorkerCaches below.
@@ -147,7 +149,7 @@ public class SigninManagerTest {
         // nativeSignOut should only be called when signOut() is triggered on
         // the Java side of the JNI boundary. This test instead initiates sign-out
         // from the native side.
-        verify(mSigninManager, never()).nativeSignOut(anyLong());
+        verify(mSigninManager, never()).nativeSignOut(anyLong(), anyInt());
 
         // Sign-out should only clear the service worker cache when the domain is null.
         verify(mSigninManager, never()).nativeWipeProfileData(anyLong());

chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/SyncTestRule.java

@@ -18,6 +18,7 @@ import org.chromium.chrome.browser.identity.UniqueIdentificationGenerator;
 import org.chromium.chrome.browser.identity.UniqueIdentificationGeneratorFactory;
 import org.chromium.chrome.browser.identity.UuidBasedUniqueIdentificationGenerator;
 import org.chromium.chrome.browser.signin.SigninManager;
+import org.chromium.chrome.browser.signin.SignoutReason;
 import org.chromium.chrome.test.ChromeActivityTestRule;
 import org.chromium.chrome.test.util.ApplicationTestUtils;
 import org.chromium.chrome.test.util.browser.signin.SigninTestUtil;
@@ -159,7 +160,7 @@ public class SyncTestRule extends ChromeActivityTestRule<ChromeActivity> {
         ThreadUtils.runOnUiThreadBlocking(new Runnable() {
             @Override
             public void run() {
-                SigninManager.get().signOut(new Runnable() {
+                SigninManager.get().signOut(SignoutReason.SIGNOUT_TEST, new Runnable() {
                     @Override
                     public void run() {
                         s.release();

chrome/browser/android/signin/signin_manager_android.cc

@@ -212,12 +212,14 @@ void SigninManagerAndroid::OnSignInCompleted(
 }
 
 void SigninManagerAndroid::SignOut(JNIEnv* env,
-                                   const JavaParamRef<jobject>& obj) {
-  // TODO(bauerb): This is not only called for a user-triggered signout.
-  // We should pass the reason in here from Java.
-  SigninManagerFactory::GetForProfile(profile_)
-      ->SignOut(signin_metrics::USER_CLICKED_SIGNOUT_SETTINGS,
-                signin_metrics::SignoutDelete::IGNORE_METRIC);
+                                   const JavaParamRef<jobject>& obj,
+                                   jint signoutReason) {
+  SigninManagerFactory::GetForProfile(profile_)->SignOut(
+      static_cast<signin_metrics::ProfileSignout>(signoutReason),
+      // Always use IGNORE_METRIC for the profile deletion argument. Chrome
+      // Android has just a single-profile which is never deleted upon
+      // sign-out.
+      signin_metrics::SignoutDelete::IGNORE_METRIC);
 }
 
 base::android::ScopedJavaLocalRef<jstring>

chrome/browser/android/signin/signin_manager_android.h

@@ -46,7 +46,9 @@ class SigninManagerAndroid : public SigninManagerBase::Observer {
                          const base::android::JavaParamRef<jobject>& obj,
                          const base::android::JavaParamRef<jstring>& username);
 
-  void SignOut(JNIEnv* env, const base::android::JavaParamRef<jobject>& obj);
+  void SignOut(JNIEnv* env,
+               const base::android::JavaParamRef<jobject>& obj,
+               jint signoutReason);
 
   base::android::ScopedJavaLocalRef<jstring> GetManagementDomain(
       JNIEnv* env,

components/signin/core/browser/signin_metrics.h

@@ -26,6 +26,8 @@ enum DifferentPrimaryAccounts {
 };
 
 // Track all the ways a profile can become signed out as a histogram.
+// GENERATED_JAVA_ENUM_PACKAGE: org.chromium.chrome.browser.signin
+// GENERATED_JAVA_CLASS_NAME_OVERRIDE: SignoutReason
 enum ProfileSignout {
   // The value used within unit tests
   SIGNOUT_TEST = 0,