Streamline IdentityManager's interaction with its backing classes |
||
Issue descriptionWe originally designed the implementation of IdentityManager to mimic its eventual interaction with the Identity Service, having it interact with its backing classes from //components/signin asynchronously and cache information. However, this is proving difficult and complex to maintain as the rest of the codebase continues to interact with these classes synchronously; it has been the source of several subtle race conditions and bugs. In the interest of converting the codebase to IdentityManager as quickly and painlessly as possible, we are going to make IdentityManager a straight passthrough to its backing classes for the duration of the conversion. This does not impact the long-term vision or the shape of the APIs.
,
Sep 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5a0a4475e31b66f4ee488bdd7758b01773a3b95e commit 5a0a4475e31b66f4ee488bdd7758b01773a3b95e Author: Colin Blundell <blundell@chromium.org> Date: Tue Sep 18 11:31:45 2018 Have IdentityManager access SigninManager synchronously This CL streamlines IdentityManager's interaction with SigninManager to have IdentityManager act as a straight passthrough rather than updating its internal state via SigninManager::DiagnosticsClient. The motivation is to enable conversion of the codebase to IdentityManager as smoothly and painlessly as possible, as explained in detail in crbug.com/883722 . Bug: 883722 Change-Id: I454fd180b3203311640afd7664c19ac6220ef917 Reviewed-on: https://chromium-review.googlesource.com/1224790 Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Commit-Queue: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/heads/master@{#591998} [modify] https://crrev.com/5a0a4475e31b66f4ee488bdd7758b01773a3b95e/services/identity/public/cpp/identity_manager.cc [modify] https://crrev.com/5a0a4475e31b66f4ee488bdd7758b01773a3b95e/services/identity/public/cpp/identity_manager.h
,
Sep 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/987b42a8d9b2094c190264b019777e30e59b43c8 commit 987b42a8d9b2094c190264b019777e30e59b43c8 Author: Colin Blundell <blundell@chromium.org> Date: Wed Sep 19 10:02:59 2018 Remove SigninManager::DiagnosticsClient It's no longer used after https://chromium-review.googlesource.com/c/chromium/src/+/1224790. Bug: 883722 Change-Id: Ie55a7941fca82f6558cd2f2ecf0a65f2b345b701 Reviewed-on: https://chromium-review.googlesource.com/1224792 Reviewed-by: David Roger <droger@chromium.org> Commit-Queue: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/heads/master@{#592357} [modify] https://crrev.com/987b42a8d9b2094c190264b019777e30e59b43c8/components/signin/core/browser/signin_manager.cc [modify] https://crrev.com/987b42a8d9b2094c190264b019777e30e59b43c8/components/signin/core/browser/signin_manager.h
,
Sep 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ad9d4c2b86d8b307d3a9d791a522ec10d191bfde commit ad9d4c2b86d8b307d3a9d791a522ec10d191bfde Author: Colin Blundell <blundell@chromium.org> Date: Wed Sep 19 15:09:49 2018 [IdentityManager] Access PO2TS synchronously for getting accounts This CL streamlines IdentityManager's interaction with ProfileOAuth2TokenService when getting accounts with refresh tokens to have IdentityManager act as a straight passthrough rather than using its internal state that is updated via Po2TS::DiagnosticsClient. The motivation is to enable conversion of the codebase to IdentityManager as smoothly and painlessly as possible, as explained in detail in crbug.com/883722 . IdentityManager's internal state is currently still used for obtaining the AccountInfo to send in the OnRefreshToken{Updated, Removed}ForAccount callbacks. Changing that will require first modifying OnRefreshTokenRemovedForAccount() to pass only the account ID, as at the time that PO2TS::OnRefreshTokenRevoked() is fired, AccountTrackerService does not necessarily know about the account anymore. Bug: 883722 Change-Id: Ia13c7f9162449fa0ea60de5fb9e67b5373a82dea Reviewed-on: https://chromium-review.googlesource.com/1225758 Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Commit-Queue: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/heads/master@{#592399} [modify] https://crrev.com/ad9d4c2b86d8b307d3a9d791a522ec10d191bfde/services/identity/public/cpp/identity_manager.cc
,
Sep 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/63d23fa9cf8ed916108b8427bf9d4504eb9322bd commit 63d23fa9cf8ed916108b8427bf9d4504eb9322bd Author: Colin Blundell <blundell@chromium.org> Date: Thu Sep 20 09:01:55 2018 IdentityManager: Have OnRefreshTokenRemovedForAccount() pass account ID We will shortly be changing IdentityManager to fire IdentityManager::Observer::OnRefreshTokenRemovedForAccount() as a straight passthrough from ProfileOAuth2TokenService::Observer::OnRefreshTokenRevoked() as part of crbug.com/883722 . However, this raises a challenge in that the AccountInfo isn't guaranteed to be in AccountTrackerService when this callback is received by IdentityManager. To prepare for this change, this CL changes IdentityManager::Observer::OnRefreshTokenRemovedForAccount() to pass the account ID. No observer requires anything else from the AccountInfo that is currently being passed. Bug: 883722 Change-Id: Iccd921a11ab229b897028987e538cc799ed35026 Reviewed-on: https://chromium-review.googlesource.com/1225980 Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Commit-Queue: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/heads/master@{#592726} [modify] https://crrev.com/63d23fa9cf8ed916108b8427bf9d4504eb9322bd/components/browser_sync/sync_auth_manager.cc [modify] https://crrev.com/63d23fa9cf8ed916108b8427bf9d4504eb9322bd/components/browser_sync/sync_auth_manager.h [modify] https://crrev.com/63d23fa9cf8ed916108b8427bf9d4504eb9322bd/components/gcm_driver/account_tracker.cc [modify] https://crrev.com/63d23fa9cf8ed916108b8427bf9d4504eb9322bd/components/gcm_driver/account_tracker.h [modify] https://crrev.com/63d23fa9cf8ed916108b8427bf9d4504eb9322bd/components/invalidation/impl/profile_identity_provider.cc [modify] https://crrev.com/63d23fa9cf8ed916108b8427bf9d4504eb9322bd/components/invalidation/impl/profile_identity_provider.h [modify] https://crrev.com/63d23fa9cf8ed916108b8427bf9d4504eb9322bd/services/identity/public/cpp/identity_manager.cc [modify] https://crrev.com/63d23fa9cf8ed916108b8427bf9d4504eb9322bd/services/identity/public/cpp/identity_manager.h [modify] https://crrev.com/63d23fa9cf8ed916108b8427bf9d4504eb9322bd/services/identity/public/cpp/identity_manager_unittest.cc [modify] https://crrev.com/63d23fa9cf8ed916108b8427bf9d4504eb9322bd/services/identity/public/cpp/identity_test_utils.cc
,
Sep 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4aea5050b73c609924eb9b8b9a51cbff5ec0e4f9 commit 4aea5050b73c609924eb9b8b9a51cbff5ec0e4f9 Author: Colin Blundell <blundell@chromium.org> Date: Thu Sep 20 14:33:27 2018 Have IdentityManager always pass on refresh token removal notifications ProfileOAuth2TokenService has a corner case wherein during startup, it can fire token removal notifications for accounts for which it has never previously filed a token available notification. IdentityManager currently swallows such notifications. However, as we streamline IdentityManager to be just a straight pass-through to its backing classes ( crbug.com/883722 ), IdentityManager will no longer be able to detect this case (because it won't be maintaining any cached info from the token available notifications). This CL makes the behavioral change of having IdentityManager always pass on token removal notifications from ProfileOAuth2TokenService. The behavioral impact is that consumers of IdentityManager::Observer::OnRefreshTokenRemovedForAccount() will now see a removal notification in the corner case described above. However, all such consumers were previously consumers of ProfileOAuth2TokenService::Observer::OnRefreshTokenRevoked() before their conversion and hence were previously seeing the removal notification in this corner case. Thus, the behavioral change does not seem to have a concrete impact on consumers one way or the other. Bug: 883722 Change-Id: I131ad8dbe12d73772845fd2eb565cd3477779c36 Reviewed-on: https://chromium-review.googlesource.com/1225878 Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Commit-Queue: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/heads/master@{#592794} [modify] https://crrev.com/4aea5050b73c609924eb9b8b9a51cbff5ec0e4f9/services/identity/public/cpp/identity_manager.cc [modify] https://crrev.com/4aea5050b73c609924eb9b8b9a51cbff5ec0e4f9/services/identity/public/cpp/identity_manager.h [modify] https://crrev.com/4aea5050b73c609924eb9b8b9a51cbff5ec0e4f9/services/identity/public/cpp/identity_manager_unittest.cc
,
Sep 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b30f7f8244ae3cbb1614adc5a323d31624332184 commit b30f7f8244ae3cbb1614adc5a323d31624332184 Author: Colin Blundell <blundell@chromium.org> Date: Fri Sep 21 10:50:29 2018 IdentityManager: Remove caching of accounts with refresh tokens This CL streamlines IdentityManager's interaction with AccountTrackerService when firing updates for accounts with refresh tokens. Rather than caching state locally in response to PO2TS::DiagnosticsClient callbacks, IdentityManager simply obtains the AccountInfo from AccountTracker in the PO2TS observer callback itself. This CL eliminates the need for any cached state in IdentityManager related to accounts with refresh tokens. The motivation is to enable conversion of the codebase to IdentityManager as smoothly and painlessly as possible, as explained in detail in crbug.com/883722 . Bug: 883722 Change-Id: Ic13d5be5a5f17411c167d19da7f8444e6ec257bc Reviewed-on: https://chromium-review.googlesource.com/1228197 Commit-Queue: Colin Blundell <blundell@chromium.org> Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Cr-Commit-Position: refs/heads/master@{#593142} [modify] https://crrev.com/b30f7f8244ae3cbb1614adc5a323d31624332184/services/identity/public/cpp/identity_manager.cc [modify] https://crrev.com/b30f7f8244ae3cbb1614adc5a323d31624332184/services/identity/public/cpp/identity_manager.h
,
Sep 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0a263965e6000b96b55bd3dbf07d72b282fe6845 commit 0a263965e6000b96b55bd3dbf07d72b282fe6845 Author: Colin Blundell <blundell@chromium.org> Date: Mon Sep 24 10:21:24 2018 Remove PO2TS::DiagnosticsClient This infrastructure is no longer used after https://chromium-review.googlesource.com/c/chromium/src/+/1228197. Bug: 883722 Change-Id: I9871ce8297ab42a8b173e61880570e076ae9277c Reviewed-on: https://chromium-review.googlesource.com/1235575 Commit-Queue: Colin Blundell <blundell@chromium.org> Reviewed-by: David Roger <droger@chromium.org> Cr-Commit-Position: refs/heads/master@{#593497} [modify] https://crrev.com/0a263965e6000b96b55bd3dbf07d72b282fe6845/components/signin/core/browser/profile_oauth2_token_service.cc [modify] https://crrev.com/0a263965e6000b96b55bd3dbf07d72b282fe6845/components/signin/core/browser/profile_oauth2_token_service.h
,
Sep 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/575d2274359529d2010b14fef42981ec99ad5777 commit 575d2274359529d2010b14fef42981ec99ad5777 Author: Colin Blundell <blundell@chromium.org> Date: Mon Sep 24 15:32:21 2018 OAuth2TokenService.java: Remove unused methods and their tests OAuth2TokenService.java has methods that trigger the firing of various refresh token-related notifications by ProfileOAuth2TokenService [the C++ class]. These methods are not used (thanks to bsazonov@ for detecting that!), and their tests are problematic, as they cause the notifications to be fired without the accounts actually being available in PO2TS; this fact violates an invariant that we would like to put in place wherein PO2TS only fires refresh token available notifications for accounts for which it has a refresh token. This CL simply eliminates these unused methods and their tests. TBR=msarda@chromium.org Bug: 883722 Change-Id: Ib44be7db2fbe5341588a9e7ff786f5d166d1ae6c Reviewed-on: https://chromium-review.googlesource.com/1240275 Commit-Queue: Colin Blundell <blundell@chromium.org> Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Reviewed-by: Boris Sazonov <bsazonov@chromium.org> Cr-Commit-Position: refs/heads/master@{#593546} [modify] https://crrev.com/575d2274359529d2010b14fef42981ec99ad5777/chrome/android/java/src/org/chromium/chrome/browser/signin/OAuth2TokenService.java [modify] https://crrev.com/575d2274359529d2010b14fef42981ec99ad5777/chrome/android/javatests/src/org/chromium/chrome/browser/signin/OAuth2TokenServiceIntegrationTest.java [modify] https://crrev.com/575d2274359529d2010b14fef42981ec99ad5777/chrome/browser/signin/oauth2_token_service_delegate_android.cc [modify] https://crrev.com/575d2274359529d2010b14fef42981ec99ad5777/chrome/browser/signin/oauth2_token_service_delegate_android.h
,
Sep 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3e1fd5d31204628a7fd552a3c85174d6801eebe6 commit 3e1fd5d31204628a7fd552a3c85174d6801eebe6 Author: Colin Blundell <blundell@chromium.org> Date: Tue Sep 25 07:39:22 2018 [IdentityManager] Dedupe population of account info for supervised users Simple cleanup CL. Bug: 883722 Change-Id: I48e30666169d23de0d1375e2daec2268d048067d Reviewed-on: https://chromium-review.googlesource.com/1235994 Commit-Queue: Colin Blundell <blundell@chromium.org> Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Cr-Commit-Position: refs/heads/master@{#593848} [modify] https://crrev.com/3e1fd5d31204628a7fd552a3c85174d6801eebe6/services/identity/public/cpp/identity_manager.cc [modify] https://crrev.com/3e1fd5d31204628a7fd552a3c85174d6801eebe6/services/identity/public/cpp/identity_manager.h
,
Oct 22
|
||
►
Sign in to add a comment |
||
Comment 1 by blundell@chromium.org
, Sep 13