Simplify SyncableService by avoiding unnecessary base interface

There is no need to cast SyncableService instances to interface
SyncChangeProcessor, neither do these services need to implement any
functions in there, since they are not exercised.

One exception is SyncChangeProcessor::UpdateDataTypeContext(), which
is called from SharedChangeProcessor, but there is no SyncableService
that implements any functionality in such function.

In order to keep the patch less intrusive, GetAllSyncData() is kept
around, now moved to SyncableService, since all implementations provide
such functionality. However, we don't seem to call that function from
anywhere, so a TODO has been added to simplify that in future patches.

TBR=groby@chromium.org

Bug: 870624
Change-Id: I54b33cdd1ecc5584a806d3ff0fc0f3f6708467ca
Reviewed-on: https://chromium-review.googlesource.com/1243809Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594702}

chrome/browser/extensions/api/storage/sync_storage_backend.cc

@@ -11,6 +11,7 @@
 #include "chrome/browser/extensions/api/storage/settings_sync_processor.h"
 #include "chrome/browser/extensions/api/storage/settings_sync_util.h"
 #include "chrome/browser/extensions/api/storage/syncable_settings_storage.h"
+#include "components/sync/model/sync_change_processor.h"
 #include "components/sync/model/sync_error_factory.h"
 #include "extensions/browser/api/storage/backend_task_runner.h"
 

chrome/browser/spellchecker/spellcheck_custom_dictionary.cc

@@ -6,6 +6,7 @@
 
 #include <stddef.h>
 
+#include <algorithm>
 #include <functional>
 #include <utility>
 #include <vector>
@@ -23,6 +24,7 @@
 #include "components/spellcheck/browser/spellcheck_host_metrics.h"
 #include "components/spellcheck/common/spellcheck_common.h"
 #include "components/sync/model/sync_change.h"
+#include "components/sync/model/sync_change_processor.h"
 #include "components/sync/model/sync_error_factory.h"
 #include "components/sync/protocol/sync.pb.h"
 #include "content/public/browser/browser_task_traits.h"

chrome/browser/supervised_user/supervised_user_settings_service.cc

@@ -6,6 +6,7 @@
 
 #include <stddef.h>
 
+#include <set>
 #include <utility>
 
 #include "base/callback.h"
@@ -20,6 +21,7 @@
 #include "components/prefs/json_pref_store.h"
 #include "components/prefs/pref_filter.h"
 #include "components/sync/model/sync_change.h"
+#include "components/sync/model/sync_change_processor.h"
 #include "components/sync/model/sync_error_factory.h"
 #include "components/sync/protocol/sync.pb.h"
 #include "content/public/browser/browser_thread.h"

chrome/browser/supervised_user/supervised_user_whitelist_service.cc

@@ -24,6 +24,7 @@
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
 #include "components/sync/model/sync_change.h"
+#include "components/sync/model/sync_change_processor.h"
 #include "components/sync/model/sync_data.h"
 #include "components/sync/model/sync_error.h"
 #include "components/sync/model/sync_error_factory.h"

chrome/browser/supervised_user/supervised_user_whitelist_service.h

@@ -9,6 +9,7 @@
 #include <memory>
 #include <set>
 #include <string>
+#include <vector>
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"

chrome/browser/supervised_user/supervised_user_whitelist_service_unittest.cc

@@ -24,6 +24,7 @@
 #include "chrome/test/base/testing_profile.h"
 #include "components/prefs/scoped_user_pref_update.h"
 #include "components/sync/model/sync_change.h"
+#include "components/sync/model/sync_change_processor.h"
 #include "components/sync/model/sync_error_factory.h"
 #include "components/sync/protocol/sync.pb.h"
 #include "content/public/test/test_browser_thread_bundle.h"

chrome/browser/themes/theme_syncable_service.cc

@@ -6,6 +6,7 @@
 
 #include <stddef.h>
 
+#include <string>
 #include <utility>
 
 #include "base/strings/stringprintf.h"
