SyncServiceCrypto shouldn't modify DataTypeManager |
|||
Issue descriptionSyncServiceCrypto currently gets passed a (non-const) DataTypeManager* from ProfileSyncService, and does questionable things with it. In particular, there are a few situations where it calls Configure(). Those calls are supposed to be guarded by ProfileSyncService::CanConfigureDataTypes(), but of course SyncServiceCrypto doesn't have access to that and so doesn't check - see bug 825408. A bit more generally, the current setup makes it very hard to reason about DataTypeManager's state, since (at least) two classes independently mess with it. We should refactor this so that all modifying calls to DataTypeManager go through ProfileSyncService.
,
Jul 10
Note: https://crrev.com/c/1094878 fixed the missing CanConfigureDataTypes checks in SyncServiceCrypto. I'm not aware of any remaining bugs due to this, but it's still mighty ugly that SyncServiceCrypto gets access to the DataTypeManager and the SyncEngine, both of which are supposed to be owned and managed by ProfileSyncService.
,
Jul 27
,
Jul 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/44c147a996a01562177ff939867aa7199ec72e09 commit 44c147a996a01562177ff939867aa7199ec72e09 Author: Mikel Astiz <mastiz@chromium.org> Date: Tue Jul 31 07:29:37 2018 Centralize all Configure() calls to DataTypeManager Most callers anyway need to retrieve information from ProfileSyncService about the selected datatypes, so let's instead make sure all paths to DataTypeManager::Configure() are centralized in ProfileSyncService. This will simplify extending the functions signature in follow-up patches. Bug: 866814 , 851476 Change-Id: Ied3975b9ed833d34aee1802aae36c1332b3b68e0 Reviewed-on: https://chromium-review.googlesource.com/1152812 Commit-Queue: Mikel Astiz <mastiz@chromium.org> Reviewed-by: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#579332} [modify] https://crrev.com/44c147a996a01562177ff939867aa7199ec72e09/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/44c147a996a01562177ff939867aa7199ec72e09/components/browser_sync/profile_sync_service.h [modify] https://crrev.com/44c147a996a01562177ff939867aa7199ec72e09/components/browser_sync/profile_sync_service_unittest.cc [modify] https://crrev.com/44c147a996a01562177ff939867aa7199ec72e09/components/sync/driver/backend_migrator.cc [modify] https://crrev.com/44c147a996a01562177ff939867aa7199ec72e09/components/sync/driver/backend_migrator.h [modify] https://crrev.com/44c147a996a01562177ff939867aa7199ec72e09/components/sync/driver/backend_migrator_unittest.cc [modify] https://crrev.com/44c147a996a01562177ff939867aa7199ec72e09/components/sync/driver/data_type_manager.h [modify] https://crrev.com/44c147a996a01562177ff939867aa7199ec72e09/components/sync/driver/data_type_manager_impl.cc [modify] https://crrev.com/44c147a996a01562177ff939867aa7199ec72e09/components/sync/driver/data_type_manager_impl.h [modify] https://crrev.com/44c147a996a01562177ff939867aa7199ec72e09/components/sync/driver/data_type_manager_impl_unittest.cc [modify] https://crrev.com/44c147a996a01562177ff939867aa7199ec72e09/components/sync/driver/data_type_manager_mock.h [modify] https://crrev.com/44c147a996a01562177ff939867aa7199ec72e09/components/sync/driver/sync_service_crypto.cc [modify] https://crrev.com/44c147a996a01562177ff939867aa7199ec72e09/components/sync/driver/sync_service_crypto.h
,
Jul 31
|
|||
►
Sign in to add a comment |
|||
Comment 1 by mastiz@chromium.org
, Jul 3