Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1256283002:
GAIA ID migration for Android (Closed)
DescriptionGAIA ID migration for Android.
Much work has been done over the last year to prepare Chrome to use Gaia obfuscated Ids as the unique identifier for Google accounts in Chrome.
This cl implements this work for Android platform. On Android platform, Google accounts' credentials are maintained in Android, Chrome list accounts name and get access tokens from Android. Chrome must use account name as identification to communicate with Android. In addition, Chrome can only fetch accounts' Gaia Ids through GoogleAuthUtil.getAccountId which is a blocked interface. These facts make Gaia Id migration a little bit challenge for Android. Please refer below doc for details.
https://docs.google.com/a/google.com/document/d/1XPVUmpm3OFU8ZbYTSxOVtxb0zOLRnRXR1OR5Pp4ryb0/edit?usp=sharing
BUG=341408
Committed: https://crrev.com/63286f25d86e92e541e611092439d4b8daface85
Cr-Commit-Position: refs/heads/master@{#348617}
Patch Set 1 : #Patch Set 2 #Patch Set 3 : #Patch Set 4 : full version #Patch Set 5 : simple version #
Total comments: 40
Patch Set 6 : full version #
Total comments: 56
Patch Set 7 : full version #
Total comments: 10
Patch Set 8 : full version #Patch Set 9 : rebase #
Total comments: 4
Patch Set 10 : full version #
Total comments: 40
Patch Set 11 : #
Total comments: 7
Patch Set 12 : rebase #Patch Set 13 : rebase & solve tests #
Total comments: 7
Patch Set 14 : address comments #Patch Set 15 : #Patch Set 16 : update #Messages
Total messages: 109 (69 generated)
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1256283002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1256283002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
gogerald@chromium.org changed reviewers: + rogerta@chromium.org
Hi Roger, Please review this draft version of my cl for Clank Gaia Id Migration. It contains the idea of how I plan to implement it. Ganggui,
Hi, Here a simple solution of this problem. Pros: It is very simple without adding any extra synchronization. Cons: Services may still use email as account id before sign in, like pre-signin profile download, policy check before sign in. All the services will use Gaia Id as account Id after sign in. Personally, I think this is acceptable. Ganggui,
Thanks Ganggui! Some initial comments. https://codereview.chromium.org/1256283002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountIdProvider.java (right): https://codereview.chromium.org/1256283002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountIdProvider.java:72: } What is the reason for moving this code? If the implementation should live here, then should probably create the instance in getInstance() and then remove the method getGoogleAccountIdProvider(). https://codereview.chromium.org/1256283002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java (right): https://codereview.chromium.org/1256283002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java:246: AccountIdProvider.setInstance(AccountIdProvider.getGoogleAccountIdProvider()); Should already be created by now. Under what cases would AccountIdProvider.getInstance() return null? https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/android... File chrome/browser/android/profiles/profile_downloader_android.cc (right): https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/android... chrome/browser/android/profiles/profile_downloader_android.cc:194: std::string accountid = This variable name is too similar with account_id. I'd suggest something like account_id_from_tracker. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/android... chrome/browser/android/profiles/profile_downloader_android.cc:195: account_tracker_service->FindAccountInfoByEmail(email).gaia; If migration state != NOT_STARTED, then FindAccountInfoByEmail() *MUST* return a non-empty account_id. When does it not do so? Seems like we should DCHECK. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/android... File chrome/browser/android/signin/signin_manager_android.cc (right): https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/android... chrome/browser/android/signin/signin_manager_android.cc:110: account_id = username_; Under what cases would we want to use username as account_id? Before migration, account_id is already an email, no need to copy. During or after migration, using username as account_id will not work. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/policy/... File chrome/browser/policy/cloud/user_policy_signin_service_mobile.cc (right): https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/policy/... chrome/browser/policy/cloud/user_policy_signin_service_mobile.cc:82: RegisterForPolicyInternal(username, "", access_token, callback); This won't work on non-android platforms. Since this file ends with _mobile.cc, I assume this only means iOS. I think we need account_id arg here too. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/policy/... chrome/browser/policy/cloud/user_policy_signin_service_mobile.cc:206: oauth2_token_service_, signin_manager()->GetAuthenticatedAccountId(), Good catch. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/policy/... File chrome/browser/policy/cloud/user_policy_signin_service_mobile.h (right): https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/policy/... chrome/browser/policy/cloud/user_policy_signin_service_mobile.h:48: // or once it is determined that |username| is not a managed account. Please add documentation for |account_id| arg. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/policy/... chrome/browser/policy/cloud/user_policy_signin_service_mobile.h:70: const std::string& accountId, Use account_id. Also in cc file. Also above function. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... File chrome/browser/signin/oauth2_token_service_delegate_android.cc (right): https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:169: account_tracker_service_ = account_tracker_service; DCHECK(account_tracker_service); ? https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:178: } Is it correct to mark migration done here? Since Java_OAuth2TokenService_validateAccounts() is now async, seems like we should wait until it is finished. Also, don't we need to wait for signin manager to finish its migration before we mark as done? Signin manager already marks the migration as complete. Also we had discussed that migration cannot begin on android until we have a mapping of all gaiaid<->username in place. It's not clear in this CL that ATS will make the right choice to begin the migration. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:183: std::string accountid = MapUserAccountIdToOs(account_id); local_account_id ? Same below. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:202: OAuth2TokenServiceDelegateAndroid::GetPersistentAccounts() { Today accounts are persisted as usernames. During the migration of O2TS on android, this should be changed to account_ids. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:222: } When does this mapping need to be done here? Before migration, accounts are persisted as usernames, and we want to return usernames. During and after migration, accounts are persisted as account_ids and we want to return account_ids. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:227: OAuth2TokenServiceDelegateAndroid::GetSystemAccounts() { Rename this to GetSystemUsernames(). The system always deals with usernames, or emails. I think this makes it less confusing with account_ids. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:245: accountid = account_id; When would MapUserAccountIdToOs() return an empty string? In these cases, I suspect it would be better for it to return the argument it was passed. That way callers don't need to check for empty and use the arg passed to MapUserAccountIdToOs(). https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:287: bool force_notifications) { I don't think we should be validating accounts with usernames. We should be validating accounts with account_ids. GetPersistentAccounts() should return account_ids. The usernames returned by GetSystemAccounts() should be mapped to account_ids, and then validation can happen with account ids. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:456: std::vector<std::string> accounts = GetPersistentAccounts(); This is case where the code assumes GetPersistentAccounts() return account_ids. However, GetPersistentAccounts() returns usernames in this CL. I think it's correct for GetPersistentAccounts() to return accounts_ids, so GetPersistentAccounts() should be changed to return them. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:471: std::string OAuth2TokenServiceDelegateAndroid::MapUserAccountIdToOs( Maybe call this function: MapAccountIdToSystemUsername() The function below could be called: MapSystemUsernameToAccountId() https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:500: &account_names); DCHECK that arrays are same length.
Patchset #6 (id:220001) has been deleted
Patchset #6 (id:240001) has been deleted
Patchset #6 (id:260001) has been deleted
Hi Roger, Please help review this updated cl for Gaia Id migration on Clank. Thanks, Ganggui, https://codereview.chromium.org/1256283002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountIdProvider.java (right): https://codereview.chromium.org/1256283002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountIdProvider.java:72: } On 2015/08/12 15:28:46, Roger Tawa wrote: > What is the reason for moving this code? If the implementation should live > here, then should probably create the instance in getInstance() and then remove > the method getGoogleAccountIdProvider(). Acknowledged. https://codereview.chromium.org/1256283002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java (right): https://codereview.chromium.org/1256283002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java:246: AccountIdProvider.setInstance(AccountIdProvider.getGoogleAccountIdProvider()); On 2015/08/12 15:28:46, Roger Tawa wrote: > Should already be created by now. Under what cases would > AccountIdProvider.getInstance() return null? AccountIdProvider.setInstance is in GoogleServiceManager which starts later than OAuth2TokenService https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/android... File chrome/browser/android/profiles/profile_downloader_android.cc (right): https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/android... chrome/browser/android/profiles/profile_downloader_android.cc:194: std::string accountid = On 2015/08/12 15:28:46, Roger Tawa wrote: > This variable name is too similar with account_id. I'd suggest something like > account_id_from_tracker. Acknowledged. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/android... chrome/browser/android/profiles/profile_downloader_android.cc:195: account_tracker_service->FindAccountInfoByEmail(email).gaia; On 2015/08/12 15:28:46, Roger Tawa wrote: > If migration state != NOT_STARTED, then FindAccountInfoByEmail() *MUST* return a > non-empty account_id. When does it not do so? Seems like we should DCHECK. Done. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/android... File chrome/browser/android/signin/signin_manager_android.cc (right): https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/android... chrome/browser/android/signin/signin_manager_android.cc:110: account_id = username_; On 2015/08/12 15:28:46, Roger Tawa wrote: > Under what cases would we want to use username as account_id? Before migration, > account_id is already an email, no need to copy. During or after migration, > using username as account_id will not work. In this simple solution, services may need OAuth2TokenService before ATS is seeded (pre-signin). Not the case in full solution. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/policy/... File chrome/browser/policy/cloud/user_policy_signin_service_mobile.cc (right): https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/policy/... chrome/browser/policy/cloud/user_policy_signin_service_mobile.cc:82: RegisterForPolicyInternal(username, "", access_token, callback); On 2015/08/12 15:28:47, Roger Tawa wrote: > This won't work on non-android platforms. Since this file ends with _mobile.cc, > I assume this only means iOS. I think we need account_id arg here too. This interface go with access_token, so we no need account id anymore. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/policy/... chrome/browser/policy/cloud/user_policy_signin_service_mobile.cc:206: oauth2_token_service_, signin_manager()->GetAuthenticatedAccountId(), On 2015/08/12 15:28:47, Roger Tawa wrote: > Good catch. Done. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/policy/... File chrome/browser/policy/cloud/user_policy_signin_service_mobile.h (right): https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/policy/... chrome/browser/policy/cloud/user_policy_signin_service_mobile.h:48: // or once it is determined that |username| is not a managed account. On 2015/08/12 15:28:47, Roger Tawa wrote: > Please add documentation for |account_id| arg. Done. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/policy/... chrome/browser/policy/cloud/user_policy_signin_service_mobile.h:70: const std::string& accountId, On 2015/08/12 15:28:47, Roger Tawa wrote: > Use account_id. Also in cc file. Also above function. Done. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... File chrome/browser/signin/oauth2_token_service_delegate_android.cc (right): https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:169: account_tracker_service_ = account_tracker_service; On 2015/08/12 15:28:47, Roger Tawa wrote: > DCHECK(account_tracker_service); ? Done. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:178: } On 2015/08/12 15:28:47, Roger Tawa wrote: > Is it correct to mark migration done here? Since > Java_OAuth2TokenService_validateAccounts() is now async, seems like we should > wait until it is finished. > > Also, don't we need to wait for signin manager to finish its migration before we > mark as done? Signin manager already marks the migration as complete. > > Also we had discussed that migration cannot begin on android until we have a > mapping of all gaiaid<->username in place. It's not clear in this CL that ATS > will make the right choice to begin the migration. Done. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:183: std::string accountid = MapUserAccountIdToOs(account_id); On 2015/08/12 15:28:47, Roger Tawa wrote: > local_account_id ? Same below. Done. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:202: OAuth2TokenServiceDelegateAndroid::GetPersistentAccounts() { On 2015/08/12 15:28:47, Roger Tawa wrote: > Today accounts are persisted as usernames. During the migration of O2TS on > android, this should be changed to account_ids. Done. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:222: } On 2015/08/12 15:28:47, Roger Tawa wrote: > When does this mapping need to be done here? Before migration, accounts are > persisted as usernames, and we want to return usernames. During and after > migration, accounts are persisted as account_ids and we want to return > account_ids. Done. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:227: OAuth2TokenServiceDelegateAndroid::GetSystemAccounts() { On 2015/08/12 15:28:47, Roger Tawa wrote: > Rename this to GetSystemUsernames(). The system always deals with usernames, or > emails. I think this makes it less confusing with account_ids. Acknowledged. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:245: accountid = account_id; On 2015/08/12 15:28:47, Roger Tawa wrote: > When would MapUserAccountIdToOs() return an empty string? In these cases, I > suspect it would be better for it to return the argument it was passed. That > way callers don't need to check for empty and use the arg passed to > MapUserAccountIdToOs(). Done. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:287: bool force_notifications) { On 2015/08/12 15:28:47, Roger Tawa wrote: > I don't think we should be validating accounts with usernames. We should be > validating accounts with account_ids. GetPersistentAccounts() should return > account_ids. The usernames returned by GetSystemAccounts() should be mapped to > account_ids, and then validation can happen with account ids. Done. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:456: std::vector<std::string> accounts = GetPersistentAccounts(); On 2015/08/12 15:28:47, Roger Tawa wrote: > This is case where the code assumes GetPersistentAccounts() return account_ids. > However, GetPersistentAccounts() returns usernames in this CL. I think it's > correct for GetPersistentAccounts() to return accounts_ids, so > GetPersistentAccounts() should be changed to return them. Done. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:471: std::string OAuth2TokenServiceDelegateAndroid::MapUserAccountIdToOs( On 2015/08/12 15:28:47, Roger Tawa wrote: > Maybe call this function: MapAccountIdToSystemUsername() > > The function below could be called: MapSystemUsernameToAccountId() Acknowledged. https://codereview.chromium.org/1256283002/diff/200001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:500: &account_names); On 2015/08/12 15:28:47, Roger Tawa wrote: > DCHECK that arrays are same length. Done.
Patchset #6 (id:280001) has been deleted
Patchset #6 (id:290001) has been deleted
Hi Ganggui, Here is a first pass review. Thanks for implementing as we discussed. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/ProfileDataCache.java (right): https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/ProfileDataCache.java:99: } Maybe better to remove the for loop and do: ProfileDownloader.startFetchingAccountInfoFor( mContext, mProfile, accounts, mImageSizePx, true); and have the function itself ignore null account usernames. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java (right): https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:60: private static class PendingProfileDownloads Do we need this class or can ProfileDownloader itself listen for system accounts being seeded? https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:61: implements AccountTrackerService.SystemAccountsSeedingObserver { Nit: I think the java naming pattern would be: OnSystemAccountsSeededListener instead of SystemAccountsSeedingObserver. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:65: private ArrayList<Profile> mProfiles; Why do we need arrays for context and profile? They will always be the same. On clank, we only support one profile in any case. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:80: sPendingProfileDownloads); Nit: I think the java naming pattern would be: addOnSystemAccountsSeededListener() instead of observeSystemAccountsSeeding(). https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:102: mImageSidePixels.get(0).intValue(), true); Maybe call nativeStartFetchingAccountInfoFor() directly here. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:112: * Starts fetching the account information for a given account. Nit: add comment for new arg. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java (right): https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:23: * layer, and notifys observers when it is complete. notifys --> notifies https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:70: sAccountIdProvider = AccountIdProvider.getInstance(); Looking at this code, it seems much better for AccountIdProvider.getInstance() to create the provider if not already set, exactly the way that AccountTrackerService.get() will create the tracker if not already set. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:105: observer.onSystemAccountsSeedingComplete(); Should the observer be removed from list? https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:109: private void seedingSystemAccounts() { Nit: seedingSystemAccounts --> seedSystemAccounts ? https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:117: mSemaphore.acquire(); I'm not sure you need a semaphore here. seedingSystemAccounts() can only be called from one thread. You could add a check for that, just like the account id provider does. Then you just need to start the async task when the first listener is added. You could also set a boolean to true at line 112 above to remember that the async task is running, and then set it back to false at line 142 below when it is done. This boolean would be needed to implement the forceRefresh() method. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java (right): https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java:238: implements AccountTrackerService.SystemAccountsSeedingObserver { Do we need this class, or could OAuth2TokenService implement AccountTrackerService.SystemAccountsSeedingObserver directly? https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java:256: } Why do we need a singleton PendingAccountsValidation? Why not make it a member of OAuth2TokenService? https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java:263: onSystemAccountsSeedingComplete(); Should mHasPendingAccountsValidation be set to false here? https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java:273: mForceNotifications); Make a shared function for this. Lines 270-273 duplicate lines 286-290. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java (right): https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:263: } Do we need this singleton, or can PendingSignin be a member of SigninManager? Or could SigninManager just implement AccountTrackerService.SystemAccountsSeedingObserver? https://codereview.chromium.org/1256283002/diff/310001/chrome/android/shell/j... File chrome/android/shell/java/src/org/chromium/chrome/shell/signin/AccountsChangedReceiver.java (right): https://codereview.chromium.org/1256283002/diff/310001/chrome/android/shell/j... chrome/android/shell/java/src/org/chromium/chrome/shell/signin/AccountsChangedReceiver.java:26: public class AccountsChangedReceiver extends BroadcastReceiver { For my own info, how is this class different from chrome/android/java/src/org/chromium/chrome/browser/services/AccountsChangedReceiver.java ? Do we use both classes? https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/android... File chrome/browser/android/signin/account_tracker_service_android.cc (right): https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/android... chrome/browser/android/signin/account_tracker_service_android.cc:30: ProfileManager::GetActiveUserProfile()); Should get profile from java code. Should pass it in the nativeInit() call I think. https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/android... chrome/browser/android/signin/account_tracker_service_android.cc:37: it != accounts_info.end(); ++it) { Use: for(auto info : accounts_info) { https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/signin/... File chrome/browser/signin/oauth2_token_service_delegate_android.cc (right): https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:183: accounts_id.push_back(*it); If this is empty there is a bug. I don't think it is correct to push the email back into the list. Should add a DCHECK. Nit: add { and } https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:249: ValidateAccountId(account_name); There is no need to map the account_id to a username for validation. Please use the account_id directly. Same for other calls to ValidateAccountId() below. https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:329: lock_.Release(); There is no need for a lock. All places that touch should_fire_refresh_token_loaded_ are called from ui thread. You can add a DCHECK for that too. (Note that you should never fire callbacks while holding a lock, this can cause deadlocks.) https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:479: should_fire_refresh_token_loaded_ = true; Why do you need two bools? I think accounts_have_been_validated_ should be enough. https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:489: return account_id; You don't need the if, it should work in any state. You can add a DCHECK that .email is not empty. https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:500: return account_name; You don't need the if, it should work in any state. You can add a DCHECK that .account_id is not empty. https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/signin/... File chrome/browser/signin/profile_oauth2_token_service_factory.cc (right): https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/signin/... chrome/browser/signin/profile_oauth2_token_service_factory.cc:55: AccountTrackerServiceFactory::GetInstance()->GetForProfile(profile)); Please pass the ATS in the ctor of OAuth2TokenServiceDelegateAndroid to match how MutableProfileOAuth2TokenServiceDelegate passes in dependent services. https://codereview.chromium.org/1256283002/diff/310001/components/signin/core... File components/signin/core/browser/account_tracker_service.h (right): https://codereview.chromium.org/1256283002/diff/310001/components/signin/core... components/signin/core/browser/account_tracker_service.h:141: friend class AccountTrackerServiceAndroid; Instead of making this a friend, let's add a new function above like: RemoveAccount(const std::string& account_id); that is called from ATSAndroid.
Patchset #7 (id:330001) has been deleted
Patchset #7 (id:350001) has been deleted
Patchset #7 (id:370001) has been deleted
Patchset #7 (id:390001) has been deleted
Patchset #7 (id:410001) has been deleted
Patchset #7 (id:430001) has been deleted
Patchset #7 (id:450001) has been deleted
Patchset #7 (id:470001) has been deleted
Patchset #7 (id:480001) has been deleted
Patchset #7 (id:500001) has been deleted
Patchset #7 (id:520001) has been deleted
Hi Roger, Please review updated solution. Haven't tested it on Device. Will do it when I am back to office. Thanks, Ganggui, https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/ProfileDataCache.java (right): https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/ProfileDataCache.java:99: } On 2015/08/14 15:01:21, Roger Tawa wrote: > Maybe better to remove the for loop and do: > > ProfileDownloader.startFetchingAccountInfoFor( > mContext, mProfile, accounts, mImageSizePx, true); > > and have the function itself ignore null account usernames. Acknowledged. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java (right): https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:60: private static class PendingProfileDownloads On 2015/08/14 15:01:21, Roger Tawa wrote: > Do we need this class or can ProfileDownloader itself listen for system accounts > being seeded? We could, but it would be a little readable to make it as a private class since it is only used internally and ProfileDownloader is a static glass without instance. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:61: implements AccountTrackerService.SystemAccountsSeedingObserver { On 2015/08/14 15:01:21, Roger Tawa wrote: > Nit: I think the java naming pattern would be: OnSystemAccountsSeededListener > instead of SystemAccountsSeedingObserver. Done. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:65: private ArrayList<Profile> mProfiles; On 2015/08/14 15:01:21, Roger Tawa wrote: > Why do we need arrays for context and profile? They will always be the same. > On clank, we only support one profile in any case. Done. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:80: sPendingProfileDownloads); On 2015/08/14 15:01:21, Roger Tawa wrote: > Nit: I think the java naming pattern would be: > addOnSystemAccountsSeededListener() instead of observeSystemAccountsSeeding(). Done. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:102: mImageSidePixels.get(0).intValue(), true); On 2015/08/14 15:01:21, Roger Tawa wrote: > Maybe call nativeStartFetchingAccountInfoFor() directly here. Done. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:112: * Starts fetching the account information for a given account. On 2015/08/14 15:01:21, Roger Tawa wrote: > Nit: add comment for new arg. Done. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java (right): https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:23: * layer, and notifys observers when it is complete. On 2015/08/14 15:01:22, Roger Tawa wrote: > notifys --> notifies Done. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:70: sAccountIdProvider = AccountIdProvider.getInstance(); On 2015/08/14 15:01:21, Roger Tawa wrote: > Looking at this code, it seems much better for AccountIdProvider.getInstance() > to create the provider if not already set, exactly the way that > AccountTrackerService.get() will create the tracker if not already set. Acknowledged. No need setInstance right now. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:105: observer.onSystemAccountsSeedingComplete(); On 2015/08/14 15:01:22, Roger Tawa wrote: > Should the observer be removed from list? Here, the calling of onSystemAccountsSeedingComplete is to make sure observer will not miss the event, the listener is not going to be removed after being added. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:109: private void seedingSystemAccounts() { On 2015/08/14 15:01:21, Roger Tawa wrote: > Nit: seedingSystemAccounts --> seedSystemAccounts ? Done. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:117: mSemaphore.acquire(); On 2015/08/14 15:01:22, Roger Tawa wrote: > I'm not sure you need a semaphore here. seedingSystemAccounts() can only be > called from one thread. You could add a check for that, just like the account > id provider does. Then you just need to start the async task when the first > listener is added. > > You could also set a boolean to true at line 112 above to remember that the > async task is running, and then set it back to false at line 142 below when it > is done. This boolean would be needed to implement the forceRefresh() method. I am considering a extreme case. Accounts on device may changed before the first async task doInBackground finished (assume it needs a very long time). So we may have two async tasks at the same time. In addition, We don't know which doInBackground finish first, semaphore makes sure first in first out. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java (right): https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java:238: implements AccountTrackerService.SystemAccountsSeedingObserver { On 2015/08/14 15:01:22, Roger Tawa wrote: > Do we need this class, or could OAuth2TokenService implement > AccountTrackerService.SystemAccountsSeedingObserver directly? Done. Move it to OAuth2TokenService to make class relationship more explicit. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java:256: } On 2015/08/14 15:01:22, Roger Tawa wrote: > Why do we need a singleton PendingAccountsValidation? Why not make it a member > of OAuth2TokenService? Done. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java:263: onSystemAccountsSeedingComplete(); On 2015/08/14 15:01:22, Roger Tawa wrote: > Should mHasPendingAccountsValidation be set to false here? onSystemAccountsSeedingComplete() will do that https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java:273: mForceNotifications); On 2015/08/14 15:01:22, Roger Tawa wrote: > Make a shared function for this. Lines 270-273 duplicate lines 286-290. Done. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java (right): https://codereview.chromium.org/1256283002/diff/310001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:263: } On 2015/08/14 15:01:22, Roger Tawa wrote: > Do we need this singleton, or can PendingSignin be a member of SigninManager? > Or could SigninManager just implement > AccountTrackerService.SystemAccountsSeedingObserver? Done. https://codereview.chromium.org/1256283002/diff/310001/chrome/android/shell/j... File chrome/android/shell/java/src/org/chromium/chrome/shell/signin/AccountsChangedReceiver.java (right): https://codereview.chromium.org/1256283002/diff/310001/chrome/android/shell/j... chrome/android/shell/java/src/org/chromium/chrome/shell/signin/AccountsChangedReceiver.java:26: public class AccountsChangedReceiver extends BroadcastReceiver { On 2015/08/14 15:01:22, Roger Tawa wrote: > For my own info, how is this class different from > chrome/android/java/src/org/chromium/chrome/browser/services/AccountsChangedReceiver.java > ? Do we use both classes? Not quite clear the differences, they may serve different entities. https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/android... File chrome/browser/android/signin/account_tracker_service_android.cc (right): https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/android... chrome/browser/android/signin/account_tracker_service_android.cc:30: ProfileManager::GetActiveUserProfile()); On 2015/08/14 15:01:22, Roger Tawa wrote: > Should get profile from java code. Should pass it in the nativeInit() call I > think. Java code get profile from C++ layer and there is only one profile on Android, so it may be OK here as we did for SigninManagerAndroid. https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/android... chrome/browser/android/signin/account_tracker_service_android.cc:37: it != accounts_info.end(); ++it) { On 2015/08/14 15:01:22, Roger Tawa wrote: > Use: > > for(auto info : accounts_info) { Done. https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/signin/... File chrome/browser/signin/oauth2_token_service_delegate_android.cc (right): https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:183: accounts_id.push_back(*it); On 2015/08/14 15:01:22, Roger Tawa wrote: > If this is empty there is a bug. I don't think it is correct to push the email > back into the list. Should add a DCHECK. > > Nit: add { and } Done. https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:249: ValidateAccountId(account_name); On 2015/08/14 15:01:22, Roger Tawa wrote: > There is no need to map the account_id to a username for validation. Please use > the account_id directly. Same for other calls to ValidateAccountId() below. Done. https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:329: lock_.Release(); On 2015/08/14 15:01:22, Roger Tawa wrote: > There is no need for a lock. All places that touch > should_fire_refresh_token_loaded_ are called from ui thread. You can add a > DCHECK for that too. > > (Note that you should never fire callbacks while holding a lock, this can cause > deadlocks.) Done. https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:479: should_fire_refresh_token_loaded_ = true; On 2015/08/14 15:01:22, Roger Tawa wrote: > Why do you need two bools? I think accounts_have_been_validated_ should be > enough. The reason is that we don't want fire refresh token loaded multiple times as on desktop platform, however validation may not finish yet when LoadCredentials() is called, so we need to tell validation to fire refresh token loaded. Change to use one variable with more status. https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:489: return account_id; On 2015/08/14 15:01:22, Roger Tawa wrote: > You don't need the if, it should work in any state. You can add a DCHECK that > .email is not empty. Acknowledged. Always over worried about the efficiency :). https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/signin/... chrome/browser/signin/oauth2_token_service_delegate_android.cc:500: return account_name; On 2015/08/14 15:01:22, Roger Tawa wrote: > You don't need the if, it should work in any state. You can add a DCHECK that > .account_id is not empty. Done. https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/signin/... File chrome/browser/signin/profile_oauth2_token_service_factory.cc (right): https://codereview.chromium.org/1256283002/diff/310001/chrome/browser/signin/... chrome/browser/signin/profile_oauth2_token_service_factory.cc:55: AccountTrackerServiceFactory::GetInstance()->GetForProfile(profile)); On 2015/08/14 15:01:22, Roger Tawa wrote: > Please pass the ATS in the ctor of OAuth2TokenServiceDelegateAndroid to match > how MutableProfileOAuth2TokenServiceDelegate passes in dependent services. Done. https://codereview.chromium.org/1256283002/diff/310001/components/signin/core... File components/signin/core/browser/account_tracker_service.h (right): https://codereview.chromium.org/1256283002/diff/310001/components/signin/core... components/signin/core/browser/account_tracker_service.h:141: friend class AccountTrackerServiceAndroid; On 2015/08/14 15:01:22, Roger Tawa wrote: > Instead of making this a friend, let's add a new function above like: > > RemoveAccount(const std::string& account_id); > > that is called from ATSAndroid. Done.
Thanks Ganggui, a few comments below. https://codereview.chromium.org/1256283002/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java (right): https://codereview.chromium.org/1256283002/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:76: if (sPendingProfileDownloads == null) { Should we check that this is the UI thread? https://codereview.chromium.org/1256283002/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java (right): https://codereview.chromium.org/1256283002/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:98: mSemaphore.acquire(); seedSystemAccounts() can only be called from UI thread. You should add a check for that. Because of this, you can check at line 91 is the state is SEEDING_NOT_STARTED. If so, set state to SEEDING_IN_PROGRESS and proceed with this function, otherwise just return. I don't think you need a semaphore for synchronization. Where is SEEDING_IN_PROGRESS set right now, I don't see it in this file. https://codereview.chromium.org/1256283002/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/SigninHelper.java (right): https://codereview.chromium.org/1256283002/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/SigninHelper.java:206: mAccountTrackerService.forceRefresh(); Nit: don't really need a member, just call AccountTrackerService.get(mContext) here directly? https://codereview.chromium.org/1256283002/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java (right): https://codereview.chromium.org/1256283002/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:262: cancelSignIn(); If the force refresh happened because of an account change, do we still want to cancel? Might be OK to leave as is though, that sounds like a really unlikely scenario. https://codereview.chromium.org/1256283002/diff/540001/chrome/browser/policy/... File chrome/browser/policy/cloud/user_policy_signin_service_mobile.cc (right): https://codereview.chromium.org/1256283002/diff/540001/chrome/browser/policy/... chrome/browser/policy/cloud/user_policy_signin_service_mobile.cc:82: RegisterForPolicyInternal(username, "", access_token, callback); I believe this needs to be fixed before commit.
Hi Roger, Please review updated CL. I have tested it on device. https://codereview.chromium.org/1256283002/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java (right): https://codereview.chromium.org/1256283002/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:76: if (sPendingProfileDownloads == null) { On 2015/08/19 14:37:26, Roger Tawa wrote: > Should we check that this is the UI thread? Done. https://codereview.chromium.org/1256283002/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java (right): https://codereview.chromium.org/1256283002/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:98: mSemaphore.acquire(); On 2015/08/19 14:37:26, Roger Tawa wrote: > seedSystemAccounts() can only be called from UI thread. You should add a check > for that. > > Because of this, you can check at line 91 is the state is SEEDING_NOT_STARTED. > If so, set state to SEEDING_IN_PROGRESS and proceed with this function, > otherwise just return. I don't think you need a semaphore for synchronization. > Where is SEEDING_IN_PROGRESS set right now, I don't see it in this file. Done. Add one more flag to get rid of semaphore. https://codereview.chromium.org/1256283002/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/SigninHelper.java (right): https://codereview.chromium.org/1256283002/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/SigninHelper.java:206: mAccountTrackerService.forceRefresh(); On 2015/08/19 14:37:26, Roger Tawa wrote: > Nit: don't really need a member, just call AccountTrackerService.get(mContext) > here directly? Acknowledged. Just make it align with OAuth2TokenService and so on. Also make class dependency a little bit clear to get the instance in SigninHelper constructor. https://codereview.chromium.org/1256283002/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java (right): https://codereview.chromium.org/1256283002/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:262: cancelSignIn(); On 2015/08/19 14:37:26, Roger Tawa wrote: > If the force refresh happened because of an account change, do we still want to > cancel? Might be OK to leave as is though, that sounds like a really unlikely > scenario. Acknowledged. Unlikely, but could happen in theory. https://codereview.chromium.org/1256283002/diff/540001/chrome/browser/policy/... File chrome/browser/policy/cloud/user_policy_signin_service_mobile.cc (right): https://codereview.chromium.org/1256283002/diff/540001/chrome/browser/policy/... chrome/browser/policy/cloud/user_policy_signin_service_mobile.cc:82: RegisterForPolicyInternal(username, "", access_token, callback); On 2015/08/19 14:37:26, Roger Tawa wrote: > I believe this needs to be fixed before commit. Acknowledged.
Patchset #9 (id:580001) has been deleted
lgtm Thanks Ganggui. https://codereview.chromium.org/1256283002/diff/600001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java (right): https://codereview.chromium.org/1256283002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:67: && !sForceRefreshed) { I don't think we need the !sForceRefreshed check here. Same for line 85. https://codereview.chromium.org/1256283002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:133: sForceRefreshed = true; Nit: rename to sForceRefresh
gogerald@chromium.org changed reviewers: + bartfab@chromium.org, nyquist@chromium.org
bartfab@chromium.org: Please review changes in chrome/browser/policy/* components/policy/* nyquist@chromium.org: Please review changes in chrome/android/* chrome/browser/android/* https://codereview.chromium.org/1256283002/diff/600001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java (right): https://codereview.chromium.org/1256283002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:67: && !sForceRefreshed) { On 2015/08/21 20:29:15, Roger Tawa wrote: > I don't think we need the !sForceRefreshed check here. Same for line 85. Done. https://codereview.chromium.org/1256283002/diff/600001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:133: sForceRefreshed = true; On 2015/08/21 20:29:15, Roger Tawa wrote: > Nit: rename to sForceRefresh Done.
Patchset #11 (id:640001) has been deleted
https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java (right): https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:65: private ArrayList<Profile> mProfiles; Could all these be final? https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:70: mProfiles = new ArrayList<Profile>(); You can just use new ArrayList<>(); https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:90: public void onSystemAccountsSeedingComplete() { Missing @Override https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:92: while (numberOfPendingRequests > 0) { numberOfPendingRequests is never updated within the loop? Were you planning on reducing it by one after each iteration? https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:95: ProfileDownloader.nativeStartFetchingAccountInfoFor(mProfiles.get(0), Just remove |ProfileDownloader.| from this. https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:96: mAccountIds.get(0), mImageSidePixels.get(0).intValue(), true); You don't need .intValue() https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:97: mProfiles.remove(0); This isn't thread safe, but I'm guessing that's not important here, since it's a private class and the get()-method is only available on the UI thread? https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:103: public void onSystemAccountsForceRefreshed() { Missing @Override https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java (right): https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:9: import android.util.Log; Use org.chromium.base.Log https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:21: private static final String TAG = "AccountTrackerService"; cr.AccountTrackerService or just cr.signin or something along those lines? https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:42: void onSystemAccountsForceRefreshed(); It's a little bit unclear from this whether the accounts have already been force refreshed or the force refresh is just now starting from this. Could you add an explanatory comment? https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:45: private static final ObserverList<OnSystemAccountsSeededListener> Could this be non-static? https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:80: public static void addSystemAccountsSeededListener(OnSystemAccountsSeededListener observer) { Why is this method static? Could this not be an instance method? https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:93: java.util.List<String> accountNamesList = accountManagerHelper.getGoogleAccountNames(); Just import this class. We don't usually fully specify package name unless it is strictly necessary (for example if class names overlap) https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:112: sSystemAccountsSeedingStatus = SystemAccountsSeedingStatus.SEEDING_DONE; This is assigning to a static from an instance of this class. Does the status field need to be static? https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:117: }.execute(); Could you specify which executor you want? AsyncTask.THREAD_POOL_EXECUTOR or something along those lines? https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:120: private void notifyObserversOnSeedingComplete() { Either make static, or (preferrably) make the observer non-static. Same below for the other publish-method. https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:134: seedSystemAccounts(); Are we guaranteed that this call will not lead to notifications to the observer that will again lead to those classes calling forceRefresh() as a result of the notification, triggering an endless loop? I was thinking about both the onSystemAccountsForceRefreshed and onSystemAccountsSeedingComplete. https://codereview.chromium.org/1256283002/diff/620001/chrome/browser/android... File chrome/browser/android/profiles/profile_downloader_android.cc (right): https://codereview.chromium.org/1256283002/diff/620001/chrome/browser/android... chrome/browser/android/profiles/profile_downloader_android.cc:193: account_tracker_service->FindAccountInfoByEmail(email).account_id, email, Is FindAccountInfoByEmail guaranteed to find the account at this point, ensuring .account_id will always be valid? https://codereview.chromium.org/1256283002/diff/620001/chrome/browser/android... File chrome/browser/android/signin/account_tracker_service_android.cc (right): https://codereview.chromium.org/1256283002/diff/620001/chrome/browser/android... chrome/browser/android/signin/account_tracker_service_android.cc:32: // Clear no longer existed accounts from AccountTrackerService first. Nit: Clear accounts no longer existing from ...
Patchset #11 (id:660001) has been deleted
Hi Tommy, please review updated CL, thanks! https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java (right): https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:65: private ArrayList<Profile> mProfiles; On 2015/08/28 19:18:25, nyquist wrote: > Could all these be final? Done. https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:70: mProfiles = new ArrayList<Profile>(); On 2015/08/28 19:18:26, nyquist wrote: > You can just use new ArrayList<>(); Done. https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:90: public void onSystemAccountsSeedingComplete() { On 2015/08/28 19:18:25, nyquist wrote: > Missing @Override Done. https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:92: while (numberOfPendingRequests > 0) { On 2015/08/28 19:18:26, nyquist wrote: > numberOfPendingRequests is never updated within the loop? Were you planning on > reducing it by one after each iteration? Done. https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:95: ProfileDownloader.nativeStartFetchingAccountInfoFor(mProfiles.get(0), On 2015/08/28 19:18:25, nyquist wrote: > Just remove |ProfileDownloader.| from this. Done. https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:96: mAccountIds.get(0), mImageSidePixels.get(0).intValue(), true); On 2015/08/28 19:18:26, nyquist wrote: > You don't need .intValue() Done. https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:97: mProfiles.remove(0); On 2015/08/28 19:18:25, nyquist wrote: > This isn't thread safe, but I'm guessing that's not important here, since it's a > private class and the get()-method is only available on the UI thread? Yes, and the observers are called in UI thread. https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/profiles/ProfileDownloader.java:103: public void onSystemAccountsForceRefreshed() { On 2015/08/28 19:18:25, nyquist wrote: > Missing @Override Done. https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java (right): https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:9: import android.util.Log; On 2015/08/28 19:18:26, nyquist wrote: > Use org.chromium.base.Log Done. https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:21: private static final String TAG = "AccountTrackerService"; On 2015/08/28 19:18:26, nyquist wrote: > cr.AccountTrackerService or just cr.signin or something along those lines? Done. https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:42: void onSystemAccountsForceRefreshed(); On 2015/08/28 19:18:26, nyquist wrote: > It's a little bit unclear from this whether the accounts have already been force > refreshed or the force refresh is just now starting from this. Could you add an > explanatory comment? Done. https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:45: private static final ObserverList<OnSystemAccountsSeededListener> On 2015/08/28 19:18:26, nyquist wrote: > Could this be non-static? Done. Yes, we could, the original thinking is to make observer could be added without instance and we only have one instance. https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:80: public static void addSystemAccountsSeededListener(OnSystemAccountsSeededListener observer) { On 2015/08/28 19:18:26, nyquist wrote: > Why is this method static? Could this not be an instance method? Done. https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:93: java.util.List<String> accountNamesList = accountManagerHelper.getGoogleAccountNames(); On 2015/08/28 19:18:26, nyquist wrote: > Just import this class. We don't usually fully specify package name unless it is > strictly necessary (for example if class names overlap) Done. https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:112: sSystemAccountsSeedingStatus = SystemAccountsSeedingStatus.SEEDING_DONE; On 2015/08/28 19:18:26, nyquist wrote: > This is assigning to a static from an instance of this class. Does the status > field need to be static? Acknowledged. https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:117: }.execute(); On 2015/08/28 19:18:26, nyquist wrote: > Could you specify which executor you want? AsyncTask.THREAD_POOL_EXECUTOR or > something along those lines? Done. https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:120: private void notifyObserversOnSeedingComplete() { On 2015/08/28 19:18:26, nyquist wrote: > Either make static, or (preferrably) make the observer non-static. Same below > for the other publish-method. Acknowledged. https://codereview.chromium.org/1256283002/diff/620001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:134: seedSystemAccounts(); On 2015/08/28 19:18:26, nyquist wrote: > Are we guaranteed that this call will not lead to notifications to the observer > that will again lead to those classes calling forceRefresh() as a result of the > notification, triggering an endless loop? I was thinking about both the > onSystemAccountsForceRefreshed and onSystemAccountsSeedingComplete. Unlikely. First, force refresh is only happened when system accounts changed or user manually signed out. Both of them are not triggered by observer. Secondly, all these observers are called on UI thread and should clean their local status after processing OnSystemAccountsForceRefreshed/notifyObserversOnForceRefreshed notification. If the endless loop happened, then there is definitely a usage error. https://codereview.chromium.org/1256283002/diff/620001/chrome/browser/android... File chrome/browser/android/profiles/profile_downloader_android.cc (right): https://codereview.chromium.org/1256283002/diff/620001/chrome/browser/android... chrome/browser/android/profiles/profile_downloader_android.cc:193: account_tracker_service->FindAccountInfoByEmail(email).account_id, email, On 2015/08/28 19:18:26, nyquist wrote: > Is FindAccountInfoByEmail guaranteed to find the account at this point, ensuring > .account_id will always be valid? Yes, it is guaranteed, since ProfileDownloader.java has checked the status with AccountTrackerSerice.isSystemAccountsSeeded(). https://codereview.chromium.org/1256283002/diff/620001/chrome/browser/android... File chrome/browser/android/signin/account_tracker_service_android.cc (right): https://codereview.chromium.org/1256283002/diff/620001/chrome/browser/android... chrome/browser/android/signin/account_tracker_service_android.cc:32: // Clear no longer existed accounts from AccountTrackerService first. On 2015/08/28 19:18:26, nyquist wrote: > Nit: Clear accounts no longer existing from ... Done.
chrome/browser/policy/* components/policy/* LGTM Sorry for the delay. I was traveling.
The code lgtm, but it feels like this core piece of the signin-infrastructure should be well tested. Would it be possible to add tests to the AccountTrackerService in a follow up CL? https://codereview.chromium.org/1256283002/diff/680001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java (right): https://codereview.chromium.org/1256283002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:27: private boolean mForceRefresh = false; Nit: You don't need to initialize class members in Java, so remove |= false| https://codereview.chromium.org/1256283002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:50: new ObserverList<OnSystemAccountsSeededListener>(); Nit: Just new ObserverList<>(); https://codereview.chromium.org/1256283002/diff/680001/chrome/android/shell/j... File chrome/android/shell/java/src/org/chromium/chrome/shell/signin/AccountsChangedReceiver.java (right): https://codereview.chromium.org/1256283002/diff/680001/chrome/android/shell/j... chrome/android/shell/java/src/org/chromium/chrome/shell/signin/AccountsChangedReceiver.java:39: AccountTrackerService.get(context).forceRefresh(); Do you need to import org.chromium.chrome.browser.signin.AccountTrackerService to get chrome_shell_apk to compile?
The CQ bit was checked by gogerald@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org, bartfab@chromium.org, nyquist@chromium.org Link to the patchset: https://codereview.chromium.org/1256283002/#ps700001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1256283002/700001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1256283002/700001
Yes, definitely, integration tests will be introduced in the upcoming CL. On Wed, Sep 2, 2015 at 5:18 PM, <nyquist@chromium.org> wrote: > The code lgtm, but it feels like this core piece of the > signin-infrastructure > should be well tested. Would it be possible to add tests to the > AccountTrackerService in a follow up CL? > > > > https://codereview.chromium.org/1256283002/diff/680001/chrome/android/java/sr... > File > > chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java > (right): > > > https://codereview.chromium.org/1256283002/diff/680001/chrome/android/java/sr... > > chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:27: > private boolean mForceRefresh = false; > Nit: You don't need to initialize class members in Java, so remove |= > false| > > > https://codereview.chromium.org/1256283002/diff/680001/chrome/android/java/sr... > > chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:50: > new ObserverList<OnSystemAccountsSeededListener>(); > Nit: Just new ObserverList<>(); > > > https://codereview.chromium.org/1256283002/diff/680001/chrome/android/shell/j... > File > > chrome/android/shell/java/src/org/chromium/chrome/shell/signin/AccountsChangedReceiver.java > (right): > > > https://codereview.chromium.org/1256283002/diff/680001/chrome/android/shell/j... > > chrome/android/shell/java/src/org/chromium/chrome/shell/signin/AccountsChangedReceiver.java:39: > AccountTrackerService.get(context).forceRefresh(); > Do you need to import > org.chromium.chrome.browser.signin.AccountTrackerService to get > chrome_shell_apk to compile? > > https://codereview.chromium.org/1256283002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gogerald@chromium.org
The CQ bit was unchecked by gogerald@chromium.org
The CQ bit was checked by gogerald@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1256283002/700001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1256283002/700001
The CQ bit was unchecked by gogerald@chromium.org
Patchset #12 (id:700001) has been deleted
Patchset #14 (id:760001) has been deleted
The CQ bit was checked by gogerald@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org, nyquist@chromium.org, rogerta@chromium.org Link to the patchset: https://codereview.chromium.org/1256283002/#ps800001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1256283002/800001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1256283002/800001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by gogerald@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1256283002/800001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1256283002/800001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
Patchset #16 (id:820001) has been deleted
Patchset #17 (id:860001) has been deleted
Patchset #17 (id:880001) has been deleted
Patchset #18 (id:920001) has been deleted
Patchset #18 (id:940001) has been deleted
Patchset #13 (id:740001) has been deleted
Patchset #13 (id:780001) has been deleted
Patchset #13 (id:800001) has been deleted
Patchset #13 (id:840001) has been deleted
Patchset #12 (id:720001) has been deleted
Patchset #13 (id:960001) has been deleted
gogerald@chromium.org changed reviewers: - bartfab@chromium.org, rogerta@chromium.org
Hi Tommy, I did few changes of this CL for tests, focus in below files. Please help take another look. AccountTrackerService.java 1), Add one more interface and flag for test syncForceRefreshForTest(), mSyncForceRefreshedForTest. 2). Add a new private interface to check whether fetched accountIds are valid since AccountIdProvider may not be available in special situation. OAuth2TokenServiceIntegrationTest.java OAuth2TokenServiceTest.java SyncTestBase.java Thanks, Ganggui, https://codereview.chromium.org/1256283002/diff/680001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java (right): https://codereview.chromium.org/1256283002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:27: private boolean mForceRefresh = false; On 2015/09/02 21:18:16, nyquist wrote: > Nit: You don't need to initialize class members in Java, so remove |= false| Done. https://codereview.chromium.org/1256283002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:50: new ObserverList<OnSystemAccountsSeededListener>(); On 2015/09/02 21:18:16, nyquist wrote: > Nit: Just new ObserverList<>(); Done. https://codereview.chromium.org/1256283002/diff/680001/chrome/android/shell/j... File chrome/android/shell/java/src/org/chromium/chrome/shell/signin/AccountsChangedReceiver.java (right): https://codereview.chromium.org/1256283002/diff/680001/chrome/android/shell/j... chrome/android/shell/java/src/org/chromium/chrome/shell/signin/AccountsChangedReceiver.java:39: AccountTrackerService.get(context).forceRefresh(); On 2015/09/02 21:18:16, nyquist wrote: > Do you need to import org.chromium.chrome.browser.signin.AccountTrackerService > to get chrome_shell_apk to compile? Done.
Where did the change to AccountsChangedReceiver.java go? Also, please update the first line of the issue description to be: 'GAIA ID migration for Android' It's probably helpful to also change the subject of the issue, but the first line of the CL description is the one that goes into the git commit, so that's the important part. Secondly, please clarify more about this why this CL is important in the CL description. You can keep what you have now, but also add things like: - How did this work before? - What is special about the Android port? As in; in what way does it behave differently than other platforms? - Why does some things live in SigninManager vs OAuth2TokenService vs AccountTrackerService, etc. Having a clear CL description helps future readers of the code to understand why decisions where made. https://codereview.chromium.org/1256283002/diff/680001/chrome/android/shell/j... File chrome/android/shell/java/src/org/chromium/chrome/shell/signin/AccountsChangedReceiver.java (right): https://codereview.chromium.org/1256283002/diff/680001/chrome/android/shell/j... chrome/android/shell/java/src/org/chromium/chrome/shell/signin/AccountsChangedReceiver.java:39: AccountTrackerService.get(context).forceRefresh(); On 2015/09/10 23:43:25, gogerald1 wrote: > On 2015/09/02 21:18:16, nyquist wrote: > > Do you need to import org.chromium.chrome.browser.signin.AccountTrackerService > > to get chrome_shell_apk to compile? > > Done. Where did this line go? Just removed? https://codereview.chromium.org/1256283002/diff/980001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java (right): https://codereview.chromium.org/1256283002/diff/980001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:116: if (!mSyncForceRefreshedForTest) { Just do: if (mSyncForceRefreshedForTest) return; Otherwise the indent gets really long. https://codereview.chromium.org/1256283002/diff/980001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:118: seedSystemAccounts(); Optional nit: How about just: if (mForceRefresh) { seedSystemAccounts(); return; } That would remove another indent level and clarify to the reader that nothing else will happen if a refresh was forced. https://codereview.chromium.org/1256283002/diff/980001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:135: private boolean isAccountIdsValid(String[] accountIds) { Plural, so areAccountsIdsValid https://codereview.chromium.org/1256283002/diff/980001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:136: boolean isValid = true; Remove this and just return true in the end of the method. https://codereview.chromium.org/1256283002/diff/980001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/AccountTrackerService.java:138: if (accountIds[i] == null) { Just: if (accountIds[i] == null) return false; https://codereview.chromium.org/1256283002/diff/980001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceIntegrationTest.java (right): https://codereview.chromium.org/1256283002/diff/980001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceIntegrationTest.java:100: private void seedAccountTrackerService(Context context) { Add final to this argument https://codereview.chromium.org/1256283002/diff/980001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceIntegrationTest.java:101: final Context mContext = context; remove this since it's not necessary. Also, for future reference, the m-prefix is only for instance members.
Patchset #15 (id:1020001) has been deleted
Patchset #15 (id:1040001) has been deleted
The CQ bit was checked by gogerald@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org, nyquist@chromium.org, rogerta@chromium.org Link to the patchset: https://codereview.chromium.org/1256283002/#ps1080001 (title: "update")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1256283002/1080001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1256283002/1080001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gogerald@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1256283002/1080001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1256283002/1080001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gogerald@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1256283002/1080001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1256283002/1080001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gogerald@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1256283002/1080001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1256283002/1080001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gogerald@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1256283002/1080001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1256283002/1080001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gogerald@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1256283002/1080001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1256283002/1080001
Message was sent while issue was closed.
Committed patchset #16 (id:1080001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/63286f25d86e92e541e611092439d4b8daface85 Cr-Commit-Position: refs/heads/master@{#348617}
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/63286f25d86e92e541e611092439d4b8daface85 Cr-Commit-Position: refs/heads/master@{#348617} |
