[Sync] Remove UIDataTypeController

components/sync/driver/non_ui_data_type_controller.h

Index: components/sync/driver/non_ui_data_type_controller.h
diff --git a/components/sync/driver/non_ui_data_type_controller.h b/components/sync/driver/non_ui_data_type_controller.h
index 3b01e15ec92812378745b155a0c288603302c24f..61062bdd258e1780b923feaf76fd366f3d644612 100644
--- a/components/sync/driver/non_ui_data_type_controller.h
+++ b/components/sync/driver/non_ui_data_type_controller.h
@@ -11,6 +11,7 @@
 #include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#include "base/sequenced_task_runner.h"
 #include "components/sync/driver/directory_data_type_controller.h"
 #include "components/sync/driver/shared_change_processor.h"
 
@@ -23,20 +24,27 @@ struct UserShare;
 class NonUIDataTypeController : public DirectoryDataTypeController {

[maxbogue, 2016/11/18 19:31:03]

This is now a Very Bad Name for this class, as UI types now use it. What would
you think of AsyncDirectoryTypeController instead? (I also think
DirectoryDataTypeController should just be DirectoryTypeController but that
could be a separate CL.)

[pavely, 2016/11/19 01:11:42]

Async in AsyncDirectoryTypeController doesn't mean anything (async as opposed to
what?). I would rather prefer "Controller for Directory backed SyncableService
based type" and its counterpart "Controller for Directory based Old API based
type" (FrontendDataTypeController).

That being said... Because of the issues with SessionsSyncManager
UIDataTypeController is back. Renaming will have to wait till better times.

  public:
   // |dump_stack| is called when an unrecoverable error occurs.
-  NonUIDataTypeController(ModelType type,
-                          const base::Closure& dump_stack,
-                          SyncClient* sync_client);
+  NonUIDataTypeController(
+      ModelType type,
+      const base::Closure& dump_stack,
+      SyncClient* sync_client,
+      ModelSafeGroup model_safe_group,
+      const scoped_refptr<base::SequencedTaskRunner>& backend_thread);

[maxbogue, 2016/11/18 19:31:03]

by value

[pavely, 2016/11/19 01:11:42]

Done.

   ~NonUIDataTypeController() override;
 
   // DataTypeController interface.
   void LoadModels(const ModelLoadCallback& model_load_callback) override;
   void StartAssociating(const StartCallback& start_callback) override;
   void Stop() override;
-  ModelSafeGroup model_safe_group() const override = 0;
   ChangeProcessor* GetChangeProcessor() const override;
   std::string name() const override;
   State state() const override;
 
+  // Used by tests to override the factory used to create
+  // GenericChangeProcessors.
+  void SetGenericChangeProcessorFactoryForTest(
+      std::unique_ptr<GenericChangeProcessorFactory> factory);
+
  protected:
   // For testing only.
   NonUIDataTypeController();
@@ -58,9 +66,11 @@ class NonUIDataTypeController : public DirectoryDataTypeController {
   // Posts the given task to the backend thread, i.e. the thread the
   // datatype lives on.  Return value: True if task posted successfully,
   // false otherwise.
+  // Default implementation posts task to backend_thread_. Types that don't use
+  // TaskRunner need to override this method.
   virtual bool PostTaskOnBackendThread(

[maxbogue, 2016/11/18 19:31:03]

Can we change this to PostTaskOnModelThread?

[pavely, 2016/11/19 01:11:42]

The names don't feel that different to me, but ok.

       const tracked_objects::Location& from_here,
-      const base::Closure& task) = 0;
+      const base::Closure& task);
 
   // Start up complete, update the state and invoke the callback.
   virtual void StartDone(DataTypeController::ConfigureResult start_result,
@@ -107,6 +117,10 @@ class NonUIDataTypeController : public DirectoryDataTypeController {
   // passed to SharedChangeProcessor::Connect on the model thread.
   UserShare* user_share_;
 
+  // Factory is used by tests to inject custom implementation of
+  // GenericChangeProcessor.
+  std::unique_ptr<GenericChangeProcessorFactory> processor_factory_;

[maxbogue, 2016/11/18 19:31:03]

We have so many processor classes that I actually think "gcp_factory_" would be
clearer here.

[pavely, 2016/11/19 01:11:42]

The name came from ui_data_type_controller.h and I actually like it better. I
more often refer to a processor by the word "processor". If I see gcp_factory_
in the code it will be harder to recall what gcp is, where is processor will
refer to different class in different controller, but it is usually clear from
context.

+
   // State of this datatype controller.
   State state_;
 
@@ -114,6 +128,10 @@ class NonUIDataTypeController : public DirectoryDataTypeController {
   StartCallback start_callback_;
   ModelLoadCallback model_load_callback_;
 
+  // Task runner of the backend thread. Can be nullptr in which case datatype
+  // controller needs to override PostTaskOnBackendThread().
+  scoped_refptr<base::SequencedTaskRunner> backend_thread_;

[maxbogue, 2016/11/18 19:31:03]

I'd prefer model_thread_ for consistency.

[pavely, 2016/11/19 01:11:42]

backend_thread_ goes more consistent with PostTaskOnBackendThread() function
name.

+
   // The shared change processor is the thread-safe interface to the
   // datatype.  We hold a reference to it from the UI thread so that
   // we can call Disconnect() on it from Stop()/StartDoneImpl().  Most