[Sync] Propagate failed configuration types to DataTypeManagerImpl

This allows us to correctly identify the types that failed configuration.

Added ModelTypeBitSetToSet function.

BUG=105589
TEST=


Review URL: http://codereview.chromium.org/8727009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@112151 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 91 insertions, 39 deletions

diff --git a/chrome/browser/sync/glue/data_type_manager_impl.cc b/chrome/browser/sync/glue/data_type_manager_impl.cc
index 70df5df..63544c7 100644
--- a/chrome/browser/sync/glue/data_type_manager_impl.cc
+++ b/chrome/browser/sync/glue/data_type_manager_impl.cc
@@ -271,16 +271,22 @@ bool DataTypeManagerImpl::ProcessReconfigure() {
   return true;
 }
 
-void DataTypeManagerImpl::DownloadReady(bool success) {
+void DataTypeManagerImpl::DownloadReady(
+    const syncable::ModelTypeSet& failed_configuration_types) {
   DCHECK_EQ(state_, DOWNLOAD_PENDING);
 
-  // Ignore |success| if we need to reconfigure anyway.
+  // Ignore |failed_configuration_types| if we need to reconfigure
+  // anyway.
   if (ProcessReconfigure()) {
     return;
   }
 
-  if (!success) {
-    SyncError error(FROM_HERE, "Download failed", needs_start_[0]->type());
+  if (!failed_configuration_types.empty()) {
+    std::string error_msg =
+        "Configuration failed for types " +
+        syncable::ModelTypeSetToString(failed_configuration_types);
+    SyncError error(FROM_HERE, error_msg,
+                    *failed_configuration_types.begin());
     Abort(UNRECOVERABLE_ERROR, error);
     return;
   }
diff --git a/chrome/browser/sync/glue/data_type_manager_impl.h b/chrome/browser/sync/glue/data_type_manager_impl.h
index 8c79654..64afb34 100644
--- a/chrome/browser/sync/glue/data_type_manager_impl.h
+++ b/chrome/browser/sync/glue/data_type_manager_impl.h
@@ -66,7 +66,8 @@ class DataTypeManagerImpl : public DataTypeManager {
   bool ProcessReconfigure();
 
   void Restart(sync_api::ConfigureReason reason, bool enable_nigori);
-  void DownloadReady(bool success);
+  void DownloadReady(
+      const syncable::ModelTypeSet& failed_configuration_types);
   void NotifyStart();
   void NotifyDone(const ConfigureResult& result);
   void SetBlockedAndNotify();
diff --git a/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc b/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc
index 5367ea6..3994d21 100644
--- a/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc
+++ b/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc
@@ -58,9 +58,9 @@ void DoConfigureDataTypes(
     const syncable::ModelTypeSet& types_to_add,
     const syncable::ModelTypeSet& types_to_remove,
     sync_api::ConfigureReason reason,
-    base::Callback<void(bool)> ready_task,
+    base::Callback<void(const syncable::ModelTypeSet&)> ready_task,
     bool enable_nigori) {
-  ready_task.Run(true);
+  ready_task.Run(syncable::ModelTypeSet());
 }
 
 void QuitMessageLoop() {
@@ -241,8 +241,9 @@ class DataTypeManagerImplTest : public testing::Test {
     EXPECT_EQ(DataTypeManager::STOPPED, dtm.state());
   }
 
-  void RunConfigureWhileDownloadPendingTest(bool enable_nigori,
-                                            bool first_configure_result) {
+  void RunConfigureWhileDownloadPendingTest(
+      bool enable_nigori,
+      const syncable::ModelTypeSet& first_configure_result) {
     DataTypeControllerMock* bookmark_dtc = MakeBookmarkDTC();
     SetStartStopExpectations(bookmark_dtc);
     controllers_[syncable::BOOKMARKS] = bookmark_dtc;
@@ -254,7 +255,7 @@ class DataTypeManagerImplTest : public testing::Test {
     DataTypeManagerImpl dtm(&backend_, &controllers_);
     SetConfigureStartExpectation();
     SetConfigureDoneExpectation(DataTypeManager::OK);
-    base::Callback<void(bool)> task;
+    base::Callback<void(const syncable::ModelTypeSet&)> task;
     // Grab the task the first time this is called so we can configure
     // before it is finished.
     EXPECT_CALL(backend_, ConfigureDataTypes(_, _, _, _, enable_nigori)).
@@ -562,24 +563,30 @@ TEST_F(DataTypeManagerImplTest, OneControllerFailsAssociation) {
 
 
 TEST_F(DataTypeManagerImplTest, ConfigureWhileDownloadPending) {
-  RunConfigureWhileDownloadPendingTest(true /* enable_nigori */,
-                                       true /* first_configure_result */);
+  RunConfigureWhileDownloadPendingTest(
+      true /* enable_nigori */,
+      syncable::ModelTypeSet() /* first_configure_result */);
 }
 
 TEST_F(DataTypeManagerImplTest, ConfigureWhileDownloadPendingWithoutNigori) {
-  RunConfigureWhileDownloadPendingTest(false /* enable_nigori */,
-                                       true /* first_configure_result */);
+  RunConfigureWhileDownloadPendingTest(
+      false /* enable_nigori */,
+      syncable::ModelTypeSet() /* first_configure_result */);
 }
 
 TEST_F(DataTypeManagerImplTest, ConfigureWhileDownloadPendingFail) {
-  RunConfigureWhileDownloadPendingTest(true /* enable_nigori */,
-                                       false /* first_configure_result */);
+  syncable::ModelTypeSet first_configure_result;
+  first_configure_result.insert(syncable::BOOKMARKS);
+  RunConfigureWhileDownloadPendingTest(
+      true /* enable_nigori */, first_configure_result);
 }
 
 TEST_F(DataTypeManagerImplTest,
        ConfigureWhileDownloadPendingFailWithoutNigori) {
-  RunConfigureWhileDownloadPendingTest(false /* enable_nigori */,
-                                       false /* first_configure_result */);
+  syncable::ModelTypeSet first_configure_result;
+  first_configure_result.insert(syncable::BOOKMARKS);
+  RunConfigureWhileDownloadPendingTest(
+      false /* enable_nigori */, first_configure_result);
 }
 
 TEST_F(DataTypeManagerImplTest, StopWhileDownloadPending) {
@@ -590,7 +597,7 @@ TEST_F(DataTypeManagerImplTest, StopWhileDownloadPending) {
   DataTypeManagerImpl dtm(&backend_, &controllers_);
   SetConfigureStartExpectation();
   SetConfigureDoneExpectation(DataTypeManager::ABORTED);
-  base::Callback<void(bool)> task;
+  base::Callback<void(const syncable::ModelTypeSet&)> task;
   // Grab the task the first time this is called so we can stop
   // before it is finished.
   EXPECT_CALL(backend_, ConfigureDataTypes(_, _, _, _, true)).
@@ -607,5 +614,5 @@ TEST_F(DataTypeManagerImplTest, StopWhileDownloadPending) {
 
   // It should be perfectly safe to run this task even though the DTM
   // has been stopped.
-  task.Run(true);
+  task.Run(syncable::ModelTypeSet());
 }
diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc
index 0672999..38c7f02 100644
--- a/chrome/browser/sync/glue/sync_backend_host.cc
+++ b/chrome/browser/sync/glue/sync_backend_host.cc
@@ -250,7 +250,7 @@ void SyncBackendHost::ConfigureDataTypes(
     const syncable::ModelTypeSet& types_to_add,
     const syncable::ModelTypeSet& types_to_remove,
     sync_api::ConfigureReason reason,
-    base::Callback<void(bool)> ready_task,
+    base::Callback<void(const syncable::ModelTypeSet&)> ready_task,
     bool enable_nigori) {
   syncable::ModelTypeSet types_to_add_with_nigori = types_to_add;
   syncable::ModelTypeSet types_to_remove_with_nigori = types_to_remove;
@@ -723,6 +723,15 @@ void SyncBackendHost::Core::HandleInitializationCompletedOnFrontendLoop(
   host_->HandleInitializationCompletedOnFrontendLoop(js_backend, success);
 }
 
+void SyncBackendHost::Core::HandleNigoriConfigurationCompletedOnFrontendLoop(
+    const WeakHandle<JsBackend>& js_backend,
+    const syncable::ModelTypeSet& failed_configuration_types) {
+  if (!host_)
+    return;
+  host_->HandleInitializationCompletedOnFrontendLoop(
+      js_backend, failed_configuration_types.empty());
+}
+
 void SyncBackendHost::Core::StartSavingChanges() {
   // We may already be shut down.
   if (!sync_loop_)
@@ -835,17 +844,23 @@ void SyncBackendHost::Core::HandleSyncCycleCompletedOnFrontendLoop(
     DCHECK(
         std::includes(state->types_to_add.begin(), state->types_to_add.end(),
                       state->added_types.begin(), state->added_types.end()));
+    syncable::ModelTypeSet initial_sync_ended =
+        syncable::ModelTypeBitSetToSet(snapshot->initial_sync_ended);
+    syncable::ModelTypeSet failed_configuration_types;
+    std::set_difference(
+        state->added_types.begin(), state->added_types.end(),
+        initial_sync_ended.begin(), initial_sync_ended.end(),
+        std::inserter(failed_configuration_types,
+                      failed_configuration_types.end()));
     SVLOG(1)
         << "Added types: "
         << syncable::ModelTypeSetToString(state->added_types)
         << ", configured types: "
-        << syncable::ModelTypeBitSetToString(snapshot->initial_sync_ended);
-    syncable::ModelTypeBitSet added_types =
-        syncable::ModelTypeBitSetFromSet(state->added_types);
-    bool found_all_added =
-        (added_types & snapshot->initial_sync_ended) == added_types;
-    state->ready_task.Run(found_all_added);
-    if (!found_all_added)
+        << syncable::ModelTypeSetToString(initial_sync_ended)
+        << ", failed configuration types: "
+        << syncable::ModelTypeSetToString(failed_configuration_types);
+    state->ready_task.Run(failed_configuration_types);
+    if (!failed_configuration_types.empty())
       return;
   }
 
@@ -916,9 +931,10 @@ void SyncBackendHost::HandleInitializationCompletedOnFrontendLoop(
           syncable::ModelTypeSet(),
           syncable::ModelTypeSet(),
           sync_api::CONFIGURE_REASON_NEW_CLIENT,
+          // Calls back into this function.
           base::Bind(
               &SyncBackendHost::Core::
-                  HandleInitializationCompletedOnFrontendLoop,
+                  HandleNigoriConfigurationCompletedOnFrontendLoop,
               core_.get(), js_backend),
           true);
       break;
@@ -995,7 +1011,8 @@ void SyncBackendHost::FinishConfigureDataTypesOnFrontendLoop() {
   if (pending_config_mode_state_->added_types.empty()) {
     SVLOG(1) << "No new types added; calling ready_task directly";
     // No new types - just notify the caller that the types are available.
-    pending_config_mode_state_->ready_task.Run(true);
+    const syncable::ModelTypeSet failed_configuration_types;
+    pending_config_mode_state_->ready_task.Run(failed_configuration_types);
   } else {
     pending_download_state_.reset(pending_config_mode_state_.release());
 
diff --git a/chrome/browser/sync/glue/sync_backend_host.h b/chrome/browser/sync/glue/sync_backend_host.h
index 81af6a9..813e0f0 100644
--- a/chrome/browser/sync/glue/sync_backend_host.h
+++ b/chrome/browser/sync/glue/sync_backend_host.h
@@ -189,14 +189,14 @@ class SyncBackendHost {
   void Shutdown(bool sync_disabled);
 
   // Changes the set of data types that are currently being synced.
-  // The ready_task will be run when all of the requested data types
-  // are up-to-date and ready for activation.  The task will be
-  // cancelled upon shutdown.
+  // The ready_task will be run when configuration is done with the
+  // set of all types that failed configuration (i.e., if its argument
+  // is non-empty, then an error was encountered).
   virtual void ConfigureDataTypes(
       const syncable::ModelTypeSet& types_to_add,
       const syncable::ModelTypeSet& types_to_remove,
       sync_api::ConfigureReason reason,
-      base::Callback<void(bool)> ready_task,
+      base::Callback<void(const syncable::ModelTypeSet&)> ready_task,
       bool enable_nigori);
 
   // Makes an asynchronous call to syncer to switch to config mode. When done
@@ -418,6 +418,12 @@ class SyncBackendHost {
         const WeakHandle<JsBackend>& js_backend,
         bool success);
 
+    // Called when configuration of the Nigori node has completed as
+    // part of the initialization process.
+    void HandleNigoriConfigurationCompletedOnFrontendLoop(
+        const WeakHandle<JsBackend>& js_backend,
+        const syncable::ModelTypeSet& failed_configuration_types);
+
    private:
     friend class base::RefCountedThreadSafe<SyncBackendHost::Core>;
     friend class SyncBackendHostForProfileSyncTest;
@@ -542,9 +548,10 @@ class SyncBackendHost {
     PendingConfigureDataTypesState();
     ~PendingConfigureDataTypesState();
 
-    // A task that should be called once data type configuration is
-    // complete.
-    base::Callback<void(bool)> ready_task;
+    // The ready_task will be run when configuration is done with the
+    // set of all types that failed configuration (i.e., if its
+    // argument is non-empty, then an error was encountered).
+    base::Callback<void(const syncable::ModelTypeSet&)> ready_task;
 
     // The set of types that we are waiting to be initially synced in a
     // configuration cycle.
diff --git a/chrome/browser/sync/glue/sync_backend_host_mock.cc b/chrome/browser/sync/glue/sync_backend_host_mock.cc
index 1e5705a..4aa8d04 100644
--- a/chrome/browser/sync/glue/sync_backend_host_mock.cc
+++ b/chrome/browser/sync/glue/sync_backend_host_mock.cc
@@ -9,7 +9,7 @@ namespace browser_sync {
 using ::testing::_;
 
 ACTION(InvokeTask) {
-  arg3.Run(true);
+  arg3.Run(syncable::ModelTypeSet());
 }
 
 SyncBackendHostMock::SyncBackendHostMock() {
diff --git a/chrome/browser/sync/glue/sync_backend_host_mock.h b/chrome/browser/sync/glue/sync_backend_host_mock.h
index 1c46892..73cea91 100644
--- a/chrome/browser/sync/glue/sync_backend_host_mock.h
+++ b/chrome/browser/sync/glue/sync_backend_host_mock.h
@@ -25,7 +25,7 @@ class SyncBackendHostMock : public SyncBackendHost {
                void(const std::set<syncable::ModelType>&,
                     const std::set<syncable::ModelType>&,
                     sync_api::ConfigureReason,
-                    base::Callback<void(bool)>,
+                    base::Callback<void(const syncable::ModelTypeSet&)>,
                     bool));
   MOCK_METHOD0(StartSyncingWithServer, void());
 };
diff --git a/chrome/browser/sync/syncable/model_type.cc b/chrome/browser/sync/syncable/model_type.cc
index ccce169..42cfc39 100644
--- a/chrome/browser/sync/syncable/model_type.cc
+++ b/chrome/browser/sync/syncable/model_type.cc
@@ -372,6 +372,17 @@ ModelTypeBitSet ModelTypeBitSetFromSet(const ModelTypeSet& set) {
   return bitset;
 }
 
+ModelTypeSet ModelTypeBitSetToSet(const ModelTypeBitSet& bit_set) {
+  ModelTypeSet set;
+  for (int i = FIRST_REAL_MODEL_TYPE; i < MODEL_TYPE_COUNT; ++i) {
+    syncable::ModelType type = syncable::ModelTypeFromInt(i);
+    if (bit_set[type]) {
+      set.insert(type);
+    }
+  }
+  return set;
+}
+
 ListValue* ModelTypeBitSetToValue(const ModelTypeBitSet& model_types) {
   ListValue* value = new ListValue();
   for (int i = FIRST_REAL_MODEL_TYPE; i < MODEL_TYPE_COUNT; ++i) {
diff --git a/chrome/browser/sync/syncable/model_type.h b/chrome/browser/sync/syncable/model_type.h
index 1010299..f2dc74f 100644
--- a/chrome/browser/sync/syncable/model_type.h
+++ b/chrome/browser/sync/syncable/model_type.h
@@ -143,6 +143,9 @@ std::string ModelTypeBitSetToString(const ModelTypeBitSet& model_types);
 // Convert a ModelTypeSet to a ModelTypeBitSet.
 ModelTypeBitSet ModelTypeBitSetFromSet(const ModelTypeSet& set);
 
+// Convert a ModelTypeBitSet to a ModelTypeSet.
+ModelTypeSet ModelTypeBitSetToSet(const ModelTypeBitSet& bit_set);
+
 // Caller takes ownership of returned list.
 base::ListValue* ModelTypeBitSetToValue(const ModelTypeBitSet& model_types);