Improve ProfileSyncService's "PlatformSyncAllowedProvider" handling |
||||
Issue descriptionPlatformSyncAllowedProvider is a callback that returns the state of Android's "Master Sync" toggle. There are several things wrong with this system: - The callback is passed in after PSS is first initialized, so at that point PSS might have already started up. At this point, we don't even check the value of the callback. - PSS doesn't know when its value changes, so the Android code anyway has to call RequestStart/Stop as appropriate. So having a callback and all the necessary plumbing seems useless; might as well just have a SetSyncAllowedByPlatform(bool allowed). Related: bug 566134 , bug 568771
,
Jul 26
Re #1: This works because there are actually *two* Android-level Sync flags: "MasterSync" and "ChromeSync", and the Android code tries to keep the state of "ChromeSync" consistent with the SyncRequested flag [1]. That seems problematic because the "ChromeSync" flag is set both from SyncRequested and from some OS-level concept [2]. [1] https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java?rcl=2f3418b8ea95005302e1d8f916d3adf87d01ae47&l=182 [2] https://cs.chromium.org/chromium/src/components/sync/android/java/src/org/chromium/components/sync/AndroidSyncSettings.java?rcl=1dbc3a455c5f0420e2ba571ba9ee5819a172fc60&l=316
,
Oct 9
,
Oct 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f0d63432c51741e6dc0bf48f0b84e4afb63f887f commit f0d63432c51741e6dc0bf48f0b84e4afb63f887f Author: Marc Treib <treib@chromium.org> Date: Fri Oct 12 13:42:49 2018 ProfileSyncService cleanup: Remove platform_sync_allowed_provider_ ProfileSyncService::platform_sync_allowed_provider_ was a callback that returned whether Android's "Master Sync" toggle is enabled, and so Sync shouldn't start up. However, PSS didn't know when the value changed, so the callback was kinda useless, and PSS could easily get into an inconsistent state (where there is a disable reason, but Sync is still running). Things were working out in practice only because the Android UI code also called RequestStart/Stop as necessary. So, since PSS needs change notifications anyway, let's make that explicit: This CL replaces the callback by a bool, which can be set explicitly. Bug: 867901 Change-Id: I5b7e9f4657d1f565d9643984eb52ec1df26b5c90 Reviewed-on: https://chromium-review.googlesource.com/c/1269870 Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#599195} [modify] https://crrev.com/f0d63432c51741e6dc0bf48f0b84e4afb63f887f/chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java [modify] https://crrev.com/f0d63432c51741e6dc0bf48f0b84e4afb63f887f/chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java [modify] https://crrev.com/f0d63432c51741e6dc0bf48f0b84e4afb63f887f/chrome/browser/sync/profile_sync_service_android.cc [modify] https://crrev.com/f0d63432c51741e6dc0bf48f0b84e4afb63f887f/chrome/browser/sync/profile_sync_service_android.h [modify] https://crrev.com/f0d63432c51741e6dc0bf48f0b84e4afb63f887f/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/f0d63432c51741e6dc0bf48f0b84e4afb63f887f/components/browser_sync/profile_sync_service.h
,
Oct 31
,
Nov 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d1012fccb7dd1741f016d405e8f614f31e07119b commit d1012fccb7dd1741f016d405e8f614f31e07119b Author: Marc Treib <treib@chromium.org> Date: Tue Nov 06 17:10:30 2018 Stop sync when AllowedByPlatform is set to false This currently has no practical effect (the only call site also calls RequestStop anyway), but it prevents ProfileSyncService from getting into an inconsistent state. Bug: 867901 Change-Id: Iaaa72405eac6fb81ee739d2cbeea6ddf52960996 Reviewed-on: https://chromium-review.googlesource.com/c/1319716 Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#605716} [modify] https://crrev.com/d1012fccb7dd1741f016d405e8f614f31e07119b/components/browser_sync/profile_sync_service.cc
,
Nov 6
Let's call this close enough. There's still the issue of the two disable reasons getting tangled, but that's for another day...
,
Jan 11
Filed bug 921025 for untangling the two toggles. |
||||
►
Sign in to add a comment |
||||
Comment 1 by treib@chromium.org
, Jul 26