@@ -14,6 +15,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/themes/theme_service.h"
 #include "chrome/common/extensions/sync_helper.h"
+#include "components/sync/model/sync_change_processor.h"
 #include "components/sync/protocol/sync.pb.h"
 #include "components/sync/protocol/theme_specifics.pb.h"
 #include "extensions/browser/disable_reason.h"

components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc

@@ -22,6 +22,7 @@
 #include "components/autofill/core/browser/webdata/autofill_table.h"
 #include "components/autofill/core/browser/webdata/autofill_webdata_service.h"
 #include "components/autofill/core/common/autofill_constants.h"
+#include "components/sync/model/sync_change_processor.h"
 #include "components/sync/model/sync_error.h"
 #include "components/sync/model/sync_error_factory.h"
 #include "components/sync/protocol/sync.pb.h"
@@ -644,6 +645,11 @@ void AutofillProfileSyncableService::ActOnChange(
   }
 }
 
+void AutofillProfileSyncableService::set_sync_processor(
+    syncer::SyncChangeProcessor* sync_processor) {
+  sync_processor_.reset(sync_processor);
+}
+
 syncer::SyncData AutofillProfileSyncableService::CreateData(
     const AutofillProfile& profile) {
   sync_pb::EntitySpecifics specifics;

components/autofill/core/browser/webdata/autofill_profile_syncable_service.h

@@ -108,9 +108,7 @@ class AutofillProfileSyncableService
 
   // For unit tests.
   AutofillProfileSyncableService();
-  void set_sync_processor(syncer::SyncChangeProcessor* sync_processor) {
-    sync_processor_.reset(sync_processor);
-  }
+  void set_sync_processor(syncer::SyncChangeProcessor* sync_processor);
 
   // Creates syncer::SyncData based on supplied |profile|.
   // Exposed for unit tests.

components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc

@@ -17,6 +17,7 @@
 #include "components/autofill/core/browser/country_names.h"
 #include "components/autofill/core/browser/webdata/autofill_change.h"
 #include "components/autofill/core/common/autofill_constants.h"
+#include "components/sync/model/sync_change_processor.h"
 #include "components/sync/model/sync_error_factory.h"
 #include "components/sync/model/sync_error_factory_mock.h"
 #include "components/sync/protocol/sync.pb.h"

components/autofill/core/browser/webdata/autofill_wallet_syncable_service.cc

@@ -6,6 +6,7 @@
 
 #include <stddef.h>
 
+#include <algorithm>
 #include <map>
 #include <utility>
 
@@ -19,6 +20,7 @@
 #include "components/autofill/core/browser/webdata/autofill_webdata_backend.h"
 #include "components/autofill/core/browser/webdata/autofill_webdata_service.h"
 #include "components/autofill/core/common/autofill_util.h"
+#include "components/sync/model/sync_change_processor.h"
 #include "components/sync/model/sync_error_factory.h"
 #include "components/sync/protocol/sync.pb.h"
 

components/password_manager/core/browser/password_syncable_service.cc

@@ -17,6 +17,7 @@
 #include "components/password_manager/core/browser/password_manager_metrics_util.h"
 #include "components/password_manager/core/browser/password_store_sync.h"
 #include "components/password_manager/core/common/password_manager_features.h"
+#include "components/sync/model/sync_change_processor.h"
 #include "components/sync/model/sync_error_factory.h"
 #include "net/base/escape.h"
 

components/password_manager/core/browser/password_syncable_service_unittest.cc

@@ -19,6 +19,7 @@
 #include "components/password_manager/core/browser/mock_password_store.h"
 #include "components/password_manager/core/common/password_manager_features.h"
 #include "components/sync/model/sync_change_processor.h"
+#include "components/sync/model/sync_change_processor_wrapper_for_test.h"
 #include "components/sync/model/sync_error.h"
 #include "components/sync/model/sync_error_factory_mock.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -191,11 +192,6 @@ class PasswordSyncableServiceWrapper {
 
   PasswordSyncableService* service() { return service_.get(); }
 
-  // Returnes the scoped_ptr to |service_| thus NULLing out it.
-  std::unique_ptr<syncer::SyncChangeProcessor> ReleaseSyncableService() {
-    return std::move(service_);
-  }
-
  private:
   scoped_refptr<MockPasswordStore> password_store_;
   std::unique_ptr<PasswordSyncableService> service_;
@@ -466,7 +462,8 @@ TEST_F(PasswordSyncableServiceTest, MergeDataAndPushBack) {
       other_service_wrapper.service()->GetAllSyncData(syncer::PASSWORDS);
   service()->MergeDataAndStartSyncing(
       syncer::PASSWORDS, other_service_data,
-      other_service_wrapper.ReleaseSyncableService(),
+      std::make_unique<syncer::SyncChangeProcessorWrapperForTest>(
+          other_service_wrapper.service()),
       std::unique_ptr<syncer::SyncErrorFactory>());
 }
 

components/search_engines/template_url_service.cc

@@ -4,6 +4,8 @@
 
 #include "components/search_engines/template_url_service.h"
 
+#include <algorithm>
+
 #include "base/auto_reset.h"
 #include "base/callback.h"
 #include "base/debug/crash_logging.h"
@@ -22,6 +24,7 @@
 #include "components/search_engines/template_url_service_client.h"
 #include "components/search_engines/template_url_service_observer.h"
 #include "components/search_engines/util.h"
+#include "components/sync/model/sync_change_processor.h"
 #include "components/sync/model/sync_error_factory.h"
 #include "components/sync/protocol/search_engine_specifics.pb.h"
 #include "components/sync/protocol/sync.pb.h"
@@ -2146,7 +2149,6 @@ void TemplateURLService::ResolveSyncKeywordConflict(
   change_list->push_back(syncer::SyncChange(FROM_HERE,
       syncer::SyncChange::ACTION_UPDATE,
       sync_data));
-
 }
 
 void TemplateURLService::MergeInSyncTemplateURL(

components/search_engines/template_url_service.h

@@ -7,6 +7,13 @@
 
 #include <stddef.h>
 
+#include <map>
+#include <memory>
+#include <set>
+#include <string>
+#include <utility>
+#include <vector>
+
 #include "base/callback_list.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"

components/sync/driver/shared_change_processor.cc

@@ -107,12 +107,6 @@ void SharedChangeProcessor::StartAssociation(
       return;
     }
 
-    std::string datatype_context;
-    if (GetDataTypeContext(&datatype_context)) {
-      local_service_->UpdateDataTypeContext(
-          type_, SyncChangeProcessor::NO_REFRESH, datatype_context);
-    }
-
     syncer_merge_result.set_num_items_before_association(
         initial_sync_data.size());
     // Passes a reference to |shared_change_processor|.

components/sync/model/fake_syncable_service.cc

@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "base/location.h"
+#include "components/sync/model/sync_change_processor.h"
 #include "components/sync/model/sync_error_factory.h"
 
 namespace syncer {

components/sync/model/sync_change.h

@@ -71,6 +71,8 @@ class SyncChange {
 // gmock printer helper.
 void PrintTo(const SyncChange& sync_change, std::ostream* os);
 
+using SyncChangeList = std::vector<SyncChange>;
+
 }  // namespace syncer
 
 #endif  // COMPONENTS_SYNC_MODEL_SYNC_CHANGE_H_

components/sync/model/sync_change_processor.h

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "components/sync/base/model_type.h"
+#include "components/sync/model/sync_change.h"
 #include "components/sync/model/sync_data.h"
 #include "components/sync/model/sync_error.h"
 
@@ -18,12 +19,8 @@ class Location;
 
 namespace syncer {
 
-class SyncChange;
-
 class LocalChangeObserver;
 
-using SyncChangeList = std::vector<SyncChange>;
-
 // An interface for services that handle receiving SyncChanges.
 class SyncChangeProcessor {
  public:

components/sync/model/sync_change_processor_wrapper_for_test.cc

@@ -4,12 +4,25 @@
 
 #include "components/sync/model/sync_change_processor_wrapper_for_test.h"
 
+#include "base/bind.h"
+#include "components/sync/model/syncable_service.h"
+
 namespace syncer {
 
 SyncChangeProcessorWrapperForTest::SyncChangeProcessorWrapperForTest(
     SyncChangeProcessor* wrapped)
-    : wrapped_(wrapped) {
-  DCHECK(wrapped_);
+    : process_sync_changes_(
+          base::BindRepeating(&SyncChangeProcessor::ProcessSyncChanges,
+                              base::Unretained(wrapped))) {
+  DCHECK(wrapped);
+}
+
+SyncChangeProcessorWrapperForTest::SyncChangeProcessorWrapperForTest(
+    SyncableService* wrapped)
+    : process_sync_changes_(
+          base::BindRepeating(&SyncableService::ProcessSyncChanges,
+                              base::Unretained(wrapped))) {
+  DCHECK(wrapped);
 }
 
 SyncChangeProcessorWrapperForTest::~SyncChangeProcessorWrapperForTest() {}
@@ -17,12 +30,13 @@ SyncChangeProcessorWrapperForTest::~SyncChangeProcessorWrapperForTest() {}
 SyncError SyncChangeProcessorWrapperForTest::ProcessSyncChanges(
     const base::Location& from_here,
     const SyncChangeList& change_list) {
-  return wrapped_->ProcessSyncChanges(from_here, change_list);
+  return process_sync_changes_.Run(from_here, change_list);
 }
 
 SyncDataList SyncChangeProcessorWrapperForTest::GetAllSyncData(
     ModelType type) const {
-  return wrapped_->GetAllSyncData(type);
+  NOTREACHED();
+  return SyncDataList();
 }
 
 }  // namespace syncer

components/sync/model/sync_change_processor_wrapper_for_test.h

@@ -5,12 +5,16 @@
 #ifndef COMPONENTS_SYNC_MODEL_SYNC_CHANGE_PROCESSOR_WRAPPER_FOR_TEST_H_
 #define COMPONENTS_SYNC_MODEL_SYNC_CHANGE_PROCESSOR_WRAPPER_FOR_TEST_H_
 
+#include "base/callback.h"
 #include "base/macros.h"
 #include "components/sync/model/sync_change_processor.h"
 
 namespace syncer {
 
-// A wrapper class for use in tests.
+class SyncableService;
+
+// A wrapper class for use in tests that forwards changes to a SyncableService
+// or a SyncChangeProcessor;
 class SyncChangeProcessorWrapperForTest : public SyncChangeProcessor {
  public:
   // Create a SyncChangeProcessorWrapperForTest.
@@ -18,6 +22,8 @@ class SyncChangeProcessorWrapperForTest : public SyncChangeProcessor {
   // All method calls are forwarded to |wrapped|. Caller maintains ownership
   // of |wrapped| and is responsible for ensuring it outlives this object.
   explicit SyncChangeProcessorWrapperForTest(SyncChangeProcessor* wrapped);
+  // Overload for SyncableService.
+  explicit SyncChangeProcessorWrapperForTest(SyncableService* wrapped);
   ~SyncChangeProcessorWrapperForTest() override;
 
   // SyncChangeProcessor implementation.
@@ -26,7 +32,9 @@ class SyncChangeProcessorWrapperForTest : public SyncChangeProcessor {
   SyncDataList GetAllSyncData(ModelType type) const override;
 
  private:
-  SyncChangeProcessor* const wrapped_;
+  const base::RepeatingCallback<SyncError(const base::Location& from_here,
+                                          const SyncChangeList& change_list)>
+      process_sync_changes_;
 
   DISALLOW_COPY_AND_ASSIGN(SyncChangeProcessorWrapperForTest);
 };

components/sync/model/syncable_service.cc

@@ -6,6 +6,8 @@
 
 namespace syncer {
 
+SyncableService::SyncableService() {}
+
 SyncableService::~SyncableService() {}
 
 }  // namespace syncer

components/sync/model/syncable_service.h

@@ -9,24 +9,26 @@
 #include <vector>
 
 #include "base/callback.h"
-#include "base/compiler_specific.h"
 #include "base/memory/weak_ptr.h"
 #include "components/sync/base/model_type.h"
-#include "components/sync/model/sync_change_processor.h"
+#include "components/sync/model/sync_change.h"
 #include "components/sync/model/sync_data.h"
 #include "components/sync/model/sync_error.h"
 #include "components/sync/model/sync_merge_result.h"
 
 namespace syncer {
 
+class SyncChangeProcessor;
 class SyncErrorFactory;
 
 // TODO(zea): remove SupportsWeakPtr in favor of having all SyncableService
 // implementers provide a way of getting a weak pointer to themselves.
 // See crbug.com/100114.
-class SyncableService : public SyncChangeProcessor,
-                        public base::SupportsWeakPtr<SyncableService> {
+class SyncableService : public base::SupportsWeakPtr<SyncableService> {
  public:
+  SyncableService();
+  virtual ~SyncableService();
+
   // A StartSyncFlare is useful when your SyncableService has a need for sync
   // to start ASAP. This is typically for one of three reasons:
   // 1) Because a local change event has occurred but MergeDataAndStartSyncing
@@ -64,11 +66,15 @@ class SyncableService : public SyncChangeProcessor,
   // Returns: A default SyncError (IsSet() == false) if no errors were
   //          encountered, and a filled SyncError (IsSet() == true)
   //          otherwise.
-  SyncError ProcessSyncChanges(const base::Location& from_here,
-                               const SyncChangeList& change_list) override = 0;
+  virtual SyncError ProcessSyncChanges(const base::Location& from_here,
+                                       const SyncChangeList& change_list) = 0;
+
+  // TODO(crbug.com/870624): We don't seem to use this function anywhere, so
+  // we should simply remove it and simplify all implementations.
+  virtual SyncDataList GetAllSyncData(ModelType type) const = 0;
 
- protected:
-  ~SyncableService() override;
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SyncableService);
 };
 
 }  // namespace syncer

components/sync_preferences/pref_model_associator.cc

@@ -22,6 +22,7 @@
 #include "components/prefs/persistent_pref_store.h"
 #include "components/prefs/pref_service.h"
 #include "components/sync/model/sync_change.h"
+#include "components/sync/model/sync_change_processor.h"
 #include "components/sync/model/sync_error_factory.h"
 #include "components/sync/protocol/preference_specifics.pb.h"
 #include "components/sync/protocol/sync.pb.h"

components/sync_preferences/pref_service_syncable_unittest.cc

@@ -20,6 +20,7 @@
 #include "components/prefs/scoped_user_pref_update.h"
 #include "components/prefs/testing_pref_store.h"
 #include "components/sync/model/sync_change.h"
+#include "components/sync/model/sync_change_processor.h"
 #include "components/sync/model/sync_data.h"
 #include "components/sync/model/sync_error_factory_mock.h"
 #include "components/sync/model/syncable_service.h"

components/sync_sessions/sessions_sync_manager.cc

@@ -17,6 +17,7 @@
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "build/build_config.h"
 #include "components/sync/base/hash_util.h"
+#include "components/sync/model/sync_change_processor.h"
 #include "components/sync/model/sync_error.h"
 #include "components/sync/model/sync_error_factory.h"
 #include "components/sync/model/sync_merge_result.h"

components/sync_sessions/sessions_sync_manager_unittest.cc

@@ -10,6 +10,7 @@
 #include "base/strings/stringprintf.h"
 #include "build/build_config.h"
 #include "components/prefs/testing_pref_service.h"
+#include "components/sync/model/sync_change_processor.h"
 #include "components/sync/model/sync_error_factory_mock.h"
 #include "components/sync_sessions/mock_sync_sessions_client.h"
 #include "components/sync_sessions/session_sync_prefs.h"