Introduce StorageOption during datatype configuration

This allows controllers to start datatypes in ephemeral mode,
which is only supported by ModelTypeController.

Bug: 866814
Change-Id: I20630c2ed52872baf0ecd23cccbd08f505e8835b
Reviewed-on: https://chromium-review.googlesource.com/1151299
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580205}

components/browser_sync/profile_sync_components_factory_impl.cc

@@ -312,8 +312,25 @@ ProfileSyncComponentsFactoryImpl::CreateCommonDataTypeControllers(
   }
 
   if (base::FeatureList::IsEnabled(switches::kSyncUserConsentSeparateType)) {
-    controllers.push_back(CreateModelTypeControllerForModelRunningOnUIThread(
-        syncer::USER_CONSENTS));
+    // Forward both on-disk and in-memory storage modes to the same delegate,
+    // since behavior for USER_CONSENTS does not differ (they are always
+    // persisted).
+    // TODO(crbug.com/867801): Replace the proxy delegates below with a simpler
+    // forwarding delegate that involves no posting of tasks.
+    controllers.push_back(std::make_unique<ModelTypeController>(
+        syncer::USER_CONSENTS,
+        /*delegate_on_disk=*/
+        std::make_unique<syncer::ProxyModelTypeControllerDelegate>(
+            ui_thread_,
+            base::BindRepeating(
+                &syncer::SyncClient::GetControllerDelegateForModelType,
+                base::Unretained(sync_client_), syncer::USER_CONSENTS)),
+        /*delegate_in_memory=*/
+        std::make_unique<syncer::ProxyModelTypeControllerDelegate>(
+            ui_thread_,
+            base::BindRepeating(
+                &syncer::SyncClient::GetControllerDelegateForModelType,
+                base::Unretained(sync_client_), syncer::USER_CONSENTS))));
   }
 
   return controllers;

components/browser_sync/profile_sync_service.cc

