Move [Profile]SyncService to Identity Service |
|||||||||||||
Issue descriptionCurrently, ProfileSyncService and its surrounding code in components/sync/ and components/browser_sync/ use the old signin APIs around SigninManager. They should be moved to the new Identity Service, in particular IdentityManager from services/identity/public/cpp/.
,
Mar 23 2018
And some first assessments on what to do about them/how hard it'll be: 1) Should be easy to just hand out an AccountInfo for the Sync account instead of exposing SigninManager. 2) Should map neatly to IdentityManager::Observer. 3) Not sure, this doesn't seem to be exposed in the new API yet. 4) Not exposed in the new API yet (should it be?) 5) I think this simply won't be necessary anymore; IdentityManager's "primary account" concept should cover having a refresh token. 6) Replaced by PrimaryAccountAccessTokenFetcher. 7) No idea. DICE should eventually make this obsolete, but we'll need something in the meantime. Colin, please holler if any of the above seems wrong to you!
,
Mar 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9ceadee11c6644629d54ef346fea694cbe882184 commit 9ceadee11c6644629d54ef346fea694cbe882184 Author: Marc Treib <treib@chromium.org> Date: Mon Mar 26 09:14:32 2018 Sync cleanup: Remove a bunch of unused code and includes I was mostly looking for components/signin/-related things, but discovered a bunch more stuff along the way, mostly in ProfileSyncComponentsFactoryImpl. Bug: 825190 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: I1b17d00db3cbefa50a7fabaf0b3e55e7ea567610 Reviewed-on: https://chromium-review.googlesource.com/978211 Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#545747} [modify] https://crrev.com/9ceadee11c6644629d54ef346fea694cbe882184/chrome/browser/sync/chrome_sync_client.cc [modify] https://crrev.com/9ceadee11c6644629d54ef346fea694cbe882184/components/browser_sync/profile_sync_components_factory_impl.cc [modify] https://crrev.com/9ceadee11c6644629d54ef346fea694cbe882184/components/browser_sync/profile_sync_components_factory_impl.h [modify] https://crrev.com/9ceadee11c6644629d54ef346fea694cbe882184/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/9ceadee11c6644629d54ef346fea694cbe882184/components/browser_sync/profile_sync_service.h [modify] https://crrev.com/9ceadee11c6644629d54ef346fea694cbe882184/components/sync/driver/glue/sync_backend_host_impl.cc [modify] https://crrev.com/9ceadee11c6644629d54ef346fea694cbe882184/ios/chrome/browser/sync/ios_chrome_sync_client.mm
,
Mar 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3f9b3e12160d0b886bc8fee9e792f925c675ad70 commit 3f9b3e12160d0b886bc8fee9e792f925c675ad70 Author: Marc Treib <treib@chromium.org> Date: Tue Mar 27 08:46:38 2018 Sync: don't expose SigninManager from SyncService As a first (baby) step of removing the old signin APIs from Sync. Bug: 825190 Change-Id: I1554c96c54555083f06963a72d2c38248afeb960 Reviewed-on: https://chromium-review.googlesource.com/978186 Reviewed-by: Colin Blundell <blundell@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#546065} [modify] https://crrev.com/3f9b3e12160d0b886bc8fee9e792f925c675ad70/chrome/browser/sync/test/integration/profile_sync_service_harness.cc [modify] https://crrev.com/3f9b3e12160d0b886bc8fee9e792f925c675ad70/chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc [modify] https://crrev.com/3f9b3e12160d0b886bc8fee9e792f925c675ad70/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/3f9b3e12160d0b886bc8fee9e792f925c675ad70/components/sync/BUILD.gn [modify] https://crrev.com/3f9b3e12160d0b886bc8fee9e792f925c675ad70/components/sync/driver/about_sync_util.cc [modify] https://crrev.com/3f9b3e12160d0b886bc8fee9e792f925c675ad70/components/sync/driver/fake_sync_service.cc [modify] https://crrev.com/3f9b3e12160d0b886bc8fee9e792f925c675ad70/components/sync/driver/fake_sync_service.h [modify] https://crrev.com/3f9b3e12160d0b886bc8fee9e792f925c675ad70/components/sync/driver/sync_service.h [modify] https://crrev.com/3f9b3e12160d0b886bc8fee9e792f925c675ad70/components/sync/driver/sync_service_base.cc [modify] https://crrev.com/3f9b3e12160d0b886bc8fee9e792f925c675ad70/components/sync/driver/sync_service_base.h
,
Mar 27 2018
This analysis LGTM, Marc! Answers to questions in c#2: 3. For now, I would just pass in a callback that returns this device ID when invoked (or even just pass in the device ID if you can determine that it's safe to move the invocation point to PSS's construction). I looked at the code implementing GetSigninScopedDeviceId() and it's not at all clear at this point whether this should live in the Identity Service or somewhere else long-term. Notably, that function is not used by //components/signin. 4. Yes, it should. This has been on my backlog of things to design and implement, and this use case adds a forcing function for it. If you get started on the other pieces, I can prioritize bringing up this API surface. 7. Can we leave this part alone at this time? i.e., does anything about the interactions with GaiaCookieManagerService need to change to support your motivations for doing this refactoring? From my side, I have been deliberately leaving GaiaCookieManagerService TBD.
,
Mar 27 2018
Thanks Colin! 3. I don't know what a "signin-scoped device ID" even is, but the name suggests that it's not safe to just pass in - I assume if you sign out and in again, you'll get a different one. So callback SGTM. 4. SGTM, I'll get started on the other things. 7. Yeah, I think it's fine to just leave this alone for now. Maybe we can just wait for DICE to make it obsolete :)
,
Mar 27 2018
Documenting the current state, mostly for my future self: 1 is done, 4 is blocked, 7 is punted indefinitely. 2, 3, 5, and 6 should be ready for conversion. 3 can be done independently, but the rest are all related. I'll have to see if it's possible to do any of these separately.
,
Mar 27 2018
Re: your comment about 3, you are 100% correct :). Great catch! Re: separation, 5 and 6 are clearly linked. The question about 2 is what PSS does for listening to GoogleSignInSuccess/GoogleSignedOut. Is it more than just the access token fetching flow? If it's the latter, then it's also part of converting to PrimaryAccountAccessTokenFetcher. If it's something else, then PSS will need to observe IdentityManager as well as using a PrimaryAccountAccessTokenFetcher and then 2 should be possible to do before/after 5/6 are done.
,
Mar 29 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/088dcb79d01f30f1268148e3666418745aba3b23 commit 088dcb79d01f30f1268148e3666418745aba3b23 Author: Marc Treib <treib@chromium.org> Date: Thu Mar 29 18:33:17 2018 Sync: Pass in SigninScopedDeviceIdCallback instead of using SigninClient This removes a dependency on SigninManagerBase::signin_client() Bug: 825190 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: Iec3197628788a27d808abffc883faf6001c9c0d4 Reviewed-on: https://chromium-review.googlesource.com/983555 Reviewed-by: Colin Blundell <blundell@chromium.org> Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#546867} [modify] https://crrev.com/088dcb79d01f30f1268148e3666418745aba3b23/chrome/browser/sync/profile_sync_service_factory.cc [modify] https://crrev.com/088dcb79d01f30f1268148e3666418745aba3b23/chrome/browser/sync/profile_sync_test_util.cc [modify] https://crrev.com/088dcb79d01f30f1268148e3666418745aba3b23/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/088dcb79d01f30f1268148e3666418745aba3b23/components/browser_sync/profile_sync_service.h [modify] https://crrev.com/088dcb79d01f30f1268148e3666418745aba3b23/components/browser_sync/profile_sync_service_unittest.cc [modify] https://crrev.com/088dcb79d01f30f1268148e3666418745aba3b23/components/browser_sync/profile_sync_test_util.cc [modify] https://crrev.com/088dcb79d01f30f1268148e3666418745aba3b23/components/sync/driver/sync_api_component_factory_mock.cc [modify] https://crrev.com/088dcb79d01f30f1268148e3666418745aba3b23/ios/chrome/browser/sync/ios_chrome_profile_sync_service_factory.cc [modify] https://crrev.com/088dcb79d01f30f1268148e3666418745aba3b23/ios/chrome/browser/sync/ios_chrome_profile_sync_test_util.cc
,
Apr 3 2018
Hrm, I think 5) (handling of refresh tokens) is a bit more tricky. PSS calls the following methods on OAuth2TokenService: a) StartRequest: Maps to CreateAccessTokenFetcherForPrimaryAccount. b) InvalidateAccessToken: Maps to RemoveAccessTokenFromCache. c) RefreshTokenIsAvailable: This is the tricky one, IdentityManager doesn't expose this (and it probably shouldn't). PSS uses RefreshTokenIsAvailable for two things: - In combination with the OAuth2TokenService::Observer overrides, to detect whether a newly-loaded refresh token was for the relevant (primary) account. This should map neatly to PrimaryAccountAccessTokenFetcher, except for legacy supervised users. - In CanEngineStart. I'm not entirely sure why it's necessary here, I guess just as an optimization? Like, no point in trying to start up the engine before the refresh token is loaded.
,
Apr 4 2018
Thanks! There are two separate questions here: 1. Supervised users. I'll follow up on that separately. 2. CanEngineStart(). I have the same question as you: what is the underlying reason for delaying that until we know the refresh token is available. Would you be able to dig into that some with the former sync team? If we need to make this information available in some fashion, we'll be able to.
,
Apr 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d0138da3e9025bd01618cb10b642f9961618573e commit d0138da3e9025bd01618cb10b642f9961618573e Author: Marc Treib <treib@chromium.org> Date: Wed Apr 04 15:28:21 2018 FakeProfileOAuth2TokenService: fix auto_post_fetch_response vs manual response FakeProfileOAuth2TokenService has two modes of operation: auto_post_fetch_response mode, which means it'll automatically post successful responses to all access token requests to the message loop, or regular (manual) mode, where the user calls IssueToken*/IssueError* etc. This CL adds DCHECKs to make sure the two modes are not mixed. It also fixes some ProfileSyncService tests which did just that. Bug: 825190 Change-Id: Ibed800c7defa09fb8efbf8cb8a56608731aaf70e Reviewed-on: https://chromium-review.googlesource.com/995434 Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#548067} [modify] https://crrev.com/d0138da3e9025bd01618cb10b642f9961618573e/components/browser_sync/profile_sync_service_unittest.cc [modify] https://crrev.com/d0138da3e9025bd01618cb10b642f9961618573e/components/signin/core/browser/fake_profile_oauth2_token_service.cc
,
Apr 5 2018
,
Apr 5 2018
Re CanEngineStart, I've traced the "have refresh token" condition back to at least https://codereview.chromium.org/162443004 from 2014-02. I'm also fairly sure it's safe to remove; I'll investigate.
,
Apr 5 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/86c26d7d38476ba547be782e9501389f705d9d82 commit 86c26d7d38476ba547be782e9501389f705d9d82 Author: Marc Treib <treib@chromium.org> Date: Thu Apr 05 14:48:11 2018 Sync: Plumb IdentityManager through to ProfileSyncService This CL plumbs the IdentityManager through to PSS, via SigninManagerWrapper. There are no behavioral changes, in particular the IdentityManager isn't used at all yet. Uses will follow soon (https://crrev.com/c/983914). This does require a small test change to make the newly-instantiated IdentityManager happy (without that change, it complains that its view of the primary account isn't consistent with SigninManager's). Bug: 825190 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: I8b313f5572339a2c1319725fa4012b4a41365e08 Reviewed-on: https://chromium-review.googlesource.com/995414 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/heads/master@{#548419} [modify] https://crrev.com/86c26d7d38476ba547be782e9501389f705d9d82/chrome/browser/sync/profile_sync_service_factory.cc [modify] https://crrev.com/86c26d7d38476ba547be782e9501389f705d9d82/chrome/browser/sync/profile_sync_test_util.cc [modify] https://crrev.com/86c26d7d38476ba547be782e9501389f705d9d82/chrome/browser/sync/supervised_user_signin_manager_wrapper.cc [modify] https://crrev.com/86c26d7d38476ba547be782e9501389f705d9d82/chrome/browser/sync/supervised_user_signin_manager_wrapper.h [modify] https://crrev.com/86c26d7d38476ba547be782e9501389f705d9d82/components/browser_sync/BUILD.gn [modify] https://crrev.com/86c26d7d38476ba547be782e9501389f705d9d82/components/browser_sync/DEPS [modify] https://crrev.com/86c26d7d38476ba547be782e9501389f705d9d82/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/86c26d7d38476ba547be782e9501389f705d9d82/components/browser_sync/profile_sync_service_unittest.cc [modify] https://crrev.com/86c26d7d38476ba547be782e9501389f705d9d82/components/browser_sync/profile_sync_test_util.cc [modify] https://crrev.com/86c26d7d38476ba547be782e9501389f705d9d82/components/browser_sync/profile_sync_test_util.h [modify] https://crrev.com/86c26d7d38476ba547be782e9501389f705d9d82/components/sync/BUILD.gn [modify] https://crrev.com/86c26d7d38476ba547be782e9501389f705d9d82/components/sync/driver/DEPS [modify] https://crrev.com/86c26d7d38476ba547be782e9501389f705d9d82/components/sync/driver/signin_manager_wrapper.cc [modify] https://crrev.com/86c26d7d38476ba547be782e9501389f705d9d82/components/sync/driver/signin_manager_wrapper.h [modify] https://crrev.com/86c26d7d38476ba547be782e9501389f705d9d82/components/sync/driver/sync_service_base.cc [modify] https://crrev.com/86c26d7d38476ba547be782e9501389f705d9d82/ios/chrome/browser/sync/ios_chrome_profile_sync_service_factory.cc [modify] https://crrev.com/86c26d7d38476ba547be782e9501389f705d9d82/ios/chrome/browser/sync/ios_chrome_profile_sync_test_util.cc
,
Apr 6 2018
Re: c#14: Awesome, keep me posted!
,
Apr 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/14feef5e3436f3ae61276a90cdf822a0c03afabf commit 14feef5e3436f3ae61276a90cdf822a0c03afabf Author: Marc Treib <treib@chromium.org> Date: Thu Apr 12 09:42:31 2018 ProfileSyncServiceStartupTest: Prepare for IdentityManager This merges IssueTestTokens into SimulateTestUserSignin. IdentityManager doesn't know of this distinction (signed in but no tokens), and only one of the (non-disabled) tests depends on it. Bug: 825190 Change-Id: Id6695ba2febc973c6a0b670624bb1ecf9ed3e0ae Reviewed-on: https://chromium-review.googlesource.com/1007274 Reviewed-by: Colin Blundell <blundell@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#550128} [modify] https://crrev.com/14feef5e3436f3ae61276a90cdf822a0c03afabf/components/browser_sync/profile_sync_service_startup_unittest.cc
,
Apr 13 2018
(Documenting investigation for my future self) Re checking RefreshTokenIsAvailable in CanEngineStart: Right now we can't simply remove the condition, because then we'll try to get an access token before the refresh token is loaded, which results in an error and stuff gets messed up. However, that should get resolved automatically once we use PrimaryAccountAccessTokenFetcher, which supports waiting for the refresh token.
,
Apr 16 2018
,
Apr 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0da8397c760089d806bd930e41c568e755067cb5 commit 0da8397c760089d806bd930e41c568e755067cb5 Author: Marc Treib <treib@chromium.org> Date: Mon Apr 16 09:19:55 2018 identity::MakePrimaryAccountAvailable: Take SigninManagerBase instead of Fake Bug: 825190 Change-Id: I166418dc7691b572abdee1f337c938528142efca Reviewed-on: https://chromium-review.googlesource.com/1007236 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/heads/master@{#550952} [modify] https://crrev.com/0da8397c760089d806bd930e41c568e755067cb5/services/identity/public/cpp/identity_manager.h [modify] https://crrev.com/0da8397c760089d806bd930e41c568e755067cb5/services/identity/public/cpp/identity_test_utils.cc [modify] https://crrev.com/0da8397c760089d806bd930e41c568e755067cb5/services/identity/public/cpp/identity_test_utils.h
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a20642c976a85834476eadfa61b2077c971f5327 commit a20642c976a85834476eadfa61b2077c971f5327 Author: Marc Treib <treib@chromium.org> Date: Tue Apr 17 08:35:54 2018 Migrate LocalDiscoveryUITest to IdentityManager This CL migrates LocalDiscoveryUITest from the old SigninManager + OAuth2TokenService to the new IdentityManager. TBR=vitalybuka@chromium.org (reviewed by @google.com account) Bug: 825190 , 798408 Change-Id: If301de8524ee22c52dc4a4a0a0918c941d449226 Reviewed-on: https://chromium-review.googlesource.com/1012105 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Vitaly Buka <vitalybuka@google.com> Cr-Commit-Position: refs/heads/master@{#551280} [modify] https://crrev.com/a20642c976a85834476eadfa61b2077c971f5327/chrome/browser/ui/webui/local_discovery/local_discovery_ui_browsertest.cc
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/66a3ab10cbb76414a9f576ceb512babe0a53e48d commit 66a3ab10cbb76414a9f576ceb512babe0a53e48d Author: Marc Treib <treib@chromium.org> Date: Tue Apr 17 10:23:41 2018 Identity API tests: Move from SigninManager to IdentityManager This was fun because these tests used account_id, gaia_id, and email more or less interchangably, which doesn't work well with IdentityManager (or with sanity). This CL cleans this up and clearly differentiates the three. Bug: 825190 Change-Id: Ibdebb6d295f84ef15dd2864819c98fb1274ab63c Reviewed-on: https://chromium-review.googlesource.com/1010423 Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#551288} [modify] https://crrev.com/66a3ab10cbb76414a9f576ceb512babe0a53e48d/chrome/browser/extensions/api/identity/identity_apitest.cc
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0da8397c760089d806bd930e41c568e755067cb5 commit 0da8397c760089d806bd930e41c568e755067cb5 Author: Marc Treib <treib@chromium.org> Date: Mon Apr 16 09:19:55 2018 identity::MakePrimaryAccountAvailable: Take SigninManagerBase instead of Fake Bug: 825190 Change-Id: I166418dc7691b572abdee1f337c938528142efca Reviewed-on: https://chromium-review.googlesource.com/1007236 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/heads/master@{#550952} [modify] https://crrev.com/0da8397c760089d806bd930e41c568e755067cb5/services/identity/public/cpp/identity_manager.h [modify] https://crrev.com/0da8397c760089d806bd930e41c568e755067cb5/services/identity/public/cpp/identity_test_utils.cc [modify] https://crrev.com/0da8397c760089d806bd930e41c568e755067cb5/services/identity/public/cpp/identity_test_utils.h
,
Apr 18 2018
,
Apr 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2b03edd03c0431d97c144591d86069e402bcbc79 commit 2b03edd03c0431d97c144591d86069e402bcbc79 Author: Marc Treib <treib@chromium.org> Date: Wed Apr 18 17:46:26 2018 Migrate ExtensionInstalledBubbleBrowserTest.InvokeUi_NoAction to IdentityManager IdentityManager is the new API that replaces the (semi-)deprecated SigninManager. Bug: 825190 , 798412 Change-Id: I4bb24b0ebdf0d1d964ce4ad16aa986cc56af2fa4 Reviewed-on: https://chromium-review.googlesource.com/1016760 Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#551735} [modify] https://crrev.com/2b03edd03c0431d97c144591d86069e402bcbc79/chrome/browser/ui/extensions/extension_installed_bubble_browsertest.cc
,
Apr 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c380bcec772d00621fd6c84689be5bc2c057fb13 commit c380bcec772d00621fd6c84689be5bc2c057fb13 Author: Marc Treib <treib@chromium.org> Date: Wed Apr 18 18:21:34 2018 Migrate ScreenlockPrivateApiTest to IdentityManager This changes ScreenlockPrivateApiTest to use the new IdentityManager instead of the old SigninManager. Bug: 825190 , 797952 Change-Id: I96aadde12111f4dc3abf5fec9657b72bdd86cede Reviewed-on: https://chromium-review.googlesource.com/1016302 Reviewed-by: Toni Barzic <tbarzic@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#551752} [modify] https://crrev.com/c380bcec772d00621fd6c84689be5bc2c057fb13/chrome/browser/extensions/api/screenlock_private/screenlock_private_apitest.cc
,
Apr 19 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/05915358214ab8637533d1f2f55cf2af477c377f commit 05915358214ab8637533d1f2f55cf2af477c377f Author: Marc Treib <treib@chromium.org> Date: Thu Apr 19 14:06:21 2018 ProfileSyncService: Use PrimaryAccountAccessTokenFetcher This CL migrates ProfileSyncService to use identity::PrimaryAccountAccessTokenFetcher instead of talking to ProfileOAuth2TokenService directly (via OAuth2TokenService::Consumer). This removes one OAuth2TokenService dependency; the remaining ones will follow in other CLs. Bug: 825190 Change-Id: I58da7f6aefed693455843092e1a6bb59b94f83d6 Reviewed-on: https://chromium-review.googlesource.com/1018944 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#552003} [modify] https://crrev.com/05915358214ab8637533d1f2f55cf2af477c377f/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/05915358214ab8637533d1f2f55cf2af477c377f/components/browser_sync/profile_sync_service.h [modify] https://crrev.com/05915358214ab8637533d1f2f55cf2af477c377f/components/browser_sync/profile_sync_service_unittest.cc
,
Apr 23 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/523c662a78133425ffead1c689acfb3058762023 commit 523c662a78133425ffead1c689acfb3058762023 Author: Marc Treib <treib@chromium.org> Date: Mon Apr 23 11:03:31 2018 Migrate chromeos::UserSessionManager to IdentityManager identity::IdentityManager is the new API that replaces SigninManager[Base]. Bug: 825190 , 796544, 814787 Change-Id: If6dff8e3d7e93c21bbc7392bf434dfe73da9f92d Reviewed-on: https://chromium-review.googlesource.com/1013575 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Xiyuan Xia <xiyuan@chromium.org> Reviewed-by: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/heads/master@{#552667} [modify] https://crrev.com/523c662a78133425ffead1c689acfb3058762023/chrome/browser/chromeos/login/session/user_session_manager.cc [modify] https://crrev.com/523c662a78133425ffead1c689acfb3058762023/services/identity/public/cpp/identity_manager.h
,
Apr 23 2018
,
Apr 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b218f3e0f3d8d80733e8391274eda6cc17e17dd8 commit b218f3e0f3d8d80733e8391274eda6cc17e17dd8 Author: Marc Treib <treib@chromium.org> Date: Tue Apr 24 11:39:42 2018 Update ProfileSyncService unit tests to use IdentityManager Bug: 825190 , 814787 Change-Id: I733c0dc3baca101b9ebe1b2448a31fadf815ae58 Reviewed-on: https://chromium-review.googlesource.com/1024835 Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#553051} [modify] https://crrev.com/b218f3e0f3d8d80733e8391274eda6cc17e17dd8/components/browser_sync/BUILD.gn [modify] https://crrev.com/b218f3e0f3d8d80733e8391274eda6cc17e17dd8/components/browser_sync/profile_sync_service_autofill_unittest.cc [modify] https://crrev.com/b218f3e0f3d8d80733e8391274eda6cc17e17dd8/components/browser_sync/profile_sync_service_startup_unittest.cc [modify] https://crrev.com/b218f3e0f3d8d80733e8391274eda6cc17e17dd8/services/identity/public/cpp/identity_manager.h
,
Apr 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/46a15abd9382ae51aabc3b16823b4724b7f93b50 commit 46a15abd9382ae51aabc3b16823b4724b7f93b50 Author: Marc Treib <treib@chromium.org> Date: Tue Apr 24 15:10:08 2018 Fix DCHECKs in IdentityManager::GetPrimaryAccountInfo On ChromeOS, IdentityManager::GetPrimaryAccountInfo DCHECKs that its view of the primary account info matches the one in SigninManagerBase. However, if the primary account's refresh token gets revoked, then the account gets removed from the AccountTrackerService, and SigninManagerBase will only remember the account's ID, but not the full AccountInfo. This CL updates the DCHECKs so that they only verify the account ID in this case. Bug: 825190 , 806775 Change-Id: I6cbde518e064b590109ef8ca165a1106c7ebda83 Reviewed-on: https://chromium-review.googlesource.com/1025094 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Cr-Commit-Position: refs/heads/master@{#553116} [modify] https://crrev.com/46a15abd9382ae51aabc3b16823b4724b7f93b50/chrome/browser/extensions/api/identity/identity_apitest.cc [modify] https://crrev.com/46a15abd9382ae51aabc3b16823b4724b7f93b50/services/identity/public/cpp/identity_manager.cc
,
Apr 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5c04cc4f176bb55bf6ab6d943d3919abc6721005 commit 5c04cc4f176bb55bf6ab6d943d3919abc6721005 Author: Marc Treib <treib@chromium.org> Date: Tue Apr 24 15:29:40 2018 Migrate ProfileSyncServiceTest to IdentityManager Bug: 825190 Change-Id: I5765b97568ce62ac0431aacfd6dbf01e7e5cd26a Reviewed-on: https://chromium-review.googlesource.com/1025654 Reviewed-by: Jan Krcal <jkrcal@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#553131} [modify] https://crrev.com/5c04cc4f176bb55bf6ab6d943d3919abc6721005/components/browser_sync/profile_sync_service_unittest.cc
,
Apr 25 2018
,
Apr 25 2018
,
Apr 25 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d697bdd75db3145a2d02278f1a435f1349449319 commit d697bdd75db3145a2d02278f1a435f1349449319 Author: Marc Treib <treib@chromium.org> Date: Wed Apr 25 15:58:24 2018 Migrate MultiProfileFileManagerBrowserTest to IdentityManager identity::IdentityManager is the new API that replaces SigninManager[Base]. Bug: 825190 , 814307 Change-Id: I3cf7261fad64a63bdc449c190f7d175025408d7c Reviewed-on: https://chromium-review.googlesource.com/1025751 Reviewed-by: Kazuhiro Inaba <kinaba@chromium.org> Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#553581} [modify] https://crrev.com/d697bdd75db3145a2d02278f1a435f1349449319/chrome/browser/chromeos/file_manager/file_manager_browsertest.cc [modify] https://crrev.com/d697bdd75db3145a2d02278f1a435f1349449319/services/identity/public/cpp/identity_manager.h
,
Apr 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1e3550d572933d5dd57a47ce32d7a56f53f01887 commit 1e3550d572933d5dd57a47ce32d7a56f53f01887 Author: Marc Treib <treib@chromium.org> Date: Thu Apr 26 08:14:01 2018 Migrate Arc unit tests to IdentityManager identity::IdentityManager is the new API that replaces SigninManager[Base]. This includes ArcTermsOfServiceDefaultNegotiatorTest and ArcSupportHostTest. Bug: 825190 , 731023 Change-Id: I316c90c6d278e52d723468d63bc21cd288b2642b Reviewed-on: https://chromium-review.googlesource.com/1025898 Reviewed-by: Yusuke Sato <yusukes@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#553949} [modify] https://crrev.com/1e3550d572933d5dd57a47ce32d7a56f53f01887/chrome/browser/chromeos/arc/arc_support_host_unittest.cc [modify] https://crrev.com/1e3550d572933d5dd57a47ce32d7a56f53f01887/chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator_unittest.cc
,
Apr 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a69dac0d9ec7cbae1f079b6d04bc2a2d6c1cf2cc commit a69dac0d9ec7cbae1f079b6d04bc2a2d6c1cf2cc Author: Marc Treib <treib@chromium.org> Date: Thu Apr 26 09:39:52 2018 Migrate MultiProfileDownloadNotificationTest to IdentityManager identity::IdentityManager is the new API that replaces SigninManager[Base]. The changes here are copied almost verbatim from crrev.com/c/1025751. Bug: 825190 , 814307 Change-Id: If68afd361fd457284fdb0ed7ef41a4d121055a84 Reviewed-on: https://chromium-review.googlesource.com/1025693 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Reviewed-by: David Trainor <dtrainor@chromium.org> Cr-Commit-Position: refs/heads/master@{#553964} [modify] https://crrev.com/a69dac0d9ec7cbae1f079b6d04bc2a2d6c1cf2cc/chrome/browser/download/notification/download_notification_interactive_uitest.cc [modify] https://crrev.com/a69dac0d9ec7cbae1f079b6d04bc2a2d6c1cf2cc/services/identity/public/cpp/identity_manager.h
,
Apr 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/aab579140be16a85c587bbdc696f9393ba2bc08c commit aab579140be16a85c587bbdc696f9393ba2bc08c Author: Marc Treib <treib@chromium.org> Date: Thu Apr 26 10:43:38 2018 Sync: Use IdentityManager instead of SigninManager This CL migrates ProfileSyncService to IdentityManager for getting info on the primary account. This includes switching from SigninManagerBase::Observer to IdentityManager::Observer. The only remaining use of SigninManager within PSS it to call SignOut() which isn't exposed by IdentityManager yet. Bug: 825190 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: Icf9398d63794cab03cb559adba5c767e18c31c0a Reviewed-on: https://chromium-review.googlesource.com/983914 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#553973} [modify] https://crrev.com/aab579140be16a85c587bbdc696f9393ba2bc08c/chrome/browser/sync/test/integration/profile_sync_service_harness.cc [modify] https://crrev.com/aab579140be16a85c587bbdc696f9393ba2bc08c/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/aab579140be16a85c587bbdc696f9393ba2bc08c/components/browser_sync/profile_sync_service.h [modify] https://crrev.com/aab579140be16a85c587bbdc696f9393ba2bc08c/components/browser_sync/profile_sync_service_unittest.cc [modify] https://crrev.com/aab579140be16a85c587bbdc696f9393ba2bc08c/components/sync/driver/signin_manager_wrapper.cc [modify] https://crrev.com/aab579140be16a85c587bbdc696f9393ba2bc08c/components/sync/driver/sync_service_base.cc
,
Apr 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/78ad1e6721949ebe27212347bed02ab4a08fcc7a commit 78ad1e6721949ebe27212347bed02ab4a08fcc7a Author: Max Morin <maxmorin@chromium.org> Date: Thu Apr 26 11:20:28 2018 Revert "Migrate Arc unit tests to IdentityManager" This reverts commit 1e3550d572933d5dd57a47ce32d7a56f53f01887. Reason for revert: Failing tests, see crbug.com/837201 Original change's description: > Migrate Arc unit tests to IdentityManager > > identity::IdentityManager is the new API that replaces SigninManager[Base]. > > This includes ArcTermsOfServiceDefaultNegotiatorTest and ArcSupportHostTest. > > Bug: 825190 , 731023 > Change-Id: I316c90c6d278e52d723468d63bc21cd288b2642b > Reviewed-on: https://chromium-review.googlesource.com/1025898 > Reviewed-by: Yusuke Sato <yusukes@chromium.org> > Commit-Queue: Marc Treib <treib@chromium.org> > Cr-Commit-Position: refs/heads/master@{#553949} TBR=yusukes@chromium.org,treib@chromium.org Change-Id: Iba9649631ec8633c0b89b867a60e89d3d63a01ad No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 825190 , 731023, 837201 Reviewed-on: https://chromium-review.googlesource.com/1030150 Reviewed-by: Max Morin <maxmorin@chromium.org> Commit-Queue: Max Morin <maxmorin@chromium.org> Cr-Commit-Position: refs/heads/master@{#553979} [modify] https://crrev.com/78ad1e6721949ebe27212347bed02ab4a08fcc7a/chrome/browser/chromeos/arc/arc_support_host_unittest.cc [modify] https://crrev.com/78ad1e6721949ebe27212347bed02ab4a08fcc7a/chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator_unittest.cc
,
Apr 26 2018
,
Apr 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fd8a3a1f08a5836029eaee2f6f7dbf24715ddad6 commit fd8a3a1f08a5836029eaee2f6f7dbf24715ddad6 Author: Marc Treib <treib@chromium.org> Date: Thu Apr 26 12:49:02 2018 Revert "Sync: Use IdentityManager instead of SigninManager" This reverts commit aab579140be16a85c587bbdc696f9393ba2bc08c. Reason for revert: Depends on crrev.com/c/1025898 which was reverted Original change's description: > Sync: Use IdentityManager instead of SigninManager > > This CL migrates ProfileSyncService to IdentityManager for getting > info on the primary account. This includes switching from > SigninManagerBase::Observer to IdentityManager::Observer. > The only remaining use of SigninManager within PSS it to call > SignOut() which isn't exposed by IdentityManager yet. > > Bug: 825190 > Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs > Change-Id: Icf9398d63794cab03cb559adba5c767e18c31c0a > Reviewed-on: https://chromium-review.googlesource.com/983914 > Commit-Queue: Marc Treib <treib@chromium.org> > Reviewed-by: Mikel Astiz <mastiz@chromium.org> > Cr-Commit-Position: refs/heads/master@{#553973} TBR=blundell@chromium.org,treib@chromium.org,mastiz@chromium.org Change-Id: Ib40dbff0f1c8e5fba95cb9881681361abec5aeec No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 825190 , 837201 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Reviewed-on: https://chromium-review.googlesource.com/1030372 Reviewed-by: Marc Treib <treib@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#553997} [modify] https://crrev.com/fd8a3a1f08a5836029eaee2f6f7dbf24715ddad6/chrome/browser/sync/test/integration/profile_sync_service_harness.cc [modify] https://crrev.com/fd8a3a1f08a5836029eaee2f6f7dbf24715ddad6/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/fd8a3a1f08a5836029eaee2f6f7dbf24715ddad6/components/browser_sync/profile_sync_service.h [modify] https://crrev.com/fd8a3a1f08a5836029eaee2f6f7dbf24715ddad6/components/browser_sync/profile_sync_service_unittest.cc [modify] https://crrev.com/fd8a3a1f08a5836029eaee2f6f7dbf24715ddad6/components/sync/driver/signin_manager_wrapper.cc [modify] https://crrev.com/fd8a3a1f08a5836029eaee2f6f7dbf24715ddad6/components/sync/driver/sync_service_base.cc
,
Apr 30 2018
Note on 4. (SigninManager::SignOut call): There's bug 246839 to change the behavior to just stop Sync instead of fully signing out. So if we do that first, then we don't need SignOut support in IdentityManager.
,
May 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a5d0c430e0a16f0c67bea814c5674c3f96dd259b commit a5d0c430e0a16f0c67bea814c5674c3f96dd259b Author: Marc Treib <treib@chromium.org> Date: Fri May 04 09:06:46 2018 Reland "Migrate Arc unit tests to IdentityManager" Original CL: https://crrev.com/c/1025898 Revert: https://crrev.com/c/1030150 Patchset 1 contains the original CL. What's changed from there is to set up the primary account without providing a refresh token (like the tests did before this change). Original change's description: > Revert "Migrate Arc unit tests to IdentityManager" > > This reverts commit 1e3550d572933d5dd57a47ce32d7a56f53f01887. > > Reason for revert: Failing tests, see crbug.com/837201 > > Original change's description: > > Migrate Arc unit tests to IdentityManager > > > > identity::IdentityManager is the new API that replaces SigninManager[Base]. > > > > This includes ArcTermsOfServiceDefaultNegotiatorTest and ArcSupportHostTest. > > > > Bug: 825190 , 731023 > > Change-Id: I316c90c6d278e52d723468d63bc21cd288b2642b > > Reviewed-on: https://chromium-review.googlesource.com/1025898 > > Reviewed-by: Yusuke Sato <yusukes@chromium.org> > > Commit-Queue: Marc Treib <treib@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#553949} > > TBR=yusukes@chromium.org,treib@chromium.org > > Change-Id: Iba9649631ec8633c0b89b867a60e89d3d63a01ad > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: 825190 , 731023, 837201 > Reviewed-on: https://chromium-review.googlesource.com/1030150 > Reviewed-by: Max Morin <maxmorin@chromium.org> > Commit-Queue: Max Morin <maxmorin@chromium.org> > Cr-Commit-Position: refs/heads/master@{#553979} Bug: 825190 , 731023, 837201 Change-Id: I83753ba9036eb9f93ba32c74302dbd151b03c0bf Reviewed-on: https://chromium-review.googlesource.com/1030451 Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Reviewed-by: Yusuke Sato <yusukes@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#556017} [modify] https://crrev.com/a5d0c430e0a16f0c67bea814c5674c3f96dd259b/chrome/browser/chromeos/arc/arc_support_host_unittest.cc [modify] https://crrev.com/a5d0c430e0a16f0c67bea814c5674c3f96dd259b/chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator_unittest.cc [modify] https://crrev.com/a5d0c430e0a16f0c67bea814c5674c3f96dd259b/services/identity/public/cpp/identity_manager.h
,
May 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0a3600c1bf36d80734992e8998447ac3edff997c commit 0a3600c1bf36d80734992e8998447ac3edff997c Author: Marc Treib <treib@chromium.org> Date: Fri May 04 11:12:28 2018 Reland "Sync: Use IdentityManager instead of SigninManager" This reverts commit fd8a3a1f08a5836029eaee2f6f7dbf24715ddad6. Reason for revert: The depended-on CL has been relanded as https://crrev.com/c/1030451 Original change's description: > Revert "Sync: Use IdentityManager instead of SigninManager" > > This reverts commit aab579140be16a85c587bbdc696f9393ba2bc08c. > > Reason for revert: Depends on crrev.com/c/1025898 which was reverted > > Original change's description: > > Sync: Use IdentityManager instead of SigninManager > > > > This CL migrates ProfileSyncService to IdentityManager for getting > > info on the primary account. This includes switching from > > SigninManagerBase::Observer to IdentityManager::Observer. > > The only remaining use of SigninManager within PSS it to call > > SignOut() which isn't exposed by IdentityManager yet. > > > > Bug: 825190 > > Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs > > Change-Id: Icf9398d63794cab03cb559adba5c767e18c31c0a > > Reviewed-on: https://chromium-review.googlesource.com/983914 > > Commit-Queue: Marc Treib <treib@chromium.org> > > Reviewed-by: Mikel Astiz <mastiz@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#553973} > > TBR=blundell@chromium.org,treib@chromium.org,mastiz@chromium.org > > Change-Id: Ib40dbff0f1c8e5fba95cb9881681361abec5aeec > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: 825190 , 837201 > Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs > Reviewed-on: https://chromium-review.googlesource.com/1030372 > Reviewed-by: Marc Treib <treib@chromium.org> > Commit-Queue: Marc Treib <treib@chromium.org> > Cr-Commit-Position: refs/heads/master@{#553997} TBR=blundell@chromium.org,treib@chromium.org,mastiz@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 825190 , 837201 Change-Id: If6858f033ac8390acfc904379afbf9726a6301c2 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Reviewed-on: https://chromium-review.googlesource.com/1043885 Reviewed-by: Marc Treib <treib@chromium.org> Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#556026} [modify] https://crrev.com/0a3600c1bf36d80734992e8998447ac3edff997c/chrome/browser/sync/test/integration/profile_sync_service_harness.cc [modify] https://crrev.com/0a3600c1bf36d80734992e8998447ac3edff997c/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/0a3600c1bf36d80734992e8998447ac3edff997c/components/browser_sync/profile_sync_service.h [modify] https://crrev.com/0a3600c1bf36d80734992e8998447ac3edff997c/components/browser_sync/profile_sync_service_unittest.cc [modify] https://crrev.com/0a3600c1bf36d80734992e8998447ac3edff997c/components/sync/driver/signin_manager_wrapper.cc [modify] https://crrev.com/0a3600c1bf36d80734992e8998447ac3edff997c/components/sync/driver/sync_service_base.cc
,
May 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/95d7666a63d555c0fb7a606fda867bc3cb0693d0 commit 95d7666a63d555c0fb7a606fda867bc3cb0693d0 Author: Marc Treib <treib@chromium.org> Date: Mon May 07 12:59:27 2018 ProfileSyncService: Get access tokens via IdentityManager instead of talking to ProfileOAuth2TokenService directly. Bug: 825190 Change-Id: I80f1e52857f3fd8effbb6351a22e5bd86062d326 Reviewed-on: https://chromium-review.googlesource.com/1028150 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/heads/master@{#556422} [modify] https://crrev.com/95d7666a63d555c0fb7a606fda867bc3cb0693d0/components/browser_sync/profile_sync_service.cc
,
May 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/03745aacfdd53c11ad10c4d5539912b701b4da71 commit 03745aacfdd53c11ad10c4d5539912b701b4da71 Author: Marc Treib <treib@chromium.org> Date: Mon May 07 15:34:46 2018 Cleanup: remove SigninManagerWrapper methods for querying account ID/email Instead, use GetAuthenticatedAccountInfo throughout ProfileSyncService. One more step on the way to eventually getting rid of SigninManagerWrapper. Bug: 825190 Change-Id: Icca7037b2f47f1bba3c549080f97ba3824ad9f77 Reviewed-on: https://chromium-review.googlesource.com/1046950 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#556449} [modify] https://crrev.com/03745aacfdd53c11ad10c4d5539912b701b4da71/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/03745aacfdd53c11ad10c4d5539912b701b4da71/components/sync/driver/signin_manager_wrapper.cc [modify] https://crrev.com/03745aacfdd53c11ad10c4d5539912b701b4da71/components/sync/driver/signin_manager_wrapper.h
,
May 8 2018
Taking stock once again: There's one remaining usage of SigninManager, to call SignOut when receiving DISABLE_SYNC_ON_CLIENT from the server. We might be able to just get rid of that, see bug 246839. There are several remaining uses of OAuth2TokenService: - CanEngineStart uses RefreshTokenIsAvailable (plus a related use in OnPrimaryAccountSet): Currently engine initialization can only happen once the refresh token is there. I'm hoping this can be avoided by switching from PrimaryAccountAccessTokenFetcher::Mode::kImmediate to kWaitUntilAvailable. - We register as an observer, to get notified of refresh token changes. The above should also make OnRefreshTokenAvailable and OnRefreshTokensLoaded unnecessary, but I don't know about OnRefreshTokenRevoked. This might have to be added to IdentityManager ( bug 806775 ). - There's a new call to OAuth2TokenService::GetAuthError in OnRefreshTokensLoaded for DICE: If the refresh token was locally rejected because the user signed out of the content area, then Sync has to stop. In practice, the refresh token is replaced by a special dummy one, so this isn't automatically handled by checking for a refresh token being available. Maybe we need to expose auth errors from IdentityManager?
,
May 8 2018
,
May 8 2018
,
May 25 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/741892b17ef4f6bab06fd593ac40346c80b5e0f3 commit 741892b17ef4f6bab06fd593ac40346c80b5e0f3 Author: Marc Treib <treib@chromium.org> Date: Fri May 25 20:48:10 2018 ProfileSyncService: Remove CanEngineStart CanEngineStart specified the required conditions for creating the SyncEngine object: CanSyncStart() plus a refresh token being available. The refresh token condition was there because creating the engine will trigger a request for an access token. Previously, that request would fail in unhelpful ways if the refresh token wasn't loaded yet. Now however, PrimaryAccountAccessTokenFetcher handles that situation just fine, it'll simply wait for the refresh token. So this CL removes the refresh token condition from ProfileSyncService. Bug: 825190 , 842697 Change-Id: Ica57945f93efcdf8597c1bdfdbda19a0b879e6d9 Reviewed-on: https://chromium-review.googlesource.com/997796 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Achuith Bhandarkar <achuith@chromium.org> Reviewed-by: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#562000} [modify] https://crrev.com/741892b17ef4f6bab06fd593ac40346c80b5e0f3/chrome/browser/chromeos/profiles/profile_helper.cc [modify] https://crrev.com/741892b17ef4f6bab06fd593ac40346c80b5e0f3/chrome/browser/ui/webui/sync_internals_browsertest.js [modify] https://crrev.com/741892b17ef4f6bab06fd593ac40346c80b5e0f3/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/741892b17ef4f6bab06fd593ac40346c80b5e0f3/components/browser_sync/profile_sync_service.h [modify] https://crrev.com/741892b17ef4f6bab06fd593ac40346c80b5e0f3/components/browser_sync/profile_sync_service_mock.h [modify] https://crrev.com/741892b17ef4f6bab06fd593ac40346c80b5e0f3/components/browser_sync/profile_sync_service_startup_unittest.cc [modify] https://crrev.com/741892b17ef4f6bab06fd593ac40346c80b5e0f3/components/browser_sync/sync_auth_manager.cc [modify] https://crrev.com/741892b17ef4f6bab06fd593ac40346c80b5e0f3/components/browser_sync/sync_auth_manager.h
,
May 29 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9598bfd62f139a2b99184655ddb4963c43809272 commit 9598bfd62f139a2b99184655ddb4963c43809272 Author: Marc Treib <treib@chromium.org> Date: Tue May 29 07:28:24 2018 ProfileSyncService: TryStart in OnPrimaryAccountSet, not OnRefreshTokenAvailable After https://crrev.com/c/997796, Sync startup doesn't depend on refresh token availability anymore, so OnRefreshTokenAvailable is not a relevant startup signal. The primary account changing *is* a valid signal though, per the IsSignedIn() condition in CanSyncStart(). This is another step towards making ProfileSyncService refresh-token-agnostic. Bug: 825190 , 842697 Change-Id: I0e5e001e54ec54301a0a6f1333718b5a7e3b13c4 Reviewed-on: https://chromium-review.googlesource.com/1073413 Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#562359} [modify] https://crrev.com/9598bfd62f139a2b99184655ddb4963c43809272/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/9598bfd62f139a2b99184655ddb4963c43809272/components/browser_sync/profile_sync_service.h [modify] https://crrev.com/9598bfd62f139a2b99184655ddb4963c43809272/components/browser_sync/profile_sync_service_startup_unittest.cc [modify] https://crrev.com/9598bfd62f139a2b99184655ddb4963c43809272/components/sync/engine/net/http_bridge.cc [modify] https://crrev.com/9598bfd62f139a2b99184655ddb4963c43809272/components/sync/engine/net/http_bridge.h
,
Jun 5 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9711f3c628f46fda1c20e865ecfa74f0d9415b7c commit 9711f3c628f46fda1c20e865ecfa74f0d9415b7c Author: Marc Treib <treib@chromium.org> Date: Tue Jun 05 08:18:20 2018 Remove ProfileSyncService::OnRefreshTokenAvailable Instead, implement the necessary logic in SyncAuthManager. This also allows us to make SyncAuthManager::RequestAccessToken private, reducing the API between ProfileSyncService and SyncAuthManager by 2 methods. Bug: 842697 , 825190 ) Change-Id: Id2ba5dc26d8cb2a1abde8566bade8f9cca763d36 Reviewed-on: https://chromium-review.googlesource.com/1085306 Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#564402} [modify] https://crrev.com/9711f3c628f46fda1c20e865ecfa74f0d9415b7c/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/9711f3c628f46fda1c20e865ecfa74f0d9415b7c/components/browser_sync/profile_sync_service.h [modify] https://crrev.com/9711f3c628f46fda1c20e865ecfa74f0d9415b7c/components/browser_sync/sync_auth_manager.cc [modify] https://crrev.com/9711f3c628f46fda1c20e865ecfa74f0d9415b7c/components/browser_sync/sync_auth_manager.h
,
Jun 6 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f0bd58341f6ae77974df0d01cf7c8e456c570101 commit f0bd58341f6ae77974df0d01cf7c8e456c570101 Author: Colin Blundell <blundell@chromium.org> Date: Wed Jun 06 11:34:16 2018 Sync: Don't call OnRefreshTokenAvailable when primary account is set This CL removes the callthrough from SyncAuthManager::OnPrimaryAccountSet() to SyncAuthManager::OnRefreshTokenAvailable(). The former currently calls through to the latter when a refresh token is available. However, that call will be a no-op. OnRefreshTokenAvailable() does two things: 1. If the primary account is updated with an invalid refresh token, it takes action to stop sync. 2. If the primary account is updated with a valid refresh token, it checks if there is a need to fetch a new access token and fetches one if so. Neither of these are relevant when SyncAuthManager::OnPrimaryAccountSet() is invoked: - The flow resulting in 1 is started only in the context of there already being a primary account. - For 2, there will by design never be "a need to fetch a new access token" at the time of SyncAuthManager::OnPrimaryAccountSet(): at the time that the primary account is set we don't know whether Sync actually wants to start, e.g., the user might have explicitly disabled it. The initial access token fetch occurs as follows: The SyncEngine tries to connect to the server, but has no access token, so it ends up calling OnConnectionStatusChange(syncer::CONNECTION_AUTH_ERROR) which in turn causes SyncAuthManager to request a new access token. The sum of this reasoning is that SyncAuthManager::OnPrimaryAccountSet's callthrough to SyncAuthManager::OnRefreshTokenAvailable() is a no-op and can be removed. The concrete motivation for this CL is to facilitate porting this class to observe IdentityManager for token-related events. Bug: 825190 Change-Id: I77964296957facf22c60b2a62775c034af9e47a0 Reviewed-on: https://chromium-review.googlesource.com/1087964 Commit-Queue: Colin Blundell <blundell@chromium.org> Reviewed-by: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#564855} [modify] https://crrev.com/f0bd58341f6ae77974df0d01cf7c8e456c570101/components/browser_sync/sync_auth_manager.cc
,
Jun 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/170a5cffee97d1b52b29b98f1cc12c10ef8c4b42 commit 170a5cffee97d1b52b29b98f1cc12c10ef8c4b42 Author: Colin Blundell <blundell@chromium.org> Date: Thu Jun 07 11:18:00 2018 [Sync] Observe IdentityManager for refresh token updates As part of porting sync to depend only on IdentityManager for interaction with the user's Google accounts, this CL ports SyncAuthManager from observing ProfileOAuth2TokenService to observing IdentityManager for refresh token update/removal events. The conversion is straightforward. The one detail is that IdentityManager passes the high-level information of whether the token is valid or not in its "token updated" callback, but SyncAuthManager needs to cache the actual GoogleServiceAuthError that corresponds to the token being invalid. As we don't desire to change IdentityManager to pass such low-level information at this time, we simply do the mapping in SyncAuthManager. This mapping is a safe one to do as the fact that this error corresponds to an invalidated refresh token is publicly documented in google_service_auth_error.h. Bug: 825190 Change-Id: Ia1f34a284dc8f38af899582c0b22b266b248dcd3 Reviewed-on: https://chromium-review.googlesource.com/1086806 Reviewed-by: Marc Treib <treib@chromium.org> Commit-Queue: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/heads/master@{#565230} [modify] https://crrev.com/170a5cffee97d1b52b29b98f1cc12c10ef8c4b42/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/170a5cffee97d1b52b29b98f1cc12c10ef8c4b42/components/browser_sync/sync_auth_manager.cc [modify] https://crrev.com/170a5cffee97d1b52b29b98f1cc12c10ef8c4b42/components/browser_sync/sync_auth_manager.h
,
Jun 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/52999163957ec966b4d6ebf82e5eca5231d0d195 commit 52999163957ec966b4d6ebf82e5eca5231d0d195 Author: Colin Blundell <blundell@chromium.org> Date: Thu Jun 07 13:30:55 2018 Remove sync's knowledge of OAuth2TokenService As a cumulation of all the recent changes that have occurred porting Sync to talk to IdentityManager, Sync production code no longer needs to have knowledge of (Profile)OAuth2TokenService. This CL removes all remaining production dependencies, which are now unused. Sync still has test dependencies on PO2TS for exercising the production code. Over time, we will migrate that code to use the test facilities for interacting with IdentityManager. Bug: 825190 Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: I1d3820cd4fe157c953590ebdbc813443dfe9e7d9 Reviewed-on: https://chromium-review.googlesource.com/1090281 Reviewed-by: Marc Treib <treib@chromium.org> Commit-Queue: Colin Blundell <blundell@chromium.org> Cr-Commit-Position: refs/heads/master@{#565248} [modify] https://crrev.com/52999163957ec966b4d6ebf82e5eca5231d0d195/chrome/browser/sync/profile_sync_service_factory.cc [modify] https://crrev.com/52999163957ec966b4d6ebf82e5eca5231d0d195/chrome/browser/sync/profile_sync_test_util.cc [modify] https://crrev.com/52999163957ec966b4d6ebf82e5eca5231d0d195/components/browser_sync/profile_sync_service.h [modify] https://crrev.com/52999163957ec966b4d6ebf82e5eca5231d0d195/components/browser_sync/profile_sync_service_unittest.cc [modify] https://crrev.com/52999163957ec966b4d6ebf82e5eca5231d0d195/components/browser_sync/profile_sync_test_util.cc [modify] https://crrev.com/52999163957ec966b4d6ebf82e5eca5231d0d195/ios/chrome/browser/sync/ios_chrome_profile_sync_test_util.cc [modify] https://crrev.com/52999163957ec966b4d6ebf82e5eca5231d0d195/ios/chrome/browser/sync/profile_sync_service_factory.cc
,
Jun 12 2018
Documenting the state once again: There's one remaining usage of SigninManager::SignOut when receiving DISABLE_SYNC_ON_CLIENT from the server. We might eventually be able to just get rid of that, see bug 246839. In the meantime, this doesn't block bug 840703 anymore, so removing that and reducing prio.
,
Jun 12 2018
Actually, let's just merge this into bug 809031 , which was almost a duplicate to begin with. |
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by treib@chromium.org
, Mar 23 2018