Currently, there are many places that implement their own "is this Sync data type enabled?" checks, but they tend to miss some of the required conditions (of which there are many). All of these should be migrated to syncer::GetUploadToGoogleState. Non-exhaustive list: 1) https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/chrome_password_protection_service.cc?rcl=4f6543653a564533ddccf0accca9332c5b6e84df&l=561 2) https://cs.chromium.org/chromium/src/chrome/browser/history/web_history_service_factory.cc?rcl=4f6543653a564533ddccf0accca9332c5b6e84df&l=20 3) https://cs.chromium.org/chromium/src/chrome/browser/ui/search/search_tab_helper.cc?rcl=4f6543653a564533ddccf0accca9332c5b6e84df&l=97 4) https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc?rcl=ab90b51c5b60de15054a32b0bd18e4839536a1c9&l=334 5) https://cs.chromium.org/chromium/src/components/browsing_data/core/history_notice_utils.cc?rcl=ab90b51c5b60de15054a32b0bd18e4839536a1c9&l=60 6) https://cs.chromium.org/chromium/src/components/browsing_data/core/history_notice_utils.cc?rcl=ab90b51c5b60de15054a32b0bd18e4839536a1c9&l=104 7) https://cs.chromium.org/chromium/src/components/password_manager/core/browser/password_manager_util.cc?rcl=ab90b51c5b60de15054a32b0bd18e4839536a1c9&l=100 8) https://cs.chromium.org/chromium/src/components/sync/user_events/user_event_service_impl.cc?rcl=ab90b51c5b60de15054a32b0bd18e4839536a1c9&l=101 9) https://cs.chromium.org/chromium/src/components/ukm/observers/sync_disable_observer.cc?rcl=ab90b51c5b60de15054a32b0bd18e4839536a1c9&l=55 10) https://cs.chromium.org/chromium/src/components/autofill/core/browser/personal_data_manager.cc?rcl=ab90b51c5b60de15054a32b0bd18e4839536a1c9&l=130
+msarda, since probably a lot of these will change for Unity too?
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/24db2e5db7a326b0abb03ffb260e63cabba2bed2 commit 24db2e5db7a326b0abb03ffb260e63cabba2bed2 Author: Marc Treib <treib@chromium.org> Date: Mon Jul 09 16:16:16 2018 Sync: Implement GetUploadToGoogleState in terms of GetState The new GetState replaces calls to CanSyncStart, IsSyncActive, and ConfigurationDone. (Unfortunately GetAuthError is still required to distinguish transient from persistent errors.) One change in behavior is that transient auth errors now map to INITIALIZING instead of ACTIVE. That's because with GetState, we can't tell if sync was actually active before the auth error happened. But this change seems like an improvement anyway. Bug: 839834 , 856081 Change-Id: I2ccee268ffe226817289950f4db5c7f98ea69487 Reviewed-on: https://chromium-review.googlesource.com/1124567 Reviewed-by: Thomas Tangl <tangltom@chromium.org> Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org> Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org> Reviewed-by: Jan Krcal <jkrcal@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#573325} [modify] https://crrev.com/24db2e5db7a326b0abb03ffb260e63cabba2bed2/chrome/browser/extensions/api/sessions/sessions_apitest.cc [modify] https://crrev.com/24db2e5db7a326b0abb03ffb260e63cabba2bed2/components/autofill/core/browser/test_sync_service.cc [modify] https://crrev.com/24db2e5db7a326b0abb03ffb260e63cabba2bed2/components/autofill/core/browser/test_sync_service.h [modify] https://crrev.com/24db2e5db7a326b0abb03ffb260e63cabba2bed2/components/suggestions/suggestions_service_impl_unittest.cc [modify] https://crrev.com/24db2e5db7a326b0abb03ffb260e63cabba2bed2/components/sync/driver/sync_service_utils.cc [modify] https://crrev.com/24db2e5db7a326b0abb03ffb260e63cabba2bed2/components/sync/driver/sync_service_utils.h [modify] https://crrev.com/24db2e5db7a326b0abb03ffb260e63cabba2bed2/components/sync/driver/sync_service_utils_unittest.cc [modify] https://crrev.com/24db2e5db7a326b0abb03ffb260e63cabba2bed2/components/unified_consent/url_keyed_data_collection_consent_helper_unittest.cc
treib@: is there a status update here? Assigning in the interim, please unassign if apppropriate and may be a candidate for the upcoming fixit session.
Still valid, but not actually blocking anything anymore. Agreed that it could be part of the fixit.
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/86b9c6a7bdc2f83432cefae202ff6afabbb5b7fd commit 86b9c6a7bdc2f83432cefae202ff6afabbb5b7fd Author: Marc Treib <treib@chromium.org> Date: Mon Nov 19 15:43:25 2018 syncer::GetUploadToGoogleState: Check Sync-the-feature state This makes sure we don't claim to have consent (or at least be INITIALIZING) when we're running in standalone transport mode. In practice, this probably wasn't a problem, since most data types are disabled in transport mode anyway, but let's be cautious here. Bug: 856081, 906609, 871221 Change-Id: Ie6d2d0ef4f81d0f51d66ddd9a2d55daba2c1093e Reviewed-on: https://chromium-review.googlesource.com/c/1341916 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Florian Uunk <feuunk@chromium.org> Cr-Commit-Position: refs/heads/master@{#609310} [modify] https://crrev.com/86b9c6a7bdc2f83432cefae202ff6afabbb5b7fd/components/sync/driver/sync_service_utils.cc [modify] https://crrev.com/86b9c6a7bdc2f83432cefae202ff6afabbb5b7fd/components/sync/driver/sync_service_utils_unittest.cc
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eb8f8994c41f2cbe2e7a4c8c5492a845756d0084 commit eb8f8994c41f2cbe2e7a4c8c5492a845756d0084 Author: Marc Treib <treib@chromium.org> Date: Wed Nov 21 13:10:20 2018 UserEventServiceImpl: Check IsSyncFeatureEnabled If IsSyncFeatureEnabled is false, then GetPreferredDataTypes doesn't really mean anything. In practice, this wasn't an actual issue: If Sync isn't enabled, then the whole UserEventService can't upload anything anyway. But still best to be precise here. Bug: 907027 , 856081 Change-Id: I2e3ee7200118175ed9397b5a1b7d9fedd0eb7692 Reviewed-on: https://chromium-review.googlesource.com/c/1344104 Reviewed-by: vitaliii <vitaliii@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#610022} [modify] https://crrev.com/eb8f8994c41f2cbe2e7a4c8c5492a845756d0084/components/sync/user_events/user_event_service_impl.cc
Marc, is it still valid and appropriate for the next Fixit?
Yes, I think this is still valid. Maybe P3 though.
Comment 1 by treib@chromium.org
, Jun 25 2018