[sync] ifdef Ash-specific API in SyncService

No behavioral changes, as the API isn't exercised outside Ash since
https://crrev.com/c/4525902, except for unit-tests.

As per pre-existing TODOs, ideally the API shouldn't even exist, but
meanwhile let's guard it behind ifdefs to avoid abuse and bugs. This is
because the state at hands, i.e.
sync-disabled-because-of-dashboard-reset, is only reachable on ChromeOS
Ash.

Change-Id: I7dcd734bce7d272d7f61d5ab0bee40ed3d692f0e
Bug: 1443446
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4546184
Reviewed-by: Victor Vianna <victorvianna@google.com>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1150285}

chrome/browser/ui/webui/settings/people_handler_unittest.cc

@@ -516,8 +516,10 @@ TEST_F(PeopleHandlerTest,
 TEST_F(PeopleHandlerTest, RestartSyncAfterDashboardClear) {
   SigninUser();
   CreatePeopleHandler();
+#if BUILDFLAG(IS_CHROMEOS_ASH)
   ON_CALL(*mock_sync_service_, IsSyncFeatureDisabledViaDashboard())
       .WillByDefault(Return(true));
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
   ON_CALL(*mock_sync_service_, GetTransportState())
       .WillByDefault(Return(syncer::SyncService::TransportState::DISABLED));
 
@@ -528,8 +530,10 @@ TEST_F(PeopleHandlerTest, RestartSyncAfterDashboardClear) {
         // and immediately starts initializing the engine.
         ON_CALL(*mock_sync_service_, GetDisableReasons())
             .WillByDefault(Return(syncer::SyncService::DisableReasonSet()));
+#if BUILDFLAG(IS_CHROMEOS_ASH)
         ON_CALL(*mock_sync_service_, IsSyncFeatureDisabledViaDashboard())
             .WillByDefault(Return(false));
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
         ON_CALL(*mock_sync_service_, GetTransportState())
             .WillByDefault(
                 Return(syncer::SyncService::TransportState::INITIALIZING));
@@ -545,19 +549,25 @@ TEST_F(PeopleHandlerTest,
        RestartSyncAfterDashboardClearWithStandaloneTransport) {
   SigninUser();
   CreatePeopleHandler();
+
+#if BUILDFLAG(IS_CHROMEOS_ASH)
   // Clearing sync from the dashboard results in
-  // IsSyncFeatureDisabledViaDashboard() returning true. However, the sync
-  // engine has restarted in standalone transport mode.
+  // IsSyncFeatureDisabledViaDashboard() returning true. Nevertheless,
+  // the sync engine has restarted in standalone transport mode.
   ON_CALL(*mock_sync_service_, IsSyncFeatureDisabledViaDashboard())
       .WillByDefault(Return(true));
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
+
   ON_CALL(*mock_sync_service_, GetTransportState())
       .WillByDefault(Return(syncer::SyncService::TransportState::ACTIVE));
 
   // Attempting to open the setup UI should re-enable sync-the-feature.
   EXPECT_CALL(*mock_sync_service_, SetSyncFeatureRequested())
       .WillOnce([&]() {
+#if BUILDFLAG(IS_CHROMEOS_ASH)
         ON_CALL(*mock_sync_service_, IsSyncFeatureDisabledViaDashboard())
             .WillByDefault(Return(false));
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
         ON_CALL(*mock_sync_service_, GetTransportState())
             .WillByDefault(
                 Return(syncer::SyncService::TransportState::CONFIGURING));
@@ -1120,12 +1130,14 @@ TEST_F(PeopleHandlerTest, DashboardClearWhileSettingsOpen_ConfirmSoon) {
 
   handler_->HandleShowSyncSetupUI(base::Value::List());
 
+#if BUILDFLAG(IS_CHROMEOS_ASH)
   // Now sync gets reset from the dashboard (the user clicked the "Manage synced
   // data" link), which results in the first-setup-complete bit being cleared.
   // While first-setup isn't completed, IsSyncFeatureDisabledViaDashboard() also
   // returns false.
   ON_CALL(*mock_sync_service_, IsSyncFeatureDisabledViaDashboard())
       .WillByDefault(Return(false));
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
   ON_CALL(*mock_sync_service_->GetMockUserSettings(),
           IsInitialSyncFeatureSetupComplete())
       .WillByDefault(Return(false));
@@ -1168,12 +1180,14 @@ TEST_F(PeopleHandlerTest, DashboardClearWhileSettingsOpen_ConfirmLater) {
 
   handler_->HandleShowSyncSetupUI(base::Value::List());
 
+#if BUILDFLAG(IS_CHROMEOS_ASH)
   // Now sync gets reset from the dashboard (the user clicked the "Manage synced
   // data" link), which results in the first-setup-complete bit being cleared.
   // While first-setup isn't completed, IsSyncFeatureDisabledViaDashboard() also
   // returns false.
   ON_CALL(*mock_sync_service_, IsSyncFeatureDisabledViaDashboard())
       .WillByDefault(Return(false));
+#endif
   ON_CALL(*mock_sync_service_->GetMockUserSettings(),
           IsInitialSyncFeatureSetupComplete())
       .WillByDefault(Return(false));

components/metrics/demographics/demographic_metrics_provider_unittest.cc

@@ -35,10 +35,12 @@ enum TestSyncServiceState {
   SYNC_FEATURE_NOT_ENABLED,
   SYNC_FEATURE_ENABLED,
   SYNC_FEATURE_ENABLED_BUT_PAUSED,
+#if BUILDFLAG(IS_CHROMEOS_ASH)
   // Represents the user clearing sync data via dashboard. On all platforms
   // except ChromeOS (Ash), this clears the primary account (which is basically
   // SYNC_FEATURE_NOT_ENABLED). On ChromeOS Ash, Sync enters a special state.
   SYNC_FEATURE_DISABLED_ON_CHROMEOS_ASH_VIA_DASHBOARD,
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 };
 
 // Profile client for testing that gets fake Profile information and services.
@@ -86,6 +88,7 @@ class TestProfileClient : public DemographicMetricsProvider::ProfileClient {
                  sync_service_->GetTransportState());
         break;
 
+#if BUILDFLAG(IS_CHROMEOS_ASH)
       case SYNC_FEATURE_DISABLED_ON_CHROMEOS_ASH_VIA_DASHBOARD:
         sync_service_ = std::make_unique<syncer::TestSyncService>();
         sync_service_->SetSyncFeatureDisabledViaDashboard(true);
@@ -99,6 +102,7 @@ class TestProfileClient : public DemographicMetricsProvider::ProfileClient {
                   ->IsInitialSyncFeatureSetupComplete());
         CHECK(!sync_service_->IsSyncFeatureEnabled());
         break;
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
     }
   }
 
@@ -205,6 +209,7 @@ TEST(DemographicMetricsProviderTest,
                                UserDemographicsStatus::kSyncNotEnabled, 1);
 }
 
+#if BUILDFLAG(IS_CHROMEOS_ASH)
 TEST(
     DemographicMetricsProviderTest,
     ProvideSyncedUserNoisedBirthYearAndGender_SyncFeatureDisabledOnChromeOsAshViaSyncDashboard) {
@@ -228,6 +233,7 @@ TEST(
   histogram.ExpectUniqueSample("UMA.UserDemographics.Status",
                                UserDemographicsStatus::kSyncNotEnabled, 1);
 }
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 
 TEST(DemographicMetricsProviderTest,
      ProvideSyncedUserNoisedBirthYearAndGender_SyncNotEnabled) {

components/sync/service/sync_service.cc

@@ -36,7 +36,9 @@ bool SyncService::IsSyncFeatureEnabled() const {
 
 bool SyncService::CanSyncFeatureStart() const {
   return GetDisableReasons().Empty() && HasSyncConsent() &&
+#if BUILDFLAG(IS_CHROMEOS_ASH)
          !IsSyncFeatureDisabledViaDashboard() &&
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
          IsSyncFeatureConsideredRequested();
 }
 

components/sync/service/sync_service.h

@@ -14,6 +14,7 @@
 #include "base/location.h"
 #include "base/time/time.h"
 #include "base/values.h"
+#include "build/build_config.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/service/sync_service_observer.h"
@@ -306,13 +307,16 @@ class SyncService : public KeyedService {
   // instead.
   virtual bool RequiresClientUpgrade() const = 0;
 
-  // Returns true only on ChromeOS (Ash), if sync-the-feature is disabled
-  // because the user cleared data from the Sync dashboard. It can be re-enabled
-  // by invoking SetSyncFeatureRequested().
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+  // Relevant only on ChromeOS (Ash), since the state is unreachable otherwise.
+  // Returns if sync-the-feature is disabled because the user cleared data from
+  // the Sync dashboard. It can be re-enabled by invoking
+  // SetSyncFeatureRequested().
   // TODO(crbug.com/1443446): Consider removing this API, for example by
   // reporting IsInitialSyncFeatureSetupComplete()==false which is otherwise
   // unreachable on ChromeOS Ash.
   virtual bool IsSyncFeatureDisabledViaDashboard() const = 0;
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 
   //////////////////////////////////////////////////////////////////////////////
   // DERIVED STATE ACCESS

components/sync/service/sync_service_impl.cc

@@ -1196,13 +1196,15 @@ bool SyncServiceImpl::RequiresClientUpgrade() const {
   return last_actionable_error_.action == UPGRADE_CLIENT;
 }
 
+#if BUILDFLAG(IS_CHROMEOS_ASH)
 bool SyncServiceImpl::IsSyncFeatureDisabledViaDashboard() const {
   // This can return true only on ChromeOS Ash, upon DISABLE_SYNC_ON_CLIENT.
   // TODO(crbug.com/1443446): A simpler and more robust implementation for this
   // state would be to use a dedicated pref.
   return user_settings_->IsInitialSyncFeatureSetupComplete() &&
          !IsLocalSyncEnabled() && !IsSyncFeatureConsideredRequested();
 }
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 
 bool SyncServiceImpl::CanConfigureDataTypes(
     bool bypass_setup_in_progress_check) const {

components/sync/service/sync_service_impl.h

@@ -110,7 +110,9 @@ class SyncServiceImpl : public SyncService,
   GoogleServiceAuthError GetAuthError() const override;
   base::Time GetAuthErrorTime() const override;
   bool RequiresClientUpgrade() const override;
+#if BUILDFLAG(IS_CHROMEOS_ASH)
   bool IsSyncFeatureDisabledViaDashboard() const override;
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
   std::unique_ptr<SyncSetupInProgressHandle> GetSetupInProgressHandle()
       override;
   bool IsSetupInProgress() const override;

components/sync/service/sync_service_impl_unittest.cc

@@ -1004,7 +1004,10 @@ TEST_F(SyncServiceImplTest, DisableSyncOnClient) {
   ASSERT_EQ(SyncService::TransportState::ACTIVE,
             service()->GetTransportState());
   ASSERT_EQ(0, get_controller(BOOKMARKS)->model()->clear_metadata_call_count());
+
+#if BUILDFLAG(IS_CHROMEOS_ASH)
   ASSERT_FALSE(service()->IsSyncFeatureDisabledViaDashboard());
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 
   EXPECT_CALL(
       *trusted_vault_client(),

components/sync/test/fake_sync_service.cc

@@ -104,9 +104,11 @@ bool FakeSyncService::RequiresClientUpgrade() const {
   return false;
 }
 
+#if BUILDFLAG(IS_CHROMEOS_ASH)
 bool FakeSyncService::IsSyncFeatureDisabledViaDashboard() const {
   return false;
 }
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 
 void FakeSyncService::DataTypePreconditionChanged(ModelType type) {}
 

components/sync/test/fake_sync_service.h

@@ -9,6 +9,7 @@
 #include <string>
 #include <vector>
 
+#include "build/build_config.h"
 #include "components/sync/service/sync_service.h"
 #include "google_apis/gaia/google_service_auth_error.h"
 #include "url/gurl.h"
@@ -49,7 +50,9 @@ class FakeSyncService : public SyncService {
   GoogleServiceAuthError GetAuthError() const override;
   base::Time GetAuthErrorTime() const override;
   bool RequiresClientUpgrade() const override;
+#if BUILDFLAG(IS_CHROMEOS_ASH)
   bool IsSyncFeatureDisabledViaDashboard() const override;
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
   void DataTypePreconditionChanged(syncer::ModelType type) override;
   SyncTokenStatus GetSyncTokenStatusForDebugging() const override;
   bool QueryDetailedSyncStatusForDebugging(SyncStatus* result) const override;

components/sync/test/mock_sync_service.h

@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/values.h"
+#include "build/build_config.h"
 #include "components/signin/public/identity_manager/account_info.h"
 #include "components/sync/engine/cycle/sync_cycle_snapshot.h"
 #include "components/sync/model/type_entities_count.h"
@@ -47,7 +48,9 @@ class MockSyncService : public SyncService {
   MOCK_METHOD(GoogleServiceAuthError, GetAuthError, (), (const override));
   MOCK_METHOD(base::Time, GetAuthErrorTime, (), (const override));
   MOCK_METHOD(bool, RequiresClientUpgrade, (), (const override));
+#if BUILDFLAG(IS_CHROMEOS_ASH)
   MOCK_METHOD(bool, IsSyncFeatureDisabledViaDashboard, (), (const override));
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
   MOCK_METHOD(std::unique_ptr<SyncSetupInProgressHandle>,
               GetSetupInProgressHandle,
               (),

components/sync/test/test_sync_service.cc

@@ -61,10 +61,12 @@ void TestSyncService::SetHasSyncConsent(bool has_sync_consent) {
   has_sync_consent_ = has_sync_consent;
 }
 
+#if BUILDFLAG(IS_CHROMEOS_ASH)
 void TestSyncService::SetSyncFeatureDisabledViaDashboard(
     bool disabled_via_dashboard) {
   sync_feature_disabled_via_dashboard_ = disabled_via_dashboard;
 }
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 
 void TestSyncService::SetPersistentAuthError() {
   transport_state_ = TransportState::PAUSED;
@@ -144,7 +146,9 @@ void TestSyncService::FireSyncCycleCompleted() {
 }
 
 void TestSyncService::SetSyncFeatureRequested() {
+#if BUILDFLAG(IS_CHROMEOS_ASH)
   sync_feature_disabled_via_dashboard_ = false;
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 }
 
 TestSyncUserSettings* TestSyncService::GetUserSettings() {
@@ -199,9 +203,11 @@ bool TestSyncService::RequiresClientUpgrade() const {
          syncer::UPGRADE_CLIENT;
 }
 
+#if BUILDFLAG(IS_CHROMEOS_ASH)
 bool TestSyncService::IsSyncFeatureDisabledViaDashboard() const {
   return sync_feature_disabled_via_dashboard_;
 }
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 
 std::unique_ptr<SyncSetupInProgressHandle>
 TestSyncService::GetSetupInProgressHandle() {

components/sync/test/test_sync_service.h

@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/observer_list.h"
+#include "build/build_config.h"
 #include "components/signin/public/identity_manager/account_info.h"
 #include "components/sync/engine/cycle/sync_cycle_snapshot.h"
 #include "components/sync/engine/sync_status.h"
@@ -37,7 +38,9 @@ class TestSyncService : public SyncService {
   void SetAccountInfo(const CoreAccountInfo& account_info);
   void SetHasSyncConsent(bool has_consent);
   void SetSetupInProgress(bool in_progress);
+#if BUILDFLAG(IS_CHROMEOS_ASH)
   void SetSyncFeatureDisabledViaDashboard(bool disabled_via_dashboard);
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 
   // Setters to mimic common auth error scenarios. Note that these functions
   // may change the transport state, as returned by GetTransportState().
@@ -77,7 +80,9 @@ class TestSyncService : public SyncService {
   GoogleServiceAuthError GetAuthError() const override;
   base::Time GetAuthErrorTime() const override;
   bool RequiresClientUpgrade() const override;
+#if BUILDFLAG(IS_CHROMEOS_ASH)
   bool IsSyncFeatureDisabledViaDashboard() const override;
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 
   std::unique_ptr<SyncSetupInProgressHandle> GetSetupInProgressHandle()
       override;
@@ -137,7 +142,9 @@ class TestSyncService : public SyncService {
   CoreAccountInfo account_info_;
   bool has_sync_consent_ = true;
   bool setup_in_progress_ = false;
+#if BUILDFLAG(IS_CHROMEOS_ASH)
   bool sync_feature_disabled_via_dashboard_ = false;
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 
   ModelTypeSet failed_data_types_;