Port consumers of GaiaCookieManagerService to IdentityManager |
|||||||||||||||
Issue descriptionSync has a need to get the accounts stored in the user's Gaia cookie. It is not clear at this time whether this functionality should be implemented inside the Identity Service or not. It is currently provided by GaiaCookieManagerService, which intertwines this functionality with other functionality for *mutating* the state of the user's Gaia cookie based on browser policies like Mirror and DICE. The design doc here describes the approach that we are going to take in the near term to satisfy Sync's use case with a clean API while leaving the issue of whether this API will ultimately be implemented inside or outside of the Identity Service for conclusion in the future: https://docs.google.com/document/d/1hcrJ44facCSHtMGBmPusvcoP-fAR300Hi-UFez8ffYQ/edit#heading=h.y7yywe14x23d ⛆ |
|
|
,
Jul 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/edab9397d2d0a9e99a2536cb76889e05e03d52a1 commit edab9397d2d0a9e99a2536cb76889e05e03d52a1 Author: Colin Blundell <blundell@chromium.org> Date: Thu Jul 05 15:20:02 2018 Extend GaiaCookieManagerService unittest of ListAccounts() GaiaCookieManagerService::ListAccounts() takes in out-params to populate with the current cached state of the accounts and returns a bool indicating whether the cached state is valid. Its contract specifies that if the returned bool is false, the out-params should be ignored. We are shortly going to be changing the semantics of GaiaCookieManagerService::ListAccounts() to always populate its out-parameters, even if its cached state is stale. This CL prepares for that change by extending the unittests of GaiaCookieManagerService to explicitly test the current behavior (i.e., that the out-params are *not* populated with stale cached data). Bug: 859882 Change-Id: Icf28ba3be7fabbbf70b7b51acc68f0f60bed5993 Reviewed-on: https://chromium-review.googlesource.com/1126103 Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Commit-Queue: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/heads/master@{#572799} [modify] https://crrev.com/edab9397d2d0a9e99a2536cb76889e05e03d52a1/components/signin/core/browser/gaia_cookie_manager_service_unittest.cc
,
Jul 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fb671babdc29f773731e4bc92439b14da172b228 commit fb671babdc29f773731e4bc92439b14da172b228 Author: Colin Blundell <blundell@chromium.org> Date: Mon Jul 09 14:57:28 2018 GaiaCookieManagerService: Change semantics of ListAccounts() GaiaCookieManagerService::ListAccounts() takes in out-params to populate with the current cached state of the accounts and returns a bool indicating whether the cached state is valid. Its contract specifies that if the returned bool is false, the out-params should be ignored. This CL changes the semantics of GaiaCookieManagerService::ListAccounts() to always populate its out-parameters, even if its cached state is stale. We verified manually that all current clients of this method guard the usage of the out-params via a check that the returned bool is true, so this semantic change will not affect any existing clients. The reason for making this semantic change is that there is an upcoming consumer that wants to have synchronous access to the latest cached state of the accounts, regardless of whether that state is stale. Bug: 859882 Change-Id: I47fc55bb2fad6caf9a2fc5911c8f60f97e4bc5a6 Reviewed-on: https://chromium-review.googlesource.com/1128892 Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Commit-Queue: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/heads/master@{#573307} [modify] https://crrev.com/fb671babdc29f773731e4bc92439b14da172b228/components/signin/core/browser/gaia_cookie_manager_service.cc [modify] https://crrev.com/fb671babdc29f773731e4bc92439b14da172b228/components/signin/core/browser/gaia_cookie_manager_service.h [modify] https://crrev.com/fb671babdc29f773731e4bc92439b14da172b228/components/signin/core/browser/gaia_cookie_manager_service_unittest.cc
,
Jul 17
Update: After reflection, I am going to add these APIs on IdentityManager itself. That will be most straightforward and graceful for satisfying Sync's use case, and since Sync is such a primary use case for Signin (and the Identity Service), serving Sync's needs gracefully is a high design priority for this project. More details here: https://docs.google.com/document/d/1hcrJ44facCSHtMGBmPusvcoP-fAR300Hi-UFez8ffYQ/edit# As noted in the doc, this decision can easily be changed (and the APIs split out to be layered on top on the client side) if we find compelling reasons to do so in the future.
,
Jul 17
I am going to bring these APIs up via 3 CLs: - CL just threading in GaiaCookieManagerService - CL implementing observer APIs for cookie jar changes - CL for getting cached state of accounts in cookie jar
,
Jul 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e3f61fa173c7d4cbef792a05b31489b1c094ad24 commit e3f61fa173c7d4cbef792a05b31489b1c094ad24 Author: Colin Blundell <blundell@chromium.org> Date: Wed Jul 18 12:16:26 2018 [IdentityManager] Thread in GaiaCookieManagerService Upcoming work will add APIs to IdentityManager to interact with the user's Gaia accounts in the cookie jar. To implement these APIS, IdentityManager will need to interact with GaiaCookieManagerService. In preparation for that work, this CL threads GaiaCookieManagerService in to IdentityManager. Design doc: https://docs.google.com/document/d/1hcrJ44facCSHtMGBmPusvcoP-fAR300Hi-UFez8ffYQ/edit?pli=1#heading=h.y7yywe14x23d TBR=rohitrao@chromium.org Bug: 859882 Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: I40f1abd6c8beca89476567bbb94979ad42ff7df1 Reviewed-on: https://chromium-review.googlesource.com/1140156 Commit-Queue: Colin Blundell <blundell@chromium.org> Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Cr-Commit-Position: refs/heads/master@{#576011} [modify] https://crrev.com/e3f61fa173c7d4cbef792a05b31489b1c094ad24/chrome/browser/signin/identity_manager_factory.cc [modify] https://crrev.com/e3f61fa173c7d4cbef792a05b31489b1c094ad24/components/browser_sync/profile_sync_test_util.cc [modify] https://crrev.com/e3f61fa173c7d4cbef792a05b31489b1c094ad24/components/browser_sync/profile_sync_test_util.h [modify] https://crrev.com/e3f61fa173c7d4cbef792a05b31489b1c094ad24/components/gcm_driver/gcm_account_tracker_unittest.cc [modify] https://crrev.com/e3f61fa173c7d4cbef792a05b31489b1c094ad24/ios/chrome/browser/signin/identity_manager_factory.cc [modify] https://crrev.com/e3f61fa173c7d4cbef792a05b31489b1c094ad24/ios/web_view/internal/signin/web_view_identity_manager_factory.mm [modify] https://crrev.com/e3f61fa173c7d4cbef792a05b31489b1c094ad24/services/identity/public/cpp/DEPS [modify] https://crrev.com/e3f61fa173c7d4cbef792a05b31489b1c094ad24/services/identity/public/cpp/identity_manager.cc [modify] https://crrev.com/e3f61fa173c7d4cbef792a05b31489b1c094ad24/services/identity/public/cpp/identity_manager.h [modify] https://crrev.com/e3f61fa173c7d4cbef792a05b31489b1c094ad24/services/identity/public/cpp/identity_manager_unittest.cc [modify] https://crrev.com/e3f61fa173c7d4cbef792a05b31489b1c094ad24/services/identity/public/cpp/identity_test_environment.cc
,
Jul 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d8a60e779be20114b5de192bdf9685f5db2e5d9d commit d8a60e779be20114b5de192bdf9685f5db2e5d9d Author: Colin Blundell <blundell@chromium.org> Date: Fri Jul 20 13:45:55 2018 [IdentityManager] API for observing updates to accounts in Gaia cookie This CL adds an API to IdentityManager::Observer via which clients can be notified of updates to the list of Gaia accounts in the cookie jar. This API is intended to enable porting consumers of GaiaCookieManagerService::Observer::OnGaiaAccountsInCookieUpdated(). Notably, whereas the former API deals in gaia::ListedAccount structs, the IdentityManager API talks in terms of the more-mainstream AccountInfo structs (parallel to other IdentityManager::Observer APIs). The new API passes only the list of signed-in accounts, whereas the previous API has more parameters. We will add more parameters if and as we see that they are needed by consumers. Bug: 859882 Change-Id: I4ff2ece0836e73fc616462c84ba88c600a026071 Reviewed-on: https://chromium-review.googlesource.com/1140325 Reviewed-by: Marc Treib <treib@chromium.org> Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Commit-Queue: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/heads/master@{#576846} [modify] https://crrev.com/d8a60e779be20114b5de192bdf9685f5db2e5d9d/services/identity/public/cpp/identity_manager.cc [modify] https://crrev.com/d8a60e779be20114b5de192bdf9685f5db2e5d9d/services/identity/public/cpp/identity_manager.h [modify] https://crrev.com/d8a60e779be20114b5de192bdf9685f5db2e5d9d/services/identity/public/cpp/identity_manager_unittest.cc
,
Jul 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8b34f56ae64d3083023ba3672604276fca318b64 commit 8b34f56ae64d3083023ba3672604276fca318b64 Author: Colin Blundell <blundell@chromium.org> Date: Fri Jul 20 15:38:26 2018 IdentityManager: Add ability to get accounts in the cookie jar This CL adds an API to IdentityManager via which clients can obtain the latest cached state of the accounts in the Gaia cookie in the cookie jar, ordered by their order in the cookie jar. It serves as an initial step to porting consumers of GaiaCookieManagerService::ListAccounts(), although it intentionally provides only a subset of the functionality (in particular, it does not indicate whether the cached state is known to be stale). https://docs.google.com/document/d/1hcrJ44facCSHtMGBmPusvcoP-fAR300Hi-UFez8ffYQ/edit?pli=1#heading=h.w97eil1cygs2 sketches out the approach for extending this API to give IdentityManager the full functionality required to port all consumers of GaiaCookieManagerService::ListAccounts(). We are holding off on adding more functionality until we encounter use cases that require it. Bug: 859882 Change-Id: I062a9ce036aa886f1c105913eb1a774471bb63f8 Reviewed-on: https://chromium-review.googlesource.com/1141951 Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Reviewed-by: Marc Treib <treib@chromium.org> Commit-Queue: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/heads/master@{#576874} [modify] https://crrev.com/8b34f56ae64d3083023ba3672604276fca318b64/services/identity/public/cpp/identity_manager.cc [modify] https://crrev.com/8b34f56ae64d3083023ba3672604276fca318b64/services/identity/public/cpp/identity_manager.h [modify] https://crrev.com/8b34f56ae64d3083023ba3672604276fca318b64/services/identity/public/cpp/identity_manager_unittest.cc
,
Sep 5
The remaining work is to:
- Determine what other API surfaces of GaiaCookieManagerService{::Observer} will be needed by public clients
- Add the buildout of those APIs as blocking bugs here
- Burn them down
,
Nov 22
,
Nov 22
,
Dec 6
,
Dec 10
,
Dec 11
,
Dec 11
,
Dec 11
,
Jan 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b92aad536edcac81e0daa90deb484e668995d157 commit b92aad536edcac81e0daa90deb484e668995d157 Author: Antonio Gomes <tonikitoo@igalia.com> Date: Mon Jan 07 17:19:12 2019 Remove GaiaCookieManagerService dependency from AccountsInvestigator This a 2nd out 3 CLs that will make AccountInvestigator and its unittests not depend/use GaiaCookieManagerService anymore. Here AccountInvestigator replaces inheritance of GaiaCookieManagerService::Observer by IdentityManager::Observer. Note that because of the existing AccountInfo <-> ListedAccount conversions, a helper function was introduced with a TODO to convert the whole AccountInvestigator and its unit tests to use the former. BUG=859882, 909715 Change-Id: Iaf23e19622258ce63957c6254df3aac652558a08 Reviewed-on: https://chromium-review.googlesource.com/c/1396240 Commit-Queue: Antonio Gomes <tonikitoo@igalia.com> Reviewed-by: David Roger <droger@chromium.org> Cr-Commit-Position: refs/heads/master@{#620363} [modify] https://crrev.com/b92aad536edcac81e0daa90deb484e668995d157/chrome/browser/signin/account_investigator_factory.cc [modify] https://crrev.com/b92aad536edcac81e0daa90deb484e668995d157/components/signin/core/browser/account_investigator.cc [modify] https://crrev.com/b92aad536edcac81e0daa90deb484e668995d157/components/signin/core/browser/account_investigator.h [modify] https://crrev.com/b92aad536edcac81e0daa90deb484e668995d157/components/signin/core/browser/account_investigator_unittest.cc
,
Jan 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7128827cfca113f04143286836cf17a7215bc91e commit 7128827cfca113f04143286836cf17a7215bc91e Author: Lowell Manners <lowell@chromium.org> Date: Wed Jan 09 17:38:29 2019 Remove some unneeded references to GaiaCookieManagerService::Observer. This CL deletes some empty GaiaCookieManagerService::Observer overrides which created noise in grep results when searching for usages of GaiaCookieManagerService callbacks. Bug: 859882 Change-Id: Id8119e9faec009056938c7b7b1977388307914cb Reviewed-on: https://chromium-review.googlesource.com/c/1400700 Reviewed-by: Colin Blundell <blundell@chromium.org> Commit-Queue: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/heads/master@{#621204} [modify] https://crrev.com/7128827cfca113f04143286836cf17a7215bc91e/components/autofill/core/browser/personal_data_manager.h
,
Jan 11
,
Jan 11
,
Jan 15
|
||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by bugdroid1@chromium.org
, Jul 4