Decouple Sync-the-transport from Sync-the-feature, part 1

This allows ProfileSyncService to start up in "standalone transport"
mode even if the user has disabled Sync-the-feature. In particular,
the disable reasons USER_CHOICE and PLATFORM_OVERRIDE and the
FirstSetupComplete state don't prevent Sync-the-transport from
starting up.

Still missing:
- GetState should now be renamed to GetTransportState.
- UI code hasn't been updated, so UI might show wrong state.
- New integration tests.

Bug: 856179
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I59d3a4b548b279ec6cb5ca563747859dd9533618
Reviewed-on: https://chromium-review.googlesource.com/1148392Reviewed-by: Sergio Collazos <sczs@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580270}

chrome/browser/sync/test/integration/single_client_directory_sync_test.cc

@@ -9,6 +9,7 @@
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/sync/test/integration/bookmarks_helper.h"
@@ -16,6 +17,7 @@
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "chrome/browser/sync/test/integration/updated_progress_marker_checker.h"
 #include "components/browser_sync/profile_sync_service.h"
+#include "components/sync/driver/sync_driver_switches.h"
 #include "components/sync/model/model_type_store_service.h"
 #include "components/sync/syncable/directory.h"
 #include "sql/test/test_helpers.h"
@@ -75,6 +77,13 @@ class SyncUnrecoverableErrorChecker : public SingleClientStatusChangeChecker {
 
 IN_PROC_BROWSER_TEST_F(SingleClientDirectorySyncTest,
                        StopThenDisableDeletesDirectory) {
+  // If SyncStandaloneTransport is enabled, then the sync service will
+  // immediately restart (and thus recreate directory files) after RequestStop.
+  // TODO(crbug.com/856179): Rewrite this test to pass with
+  // kSyncStandaloneTransport enabled.
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndDisableFeature(switches::kSyncStandaloneTransport);
+
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   browser_sync::ProfileSyncService* sync_service = GetSyncService(0);
   FilePath directory_path = sync_service->GetSyncClient()

chrome/browser/sync/test/integration/sync_errors_test.cc

@@ -4,6 +4,7 @@
 
 #include "base/macros.h"
 #include "base/run_loop.h"
+#include "base/test/bind_test_util.h"
 #include "build/build_config.h"
 #include "chrome/browser/sync/test/integration/bookmarks_helper.h"
 #include "chrome/browser/sync/test/integration/passwords_helper.h"
@@ -30,12 +31,25 @@ class SyncDisabledChecker : public SingleClientStatusChangeChecker {
   explicit SyncDisabledChecker(ProfileSyncService* service)
       : SingleClientStatusChangeChecker(service) {}
 
+  SyncDisabledChecker(ProfileSyncService* service,
+                      base::OnceClosure condition_satisfied_callback)
+      : SingleClientStatusChangeChecker(service),
+        condition_satisfied_callback_(std::move(condition_satisfied_callback)) {
+  }
+
   bool IsExitConditionSatisfied() override {
-    return !service()->IsSetupInProgress() &&
-           !service()->IsFirstSetupComplete();
+    bool satisfied =
+        !service()->IsSetupInProgress() && !service()->IsFirstSetupComplete();
+    if (satisfied && condition_satisfied_callback_) {
+      std::move(condition_satisfied_callback_).Run();
+    }
+    return satisfied;
   }
 
   std::string GetDebugMessage() const override { return "Sync Disabled"; }
+
+ private:
+  base::OnceClosure condition_satisfied_callback_;
 };
 
 class SyncEngineStoppedChecker : public SingleClientStatusChangeChecker {
@@ -188,22 +202,31 @@ IN_PROC_BROWSER_TEST_F(SyncErrorTest, BirthdayErrorUsingActionableErrorTest) {
 
   std::string description = "Not My Fault";
   std::string url = "www.google.com";
-  EXPECT_TRUE(GetFakeServer()->TriggerActionableError(
-      sync_pb::SyncEnums::NOT_MY_BIRTHDAY,
-      description,
-      url,
+  ASSERT_TRUE(GetFakeServer()->TriggerActionableError(
+      sync_pb::SyncEnums::NOT_MY_BIRTHDAY, description, url,
       sync_pb::SyncEnums::DISABLE_SYNC_ON_CLIENT));
 
   // Now make one more change so we will do another sync.
   const BookmarkNode* node2 = AddFolder(0, 0, "title2");
   SetTitle(0, node2, "new_title2");
-  ASSERT_TRUE(SyncDisabledChecker(GetSyncService(0)).Wait());
+
+  auto condition = base::BindLambdaForTesting([&]() {
     syncer::SyncStatus status;
     GetSyncService(0)->QueryDetailedSyncStatus(&status);
-  ASSERT_EQ(status.sync_protocol_error.error_type, syncer::NOT_MY_BIRTHDAY);
-  ASSERT_EQ(status.sync_protocol_error.action, syncer::DISABLE_SYNC_ON_CLIENT);
-  ASSERT_EQ(status.sync_protocol_error.url, url);
-  ASSERT_EQ(status.sync_protocol_error.error_description, description);
+
+    // Note: If SyncStandaloneTransport is enabled, then on
+    // receiving the error, the SyncService will immediately
+    // start up again in transport mode, which resets the
+    // status. So query the status that the checker recorded at
+    // the time Sync was off. syncer::SyncStatus status =
+    // checker.GetSyncStatusAtExit();
+    EXPECT_EQ(status.sync_protocol_error.error_type, syncer::NOT_MY_BIRTHDAY);
+    EXPECT_EQ(status.sync_protocol_error.action,
+              syncer::DISABLE_SYNC_ON_CLIENT);
+    EXPECT_EQ(status.sync_protocol_error.url, url);
+    EXPECT_EQ(status.sync_protocol_error.error_description, description);
+  });
+  EXPECT_TRUE(SyncDisabledChecker(GetSyncService(0), condition).Wait());
 }
 
 // Tests that on receiving CLIENT_DATA_OBSOLETE sync engine gets restarted and

chrome/browser/ui/cocoa/app_menu/app_menu_controller_unittest.mm

@@ -161,6 +161,10 @@ class AppMenuControllerTest : public CocoaProfileTest {
   void EnableSync() {
     EXPECT_CALL(*mock_sync_service_, GetState())
         .WillRepeatedly(Return(syncer::SyncService::State::ACTIVE));
+    EXPECT_CALL(*mock_sync_service_, GetDisableReasons())
+        .WillRepeatedly(Return(syncer::SyncService::DISABLE_REASON_NONE));
+    EXPECT_CALL(*mock_sync_service_, IsFirstSetupComplete())
+        .WillRepeatedly(Return(true));
     EXPECT_CALL(*mock_sync_service_,
                 IsDataTypeControllerRunning(syncer::SESSIONS))
         .WillRepeatedly(Return(true));

chrome/browser/ui/sync/one_click_signin_sync_observer_unittest.cc

@@ -67,6 +67,8 @@ class OneClickTestProfileSyncService
 
   bool IsSetupInProgress() const override { return setup_in_progress_; }
 
+  int GetDisableReasons() const override { return DISABLE_REASON_NONE; }
+
   State GetState() const override { return state_; }
 
   void set_first_setup_complete(bool complete) {

chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc

@@ -215,6 +215,8 @@ class RecentTabsSubMenuModelTest
         .WillRepeatedly(Return(syncer::SyncService::DISABLE_REASON_NONE));
     EXPECT_CALL(*mock_sync_service_, GetState())
         .WillRepeatedly(Return(syncer::SyncService::State::ACTIVE));
+    EXPECT_CALL(*mock_sync_service_, IsFirstSetupComplete())
+        .WillRepeatedly(Return(true));
     EXPECT_CALL(*mock_sync_service_,
                 IsDataTypeControllerRunning(syncer::SESSIONS))
         .WillRepeatedly(Return(true));

chrome/browser/ui/webui/browsing_history_handler_unittest.cc

@@ -65,6 +65,10 @@ class TestSyncService : public browser_sync::TestProfileSyncService {
 
   State GetState() const override { return state_; }
 
+  int GetDisableReasons() const override { return DISABLE_REASON_NONE; }
+
+  bool IsFirstSetupComplete() const override { return true; }
+
   syncer::ModelTypeSet GetActiveDataTypes() const override {
     return syncer::ModelTypeSet::All();
   }

chrome/browser/ui/webui/sync_internals_browsertest.js

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+GEN('#include "components/sync/driver/sync_driver_switches.h"');
+
 /**
  * Test fixture for sync internals WebUI testing.
  * @constructor
@@ -56,6 +58,22 @@ SyncInternalsWebUITest.prototype = {
   }
 };
 
+function SyncInternalsWebUITestWithStandaloneTransport() {}
+
+SyncInternalsWebUITestWithStandaloneTransport.prototype = {
+  __proto__: SyncInternalsWebUITest.prototype,
+
+  featureList: ['switches::kSyncStandaloneTransport', ''],
+};
+
+function SyncInternalsWebUITestWithoutStandaloneTransport() {}
+
+SyncInternalsWebUITestWithoutStandaloneTransport.prototype = {
+  __proto__: SyncInternalsWebUITest.prototype,
+
+  featureList: ['', 'switches::kSyncStandaloneTransport'],
+};
+
 /**
  * Constant hard-coded value to return from mock getAllNodes.
  * @const
@@ -241,11 +259,18 @@ TEST_F('SyncInternalsWebUITest', 'Uninitialized', function() {
 // On chromeos, browser tests are signed in by default.  On other platforms,
 // browser tests are signed out.
 GEN('#if defined(OS_CHROMEOS)');
-TEST_F('SyncInternalsWebUITest', 'SignedIn', function() {
+TEST_F('SyncInternalsWebUITestWithStandaloneTransport', 'SignedIn', function() {
   assertNotEquals(null, chrome.sync.aboutInfo);
-  expectTrue(this.hasInDetails(true, 'Summary', 'Waiting for start request'));
+  expectTrue(this.hasInDetails(true, 'Summary', 'Initializing'));
   expectTrue(this.hasInDetails(true, 'Username', 'stub-user@example.com'));
 });
+TEST_F(
+    'SyncInternalsWebUITestWithoutStandaloneTransport', 'SignedIn', function() {
+      assertNotEquals(null, chrome.sync.aboutInfo);
+      expectTrue(
+          this.hasInDetails(true, 'Summary', 'Waiting for start request'));
+      expectTrue(this.hasInDetails(true, 'Username', 'stub-user@example.com'));
+    });
 GEN('#else');
 TEST_F('SyncInternalsWebUITest', 'SignedOut', function() {
   assertNotEquals(null, chrome.sync.aboutInfo);

components/browser_sync/profile_sync_service.cc

@@ -159,6 +159,10 @@ DataTypeController::TypeMap BuildDataTypeControllerMap(
   return type_map;
 }
 
+bool IsStandaloneTransportEnabled() {
+  return base::FeatureList::IsEnabled(switches::kSyncStandaloneTransport);
+}
+
 }  // namespace
 
 ProfileSyncService::InitParams::InitParams() = default;
@@ -245,7 +249,7 @@ void ProfileSyncService::Initialize() {
   startup_controller_ = std::make_unique<syncer::StartupController>(
       base::BindRepeating(&ProfileSyncService::GetPreferredDataTypes,
                           base::Unretained(this)),
-      base::BindRepeating(&ProfileSyncService::ShouldSyncStart,
+      base::BindRepeating(&ProfileSyncService::ShouldStartEngine,
                           base::Unretained(this)),
       base::BindRepeating(&ProfileSyncService::StartUpSlowEngineComponents,
                           base::Unretained(this)));
@@ -428,7 +432,7 @@ void ProfileSyncService::AccountStateChanged() {
     DCHECK(!engine_);
   } else {
     DCHECK(!engine_);
-    startup_controller_->TryStart(IsSetupInProgress());
+    startup_controller_->TryStart(/*force_immediate=*/IsSetupInProgress());
   }
 }
 
@@ -448,10 +452,34 @@ void ProfileSyncService::CredentialsChanged() {
   NotifyObservers();
 }
 
-bool ProfileSyncService::ShouldSyncStart(bool bypass_first_setup_check) {
-  if (!CanSyncStart()) {
+bool ProfileSyncService::IsEngineAllowedToStart() const {
+  int disable_reasons = GetDisableReasons();
+  if (IsStandaloneTransportEnabled()) {
+    // USER_CHOICE (i.e. the Sync feature toggle) and PLATFORM_OVERRIDE (i.e.
+    // Android's "MasterSync" toggle) do not prevent starting up the Sync
+    // transport.
+    const int kDisableReasonMask =
+        ~(DISABLE_REASON_USER_CHOICE | DISABLE_REASON_PLATFORM_OVERRIDE);
+    disable_reasons &= kDisableReasonMask;
+  }
+  return disable_reasons == DISABLE_REASON_NONE;
+}
+
+bool ProfileSyncService::ShouldStartEngine(
+    bool bypass_first_setup_check) const {
+  if (!IsEngineAllowedToStart()) {
     return false;
   }
+  // If standalone transport is enabled, we always start the engine as soon as
+  // we can.
+  if (IsStandaloneTransportEnabled()) {
+    return true;
+  }
+  // Without standalone transport, we generally wait for first-time setup to be
+  // complete before starting the engine (because if it isn't, we can't
+  // configure the DataTypeManager anyway). Note that if a setup is currently in
+  // progress (which requires the engine to be initialized), then
+  // |bypass_first_setup_check| will be set to true.
   return bypass_first_setup_check || IsFirstSetupComplete();
 }
 
@@ -535,7 +563,7 @@ void ProfileSyncService::OnDataTypeRequestsSyncStartup(syncer::ModelType type) {
 }
 
 void ProfileSyncService::StartUpSlowEngineComponents() {
-  DCHECK(CanSyncStart());
+  DCHECK(IsEngineAllowedToStart());
 
   engine_ = sync_client_->GetSyncApiComponentFactory()->CreateSyncEngine(
       debug_identifier_, sync_client_->GetInvalidationService(),
@@ -763,7 +791,7 @@ int ProfileSyncService::GetDisableReasons() const {
 syncer::SyncService::State ProfileSyncService::GetState() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  if (GetDisableReasons() != DISABLE_REASON_NONE) {
+  if (!IsEngineAllowedToStart()) {
     // We shouldn't have an engine while in a disabled state, with one
     // exception: When encountering an unrecoverable error, we post a task to
     // shut down instead of doing it immediately, so there's a brief timeframe
@@ -775,9 +803,6 @@ syncer::SyncService::State ProfileSyncService::GetState() const {
     return State::DISABLED;
   }
 
-  // Since there is no disable reason, Sync can start in principle.
-  DCHECK(CanSyncStart());
-
   // Typically, Sync won't start until the initial setup is at least in
   // progress. StartupController::TryStartImmediately bypasses the first setup
   // check though, so we first have to check whether the engine is initialized.
@@ -809,9 +834,10 @@ syncer::SyncService::State ProfileSyncService::GetState() const {
     return State::PENDING_DESIRED_CONFIGURATION;
   }
 
-  // The DataTypeManager shouldn't get configured (i.e. leave the STOPPED state)
-  // before the initial setup is complete.
-  DCHECK(IsFirstSetupComplete());
+  // Unless standalone transport is enabled, the DataTypeManager shouldn't get
+  // configured (i.e. leave the STOPPED state) before the initial setup is
+  // complete.
+  DCHECK(IsStandaloneTransportEnabled() || IsFirstSetupComplete());
 
   // Note that if a setup is started after the data types have been configured,
   // then they'll stay configured even though CanConfigureDataTypes will be
@@ -1296,7 +1322,13 @@ const GoogleServiceAuthError& ProfileSyncService::GetAuthError() const {
 }
 
 bool ProfileSyncService::CanConfigureDataTypes() const {
-  return data_type_manager_ && IsFirstSetupComplete() && !IsSetupInProgress();
+  // TODO(crbug.com/856179): Arguably, IsSetupInProgress() shouldn't prevent
+  // configuring data types in transport mode, but at least for now, it's
+  // easier to keep it like this. Changing this will likely require changes to
+  // the setup UI flow.
+  return data_type_manager_ &&
+         (IsFirstSetupComplete() || IsStandaloneTransportEnabled()) &&
+         !IsSetupInProgress();
 }
 
 std::unique_ptr<syncer::SyncSetupInProgressHandle>
@@ -1562,7 +1594,15 @@ void ProfileSyncService::ConfigureDataTypeManager(
   DCHECK(!configure_context.cache_guid.empty());
   DCHECK_NE(configure_context.reason, syncer::CONFIGURE_REASON_UNKNOWN);
 
-  data_type_manager_->Configure(GetPreferredDataTypes(), configure_context);
+  syncer::ModelTypeSet types = GetPreferredDataTypes();
+  // If Sync-the-feature isn't fully enabled, then only a subset of data types
+  // is supported.
+  if (!IsSyncFeatureEnabled()) {
+    DCHECK(IsStandaloneTransportEnabled());
+    const syncer::ModelTypeSet allowed_types = {syncer::USER_CONSENTS};
+    types = Intersection(types, allowed_types);
+  }
+  data_type_manager_->Configure(types, configure_context);
 }
 
 syncer::UserShare* ProfileSyncService::GetUserShare() const {
@@ -1739,6 +1779,7 @@ void ProfileSyncService::OnSyncManagedPrefChange(bool is_sync_managed) {
     StopImpl(CLEAR_DATA);
   } else {
     // Sync is no longer disabled by policy. Try starting it up if appropriate.
+    DCHECK(!engine_);
     startup_controller_->TryStart(IsSetupInProgress());
   }
 }
@@ -1966,6 +2007,14 @@ void ProfileSyncService::RequestStop(SyncStopDataFate data_fate) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   sync_prefs_.SetSyncRequested(false);
   StopImpl(data_fate);
+
+  // TODO(crbug.com/856179): Evaluate whether we can get away without a full
+  // restart (i.e. just reconfigure plus whatever cleanup is necessary).
+  // Especially in the CLEAR_DATA case, StopImpl does a lot of cleanup that
+  // might still be required.
+  if (IsStandaloneTransportEnabled()) {
+    startup_controller_->TryStart(/*force_immediate=*/false);
+  }
 }
 
 void ProfileSyncService::RequestStart() {
@@ -1980,7 +2029,12 @@ void ProfileSyncService::RequestStart() {
     sync_prefs_.SetSyncRequested(true);
     NotifyObservers();
   }
+  // If Sync-the-transport was already running, just reconfigure.
+  if (IsStandaloneTransportEnabled() && engine_initialized_) {
+    ReconfigureDatatypeManager();
+  } else {
     startup_controller_->TryStart(/*force_immediate=*/true);
+  }
 }
 
 void ProfileSyncService::ReconfigureDatatypeManager() {

components/browser_sync/profile_sync_service.h

@@ -492,8 +492,10 @@ class ProfileSyncService : public syncer::SyncService,
   void AccountStateChanged();
   void CredentialsChanged();
 
+  bool IsEngineAllowedToStart() const;
+
   // Callback for StartupController.
-  bool ShouldSyncStart(bool bypass_first_setup_check);
+  bool ShouldStartEngine(bool bypass_first_setup_check) const;
 
   // Destroys the |crypto_| object and creates a new one with fresh state.
   void ResetCryptoState();

components/sync/driver/sync_driver_switches.cc

@@ -36,6 +36,11 @@ const char kSyncShortNudgeDelayForTest[] = "sync-short-nudge-delay-for-test";
 const base::Feature kSyncClearDataOnPassphraseEncryption{
     "ClearSyncDataOnPassphraseEncryption", base::FEATURE_DISABLED_BY_DEFAULT};
 
+// If enabled, allows the Sync machinery ("transport layer") to start
+// independently of Sync-the-feature.
+const base::Feature kSyncStandaloneTransport{"SyncStandaloneTransport",
+                                             base::FEATURE_DISABLED_BY_DEFAULT};
+
 // Gates registration and construction of user events machinery. Enabled by
 // default as each use case should have their own gating feature as well.
 const base::Feature kSyncUserEvents{"SyncUserEvents",

components/sync/driver/sync_driver_switches.h

@@ -20,6 +20,7 @@ extern const char kSyncShortInitialRetryOverride[];
 extern const char kSyncShortNudgeDelayForTest[];
 
 extern const base::Feature kSyncClearDataOnPassphraseEncryption;
+extern const base::Feature kSyncStandaloneTransport;
 extern const base::Feature kSyncUserEvents;
 extern const base::Feature kSyncUserFieldTrialEvents;
 extern const base::Feature kSyncUserConsentEvents;

components/sync/driver/sync_service.cc

@@ -13,6 +13,10 @@ SyncSetupInProgressHandle::~SyncSetupInProgressHandle() {
   on_destroy_.Run();
 }
 
+bool SyncService::IsSyncFeatureEnabled() const {
+  return GetDisableReasons() == DISABLE_REASON_NONE && IsFirstSetupComplete();
+}
+
 bool SyncService::CanSyncStart() const {
   return GetDisableReasons() == DISABLE_REASON_NONE;
 }
@@ -34,6 +38,9 @@ bool SyncService::IsEngineInitialized() const {
 }
 
 bool SyncService::IsSyncActive() const {
+  if (!IsSyncFeatureEnabled()) {
+    return false;
+  }
   switch (GetState()) {
     case State::DISABLED:
     case State::WAITING_FOR_START_REQUEST:

components/sync/driver/sync_service.h

@@ -85,9 +85,8 @@ class SyncService : public DataTypeEncryptionHandler, public KeyedService {
 
   // The overall state of the SyncService, in ascending order of "activeness".
   enum class State {
-    // Sync is disabled, e.g. due to enterprise policy, or because the user has
-    // disabled it, or simply because there is no authenticated user. Call
-    // GetDisableReasons to figure out which of these it is.
+    // Sync is inactive, e.g. due to enterprise policy, or simply because there
+    // is no authenticated user.
     DISABLED,
     // Sync can start in principle, but nothing has prodded it to actually do it
     // yet. Note that during subsequent browser startups, Sync starts
@@ -144,6 +143,8 @@ class SyncService : public DataTypeEncryptionHandler, public KeyedService {
 
   // Returns the set of reasons that are keeping Sync disabled, as a bitmask of
   // DisableReason enum entries.
+  // Note: This refers to Sync-the-feature. Sync-the-transport may be running
+  // even in the presence of disable reasons.
   virtual int GetDisableReasons() const = 0;
   // Helper that returns whether GetDisableReasons() contains the given |reason|
   // (possibly among others).
@@ -153,6 +154,8 @@ class SyncService : public DataTypeEncryptionHandler, public KeyedService {
 
   // Returns the overall state of the SyncService. See the enum definition for
   // what the individual states mean.
+  // Note: This refers to Sync-the-transport, which may be active even if
+  // Sync-the-feature is disabled by the user, by enterprise policy, etc.
   // Note: If your question is "Are we actually sending this data to Google?" or
   // "Am I allowed to send this type of data to Google?", you probably want
   // syncer::GetUploadToGoogleState instead of this.
@@ -177,6 +180,13 @@ class SyncService : public DataTypeEncryptionHandler, public KeyedService {
   // DERIVED STATE ACCESS
   //////////////////////////////////////////////////////////////////////////////
 
+  // Returns whether all conditions are satisfied for Sync-the-feature to start.
+  // This means that there are no disable reasons, and first-time Sync setup has
+  // been completed by the user.
+  // Note: This does not imply that Sync is actually running. Check GetState to
+  // get the current state of Sync-the-transport.
+  bool IsSyncFeatureEnabled() const;
+
   // DEPRECATED! Use GetDisableReasons/HasDisableReason instead.
   // Equivalent to "HasDisableReason(DISABLE_REASON_UNRECOVERABLE_ERROR)".
   bool HasUnrecoverableError() const;
@@ -184,16 +194,23 @@ class SyncService : public DataTypeEncryptionHandler, public KeyedService {
   // DEPRECATED! Use GetState instead. Equivalent to
   // "GetState() == State::PENDING_DESIRED_CONFIGURATION ||
   // GetState() == State::CONFIGURING || GetState() == State::ACTIVE".
+  // Note: This refers to Sync-the-transport, which may be active even if
+  // Sync-the-feature is disabled by the user, by enterprise policy, etc.
   bool IsEngineInitialized() const;
 
   // DEPRECATED! Use GetDisableReasons/HasDisableReason instead.
   // Equivalent to having no disable reasons, i.e.
   // "GetDisableReasons() == DISABLE_REASON_NONE".
+  // Note: This refers to Sync-the-feature. Sync-the-transport may be running
+  // even if this is false.
   bool CanSyncStart() const;
 
-  // DEPRECATED! Use GetState instead. Equivalent to
-  // "GetState() == State::CONFIGURING || GetState() == State::ACTIVE".
+  // Returns whether Sync-the-feature is active, which means GetState() is
+  // either State::CONFIGURING or State::ACTIVE and IsSyncFeatureEnabled() is
+  // true.
   // To see which datatypes are actually syncing, see GetActiveDataTypes().
+  // Note: This refers to Sync-the-feature. Sync-the-transport may be active
+  // even if this is false.
   bool IsSyncActive() const;
 
   //////////////////////////////////////////////////////////////////////////////
@@ -204,15 +221,21 @@ class SyncService : public DataTypeEncryptionHandler, public KeyedService {
   // if the user is customizing sync after already completing setup once).
   // SyncService uses this to determine if it's OK to start syncing, or if the
   // user is still setting up the initial sync configuration.
+  // Note: This refers to Sync-the-feature. Sync-the-transport may be active
+  // independent of first-setup state.
   bool IsFirstSetupInProgress() const;
 
   // Whether the user has completed the initial Sync setup. This does not mean
   // that sync is currently running (due to delayed startup, unrecoverable
   // errors, or shutdown). If you want to know whether Sync is actually running,
   // use GetState instead.
+  // Note: This refers to Sync-the-feature. Sync-the-transport may be active
+  // independent of first-setup state.
   virtual bool IsFirstSetupComplete() const = 0;
 
   // Called when Sync has been setup by the user and can be started.
+  // Note: This refers to Sync-the-feature. Sync-the-transport may be active
+  // independent of first-setup state.
   virtual void SetFirstSetupComplete() = 0;
 
   //////////////////////////////////////////////////////////////////////////////
@@ -262,6 +285,8 @@ class SyncService : public DataTypeEncryptionHandler, public KeyedService {
   // engine should clear its data directory when it shuts down. Generally
   // KEEP_DATA is used when the user just stops sync, and CLEAR_DATA is used
   // when they sign out of the profile entirely.
+  // Note: This refers to Sync-the-feature. Sync-the-transport may remain active
+  // after calling this.
   virtual void RequestStop(SyncStopDataFate data_fate) = 0;
 
   // Called when a datatype (SyncableService) has a need for sync to start

ios/chrome/browser/ui/settings/clear_browsing_data_collection_view_controller_unittest.mm

@@ -178,6 +178,8 @@ TEST_F(ClearBrowsingDataCollectionViewControllerTest,
       .WillRepeatedly(Return(syncer::SyncService::DISABLE_REASON_NONE));
   EXPECT_CALL(*mock_sync_service_, GetState())
       .WillRepeatedly(Return(syncer::SyncService::State::ACTIVE));
+  EXPECT_CALL(*mock_sync_service_, IsFirstSetupComplete())
+      .WillRepeatedly(Return(true));
   EXPECT_CALL(*mock_sync_service_, GetActiveDataTypes())
       .WillRepeatedly(Return(syncer::ModelTypeSet()));
   EXPECT_CALL(*mock_sync_service_, IsUsingSecondaryPassphrase())