@@ -1537,6 +1537,7 @@ void ProfileSyncService::ConfigureDataTypeManager(
   configure_context.authenticated_account_id =
       GetAuthenticatedAccountInfo().account_id;
   configure_context.cache_guid = local_device_->GetLocalSyncCacheGUID();
+  configure_context.storage_option = syncer::ConfigureContext::STORAGE_ON_DISK;
   configure_context.reason = reason;
 
   if (!migrator_) {

components/sync/driver/async_directory_type_controller.cc

@@ -11,6 +11,7 @@
 #include "components/sync/base/bind_to_task_runner.h"
 #include "components/sync/base/data_type_histogram.h"
 #include "components/sync/base/model_type.h"
+#include "components/sync/driver/configure_context.h"
 #include "components/sync/driver/generic_change_processor_factory.h"
 #include "components/sync/driver/sync_api_component_factory.h"
 #include "components/sync/driver/sync_client.h"
@@ -46,7 +47,11 @@ void AsyncDirectoryTypeController::LoadModels(
     const ConfigureContext& configure_context,
     const ModelLoadCallback& model_load_callback) {
   DCHECK(CalledOnValidThread());
+  DCHECK_EQ(configure_context.storage_option,
+            ConfigureContext::STORAGE_ON_DISK);
+
   model_load_callback_ = model_load_callback;
+
   if (state() != NOT_RUNNING) {
     model_load_callback.Run(type(),
                             SyncError(FROM_HERE, SyncError::DATATYPE_ERROR,

components/sync/driver/configure_context.h

@@ -18,8 +18,14 @@ namespace syncer {
 // controllers, which for USS datatypes propagate analogous information to the
 // processor/bridge via DataTypeActivationRequest.
 struct ConfigureContext {
+  enum StorageOption {
+    STORAGE_ON_DISK,
+    STORAGE_IN_MEMORY,
+  };
+
   std::string authenticated_account_id;
   std::string cache_guid;
+  StorageOption storage_option = STORAGE_ON_DISK;
   ConfigureReason reason = CONFIGURE_REASON_UNKNOWN;
   // TODO(mastiz): Consider adding |requested_types| here, but currently there
   // are subtle differences across layers (e.g. where control types are

components/sync/driver/frontend_data_type_controller.cc

@@ -10,6 +10,7 @@
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "components/sync/base/data_type_histogram.h"
 #include "components/sync/base/model_type.h"
+#include "components/sync/driver/configure_context.h"
 #include "components/sync/driver/model_associator.h"
 #include "components/sync/driver/sync_client.h"
 #include "components/sync/driver/sync_service.h"
@@ -34,6 +35,9 @@ void FrontendDataTypeController::LoadModels(
     const ConfigureContext& configure_context,
     const ModelLoadCallback& model_load_callback) {
   DCHECK(CalledOnValidThread());
+  DCHECK_EQ(configure_context.storage_option,
+            ConfigureContext::STORAGE_ON_DISK);
+
   model_load_callback_ = model_load_callback;
 
   if (state_ != NOT_RUNNING) {

components/sync/driver/model_type_controller.cc

@@ -49,10 +49,20 @@ SyncStopMetadataFate TakeStrictestMetadataFate(SyncStopMetadataFate fate1,
 
 ModelTypeController::ModelTypeController(
     ModelType type,
-    std::unique_ptr<ModelTypeControllerDelegate> delegate)
-    : DataTypeController(type),
-      delegate_(std::move(delegate)),
-      state_(NOT_RUNNING) {}
+    std::unique_ptr<ModelTypeControllerDelegate> delegate_on_disk)
+    : DataTypeController(type), state_(NOT_RUNNING) {
+  delegate_map_.emplace(ConfigureContext::STORAGE_ON_DISK,
+                        std::move(delegate_on_disk));
+}
+
+ModelTypeController::ModelTypeController(
+    ModelType type,
+    std::unique_ptr<ModelTypeControllerDelegate> delegate_on_disk,
+    std::unique_ptr<ModelTypeControllerDelegate> delegate_in_memory)
+    : ModelTypeController(type, std::move(delegate_on_disk)) {
+  delegate_map_.emplace(ConfigureContext::STORAGE_IN_MEMORY,
+                        std::move(delegate_in_memory));
+}
 
 ModelTypeController::~ModelTypeController() {}
 
@@ -69,10 +79,13 @@ void ModelTypeController::LoadModels(
   DCHECK(!model_load_callback.is_null());
   DCHECK_EQ(NOT_RUNNING, state_);
 
-  model_load_callback_ = model_load_callback;
+  auto it = delegate_map_.find(configure_context.storage_option);
+  DCHECK(it != delegate_map_.end());
+  delegate_ = it->second.get();
 
   DVLOG(1) << "Sync starting for " << ModelTypeToString(type());
   state_ = MODEL_STARTING;
+  model_load_callback_ = model_load_callback;
 
   DataTypeActivationRequest request;
   request.error_handler = base::BindRepeating(
@@ -114,6 +127,7 @@ void ModelTypeController::LoadModelsDone(ConfigureResult result,
     DCHECK(model_stop_callbacks_.empty());
 
     delegate_->OnSyncStopping(model_stop_metadata_fate_);
+    delegate_ = nullptr;
 
     for (StopCallback& stop_callback : model_stop_callbacks) {
       std::move(stop_callback).Run();
@@ -238,6 +252,7 @@ void ModelTypeController::Stop(SyncStopMetadataFate metadata_fate,
       DVLOG(1) << "Stopping sync for " << ModelTypeToString(type());
       state_ = NOT_RUNNING;
       delegate_->OnSyncStopping(metadata_fate);
+      delegate_ = nullptr;
       std::move(callback).Run();
       break;
   }
@@ -248,15 +263,18 @@ DataTypeController::State ModelTypeController::state() const {
 }
 
 void ModelTypeController::GetAllNodes(const AllNodesCallback& callback) {
+  DCHECK(delegate_);
   delegate_->GetAllNodesForDebugging(callback);
 }
 
 void ModelTypeController::GetStatusCounters(
     const StatusCountersCallback& callback) {
+  DCHECK(delegate_);
   delegate_->GetStatusCountersForDebugging(callback);
 }
 
 void ModelTypeController::RecordMemoryUsageAndCountsHistograms() {
+  DCHECK(delegate_);
   delegate_->RecordMemoryUsageAndCountsHistograms();
 }
 

components/sync/driver/model_type_controller.h

@@ -10,9 +10,11 @@
 #include <vector>
 
 #include "base/callback.h"
+#include "base/containers/flat_map.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "components/sync/base/model_type.h"
+#include "components/sync/driver/configure_context.h"
 #include "components/sync/driver/data_type_controller.h"
 #include "components/sync/model/model_error.h"
 #include "components/sync/model/model_type_controller_delegate.h"
@@ -25,8 +27,14 @@ struct DataTypeActivationResponse;
 // DataTypeController implementation for Unified Sync and Storage model types.
 class ModelTypeController : public DataTypeController {
  public:
-  ModelTypeController(ModelType type,
-                      std::unique_ptr<ModelTypeControllerDelegate> delegate);
+  ModelTypeController(
+      ModelType type,
+      std::unique_ptr<ModelTypeControllerDelegate> delegate_on_disk);
+  // For datatypes that have support for STORAGE_IN_MEMORY.
+  ModelTypeController(
+      ModelType type,
+      std::unique_ptr<ModelTypeControllerDelegate> delegate_on_disk,
+      std::unique_ptr<ModelTypeControllerDelegate> delegate_in_memory);
   ~ModelTypeController() override;
 
   // DataTypeController implementation.
@@ -63,11 +71,16 @@ class ModelTypeController : public DataTypeController {
   void OnProcessorStarted(
       std::unique_ptr<DataTypeActivationResponse> activation_response);
 
-  const std::unique_ptr<ModelTypeControllerDelegate> delegate_;
+  base::flat_map<ConfigureContext::StorageOption,
+                 std::unique_ptr<ModelTypeControllerDelegate>>
+      delegate_map_;
 
   // State of this datatype controller.
   State state_;
 
+  // Owned by |delegate_map_|. Null while NOT_RUNNING.
+  ModelTypeControllerDelegate* delegate_;
+
   // Callback for use when starting the datatype (usually MODEL_STARTING, but
   // STOPPING if abort requested while starting).
   ModelLoadCallback model_load_callback_;

components/sync/driver/model_type_controller_unittest.cc

@@ -295,4 +295,71 @@ TEST_F(ModelTypeControllerTest, StopBeforeLoadModels) {
   EXPECT_EQ(DataTypeController::NOT_RUNNING, controller()->state());
 }
 
+// Tests that StorageOption is honored when the controller has been constructed
+// with two delegates.
+TEST(ModelTypeControllerWithMultiDelegateTest, ToggleStorageOption) {
+  base::MessageLoop message_loop;
+  NiceMock<MockDelegate> delegate_on_disk;
+  NiceMock<MockDelegate> delegate_in_memory;
+
+  ModelTypeController controller(
+      kTestModelType,
+      std::make_unique<ForwardingModelTypeControllerDelegate>(
+          &delegate_on_disk),
+      std::make_unique<ForwardingModelTypeControllerDelegate>(
+          &delegate_in_memory));
+
+  ConfigureContext context;
+  context.authenticated_account_id = kAccountId;
+  context.cache_guid = kCacheGuid;
+
+  ModelTypeControllerDelegate::StartCallback start_callback;
+
+  // Start sync with STORAGE_IN_MEMORY.
+  EXPECT_CALL(delegate_on_disk, OnSyncStarting(_, _)).Times(0);
+  EXPECT_CALL(delegate_in_memory, OnSyncStarting(_, _))
+      .WillOnce([&](const DataTypeActivationRequest& request,
+                    ModelTypeControllerDelegate::StartCallback callback) {
+        start_callback = std::move(callback);
+      });
+  context.storage_option = ConfigureContext::STORAGE_IN_MEMORY;
+  controller.LoadModels(context, base::DoNothing());
+
+  ASSERT_EQ(DataTypeController::MODEL_STARTING, controller.state());
+  ASSERT_TRUE(start_callback);
+
+  // Mimic completion for OnSyncStarting().
+  std::move(start_callback).Run(std::make_unique<DataTypeActivationResponse>());
+  ASSERT_EQ(DataTypeController::MODEL_LOADED, controller.state());
+
+  // Stop sync.
+  EXPECT_CALL(delegate_on_disk, OnSyncStopping(_)).Times(0);
+  EXPECT_CALL(delegate_in_memory, OnSyncStopping(_));
+  controller.Stop(CLEAR_METADATA, base::DoNothing());
+  ASSERT_EQ(DataTypeController::NOT_RUNNING, controller.state());
+
+  // Start sync with STORAGE_ON_DISK.
+  EXPECT_CALL(delegate_in_memory, OnSyncStarting(_, _)).Times(0);
+  EXPECT_CALL(delegate_on_disk, OnSyncStarting(_, _))
+      .WillOnce([&](const DataTypeActivationRequest& request,
+                    ModelTypeControllerDelegate::StartCallback callback) {
+        start_callback = std::move(callback);
+      });
+  context.storage_option = ConfigureContext::STORAGE_ON_DISK;
+  controller.LoadModels(context, base::DoNothing());
+
+  ASSERT_EQ(DataTypeController::MODEL_STARTING, controller.state());
+  ASSERT_TRUE(start_callback);
+
+  // Mimic completion for OnSyncStarting().
+  std::move(start_callback).Run(std::make_unique<DataTypeActivationResponse>());
+  ASSERT_EQ(DataTypeController::MODEL_LOADED, controller.state());
+
+  // Stop sync.
+  EXPECT_CALL(delegate_in_memory, OnSyncStopping(_)).Times(0);
+  EXPECT_CALL(delegate_on_disk, OnSyncStopping(_));
+  controller.Stop(CLEAR_METADATA, base::DoNothing());
+  ASSERT_EQ(DataTypeController::NOT_RUNNING, controller.state());
+}
+
 }  // namespace syncer

components/sync/driver/proxy_data_type_controller.cc

@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "base/values.h"
+#include "components/sync/driver/configure_context.h"
 #include "components/sync/engine/model_safe_worker.h"
 #include "components/sync/engine/model_type_configurer.h"
 #include "components/sync/model/sync_merge_result.h"
@@ -38,6 +39,8 @@ void ProxyDataTypeController::LoadModels(
     const ConfigureContext& configure_context,
     const ModelLoadCallback& model_load_callback) {
   DCHECK(CalledOnValidThread());
+  DCHECK_EQ(configure_context.storage_option,
+            ConfigureContext::STORAGE_ON_DISK);
   state_ = MODEL_LOADED;
   model_load_callback.Run(type(), SyncError());
 }