Does  crbug.com/887445  mean that it has been determined that it belongs outside of the IdentityManager? Asking because I was about to work on that bug.

Thanks for noticing this, Mario! No, we need to resolve this issue before (potentially) tackling that bug.

I investigated this a bit.

It looks like SigninManagerAndroid is a wrapper around SigninManager (and PO2TS) to allow calling its method from Java. The plan is thus the following:

1. convert SigninManagerAndroid to use IdentityManager API,
2. (eventually) export IdentityManager API to Java (i.e. IdentityManagerAndroid),
3. (long term) deprecate and remove SigninManagerAndroid.

This bug will thus track 1. which will unblock the identity servicification effort.

 Issue 887445  has been merged into this issue.

We need also a mapping for SigninManager::OnExternalSigninCompleted() in order to completely move away from SigninManager (crbug.com/889902)

CL here: https://chromium-review.googlesource.com/c/chromium/src/+/1333390

@blundell @sdefresne: As you can see in the CL, the proposed patch there doesn't completely migrate away from SigninManager due to OnExternalSigninCompleted(), which I assume it's fine for now since that can be done later on as a follow-up CL.

However, it also raises the question on how to migrate away from PO2TS and AccountTrackerService:

For AccountTrackerService, there is at the moment no APIs in the IdentityManager equivalent to AccountTrackerService::FindAccountInfoByEmail(). We could add GetAccountInfoForEmailWithRefreshToken(), but I'm not sure of whether it's a good idea to add another method like that considering that retrieving by email might not be as common as retrieving by account id?

For now, I've implemented a private utility function FindAccountInfoByEmail() in signin_manager_android.cc that simply uses GetAccountsWithRefreshTokens() to search for the target AccountInfo, please let me know what you think.


As for O2TS, I'm afraid I didn't come up with a solution yet, since the code to migrate is slightly more complex (including below my version, ported to IdentityManager):

    void SigninManagerAndroid::LogInSignedInUser(JNIEnv* env,
                                                 const JavaParamRef<jobject>& obj) {
      identity::IdentityManager* identity_manager =
          IdentityManagerFactory::GetForProfile(profile_);

      ProfileOAuth2TokenService* token_service =
          ProfileOAuth2TokenServiceFactory::GetForProfile(profile_);
      const std::string& primary_acct = identity_manager->GetPrimaryAccountId();
    
      static_cast<OAuth2TokenServiceDelegateAndroid*>(token_service->GetDelegate())
          ->ValidateAccounts(primary_acct, true);
    }

Any suggestion on how to migrate this OAuth2TokenServiceDelegateAndroid::ValidateAccounts() bit?

Thanks, and sorry for the long note here

Hi Mario,

Thank you for the detailed analysis!

Re: AccountTrackerService, let's leave it as-is for now. In the near term we'll be tackling the migration of AccountTrackerService in earnest, and we'll solve that challenge.

Re: the usage of OAuth2TokenService::GetDelegate(), I agree that that is really hairy. I will analyze that separately (I will shortly create a blocking bug for it).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/487f0f4e635c69b413e2ee79489805da0050a842

commit 487f0f4e635c69b413e2ee79489805da0050a842
Author: Mario Sanchez Prada <mario@igalia.com>
Date: Wed Nov 14 18:38:14 2018

Migrate SigninManagerAndroid to the IdentityManager

Move away from SigninManager[Base] in favor of the IdentityManager
for every single API call except for OnExternalSigninCompleted(),
which hasn't got mapped yet to the IdentityManager (crbug.com/889902).

Bug:  882464 
Change-Id: I68c8b4845caaed59dd9b2650da977fac74be06b4
Reviewed-on: https://chromium-review.googlesource.com/c/1333390
Commit-Queue: Mario Sanchez Prada <mario@igalia.com>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Ganggui Tang <gogerald@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608059}
[modify] https://crrev.com/487f0f4e635c69b413e2ee79489805da0050a842/chrome/browser/BUILD.gn
[modify] https://crrev.com/487f0f4e635c69b413e2ee79489805da0050a842/chrome/browser/android/signin/signin_manager_android.cc
[modify] https://crrev.com/487f0f4e635c69b413e2ee79489805da0050a842/chrome/browser/android/signin/signin_manager_android.h