DataTypeManagerImpl: Handle needs_reconfigure_ earlier

Reconfiguration can't happen while certain operations (e.g. loading of
data types) is ongoing. The member |needs_reconfigure_| indicates that
a reconfiguration should be performed as soon as it becomes possible.

This CL adds one additional point where a pending reconfiguration may
be started, in OnAllDataTypesReadyForConfigure(). Previously, we'd
still start the old configuration, and process the reconfigure only
after all the previous data types had been downloaded.

Bug: 1102837
Change-Id: Icc971f29229665832255baebfc545bfd34d83feb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2580882
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#835613}

components/sync/driver/data_type_manager_impl.cc

@@ -329,8 +329,13 @@ void DataTypeManagerImpl::Restart() {
 }
 
 void DataTypeManagerImpl::OnAllDataTypesReadyForConfigure() {
-  // TODO(crbug.com/1102837): Should we handle |needs_reconfigure_| here,
-  // before even starting the configuration?
+  // If a reconfigure was requested while the data types were loading, process
+  // it now.
+  if (needs_reconfigure_) {
+    configuration_types_queue_ = base::queue<ModelTypeSet>();
+    ProcessReconfigure();
+    return;
+  }
   // TODO(pavely): By now some of datatypes in |configuration_types_queue_|
   // could have failed loading and should be excluded from configuration. I need
   // to adjust |configuration_types_queue_| for such types.

components/sync/driver/data_type_manager_impl.h

@@ -58,6 +58,8 @@ class DataTypeManagerImpl : public DataTypeManager,
   void OnSingleDataTypeWillStop(ModelType type,
                                 const SyncError& error) override;
 
+  bool needs_reconfigure_for_test() const { return needs_reconfigure_; }
+
  protected:
   // Returns the priority types (control + priority user types).
   // Virtual for overriding during tests.
@@ -157,8 +159,6 @@ class DataTypeManagerImpl : public DataTypeManager,
   ModelTypeSet last_requested_types_;
 
   // Context information (e.g. the reason) for the last reconfigure attempt.
-  // Note: this will be set to a valid value only when |needs_reconfigure_| is
-  // set.
   ConfigureContext last_requested_context_;
 
   // A set of types that were enabled at the time of Restart().

components/sync/driver/data_type_manager_impl_unittest.cc

@@ -7,7 +7,6 @@
 #include <memory>
 #include <utility>
 
-#include "base/run_loop.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/task_environment.h"
 #include "components/sync/base/model_type.h"
@@ -520,15 +519,8 @@ TEST_F(SyncDataTypeManagerImplTest, ConfigureOneThenSwitch) {
   EXPECT_TRUE(configurer_.activated_types().Empty());
 }
 
-// Set up a DTM with two controllers.  Then:
-//   1) Configure with first controller (model stays loading).
-//   2) Configure with both controllers, which gets postponed because there's an
-//      ongoing configuration that cannot complete before the model loads.
-//   3) Finish starting the first controller.
-//   4) Finish the download of the first controller.
-//   5) Finish the download of the second controller.
-//   6) Stop the DTM.
 TEST_F(SyncDataTypeManagerImplTest, ConfigureModelLoading) {
+  // Set up a DTM with two controllers.
   AddController(BOOKMARKS);
   AddController(PREFERENCES);
 
@@ -537,38 +529,40 @@ TEST_F(SyncDataTypeManagerImplTest, ConfigureModelLoading) {
   SetConfigureStartExpectation();
   SetConfigureDoneExpectation(DataTypeManager::OK, DataTypeStatusTable());
 
-  // Step 1.
+  // Step 1: Configure with first controller (model stays loading).
   Configure(ModelTypeSet(BOOKMARKS));
   EXPECT_EQ(DataTypeManager::CONFIGURING, dtm_->state());
   EXPECT_TRUE(configurer_.activated_types().Empty());
 
-  // Step 2: this one gets postponed because the first configuration is ongoing.
+  // Step 2: Configure with both controllers, which gets postponed because
+  //         there's an ongoing configuration that cannot complete before the
+  //         model loads.
   ASSERT_EQ(DataTypeController::MODEL_STARTING,
             GetController(BOOKMARKS)->state());
+  ASSERT_FALSE(dtm_->needs_reconfigure_for_test());
   Configure(ModelTypeSet(BOOKMARKS, PREFERENCES));
   EXPECT_EQ(DataTypeManager::CONFIGURING, dtm_->state());
+  EXPECT_TRUE(dtm_->needs_reconfigure_for_test());
 
-  // Step 3.
+  // Step 3: Finish starting the first controller. This triggers a
+  //         reconfiguration with both data types.
   ASSERT_EQ(DataTypeController::MODEL_STARTING,
             GetController(BOOKMARKS)->state());
   GetController(BOOKMARKS)->model()->SimulateModelStartFinished();
+  EXPECT_FALSE(dtm_->needs_reconfigure_for_test());
 
-  // Step 4.
+  // Step 4: Finish the download of both data types. This completes the
+  //         configuration.
   ASSERT_EQ(DataTypeController::RUNNING, GetController(BOOKMARKS)->state());
+  ASSERT_EQ(DataTypeController::RUNNING, GetController(PREFERENCES)->state());
   EXPECT_EQ(DataTypeManager::CONFIGURING, dtm_->state());
   FinishDownload(ModelTypeSet(), ModelTypeSet());  // control types
-  FinishDownload(ModelTypeSet(BOOKMARKS), ModelTypeSet());
-  EXPECT_EQ(DataTypeManager::CONFIGURING, dtm_->state());
-
-  // Step 5.
-  ASSERT_EQ(DataTypeController::RUNNING, GetController(PREFERENCES)->state());
-  FinishDownload(ModelTypeSet(PREFERENCES), ModelTypeSet());
-
+  FinishDownload(ModelTypeSet(BOOKMARKS, PREFERENCES), ModelTypeSet());
   EXPECT_EQ(DataTypeManager::CONFIGURED, dtm_->state());
   EXPECT_EQ(ModelTypeSet(BOOKMARKS, PREFERENCES),
             configurer_.activated_types());
 
-  // Step 6.
+  // Step 5: Stop the DTM.
   dtm_->Stop(STOP_SYNC);
   EXPECT_EQ(DataTypeManager::STOPPED, dtm_->state());
   EXPECT_TRUE(configurer_.activated_types().Empty());
@@ -1201,7 +1195,7 @@ TEST_F(SyncDataTypeManagerImplTest, ModelLoadError) {
                        ModelTypeSet()));
   Configure(ModelTypeSet(BOOKMARKS));
   FinishDownload(ModelTypeSet(), ModelTypeSet());  // control types
-  FinishDownload(ModelTypeSet(BOOKMARKS), ModelTypeSet());
+  // No need to finish the download of BOOKMARKS since it was never started.
   EXPECT_EQ(DataTypeManager::CONFIGURED, dtm_->state());
   EXPECT_EQ(DataTypeController::FAILED, GetController(BOOKMARKS)->state());