Currently, the flow for setting up or (re)configuring Sync looks something like this:
ProfileSyncService* pss = ...;
1: auto setup_handle = pss->GetSetupInProgressHandle();
2: pss->RequestStart();
3: (Wait for engine to be initialized)
4: pss->OnUserChoseDatatypes(...);
5: pss->SetFirstSetupComplete();
6: setup_handle.reset();
Aside from being inconvenient and confusing (why are so many not-obviously-connected APIs involved), most of those steps also do multiple things or have gotchas:
1: The setup-in-progress handle prevents later changes from applying immediately, which makes sense. However, this also triggers an immediate startup if Sync isn't already running.
2: Does two things: a) trigger immediate startup (unnecessary because step 1 already did that), and b) clear DISABLE_REASON_USER_CHOICE (which can be set if the user previously disabled Sync (Android only, maybe iOS too?), or if Sync was reset from the dashboard).
4: Immediately stores the new preferred types to prefs.
5: Only required during first-time setup; otherwise it's already set.
6: Releasing the handle lets PSS actually apply all the above changes, *except* if another setup UI happens to be open in another tab.
Currently, the flow for setting up or (re)configuring Sync looks something like this:
ProfileSyncService* pss = ...;
1: auto setup_handle = pss->GetSetupInProgressHandle();
2: pss->RequestStart();
3: (Wait for engine to be initialized)
4: pss->OnUserChoseDatatypes(...);
5: pss->SetFirstSetupComplete();
6: setup_handle.reset();
(The flow gets a lot more convoluted when custom passphrase encryption is involved. If it is canceled (only possible during first-time setup I think), then there might be additional calls to RequestStop and/or sign-out).
Aside from being inconvenient and confusing (why are so many not-obviously-connected APIs involved), most of those steps also do multiple things or have gotchas:
1: The setup-in-progress handle prevents later changes from applying immediately, which makes sense. However, this also triggers an immediate startup if Sync isn't already running.
2: Does two things: a) trigger immediate startup (unnecessary because step 1 already did that), and b) clear DISABLE_REASON_USER_CHOICE (which can be set if the user previously disabled Sync (Android only, maybe iOS too?), or if Sync was reset from the dashboard).
4: Immediately stores the new preferred types to prefs.
5: Only required during first-time setup; otherwise it's already set.
6: Releasing the handle lets PSS actually apply all the above changes, *except* if another setup UI happens to be open in another tab.
Currently, the flow for setting up or (re)configuring Sync looks something like this:
ProfileSyncService* pss = ...;
1: auto setup_handle = pss->GetSetupInProgressHandle();
2: pss->RequestStart();
3: (Wait for engine to be initialized)
4: pss->OnUserChoseDatatypes(...);
5: pss->SetFirstSetupComplete();
6: setup_handle.reset();
(The flow gets a lot more convoluted when custom passphrase encryption is involved. If it is canceled (only possible during first-time setup I think), then there might be additional calls to RequestStop and/or sign-out).
Aside from being inconvenient and confusing (why are so many not-obviously-connected APIs involved), most of those steps also do multiple things or have gotchas:
1: The setup-in-progress handle prevents later changes from applying immediately, which makes sense. However, this also triggers an immediate startup if Sync isn't already running.
2: Does two things: a) trigger immediate startup (unnecessary because step 1 already did that), and b) clear DISABLE_REASON_USER_CHOICE (which can be set if the user previously disabled Sync (Android only, maybe iOS too?), or if Sync was reset from the dashboard).
4: Immediately stores the new preferred types to prefs.
5: Only required during first-time setup; otherwise it's already set.
6: Releasing the handle lets PSS actually apply all the above changes, *except* if another setup UI happens to be open in another tab.
Doc with more thoughts/details: https://docs.google.com/document/d/1fUnZBR5eiEPOnfVJc6dWNn54TU7r_8Ll0F-GoFcZJ3s
One related anti-feature that can probably be fixed along the way: Generally, any changes to Sync settings (e.g. preferred data types) get stored to prefs immediately, but actually apply only when 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 become active again. 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.
Proposal (not fully thought through):
Let's get rid of SetupInProgressHandle (and the whole SetupInProgress state), RequestStart/RequestStop, OnUserChoseDatatypes, and SetFirstSetupComplete. Instead, introduce a SyncUserSettings struct and a simple getter+setter for it on the SyncService. The struct could look something like this:
struct SyncUserSettings {
// Replaces RequestStart/RequestStop and maps to DISABLE_REASON_USER_CHOICE
enum SyncEnabledSetting {
ENABLED,
DISABLED_TEMPORARILY, // Maps to today's RequestStop(KEEP_DATA)
DISABLED_PERMANENTLY, // Maps to today's RequestStop(CLEAR_DATA)
};
SyncEnabledSetting enabled;
// Replaces OnUserChoseDatatypes
ModelTypeSet preferred_types;
bool first_setup_complete;
};
SyncService will also need a |NudgeToStart|, which will bypass deferred startup (but do nothing else).
Another oddity caused by this which I've just discovered: If a Sync settings tab is open during a Chrome restart, then Sync might or might not actually start, depending on whether that tab or the SyncService initializes first.
The following revision refers to this bug:
https://chromium.googlesource.com/chromium/src.git/+/6b7b7ff9e02938d7e95f182bbdc22cebe5a7930a
commit 6b7b7ff9e02938d7e95f182bbdc22cebe5a7930a
Author: Marc Treib <treib@chromium.org>
Date: Tue Dec 04 09:10:28 2018
Migrate UkmBrowserTests to SyncUserSettings
SyncUserSettings is a new class that encapsulates all the
user-configurable knobs for Sync. It replaces a bunch of setters
and getters directly on the SyncService.
Here, a bunch of calls to RequestStart and RequestStop are replaced by
GetUserSettings()->SetSyncRequested(bool).
SetSyncRequested(false) is equivalent to RequestStop(KEEP_DATA), but
it is also used to replace a bunch of RequestStop(CLEAR_DATA) calls.
However:
- These calls are all at the end of a test, so it doesn't matter if
data is kept or not. (In fact, these calls actually aren't necessary
at all, but for some reason, without them the test shutdown takes
much longer. See also https://crrev.com/c/557422).
- RequestStop(CLEAR_DATA) is generally a pretty weird thing to do:
Usually it only happens if the user signs out (removes the primary
account); it ~never needs to be called directly. So this change makes
the tests a bit less weird / more realistic.
Bug: 884159
Change-Id: I0e2347bbabdeb10846528fe03732a05ba996f43f
Reviewed-on: https://chromium-review.googlesource.com/c/1352180
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613477}
[modify] https://crrev.com/6b7b7ff9e02938d7e95f182bbdc22cebe5a7930a/chrome/browser/metrics/ukm_browsertest.cc
Some of the required changes here turned out harder than anticipated, because several tests rely on ProfileSyncServiceMock (and e.g. have EXPECT_CALLs on methods that are going away).
Comment 1 by treib@chromium.org
, Sep 14