Refactor "SetupInProgress" out of ProfileSyncService / StartupController |
|||||
Issue descriptionWhether a Sync setup UI is currently showing is not really part of ProfileSyncService's state. We should extract it somewhere else. Note that part of the "setup in progress" logic is in StartupController, and the split between ProfileSyncService and StartupController is a bit weird. Somewhat related: bug 839834
,
Jun 21 2018
Related: Currently, StartupController is not a good abstraction. The logic for "can/should Sync start" is split halfway between StartupController and ProfileSyncService, with no clear boundary. I'll attempt to clean this up a bit too.
,
Jun 21 2018
,
Jun 21 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dce7e988a6c104576abcd3735583991aeea1eb2b commit dce7e988a6c104576abcd3735583991aeea1eb2b Author: Marc Treib <treib@chromium.org> Date: Thu Jun 21 18:43:35 2018 Sync: Pass GetPreferredDataTypes as a callback into StartupController This makes a few things a bit nicer: Now StartupController doesn't have to know about the set of registered data types anymore, which are really not its business. As a consequence, it's not necessary to call Reset() before using the StartupController. This also removes one of two uses of SyncPrefs from StartupController; I hope to remove the remaining one soon. Bug: 854978 Change-Id: I75c8971342554eb190287724eff2ff3238ffbff6 Reviewed-on: https://chromium-review.googlesource.com/1110220 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Jan Krcal <jkrcal@chromium.org> Cr-Commit-Position: refs/heads/master@{#569337} [modify] https://crrev.com/dce7e988a6c104576abcd3735583991aeea1eb2b/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/dce7e988a6c104576abcd3735583991aeea1eb2b/components/sync/driver/startup_controller.cc [modify] https://crrev.com/dce7e988a6c104576abcd3735583991aeea1eb2b/components/sync/driver/startup_controller.h [modify] https://crrev.com/dce7e988a6c104576abcd3735583991aeea1eb2b/components/sync/driver/startup_controller_unittest.cc
,
Jun 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4f89d11d5c5dd286465faa321a1e73977b28fae7 commit 4f89d11d5c5dd286465faa321a1e73977b28fae7 Author: Marc Treib <treib@chromium.org> Date: Fri Jun 22 09:57:48 2018 Sync: Remove SetupInProgress state from StartupController Before this CL, StartupController had an explicit setup_in_progress_ flag, which was set and cleared by ProfileSyncService. This was unnecessary, since a) ProfileSyncService itself knows that state anyway (hence no need to forward IsSetupInProgress queries to the StartupController), and b) StartupController only needs it within TryStart, where we can just pass it in. Bug: 854978 Change-Id: Ia5eed20a993de78d1efaacb6709ca680035856fd Reviewed-on: https://chromium-review.googlesource.com/1110226 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Jan Krcal <jkrcal@chromium.org> Cr-Commit-Position: refs/heads/master@{#569567} [modify] https://crrev.com/4f89d11d5c5dd286465faa321a1e73977b28fae7/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/4f89d11d5c5dd286465faa321a1e73977b28fae7/components/sync/driver/startup_controller.cc [modify] https://crrev.com/4f89d11d5c5dd286465faa321a1e73977b28fae7/components/sync/driver/startup_controller.h [modify] https://crrev.com/4f89d11d5c5dd286465faa321a1e73977b28fae7/components/sync/driver/startup_controller_unittest.cc
,
Jun 22 2018
,
Jun 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/74976dd0544a03703ad944c5b3644d3149a6d35b commit 74976dd0544a03703ad944c5b3644d3149a6d35b Author: Marc Treib <treib@chromium.org> Date: Fri Jun 22 14:38:58 2018 Sync StartupController: Get rid of TryStartImmediately All it did was setting some sticky state and then calling TryStart(). So let's at least make that explicit by having clients call a state setter plus TryStart(). This also lets us rename the "setup_in_progress" param for TryStart() to "force_immediate" which makes a lot more sense. Bug: 854978 Change-Id: Ifd72c94847582626fa60b4a973fd93ac9691fc56 Reviewed-on: https://chromium-review.googlesource.com/1111715 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Jan Krcal <jkrcal@chromium.org> Cr-Commit-Position: refs/heads/master@{#569616} [modify] https://crrev.com/74976dd0544a03703ad944c5b3644d3149a6d35b/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/74976dd0544a03703ad944c5b3644d3149a6d35b/components/sync/driver/startup_controller.cc [modify] https://crrev.com/74976dd0544a03703ad944c5b3644d3149a6d35b/components/sync/driver/startup_controller.h [modify] https://crrev.com/74976dd0544a03703ad944c5b3644d3149a6d35b/components/sync/driver/startup_controller_unittest.cc
,
Jun 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/368ba44635c431f8de3ee67ea2f7a982206b4b1d commit 368ba44635c431f8de3ee67ea2f7a982206b4b1d Author: Marc Treib <treib@chromium.org> Date: Fri Jun 22 15:32:06 2018 Sync StartupController: Get rid of SetBypassSetupCompleteAndDeferredStartup This method set some state on the StartupController which was equivalent to calling TryStart(/*force_immediate=*/true), only that it was also sticky, i.e. it'd also affect later TryStart(false) calls. This was confusing and hard to reason about, and not actually required for anything, so this CL gets rid of it. Bug: 854978 Change-Id: Id3bf77dc56689b33904c60f178264b0e3e8629c4 Reviewed-on: https://chromium-review.googlesource.com/1111850 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Jan Krcal <jkrcal@chromium.org> Cr-Commit-Position: refs/heads/master@{#569636} [modify] https://crrev.com/368ba44635c431f8de3ee67ea2f7a982206b4b1d/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/368ba44635c431f8de3ee67ea2f7a982206b4b1d/components/sync/driver/startup_controller.cc [modify] https://crrev.com/368ba44635c431f8de3ee67ea2f7a982206b4b1d/components/sync/driver/startup_controller.h [modify] https://crrev.com/368ba44635c431f8de3ee67ea2f7a982206b4b1d/components/sync/driver/startup_controller_unittest.cc
,
Jun 25 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/116d07dd47ca07ac8a7ab9f9fdd200f7a4741c93 commit 116d07dd47ca07ac8a7ab9f9fdd200f7a4741c93 Author: Marc Treib <treib@chromium.org> Date: Mon Jun 25 11:42:33 2018 Sync StartupController: Replace CanSyncStart by ShouldSyncStart The new ShouldSyncStart includes the "FirstSetupComplete" check. With this, StartupController doesn't have any knowledge of Sync setup state anymore. As a nice side effect, this allows us to remove SyncPrefs from StartupController. Bug: 854978 Change-Id: Ia9fb178cf5a81ada4288f4c33bcc48ced4f631f4 Reviewed-on: https://chromium-review.googlesource.com/1112241 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Jan Krcal <jkrcal@chromium.org> Cr-Commit-Position: refs/heads/master@{#570008} [modify] https://crrev.com/116d07dd47ca07ac8a7ab9f9fdd200f7a4741c93/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/116d07dd47ca07ac8a7ab9f9fdd200f7a4741c93/components/browser_sync/profile_sync_service.h [modify] https://crrev.com/116d07dd47ca07ac8a7ab9f9fdd200f7a4741c93/components/sync/driver/startup_controller.cc [modify] https://crrev.com/116d07dd47ca07ac8a7ab9f9fdd200f7a4741c93/components/sync/driver/startup_controller.h [modify] https://crrev.com/116d07dd47ca07ac8a7ab9f9fdd200f7a4741c93/components/sync/driver/startup_controller_unittest.cc
,
Jul 3
The "SetupInProgress" state is gone from StartupController. It *is* still in ProfileSyncService, where it's mainly used for two things: 1) If it's true, we bypass deferred startup. 2) While it's true, CanConfigureDataTypes is false (but if they're already configured, then they stay configured). It seems to me that 1 would be much better served by some form of explicit RequestImmediateStart. 2 seems to serve two sub-cases: a) We mustn't configure the data types before the *initial* setup is completed. However, we have a separate IsFirstSetupComplete() for that. b) As an optimization: There's maybe little point in configuring data types while a setup is in progress, since they'll soon be re-configured again. I'd question how useful this optimization is in practice. In summary: I think we can just get rid of IsSetupInProgress altogether.
,
Sep 14
Also, a bit of an anti-feature that follows from 2): Generally, any changes to Sync settings (e.g. preferred data types) apply as soon as you leave the settings page. However, if another chrome://settings/syncSetup tab happens to be open, then Sync will remain in a CONFIGURING state, i.e. Sync won't actually be active. This is IMO quite unexpected; e.g. it has complicated multiple investigations in the past, because nobody expects that having a settings tab open would change things.
,
Sep 14
Let's consider this one done. The easily-refactorable parts have been done; the remaining stuff will be a bigger change that'll probably involve UI code and other APIs. I've filed bug 884159 for that. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by bugdroid1@chromium.org
, Jun 21 2018