Skip to content

Commit

Permalink
[Android] Replace usage of deprecated SigninManager.signin() method
Browse files Browse the repository at this point in the history
This cl replaces the deprecated signin() method in SigninManager that uses Android account with the new signin() method that uses CoreAccountInfo.

Low-Coverage-Reason: TRIVIAL_CHANGE
Bug: 1462264
Change-Id: I856ac24233c3c0ae8269cdbfa79369f4b24e2175
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4876884
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Reviewed-by: Alex Ilin <alexilin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1243969}
  • Loading branch information
Tanmoy Mollik committed Jan 8, 2024
1 parent bf39f12 commit d55055d
Show file tree
Hide file tree
Showing 16 changed files with 123 additions and 136 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import static org.chromium.ui.test.util.MockitoHelper.doCallback;
import static org.chromium.ui.test.util.MockitoHelper.doRunnable;

import android.accounts.Account;
import android.app.Activity;
import android.content.res.Configuration;
import android.graphics.drawable.ColorDrawable;
Expand Down Expand Up @@ -95,7 +94,6 @@
import org.chromium.chrome.test.util.browser.signin.SigninTestUtil;
import org.chromium.components.browser_ui.styles.SemanticColorUtils;
import org.chromium.components.externalauth.ExternalAuthUtils;
import org.chromium.components.signin.AccountUtils;
import org.chromium.components.signin.base.CoreAccountInfo;
import org.chromium.components.signin.identitymanager.ConsentLevel;
import org.chromium.components.signin.identitymanager.IdentityManager;
Expand Down Expand Up @@ -358,7 +356,8 @@ public void testFragmentWhenSigninIsDisabledByPolicy() {
@Test
@MediumTest
public void testFragmentWhenSigninErrorOccurs() {
mSigninTestRule.addAccount(TEST_EMAIL1, FULL_NAME1, GIVEN_NAME1, null);
CoreAccountInfo coreAccountInfo =
mSigninTestRule.addAccount(TEST_EMAIL1, FULL_NAME1, GIVEN_NAME1, null);
IdentityServicesProvider.setInstanceForTests(mIdentityServicesProviderMock);
TestThreadUtils.runOnUiThreadBlocking(
() -> {
Expand All @@ -373,7 +372,7 @@ public void testFragmentWhenSigninErrorOccurs() {
});
doCallback(/* index= */ 2, (SignInCallback callback) -> callback.onSignInAborted())
.when(mSigninManagerMock)
.signin(eq(AccountUtils.createAccountFromName(TEST_EMAIL1)), anyInt(), any());
.signin(eq(coreAccountInfo), anyInt(), any());
launchActivityWithFragment();
checkFragmentWithSelectedAccount(TEST_EMAIL1, FULL_NAME1, GIVEN_NAME1);

Expand Down Expand Up @@ -706,7 +705,7 @@ public void testContinueButtonWithTheSignedInAccount() {
.getString(R.string.sync_promo_continue_as, GIVEN_NAME1);
clickContinueButton(continueAsText);

verify(mSigninManagerMock, never()).signin(any(Account.class), anyInt(), any());
verify(mSigninManagerMock, never()).signin(any(CoreAccountInfo.class), anyInt(), any());
verify(mFirstRunPageDelegateMock).acceptTermsOfService(true);
verify(mFirstRunPageDelegateMock).advanceToNextPage();
}
Expand Down Expand Up @@ -1322,9 +1321,9 @@ private void checkContinueButtonWithChildAccount(boolean hasFullNameInButtonText
verify(mFirstRunPageDelegateMock).advanceToNextPage();

// Sign-in isn't processed by SigninFirstRunFragment for child accounts.
verify(mSigninManagerMock, never()).signin(any(Account.class), anyInt(), any());
verify(mSigninManagerMock, never()).signin(any(CoreAccountInfo.class), anyInt(), any());
verify(mSigninManagerMock, never())
.signinAndEnableSync(any(Account.class), anyInt(), any());
.signinAndEnableSync(any(CoreAccountInfo.class), anyInt(), any());
}

private void checkFragmentWhenSigninIsDisabledByPolicy() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -333,10 +333,7 @@ public void signinNoTurnSyncOn() {
assertTrue(mSigninManager.isSyncOptInAllowed());

SigninManager.SignInCallback callback = mock(SigninManager.SignInCallback.class);
mSigninManager.signin(
AccountUtils.createAccountFromName(ACCOUNT_INFO.getEmail()),
SigninAccessPoint.START_PAGE,
callback);
mSigninManager.signin(ACCOUNT_INFO, SigninAccessPoint.START_PAGE, callback);

// Signin without turning on sync shouldn't apply policies.
verify(mNativeMock, never()).fetchAndApplyCloudPolicy(anyLong(), any(), any());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
// found in the LICENSE file.
package org.chromium.chrome.browser.feed.signinbottomsheet;

import android.accounts.Account;
import android.view.View;

import androidx.annotation.Nullable;
Expand All @@ -22,9 +21,8 @@
import org.chromium.chrome.browser.ui.signin.account_picker.AccountPickerDelegate;
import org.chromium.components.browser_ui.bottomsheet.BottomSheetController;
import org.chromium.components.browser_ui.device_lock.DeviceLockActivityLauncher;
import org.chromium.components.signin.AccountUtils;
import org.chromium.components.signin.base.CoreAccountInfo;
import org.chromium.components.signin.base.GoogleServiceAuthError;
import org.chromium.components.signin.identitymanager.AccountInfoServiceProvider;
import org.chromium.components.signin.metrics.SigninAccessPoint;
import org.chromium.ui.base.WindowAndroid;
import org.chromium.ui.widget.Toast;
Expand Down Expand Up @@ -67,8 +65,7 @@ public void destroy() {}

@Override
public void signIn(
String accountEmail, Callback<GoogleServiceAuthError> onSignInErrorCallback) {
Account account = AccountUtils.createAccountFromName(accountEmail);
CoreAccountInfo accountInfo, Callback<GoogleServiceAuthError> onSignInErrorCallback) {
SigninManager.SignInCallback callback =
new SigninManager.SignInCallback() {
@Override
Expand Down Expand Up @@ -97,17 +94,12 @@ public void onSignInAborted() {
}
};

AccountInfoServiceProvider.get()
.getAccountInfoByEmail(accountEmail)
.then(
accountInfo -> {
if (mSigninManager.isSigninAllowed()) {
mSigninManager.signin(account, mSigninAccessPoint, callback);
} else {
makeSigninNotAllowedToast();
mController.hideContent(mController.getCurrentSheetContent(), true);
}
});
if (mSigninManager.isSigninAllowed()) {
mSigninManager.signin(accountInfo, mSigninAccessPoint, callback);
} else {
makeSigninNotAllowedToast();
mController.hideContent(mController.getCurrentSheetContent(), true);
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
import org.chromium.chrome.browser.ui.signin.account_picker.AccountPickerBottomSheetCoordinator;
import org.chromium.chrome.test.util.browser.signin.AccountManagerTestRule;
import org.chromium.components.browser_ui.bottomsheet.BottomSheetController;
import org.chromium.components.signin.AccountUtils;
import org.chromium.components.signin.base.CoreAccountInfo;
import org.chromium.components.signin.metrics.SigninAccessPoint;
import org.chromium.ui.base.WindowAndroid;

Expand Down Expand Up @@ -62,13 +62,15 @@ public class SigninBottomSheetCoordinatorTest {

private SigninBottomSheetCoordinator mSigninCoordinator;

private CoreAccountInfo mCoreAccountInfo;

@Before
public void setUp() {
IdentityServicesProvider.setInstanceForTests(mock(IdentityServicesProvider.class));
when(IdentityServicesProvider.get().getSigninManager(mProfileMock))
.thenReturn(mSigninManagerMock);
when(mSigninManagerMock.isSigninAllowed()).thenReturn(true);
mAccountManagerTestRule.addAccount(TEST_EMAIL);
mCoreAccountInfo = mAccountManagerTestRule.addAccount(TEST_EMAIL);
mSigninCoordinator =
new SigninBottomSheetCoordinator(
mWindowAndroidMock,
Expand All @@ -93,13 +95,13 @@ public void testSigninCompleted_callsSigninManagerAndUpdatesHistogram() {
})
.when(mSigninManagerMock)
.signin(
eq(AccountUtils.createAccountFromName(TEST_EMAIL)),
eq(mCoreAccountInfo),
eq(SigninAccessPoint.NTP_FEED_CARD_MENU_PROMO),
any());
mSigninCoordinator.signIn(TEST_EMAIL, error -> {});
mSigninCoordinator.signIn(mCoreAccountInfo, error -> {});
verify(mSigninManagerMock)
.signin(
eq(AccountUtils.createAccountFromName(TEST_EMAIL)),
eq(mCoreAccountInfo),
eq(SigninAccessPoint.NTP_FEED_CARD_MENU_PROMO),
any(SigninManager.SignInCallback.class));
histogramWatcher.assertExpected();
Expand All @@ -118,12 +120,12 @@ public void testSigninAborted_doesNotUpdateHistogram() {
})
.when(mSigninManagerMock)
.signin(
eq(AccountUtils.createAccountFromName(TEST_EMAIL)),
eq(mCoreAccountInfo),
eq(SigninAccessPoint.NTP_FEED_CARD_MENU_PROMO),
any());
mSigninCoordinator.setAccountPickerBottomSheetCoordinator(
mAccountPickerBottomSheetCoordinatorMock);
mSigninCoordinator.signIn(TEST_EMAIL, error -> {});
mSigninCoordinator.signIn(mCoreAccountInfo, error -> {});
histogramWatcher.assertExpected();
}

Expand All @@ -135,9 +137,8 @@ public void testSignInNotAllowed() {
SigninAccessPoint.NTP_FEED_CARD_MENU_PROMO);
when(mSigninManagerMock.isSigninAllowed()).thenReturn(false);
mSigninCoordinator.setToastOverrideForTesting();
mSigninCoordinator.signIn(TEST_EMAIL, error -> {});
verify(mSigninManagerMock, never())
.signin(eq(AccountUtils.createAccountFromName(TEST_EMAIL)), anyInt(), any());
mSigninCoordinator.signIn(mCoreAccountInfo, error -> {});
verify(mSigninManagerMock, never()).signin(eq(mCoreAccountInfo), anyInt(), any());
watchSigninDisabledToastShownHistogram.assertExpected();
}

Expand All @@ -159,11 +160,8 @@ public void testSigninCompleted_callSigninSuccessCallback() {
return null;
})
.when(mSigninManagerMock)
.signin(
eq(AccountUtils.createAccountFromName(TEST_EMAIL)),
eq(SigninAccessPoint.NTP_FEED_BOTTOM_PROMO),
any());
coordinator.signIn(TEST_EMAIL, error -> {});
.signin(eq(mCoreAccountInfo), eq(SigninAccessPoint.NTP_FEED_BOTTOM_PROMO), any());
coordinator.signIn(mCoreAccountInfo, error -> {});
verify(mOnSigninSuccessCallbackMock, times(1)).run();
}

Expand All @@ -185,13 +183,10 @@ public void testSigninAborted_doesNotCallSigninSuccessCallback() {
return null;
})
.when(mSigninManagerMock)
.signin(
eq(AccountUtils.createAccountFromName(TEST_EMAIL)),
eq(SigninAccessPoint.NTP_FEED_BOTTOM_PROMO),
any());
.signin(eq(mCoreAccountInfo), eq(SigninAccessPoint.NTP_FEED_BOTTOM_PROMO), any());
coordinator.setAccountPickerBottomSheetCoordinator(
mAccountPickerBottomSheetCoordinatorMock);
coordinator.signIn(TEST_EMAIL, error -> {});
coordinator.signIn(mCoreAccountInfo, error -> {});
verify(mOnSigninSuccessCallbackMock, times(0)).run();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

package org.chromium.chrome.browser.share.send_tab_to_self;

import android.accounts.Account;
import android.content.Context;

import androidx.annotation.StringRes;
Expand All @@ -22,7 +21,7 @@
import org.chromium.components.browser_ui.bottomsheet.BottomSheetController;
import org.chromium.components.browser_ui.bottomsheet.EmptyBottomSheetObserver;
import org.chromium.components.browser_ui.device_lock.DeviceLockActivityLauncher;
import org.chromium.components.signin.AccountUtils;
import org.chromium.components.signin.base.CoreAccountInfo;
import org.chromium.components.signin.base.GoogleServiceAuthError;
import org.chromium.components.signin.metrics.SigninAccessPoint;
import org.chromium.components.sync.SyncService;
Expand Down Expand Up @@ -115,11 +114,11 @@ public void destroy() {}

@Override
public void signIn(
String accountEmail, Callback<GoogleServiceAuthError> onSignInErrorCallback) {
CoreAccountInfo accountInfo,
Callback<GoogleServiceAuthError> onSignInErrorCallback) {
SigninManager signinManager = IdentityServicesProvider.get().getSigninManager(mProfile);
Account account = AccountUtils.createAccountFromName(accountEmail);
signinManager.signin(
account,
accountInfo,
SigninAccessPoint.SEND_TAB_TO_SELF_PROMO,
new SigninManager.SignInCallback() {
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import androidx.annotation.Nullable;

import org.chromium.base.Callback;
import org.chromium.base.Log;
import org.chromium.base.Promise;
import org.chromium.base.TraceEvent;
Expand Down Expand Up @@ -147,13 +148,13 @@ private void validatePrimaryAccountExists(
}
// Check whether the primary account is renamed to another account when it is not on device
AccountRenameChecker.get()
.getNewNameOfRenamedAccountAsync(oldAccount.getEmail(), coreAccountInfos)
.getNewEmailOfRenamedAccountAsync(oldAccount.getEmail(), coreAccountInfos)
.then(
newAccountName -> {
if (newAccountName != null) {
newAccountEmail -> {
if (newAccountEmail != null) {
// Sign in to the new account if the current primary account is
// renamed to a new account.
resigninAfterAccountRename(newAccountName, oldSyncConsent);
resigninAfterAccountRename(newAccountEmail, oldSyncConsent);
} else {
// Sign out if the current primary account is not renamed
mSigninManager.runAfterOperationInProgress(
Expand All @@ -165,17 +166,16 @@ private void validatePrimaryAccountExists(
});
}

private void resigninAfterAccountRename(String newAccountName, boolean shouldEnableSync) {
private void resigninAfterAccountRename(String newAccountEmail, boolean shouldEnableSync) {
if (SigninFeatureMap.isEnabled(SigninFeatures.SEED_ACCOUNTS_REVAMP)) {
throw new IllegalStateException(
"This method should never be called when SEED_ACCOUNTS_REVAMP is enabled");
}
mSigninManager.signOut(
SignoutReason.ACCOUNT_EMAIL_UPDATED,
() -> {
Callback<CoreAccountInfo> resigninCallback =
coreAccountInfo -> {
if (shouldEnableSync) {
mSigninManager.signinAndEnableSync(
AccountUtils.createAccountFromName(newAccountName),
coreAccountInfo,
SigninAccessPoint.ACCOUNT_RENAMED,
new SignInCallback() {
@Override
Expand All @@ -189,11 +189,16 @@ public void onSignInAborted() {}
});
} else {
mSigninManager.signin(
AccountUtils.createAccountFromName(newAccountName),
SigninAccessPoint.ACCOUNT_RENAMED,
null);
coreAccountInfo, SigninAccessPoint.ACCOUNT_RENAMED, null);
}
},
};
CoreAccountInfo accountInfo =
AccountUtils.findCoreAccountInfoByEmail(
mAccountManagerFacade.getCoreAccountInfos().getResult(), newAccountEmail);
assert accountInfo != null;
mSigninManager.signOut(
SignoutReason.ACCOUNT_EMAIL_UPDATED,
() -> resigninCallback.onResult(accountInfo),
false);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ class AccountPickerBottomSheetMediator
private final AccountManagerFacade mAccountManagerFacade;
private final DeviceLockActivityLauncher mDeviceLockActivityLauncher;

// TODO(crbug.com/1515277): Use CoreAccountInfo here instead.
private @Nullable String mSelectedAccountEmail;
private @Nullable String mDefaultAccountEmail;
private @Nullable String mAddedAccountEmail;
Expand Down Expand Up @@ -294,7 +295,14 @@ private void signIn() {
.clearWebSigninAccountPickerActiveDismissalCount();
}

mAccountPickerDelegate.signIn(mSelectedAccountEmail, this::onSigninFailed);
CoreAccountInfo accountInfo =
AccountUtils.findCoreAccountInfoByEmail(
mAccountManagerFacade.getCoreAccountInfos().getResult(),
mSelectedAccountEmail);
if (accountInfo == null) {
return;
}
mAccountPickerDelegate.signIn(accountInfo, this::onSigninFailed);
}

private void onSigninFailed(GoogleServiceAuthError error) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import org.chromium.chrome.test.ChromeTabbedActivityTestRule;
import org.chromium.chrome.test.util.browser.signin.AccountManagerTestRule;
import org.chromium.components.browser_ui.bottomsheet.BottomSheetController;
import org.chromium.components.signin.base.CoreAccountInfo;
import org.chromium.components.signin.base.GoogleServiceAuthError;
import org.chromium.components.signin.base.GoogleServiceAuthError.State;
import org.chromium.content_public.browser.test.util.TestThreadUtils;
Expand Down Expand Up @@ -89,7 +90,8 @@ public void destroy() {}

@Override
public void signIn(
String accountEmail, Callback<GoogleServiceAuthError> onSignInErrorCallback) {
CoreAccountInfo accountInfo,
Callback<GoogleServiceAuthError> onSignInErrorCallback) {
if (mError != null) {
onSignInErrorCallback.onResult(mError);
}
Expand Down

0 comments on commit d55055d

Please sign in to comment.