Extract account/token management from ProfileSyncService |
||
Issue descriptionProfileSyncService does too much (it implements 8 interfaces currently), and account/token handling seems like something that can nicely be extracted into a separate class.
,
May 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/30ae28eccbfd2dd18d76694c4e01ba4b739d38cd commit 30ae28eccbfd2dd18d76694c4e01ba4b739d38cd Author: Marc Treib <treib@chromium.org> Date: Thu May 17 18:07:50 2018 ProfileSyncService: Pull account tracking into a separate class This introduces a new class SyncAuthManager which keeps track of the primary account and its refresh token and auth error state. As a consequence, ProfileSyncService doesn't need to implement IdentityManager::Observer and OAuth2TokenService::Observer anymore. Bug: 842697 Change-Id: I02792420d18bf1d5a28280466d456c5b26134ec9 Reviewed-on: https://chromium-review.googlesource.com/1057508 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#559606} [modify] https://crrev.com/30ae28eccbfd2dd18d76694c4e01ba4b739d38cd/chrome/browser/sync/test/integration/profile_sync_service_harness.cc [modify] https://crrev.com/30ae28eccbfd2dd18d76694c4e01ba4b739d38cd/components/browser_sync/BUILD.gn [modify] https://crrev.com/30ae28eccbfd2dd18d76694c4e01ba4b739d38cd/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/30ae28eccbfd2dd18d76694c4e01ba4b739d38cd/components/browser_sync/profile_sync_service.h [add] https://crrev.com/30ae28eccbfd2dd18d76694c4e01ba4b739d38cd/components/browser_sync/sync_auth_manager.cc [add] https://crrev.com/30ae28eccbfd2dd18d76694c4e01ba4b739d38cd/components/browser_sync/sync_auth_manager.h [modify] https://crrev.com/30ae28eccbfd2dd18d76694c4e01ba4b739d38cd/components/suggestions/suggestions_service_impl.cc
,
May 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/938887e6d723ecc8ad4c6699630176c0ef258cc0 commit 938887e6d723ecc8ad4c6699630176c0ef258cc0 Author: Marc Treib <treib@chromium.org> Date: Fri May 18 15:58:45 2018 ProfileSyncService: Move access token handling into SyncAuthManager Followup to https://crrev.com/c/1057508 Bug: 842697 Change-Id: Iec5b08efccf62427b4e1069ea829c7fadad02deb Reviewed-on: https://chromium-review.googlesource.com/1064062 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#559908} [modify] https://crrev.com/938887e6d723ecc8ad4c6699630176c0ef258cc0/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/938887e6d723ecc8ad4c6699630176c0ef258cc0/components/browser_sync/profile_sync_service.h [modify] https://crrev.com/938887e6d723ecc8ad4c6699630176c0ef258cc0/components/browser_sync/sync_auth_manager.cc [modify] https://crrev.com/938887e6d723ecc8ad4c6699630176c0ef258cc0/components/browser_sync/sync_auth_manager.h
,
May 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2a2f1152b74bab217edb57d6283606163628b7fc commit 2a2f1152b74bab217edb57d6283606163628b7fc Author: Marc Treib <treib@chromium.org> Date: Tue May 22 11:20:08 2018 SyncAuthManager: Consistently clear the access token and pending requests Previously, SyncAuthManager sometimes cleared the access token, but left any pending requests running. Some other times, it canceled an ongoing request but left a scheduled one around, etc. This CL adds a ClearAccessTokenAndRequest helper that's called in all those cases. Bug: 842697 Change-Id: Ie694fb7440aefb65da8c3bff17173444a7b510d0 Reviewed-on: https://chromium-review.googlesource.com/1065877 Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#560532} [modify] https://crrev.com/2a2f1152b74bab217edb57d6283606163628b7fc/components/browser_sync/sync_auth_manager.cc [modify] https://crrev.com/2a2f1152b74bab217edb57d6283606163628b7fc/components/browser_sync/sync_auth_manager.h
,
May 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7931ac3005447e58354c3cc26cabd70b4153dc55 commit 7931ac3005447e58354c3cc26cabd70b4153dc55 Author: Marc Treib <treib@chromium.org> Date: Tue May 22 13:49:02 2018 Cleanup: Remove ProfileSyncService::GetCredentials After https://crrev.com/c/1057508, it was only a thin wrapper around SyncAuthManager::GetCredentials, plus some special handling for local Sync that isn't actually required. This also removes a bad and unnecessary DCHECK in SyncAuthManager::GetCredentials. Bug: 842697 Change-Id: Ic3ad20991a7484513ac4c4af44b2fd5c6a287d0a Reviewed-on: https://chromium-review.googlesource.com/1066090 Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#560551} [modify] https://crrev.com/7931ac3005447e58354c3cc26cabd70b4153dc55/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/7931ac3005447e58354c3cc26cabd70b4153dc55/components/browser_sync/profile_sync_service.h [modify] https://crrev.com/7931ac3005447e58354c3cc26cabd70b4153dc55/components/browser_sync/sync_auth_manager.cc
,
May 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/991b707758c36eb277b5bd8d7f7045bbc73b8632 commit 991b707758c36eb277b5bd8d7f7045bbc73b8632 Author: Marc Treib <treib@chromium.org> Date: Tue May 22 15:59:43 2018 SyncAuthManager: don't do read-before-write for prefs PrefService checks the previous value anyway, no need for us to do it manually too. Bug: 842697 Change-Id: I32027065bb4eea9bb18dfb0837c6d053b09defd4 Reviewed-on: https://chromium-review.googlesource.com/1068744 Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#560615} [modify] https://crrev.com/991b707758c36eb277b5bd8d7f7045bbc73b8632/components/browser_sync/sync_auth_manager.cc
,
May 23 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f0cb06d238cd9e0a85c0cf2b9a611ad05ff7cf66 commit f0cb06d238cd9e0a85c0cf2b9a611ad05ff7cf66 Author: Marc Treib <treib@chromium.org> Date: Wed May 23 06:26:27 2018 SyncAuthManager: keep token_status_.next_token_request_time in the correct state This way, GetSyncTokenStatus can just return token_status_, rather than make a copy and fix it up. Bug: 842697 Change-Id: Ic0522cb83ee1da425d88b6e58e57d843720bcad7 Reviewed-on: https://chromium-review.googlesource.com/1068875 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#560967} [modify] https://crrev.com/f0cb06d238cd9e0a85c0cf2b9a611ad05ff7cf66/components/browser_sync/sync_auth_manager.cc [modify] https://crrev.com/f0cb06d238cd9e0a85c0cf2b9a611ad05ff7cf66/components/browser_sync/sync_auth_manager.h
,
May 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9fc857e8c3a17e9f175f8dfb2c6a466ef77b362a commit 9fc857e8c3a17e9f175f8dfb2c6a466ef77b362a Author: Marc Treib <treib@chromium.org> Date: Thu May 24 08:16:52 2018 ProfileSyncService: merge OnCredentialsRejectedByClient into OnRefreshTokenRevoked Before, OnRefreshTokenRevoked triggered a notification to observers, and OnCredentialsRejectedByClient caused us to invalidate the credentials in the SyncEngine. In both cases, it actually seems better to do both of these things, so this CL merges the two. Bug: 842697 Change-Id: I81fd4578731c282284fa40cbbb0be0e1cd102656 Reviewed-on: https://chromium-review.googlesource.com/1068880 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#561419} [modify] https://crrev.com/9fc857e8c3a17e9f175f8dfb2c6a466ef77b362a/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/9fc857e8c3a17e9f175f8dfb2c6a466ef77b362a/components/browser_sync/profile_sync_service.h [modify] https://crrev.com/9fc857e8c3a17e9f175f8dfb2c6a466ef77b362a/components/browser_sync/sync_auth_manager.cc
,
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 28 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/820a8547a9bfce2b145be941e1130a419bc667a1 commit 820a8547a9bfce2b145be941e1130a419bc667a1 Author: Marc Treib <treib@chromium.org> Date: Mon May 28 10:48:33 2018 SyncAuthManager: set auth error on CREDENTIALS_REJECTED_BY_CLIENT Before this CL, the CREDENTIALS_REJECTED_BY_CLIENT error was not set as the last auth error in SyncService/SyncAuthManager, so clients that check SyncService::GetAuthError() would not know of it, and might wrongly think that Sync was still active. In practice, Sync would usually try to fetch a new access token in this situation, which would of course fail, so the error would make it there eventually. But it seems better to explicitly set it immediately. Bug: 842697 , 845502 Change-Id: I25386959647e57a619ef2207098212c0f7c1af67 Reviewed-on: https://chromium-review.googlesource.com/1071747 Reviewed-by: David Roger <droger@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#562229} [modify] https://crrev.com/820a8547a9bfce2b145be941e1130a419bc667a1/components/browser_sync/profile_sync_service_unittest.cc [modify] https://crrev.com/820a8547a9bfce2b145be941e1130a419bc667a1/components/browser_sync/sync_auth_manager.cc
,
May 28 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/15ebf5136c9344573cefef745baa0fc233669d16 commit 15ebf5136c9344573cefef745baa0fc233669d16 Author: Marc Treib <treib@chromium.org> Date: Mon May 28 12:59:04 2018 SyncAuthManager: don't call OnRefreshTokenAvailable from OnRefreshTokensLoaded This call doesn't make sense semantically, and it isn't required. Some tests depended on this call; this CL improves them to closer resemble the actual sequence of events. Bug: 842697 Change-Id: I38eb94981955c3e465ac49a884c60e582b68475e Reviewed-on: https://chromium-review.googlesource.com/1073449 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#562240} [modify] https://crrev.com/15ebf5136c9344573cefef745baa0fc233669d16/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/15ebf5136c9344573cefef745baa0fc233669d16/components/browser_sync/profile_sync_service_unittest.cc [modify] https://crrev.com/15ebf5136c9344573cefef745baa0fc233669d16/components/browser_sync/sync_auth_manager.cc [modify] https://crrev.com/15ebf5136c9344573cefef745baa0fc233669d16/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 5 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e4b1c17d105fdf7f4dc3722a319731fa72f7b2b2 commit e4b1c17d105fdf7f4dc3722a319731fa72f7b2b2 Author: Marc Treib <treib@chromium.org> Date: Tue Jun 05 08:21:59 2018 Cleanup: Remove impossible case in ProfileSyncService::AccessTokenFetched Previously, ProfileSyncService::AccessTokenFetched contained a check whether the SyncEngine exists. However, we only ever have pending access token requests while the engine exists. So this CL replaces the "if" with a DCHECK. Bug: 842697 Change-Id: I65355b00fe427ab76d9aa9739a40b30be8a84b3a Reviewed-on: https://chromium-review.googlesource.com/1085448 Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#564404} [modify] https://crrev.com/e4b1c17d105fdf7f4dc3722a319731fa72f7b2b2/components/browser_sync/profile_sync_service.cc
,
Jun 5 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5fff27695a81d7089b1143785c3b93c265bd0e37 commit 5fff27695a81d7089b1143785c3b93c265bd0e37 Author: Marc Treib <treib@chromium.org> Date: Tue Jun 05 17:38:24 2018 Loosen the coupling between ProfileSyncService and SyncAuthManager Before, SyncAuthManager got a pointer to ProfileSyncService and called public methods on it. Now, it just gets two callbacks. With this, SyncAuthManager doesn't need to know of ProfileSyncService anymore, which (among other things) makes it much easier to write tests for it. As an indirect consequence, two UKM tests have to be disabled on ChromeOS, the ones that exercise sign-out. Previously, the sign-out was faked rather crudely in ProfileSyncServiceHarness::SignoutSyncService, but this refactoring makes that impossible. Now we do a real sign-out instead, but because sign-out doesn't exist on ChromeOS, this doesn't work there. Rather than finding a new workaround, I went with disabling the tests on ChromeOS, since they exercise unreachable code anyway. Bug: 842697 Change-Id: If4f6d2fc329d688169df32bc1cd3fb211e972d52 Reviewed-on: https://chromium-review.googlesource.com/1086936 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Brian White <bcwhite@chromium.org> Reviewed-by: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#564559} [modify] https://crrev.com/5fff27695a81d7089b1143785c3b93c265bd0e37/chrome/browser/metrics/ukm_browsertest.cc [modify] https://crrev.com/5fff27695a81d7089b1143785c3b93c265bd0e37/chrome/browser/sync/test/integration/profile_sync_service_harness.cc [modify] https://crrev.com/5fff27695a81d7089b1143785c3b93c265bd0e37/chrome/browser/sync/test/integration/profile_sync_service_harness.h [modify] https://crrev.com/5fff27695a81d7089b1143785c3b93c265bd0e37/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/5fff27695a81d7089b1143785c3b93c265bd0e37/components/browser_sync/profile_sync_service.h [modify] https://crrev.com/5fff27695a81d7089b1143785c3b93c265bd0e37/components/browser_sync/sync_auth_manager.cc [modify] https://crrev.com/5fff27695a81d7089b1143785c3b93c265bd0e37/components/browser_sync/sync_auth_manager.h
,
Jun 6 2018
This is now basically done. What's still missing is adding tests for the new SyncAuthManager specifically. (It's already covered by ProfileSyncService's tests, so overall coverage isn't worse than before the refactoring, but we should still add more specific tests.)
,
Jun 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/802400d6cbf3daefda0c5187a77abae5196cdf03 commit 802400d6cbf3daefda0c5187a77abae5196cdf03 Author: Marc Treib <treib@chromium.org> Date: Thu Jun 07 15:04:17 2018 Add first unit tests for SyncAuthManager This adds tests for primary account events (sign-in/out), for access token requests, and for auth errors. Notably still missing are tests for refresh token events. Bug: 842697 Change-Id: Ic24a5bd097e1d9d0352a427fa24f6b7dbf65d908 Reviewed-on: https://chromium-review.googlesource.com/1089061 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#565265} [modify] https://crrev.com/802400d6cbf3daefda0c5187a77abae5196cdf03/components/browser_sync/BUILD.gn [modify] https://crrev.com/802400d6cbf3daefda0c5187a77abae5196cdf03/components/browser_sync/sync_auth_manager.h [add] https://crrev.com/802400d6cbf3daefda0c5187a77abae5196cdf03/components/browser_sync/sync_auth_manager_unittest.cc
,
Jun 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0dae172d77d0b339e19f20e371600cdef04b7d74 commit 0dae172d77d0b339e19f20e371600cdef04b7d74 Author: Marc Treib <treib@chromium.org> Date: Thu Jun 07 15:09:58 2018 Fix a missing notification in SyncAuthManager Previously, SyncAuthManager did not notify its client when it dropped its cached access token just before requesting a new one (which didn't seem to cause any problems in practice though). This CL adds the missing notification. As a followup to https://crrev.com/c/1089061, this also adds more tests around access token fetches and credentials changed events in SyncAuthManager (one of which exposed the missing notification). Bug: 842697 Change-Id: I6e0179355659694f011534bcef2550cc0f415528 Reviewed-on: https://chromium-review.googlesource.com/1090836 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#565267} [modify] https://crrev.com/0dae172d77d0b339e19f20e371600cdef04b7d74/components/browser_sync/sync_auth_manager.cc [modify] https://crrev.com/0dae172d77d0b339e19f20e371600cdef04b7d74/components/browser_sync/sync_auth_manager_unittest.cc
,
Jun 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9a90ccdf01a8ddab36e941066a01983ba6b5de15 commit 9a90ccdf01a8ddab36e941066a01983ba6b5de15 Author: Marc Treib <treib@chromium.org> Date: Tue Jun 12 08:13:56 2018 Add even more tests for SyncAuthManager Followup to https://crrev.com/c/1089061 and https://crrev.com/c/1090836. This one mostly adds tests around refresh token removal and invalid refresh tokens. Bug: 842697 Change-Id: Ib41a6030dc1f5e5a53995f294467d83611d212c5 Reviewed-on: https://chromium-review.googlesource.com/1091057 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Colin Blundell <blundell@chromium.org> Reviewed-by: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#566357} [modify] https://crrev.com/9a90ccdf01a8ddab36e941066a01983ba6b5de15/components/browser_sync/sync_auth_manager_unittest.cc [modify] https://crrev.com/9a90ccdf01a8ddab36e941066a01983ba6b5de15/services/identity/public/cpp/identity_test_environment.cc [modify] https://crrev.com/9a90ccdf01a8ddab36e941066a01983ba6b5de15/services/identity/public/cpp/identity_test_environment.h [modify] https://crrev.com/9a90ccdf01a8ddab36e941066a01983ba6b5de15/services/identity/public/cpp/identity_test_utils.cc [modify] https://crrev.com/9a90ccdf01a8ddab36e941066a01983ba6b5de15/services/identity/public/cpp/identity_test_utils.h
,
Jun 12 2018
Let's consider this done - the code is moved and has tests, and I have no concrete plans to add any more.
,
Jun 21 2018
Just for fun, some stats on the before [1] -> after [2] state of ProfileSyncService. # of "OnSomethingAccountRelatedChanged" methods: 6 -> 2 # of cross-calls between these methods: 2 -> 0 # of HasSyncingEngine checks in these methods: 3 -> 0 # of LoC: 2487 -> 2234 (including any other changes in the meantime) [1] https://chromium.googlesource.com/chromium/src/+/39de8ed178a3a57bd7bb9032dc562dff9d7fd35b/components/browser_sync/profile_sync_service.cc [2] https://chromium.googlesource.com/chromium/src/+/9a90ccdf01a8ddab36e941066a01983ba6b5de15/components/browser_sync/profile_sync_service.cc |
||
►
Sign in to add a comment |
||
Comment 1 by bugdroid1@chromium.org
, May 17 2018