[Sync] Propagate backed off data types to ProfileSyncService

This patch propagates cached sync status from SyngEngineImpl to the
SyncService. Also a new method to notify about changing in backed off
data types set was introduced.

These changes allow to turn off data types if they are in Backed off
state. This is done for SharingMessage data type in this patch.

Bug: 1058773
Change-Id: I34c9b12e8e9a50f3c8530ad3ff1c6ec6a34492c8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2093225
Commit-Queue: Rushan Suleymanov <rushans@google.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: vitaliii <vitaliii@chromium.org>
Reviewed-by: Michael van Ouwerkerk <mvanouwerkerk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748767}

chrome/browser/sharing/sharing_message_model_type_controller.cc

@@ -33,9 +33,13 @@ SharingMessageModelTypeController::~SharingMessageModelTypeController() {
 syncer::DataTypeController::PreconditionState
 SharingMessageModelTypeController::GetPreconditionState() const {
   DCHECK(CalledOnValidThread());
-  return syncer::IsWebSignout(sync_service_->GetAuthError())
-             ? PreconditionState::kMustStopAndClearData
-             : PreconditionState::kPreconditionsMet;
+  if (syncer::IsWebSignout(sync_service_->GetAuthError())) {
+    return PreconditionState::kMustStopAndClearData;
+  }
+  if (sync_service_->GetBackedOffDataTypes().Has(syncer::SHARING_MESSAGE)) {
+    return PreconditionState::kMustStopAndClearData;
+  }
+  return PreconditionState::kPreconditionsMet;
 }
 
 void SharingMessageModelTypeController::OnStateChanged(

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

@@ -10,6 +10,7 @@
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
 #include "chrome/browser/sync/test/integration/single_client_status_change_checker.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
+#include "components/sync/test/fake_server/fake_server_http_post_provider.h"
 
 namespace {
 
@@ -41,6 +42,16 @@ class NextCycleIterationChecker : public SingleClientStatusChangeChecker {
   base::Time last_synced_time_;
 };
 
+class BackedOffSharingMessageChecker : public SingleClientStatusChangeChecker {
+ public:
+  explicit BackedOffSharingMessageChecker(syncer::ProfileSyncService* service)
+      : SingleClientStatusChangeChecker(service) {}
+
+  bool IsExitConditionSatisfied(std::ostream* os) override {
+    return service()->GetBackedOffDataTypes().Has(syncer::SHARING_MESSAGE);
+  }
+};
+
 class SharingMessageEqualityChecker : public SingleClientStatusChangeChecker {
  public:
   SharingMessageEqualityChecker(
@@ -180,4 +191,37 @@ IN_PROC_BROWSER_TEST_F(SingleClientSharingMessageSyncTest,
   ASSERT_TRUE(NextCycleIterationChecker(GetSyncService(0)).Wait());
 }
 
+IN_PROC_BROWSER_TEST_F(SingleClientSharingMessageSyncTest,
+                       ShouldStopDataTypeWhenBackedOff) {
+  base::MockRepeatingCallback<void(const sync_pb::SharingMessageCommitError&)>
+      callback;
+
+  ASSERT_TRUE(SetupSync());
+  SharingMessageBridge* sharing_message_bridge =
+      SharingMessageBridgeFactory::GetForBrowserContext(GetProfile(0));
+  SharingMessageSpecifics specifics;
+  specifics.set_payload("payload");
+  sharing_message_bridge->SendSharingMessage(
+      std::make_unique<SharingMessageSpecifics>(specifics), callback.Get());
+
+  EXPECT_CALL(
+      callback,
+      Run(HasErrorCode(sync_pb::SharingMessageCommitError::SYNC_TURNED_OFF)));
+  ASSERT_FALSE(
+      GetSyncService(0)->GetBackedOffDataTypes().Has(syncer::SHARING_MESSAGE));
+
+  // Run the data type into backed off state before the message gets sent.
+  fake_server::FakeServerHttpPostProvider::DisableNetwork();
+
+  ASSERT_TRUE(BackedOffSharingMessageChecker(GetSyncService(0)).Wait());
+
+  EXPECT_TRUE(
+      GetSyncService(0)->GetBackedOffDataTypes().Has(syncer::SHARING_MESSAGE));
+  EXPECT_CALL(
+      callback,
+      Run(HasErrorCode(sync_pb::SharingMessageCommitError::SYNC_TURNED_OFF)));
+  sharing_message_bridge->SendSharingMessage(
+      std::make_unique<SharingMessageSpecifics>(specifics), callback.Get());
+}
+
 }  // namespace

components/sync/driver/fake_sync_service.cc

@@ -55,6 +55,10 @@ ModelTypeSet FakeSyncService::GetActiveDataTypes() const {
   return ModelTypeSet();
 }
 
+ModelTypeSet FakeSyncService::GetBackedOffDataTypes() const {
+  return ModelTypeSet();
+}
+
 void FakeSyncService::AddObserver(SyncServiceObserver* observer) {}
 
 void FakeSyncService::RemoveObserver(SyncServiceObserver* observer) {}

components/sync/driver/fake_sync_service.h

@@ -34,6 +34,7 @@ class FakeSyncService : public SyncService {
   bool IsLocalSyncEnabled() const override;
   void TriggerRefresh(const ModelTypeSet& types) override;
   ModelTypeSet GetActiveDataTypes() const override;
+  ModelTypeSet GetBackedOffDataTypes() const override;
   void AddObserver(SyncServiceObserver* observer) override;
   void RemoveObserver(SyncServiceObserver* observer) override;
   bool HasObserver(const SyncServiceObserver* observer) const override;

components/sync/driver/glue/sync_engine_impl.cc

@@ -229,7 +229,7 @@ UserShare* SyncEngineImpl::GetUserShare() const {
   return backend_->sync_manager()->GetUserShare();
 }
 
-SyncEngineImpl::Status SyncEngineImpl::GetDetailedStatus() {
+const SyncEngineImpl::Status& SyncEngineImpl::GetDetailedStatus() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(IsInitialized());
   return cached_status_;
@@ -438,7 +438,12 @@ void SyncEngineImpl::UpdateInvalidationVersions(
 
 void SyncEngineImpl::HandleSyncStatusChanged(const SyncStatus& status) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  const bool backed_off_types_changed =
+      (status.backed_off_types != cached_status_.backed_off_types);
   cached_status_ = status;
+  if (backed_off_types_changed) {
+    host_->OnBackedOffTypesChanged();
+  }
 }
 
 void SyncEngineImpl::OnCookieJarChanged(bool account_mismatch,

components/sync/driver/glue/sync_engine_impl.h

@@ -82,7 +82,7 @@ class SyncEngineImpl : public SyncEngine, public InvalidationHandler {
   void DeactivateNonBlockingDataType(ModelType type) override;
   void EnableEncryptEverything() override;
   UserShare* GetUserShare() const override;
-  Status GetDetailedStatus() override;
+  const Status& GetDetailedStatus() const override;
   void HasUnsyncedItemsForTest(
       base::OnceCallback<void(bool)> cb) const override;
   void GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out) const override;
@@ -160,7 +160,6 @@ class SyncEngineImpl : public SyncEngine, public InvalidationHandler {
   void UpdateInvalidationVersions(
       const std::map<ModelType, int64_t>& invalidation_versions);
 
-  // Stores the new |status| in local cache.
   void HandleSyncStatusChanged(const SyncStatus& status);
 
  private:

components/sync/driver/mock_sync_service.h

@@ -51,6 +51,7 @@ class MockSyncService : public SyncService {
   MOCK_CONST_METHOD0(GetRegisteredDataTypes, ModelTypeSet());
   MOCK_CONST_METHOD0(GetPreferredDataTypes, ModelTypeSet());
   MOCK_CONST_METHOD0(GetActiveDataTypes, ModelTypeSet());
+  MOCK_CONST_METHOD0(GetBackedOffDataTypes, ModelTypeSet());
 
   MOCK_METHOD0(StopAndClear, void());
   MOCK_METHOD1(OnDataTypeRequestsSyncStartup, void(ModelType type));

components/sync/driver/profile_sync_service.cc

@@ -1125,6 +1125,10 @@ void ProfileSyncService::OnActionableError(const SyncProtocolError& error) {
   NotifyObservers();
 }
 
+void ProfileSyncService::OnBackedOffTypesChanged() {
+  NotifyObservers();
+}
+
 void ProfileSyncService::OnConfigureDone(
     const DataTypeManager::ConfigureResult& result) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -1352,6 +1356,14 @@ ModelTypeSet ProfileSyncService::GetActiveDataTypes() const {
   return data_type_manager_->GetActiveDataTypes();
 }
 
+ModelTypeSet ProfileSyncService::GetBackedOffDataTypes() const {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  if (engine_ && engine_->IsInitialized()) {
+    return engine_->GetDetailedStatus().backed_off_types;
+  }
+  return ModelTypeSet();
+}
+
 void ProfileSyncService::SyncAllowedByPlatformChanged(bool allowed) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
@@ -1512,7 +1524,7 @@ ProfileSyncService::GetTypeStatusMapForDebugging() {
     return std::move(result);
   }
 
-  SyncStatus detailed_status = engine_->GetDetailedStatus();
+  const SyncStatus& detailed_status = engine_->GetDetailedStatus();
   const ModelTypeSet& throttled_types(detailed_status.throttled_types);
   const ModelTypeSet& backed_off_types(detailed_status.backed_off_types);
 

components/sync/driver/profile_sync_service.h

@@ -133,6 +133,7 @@ class ProfileSyncService : public SyncService,
   ModelTypeSet GetRegisteredDataTypes() const override;
   ModelTypeSet GetPreferredDataTypes() const override;
   ModelTypeSet GetActiveDataTypes() const override;
+  ModelTypeSet GetBackedOffDataTypes() const override;
   void StopAndClear() override;
   void OnDataTypeRequestsSyncStartup(ModelType type) override;
   void TriggerRefresh(const ModelTypeSet& types) override;
@@ -188,6 +189,7 @@ class ProfileSyncService : public SyncService,
   void OnConnectionStatusChange(ConnectionStatus status) override;
   void OnMigrationNeededForTypes(ModelTypeSet types) override;
   void OnActionableError(const SyncProtocolError& error) override;
+  void OnBackedOffTypesChanged() override;
 
   // DataTypeManagerObserver implementation.
   void OnConfigureDone(const DataTypeManager::ConfigureResult& result) override;

components/sync/driver/sync_service.h

@@ -331,6 +331,10 @@ class SyncService : public KeyedService {
   // be the empty set. Once the configuration completes the set will be updated.
   virtual ModelTypeSet GetActiveDataTypes() const = 0;
 
+  // Returns the set of currently backed off data types (e.g. returns non-empty
+  // result when the network was disabled during last sync cycle).
+  virtual ModelTypeSet GetBackedOffDataTypes() const = 0;
+
   //////////////////////////////////////////////////////////////////////////////
   // ACTIONS / STATE CHANGE REQUESTS
   //////////////////////////////////////////////////////////////////////////////

components/sync/driver/test_sync_service.cc

@@ -90,6 +90,10 @@ void TestSyncService::SetActiveDataTypes(const ModelTypeSet& types) {
   active_data_types_ = types;
 }
 
+void TestSyncService::SetBackedOffDataTypes(const ModelTypeSet& types) {
+  backed_off_data_types_ = types;
+}
+
 void TestSyncService::SetLastCycleSnapshot(const SyncCycleSnapshot& snapshot) {
   last_cycle_snapshot_ = snapshot;
 }
@@ -220,6 +224,10 @@ ModelTypeSet TestSyncService::GetActiveDataTypes() const {
   return active_data_types_;
 }
 
+ModelTypeSet TestSyncService::GetBackedOffDataTypes() const {
+  return backed_off_data_types_;
+}
+
 void TestSyncService::StopAndClear() {}
 
 void TestSyncService::OnDataTypeRequestsSyncStartup(ModelType type) {}

components/sync/driver/test_sync_service.h

@@ -38,6 +38,7 @@ class TestSyncService : public SyncService {
   void SetFirstSetupComplete(bool first_setup_complete);
   void SetPreferredDataTypes(const ModelTypeSet& types);
   void SetActiveDataTypes(const ModelTypeSet& types);
+  void SetBackedOffDataTypes(const ModelTypeSet& types);
   void SetLastCycleSnapshot(const SyncCycleSnapshot& snapshot);
   void SetUserDemographics(
       const UserDemographicsResult& user_demographics_result);
@@ -78,6 +79,7 @@ class TestSyncService : public SyncService {
   ModelTypeSet GetRegisteredDataTypes() const override;
   ModelTypeSet GetPreferredDataTypes() const override;
   ModelTypeSet GetActiveDataTypes() const override;
+  ModelTypeSet GetBackedOffDataTypes() const override;
 
   void StopAndClear() override;
   void OnDataTypeRequestsSyncStartup(ModelType type) override;
@@ -130,6 +132,7 @@ class TestSyncService : public SyncService {
 
   ModelTypeSet preferred_data_types_;
   ModelTypeSet active_data_types_;
+  ModelTypeSet backed_off_data_types_;
 
   bool detailed_sync_status_engine_available_ = false;
   SyncStatus detailed_sync_status_;

components/sync/engine/fake_sync_engine.cc

@@ -80,8 +80,8 @@ UserShare* FakeSyncEngine::GetUserShare() const {
   return nullptr;
 }
 
-SyncStatus FakeSyncEngine::GetDetailedStatus() {
-  return SyncStatus();
+const SyncStatus& FakeSyncEngine::GetDetailedStatus() const {
+  return default_sync_status_;
 }
 
 void FakeSyncEngine::HasUnsyncedItemsForTest(

components/sync/engine/fake_sync_engine.h

@@ -77,7 +77,7 @@ class FakeSyncEngine : public SyncEngine {
 
   UserShare* GetUserShare() const override;
 
-  SyncStatus GetDetailedStatus() override;
+  const SyncStatus& GetDetailedStatus() const override;
 
   void HasUnsyncedItemsForTest(
       base::OnceCallback<void(bool)> cb) const override;
@@ -102,6 +102,7 @@ class FakeSyncEngine : public SyncEngine {
  private:
   bool fail_initial_download_ = false;
   bool initialized_ = false;
+  const SyncStatus default_sync_status_;
 };
 
 }  // namespace syncer

components/sync/engine/mock_sync_engine.h

@@ -52,7 +52,7 @@ class MockSyncEngine : public SyncEngine {
   MOCK_METHOD1(Shutdown, void(ShutdownReason));
   MOCK_METHOD0(EnableEncryptEverything, void());
   MOCK_CONST_METHOD0(GetUserShare, UserShare*());
-  MOCK_METHOD0(GetDetailedStatus, SyncStatus());
+  MOCK_CONST_METHOD0(GetDetailedStatus, const SyncStatus&());
   MOCK_CONST_METHOD1(HasUnsyncedItemsForTest,
                      void(base::OnceCallback<void(bool)>));
   MOCK_CONST_METHOD1(GetModelSafeRoutingInfo, void(ModelSafeRoutingInfo*));

components/sync/engine/sync_engine.h

@@ -156,8 +156,8 @@ class SyncEngine : public ModelTypeConfigurer {
   // OnBackendInitialized().
   virtual UserShare* GetUserShare() const = 0;
 
-  // Called from any thread to obtain current detailed status information.
-  virtual SyncStatus GetDetailedStatus() = 0;
+  // Returns current detailed status information.
+  virtual const SyncStatus& GetDetailedStatus() const = 0;
 
   // Determines if the underlying sync engine has made any local changes to
   // items that have not yet been synced with the server.

components/sync/engine/sync_engine_host.h

@@ -90,6 +90,9 @@ class SyncEngineHost {
 
   // Called when the sync cycle returns there is an user actionable error.
   virtual void OnActionableError(const SyncProtocolError& error) = 0;
+
+  // Called when the set of backed off types is changed.
+  virtual void OnBackedOffTypesChanged() = 0;
 };
 
 }  // namespace syncer

components/sync/engine/sync_engine_host_stub.cc

@@ -42,4 +42,6 @@ void SyncEngineHostStub::OnMigrationNeededForTypes(ModelTypeSet types) {}
 
 void SyncEngineHostStub::OnActionableError(const SyncProtocolError& error) {}
 
+void SyncEngineHostStub::OnBackedOffTypesChanged() {}
+
 }  // namespace syncer

components/sync/engine/sync_engine_host_stub.h

@@ -39,6 +39,7 @@ class SyncEngineHostStub : public SyncEngineHost {
   void OnConnectionStatusChange(ConnectionStatus status) override;
   void OnMigrationNeededForTypes(ModelTypeSet types) override;
   void OnActionableError(const SyncProtocolError& error) override;
+  void OnBackedOffTypesChanged() override;
 };
 
 }  // namespace syncer