Revert 289115 "Revert 288464 "[Sync] Cleanup datatype configurat..."

> Revert 288464 "[Sync] Cleanup datatype configuration error handl..."
> 
> Reason for revert: see crbug.com/403098
> 
> > [Sync] Cleanup datatype configuration error handling.
> > 
> > The FailedDataTypeHandler is now informed immediately of failures, including
> > datatype errors, and is therefore authoritative source for all errors. As such,
> > partial success is no longer tracked and the various ModelTypeSets for error
> > types in configure results are removed.
> > 
> > BUG=368834,403098
> > 
> > Review URL: https://codereview.chromium.org/420633002
> 
> TBR=zea@chromium.org
> 
> Review URL: https://codereview.chromium.org/468643002

TBR=oshima@chromium.org

Review URL: https://codereview.chromium.org/469803004

Cr-Commit-Position: refs/heads/master@{#289354}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@289354 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/sync/backend_migrator.cc

@@ -164,8 +164,7 @@ void BackendMigrator::OnConfigureDoneImpl(
     return;
   }
 
-  if (result.status != sync_driver::DataTypeManager::OK &&
-      result.status != sync_driver::DataTypeManager::PARTIAL_SUCCESS) {
+  if (result.status != sync_driver::DataTypeManager::OK) {
     // If this fails, and we're disabling types, a type may or may not be
     // disabled until the user restarts the browser.  If this wasn't an abort,
     // any failure will be reported as an unrecoverable error to the UI. If it

chrome/browser/sync/backend_migrator_unittest.cc

@@ -79,13 +79,9 @@ class SyncBackendMigratorTest : public testing::Test {
       DataTypeManager::ConfigureResult result(status, requested_types);
       migrator_->OnConfigureDone(result);
     } else {
-      std::map<syncer::ModelType, syncer::SyncError> errors;
       DataTypeManager::ConfigureResult result(
           status,
-          requested_types,
-          errors,
-          syncer::ModelTypeSet(),
-          syncer::ModelTypeSet());
+          requested_types);
       migrator_->OnConfigureDone(result);
     }
     message_loop_.RunUntilIdle();

chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc

@@ -158,7 +158,7 @@ class SyncAutofillDataTypeControllerTest : public testing::Test {
   }
 
   // Passed to AutofillDTC::Start().
-  void OnStartFinished(sync_driver::DataTypeController::StartResult result,
+  void OnStartFinished(sync_driver::DataTypeController::ConfigureResult result,
                        const syncer::SyncMergeResult& local_merge_result,
                        const syncer::SyncMergeResult& syncer_merge_result) {
     last_start_result_ = result;
@@ -189,7 +189,7 @@ class SyncAutofillDataTypeControllerTest : public testing::Test {
   scoped_refptr<AutofillDataTypeController> autofill_dtc_;
 
   // Stores arguments of most recent call of OnStartFinished().
-  sync_driver::DataTypeController::StartResult last_start_result_;
+  sync_driver::DataTypeController::ConfigureResult last_start_result_;
   syncer::SyncError last_start_error_;
   base::WeakPtrFactory<SyncAutofillDataTypeControllerTest> weak_ptr_factory_;
 };

chrome/browser/sync/glue/frontend_data_type_controller.cc

@@ -262,7 +262,7 @@ void FrontendDataTypeController::AbortModelLoad() {
 }
 
 void FrontendDataTypeController::StartDone(
-    StartResult start_result,
+    ConfigureResult start_result,
     const syncer::SyncMergeResult& local_merge_result,
     const syncer::SyncMergeResult& syncer_merge_result) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -295,7 +295,7 @@ void FrontendDataTypeController::RecordAssociationTime(base::TimeDelta time) {
 #undef PER_DATA_TYPE_MACRO
 }
 
-void FrontendDataTypeController::RecordStartFailure(StartResult result) {
+void FrontendDataTypeController::RecordStartFailure(ConfigureResult result) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeStartFailures",
                             ModelTypeToHistogramInt(type()),

chrome/browser/sync/glue/frontend_data_type_controller.h

@@ -93,14 +93,14 @@ class FrontendDataTypeController : public sync_driver::DataTypeController {
 
   // Helper method for cleaning up state and running the start callback.
   virtual void StartDone(
-      StartResult start_result,
+      ConfigureResult start_result,
       const syncer::SyncMergeResult& local_merge_result,
       const syncer::SyncMergeResult& syncer_merge_result);
 
   // Record association time.
   virtual void RecordAssociationTime(base::TimeDelta time);
   // Record causes of start failure.
-  virtual void RecordStartFailure(StartResult result);
+  virtual void RecordStartFailure(ConfigureResult result);
 
   virtual sync_driver::AssociatorInterface* model_associator() const;
   virtual void set_model_associator(

chrome/browser/sync/glue/frontend_data_type_controller_mock.h

@@ -34,9 +34,9 @@ class FrontendDataTypeControllerMock : public FrontendDataTypeController {
   MOCK_METHOD0(StartModels, bool());
   MOCK_METHOD0(Associate, bool());
   MOCK_METHOD0(CreateSyncComponents, void());
-  MOCK_METHOD2(StartFailed, void(StartResult result,
+  MOCK_METHOD2(StartFailed, void(ConfigureResult result,
                                  const syncer::SyncError& error));
-  MOCK_METHOD1(FinishStart, void(StartResult result));
+  MOCK_METHOD1(FinishStart, void(ConfigureResult result));
   MOCK_METHOD0(CleanUpState, void());
   MOCK_CONST_METHOD0(model_associator, sync_driver::AssociatorInterface*());
   MOCK_METHOD1(set_model_associator,
@@ -47,7 +47,7 @@ class FrontendDataTypeControllerMock : public FrontendDataTypeController {
   MOCK_METHOD2(RecordUnrecoverableError, void(const tracked_objects::Location&,
                                               const std::string&));
   MOCK_METHOD1(RecordAssociationTime, void(base::TimeDelta time));
-  MOCK_METHOD1(RecordStartFailure, void(StartResult result));
+  MOCK_METHOD1(RecordStartFailure, void(ConfigureResult result));
 
  protected:
   virtual ~FrontendDataTypeControllerMock();

chrome/browser/sync/glue/frontend_data_type_controller_unittest.cc

@@ -75,7 +75,7 @@ class FrontendDataTypeControllerFake : public FrontendDataTypeController {
     mock_->RecordAssociationTime(time);
   }
   virtual void RecordStartFailure(
-      DataTypeController::StartResult result) OVERRIDE {
+      DataTypeController::ConfigureResult result) OVERRIDE {
     mock_->RecordStartFailure(result);
   }
  private:
@@ -120,7 +120,7 @@ class SyncFrontendDataTypeControllerTest : public testing::Test {
     EXPECT_CALL(*dtc_mock_.get(), RecordAssociationTime(_));
   }
 
-  void SetActivateExpectations(DataTypeController::StartResult result) {
+  void SetActivateExpectations(DataTypeController::ConfigureResult result) {
     EXPECT_CALL(start_callback_, Run(result, _, _));
   }
 
@@ -131,7 +131,7 @@ class SyncFrontendDataTypeControllerTest : public testing::Test {
                 WillOnce(Return(syncer::SyncError()));
   }
 
-  void SetStartFailExpectations(DataTypeController::StartResult result) {
+  void SetStartFailExpectations(DataTypeController::ConfigureResult result) {
     if (DataTypeController::IsUnrecoverableResult(result))
       EXPECT_CALL(*dtc_mock_.get(), RecordUnrecoverableError(_, _));
     EXPECT_CALL(*dtc_mock_.get(), CleanUpState());

chrome/browser/sync/glue/non_frontend_data_type_controller.cc

@@ -340,7 +340,7 @@ bool NonFrontendDataTypeController::IsOnBackendThread() {
 }
 
 void NonFrontendDataTypeController::StartDone(
-    DataTypeController::StartResult start_result,
+    DataTypeController::ConfigureResult start_result,
     const syncer::SyncMergeResult& local_merge_result,
     const syncer::SyncMergeResult& syncer_merge_result) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -359,7 +359,7 @@ void NonFrontendDataTypeController::StartDone(
 }
 
 void NonFrontendDataTypeController::StartDoneImpl(
-    DataTypeController::StartResult start_result,
+    DataTypeController::ConfigureResult start_result,
     DataTypeController::State new_state,
     const syncer::SyncMergeResult& local_merge_result,
     const syncer::SyncMergeResult& syncer_merge_result) {
@@ -398,7 +398,7 @@ void NonFrontendDataTypeController::RecordAssociationTime(
 #undef PER_DATA_TYPE_MACRO
 }
 
-void NonFrontendDataTypeController::RecordStartFailure(StartResult result) {
+void NonFrontendDataTypeController::RecordStartFailure(ConfigureResult result) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeStartFailures",
                             ModelTypeToHistogramInt(type()),

chrome/browser/sync/glue/non_frontend_data_type_controller.h

@@ -134,13 +134,13 @@ class NonFrontendDataTypeController : public sync_driver::DataTypeController {
   // Start up complete, update the state and invoke the callback.
   // Note: this is performed on the datatype's thread.
   virtual void StartDone(
-      DataTypeController::StartResult start_result,
+      DataTypeController::ConfigureResult start_result,
       const syncer::SyncMergeResult& local_merge_result,
       const syncer::SyncMergeResult& syncer_merge_result);
 
   // UI thread implementation of StartDone.
   virtual void StartDoneImpl(
-      DataTypeController::StartResult start_result,
+      DataTypeController::ConfigureResult start_result,
       DataTypeController::State new_state,
       const syncer::SyncMergeResult& local_merge_result,
       const syncer::SyncMergeResult& syncer_merge_result);
@@ -153,7 +153,7 @@ class NonFrontendDataTypeController : public sync_driver::DataTypeController {
   // Record association time. Called on Datatype's thread.
   virtual void RecordAssociationTime(base::TimeDelta time);
   // Record causes of start failure. Called on UI thread.
-  virtual void RecordStartFailure(StartResult result);
+  virtual void RecordStartFailure(ConfigureResult result);
 
   // Handles the reporting of unrecoverable error. It records stuff in
   // UMA and reports to breakpad.

chrome/browser/sync/glue/non_frontend_data_type_controller_mock.h

@@ -39,11 +39,11 @@ class NonFrontendDataTypeControllerMock : public NonFrontendDataTypeController {
   MOCK_METHOD0(CreateSyncComponents,
                ProfileSyncComponentsFactory::SyncComponents());
   MOCK_METHOD3(StartDone,
-               void(DataTypeController::StartResult result,
+               void(DataTypeController::ConfigureResult result,
                     const syncer::SyncMergeResult& local_merge_result,
                     const syncer::SyncMergeResult& syncer_merge_result));
   MOCK_METHOD4(StartDoneImpl,
-               void(DataTypeController::StartResult result,
+               void(DataTypeController::ConfigureResult result,
                     DataTypeController::State new_state,
                     const syncer::SyncMergeResult& local_merge_result,
                     const syncer::SyncMergeResult& syncer_merge_result));
@@ -53,7 +53,7 @@ class NonFrontendDataTypeControllerMock : public NonFrontendDataTypeController {
   MOCK_METHOD2(RecordUnrecoverableError, void(const tracked_objects::Location&,
                                               const std::string&));
   MOCK_METHOD1(RecordAssociationTime, void(base::TimeDelta time));
-  MOCK_METHOD1(RecordStartFailure, void(StartResult result));
+  MOCK_METHOD1(RecordStartFailure, void(ConfigureResult result));
 
  protected:
   virtual ~NonFrontendDataTypeControllerMock();

chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc

@@ -97,7 +97,7 @@ class NonFrontendDataTypeControllerFake : public NonFrontendDataTypeController {
     mock_->RecordAssociationTime(time);
   }
   virtual void RecordStartFailure(
-      DataTypeController::StartResult result) OVERRIDE {
+      DataTypeController::ConfigureResult result) OVERRIDE {
     mock_->RecordStartFailure(result);
   }
   virtual void DisconnectProcessor(
@@ -160,7 +160,7 @@ class SyncNonFrontendDataTypeControllerTest : public testing::Test {
     EXPECT_CALL(*dtc_mock_.get(), RecordAssociationTime(_));
   }
 
-  void SetActivateExpectations(DataTypeController::StartResult result) {
+  void SetActivateExpectations(DataTypeController::ConfigureResult result) {
     EXPECT_CALL(start_callback_, Run(result, _, _));
   }
 
@@ -171,7 +171,7 @@ class SyncNonFrontendDataTypeControllerTest : public testing::Test {
                 WillOnce(Return(syncer::SyncError()));
   }
 
-  void SetStartFailExpectations(DataTypeController::StartResult result) {
+  void SetStartFailExpectations(DataTypeController::ConfigureResult result) {
     if (DataTypeController::IsUnrecoverableResult(result))
       EXPECT_CALL(*dtc_mock_.get(), RecordUnrecoverableError(_, _));
     if (model_associator_) {

chrome/browser/sync/profile_sync_service.cc

@@ -1470,8 +1470,7 @@ void ProfileSyncService::OnConfigureDone(
   configure_status_ = result.status;
 
   if (backend_mode_ != SYNC) {
-    if (configure_status_ == DataTypeManager::OK ||
-        configure_status_ == DataTypeManager::PARTIAL_SUCCESS) {
+    if (configure_status_ == DataTypeManager::OK) {
       StartSyncingWithServer();
     } else if (!expect_sync_configuration_aborted_) {
       DVLOG(1) << "Backup/rollback backend failed to configure.";
@@ -1482,8 +1481,7 @@ void ProfileSyncService::OnConfigureDone(
   }
 
   if (!sync_configure_start_time_.is_null()) {
-    if (result.status == DataTypeManager::OK ||
-        result.status == DataTypeManager::PARTIAL_SUCCESS) {
+    if (result.status == DataTypeManager::OK) {
       base::Time sync_configure_stop_time = base::Time::Now();
       base::TimeDelta delta = sync_configure_stop_time -
           sync_configure_start_time_;
@@ -1507,8 +1505,7 @@ void ProfileSyncService::OnConfigureDone(
   // The possible status values:
   //    ABORT - Configuration was aborted. This is not an error, if
   //            initiated by user.
-  //    OK - Everything succeeded.
-  //    PARTIAL_SUCCESS - Some datatypes failed to start.
+  //    OK - Some or all types succeeded.
   //    Everything else is an UnrecoverableError. So treat it as such.
 
   // First handle the abort case.
@@ -1520,18 +1517,18 @@ void ProfileSyncService::OnConfigureDone(
   }
 
   // Handle unrecoverable error.
-  if (configure_status_ != DataTypeManager::OK &&
-      configure_status_ != DataTypeManager::PARTIAL_SUCCESS) {
+  if (configure_status_ != DataTypeManager::OK) {
     // Something catastrophic had happened. We should only have one
     // error representing it.
-    DCHECK_EQ(result.failed_data_types.size(),
-              static_cast<unsigned int>(1));
-    syncer::SyncError error = result.failed_data_types.begin()->second;
+    syncer::SyncError error =
+        failed_data_types_handler_.GetUnrecoverableError();
     DCHECK(error.IsSet());
     std::string message =
         "Sync configuration failed with status " +
         DataTypeManager::ConfigureStatusToString(configure_status_) +
-        " during " + syncer::ModelTypeToString(error.model_type()) +
+        " caused by " +
+        syncer::ModelTypeSetToString(
+            failed_data_types_handler_.GetUnrecoverableErrorTypes()) +
         ": " + error.message();
     LOG(ERROR) << "ProfileSyncService error: " << message;
     OnInternalUnrecoverableError(error.location(),

chrome/browser/sync/profile_sync_service_startup_unittest.cc

@@ -45,18 +45,21 @@ using testing::DoAll;
 using testing::InvokeArgument;
 using testing::Mock;
 using testing::Return;
+using testing::SaveArg;
 
 ACTION_P(InvokeOnConfigureStart, pss) {
   ProfileSyncService* service =
       static_cast<ProfileSyncService*>(pss);
   service->OnConfigureStart();
 }
 
-ACTION_P2(InvokeOnConfigureDone, pss, result) {
+ACTION_P3(InvokeOnConfigureDone, pss, error_callback, result) {
   ProfileSyncService* service =
       static_cast<ProfileSyncService*>(pss);
   DataTypeManager::ConfigureResult configure_result =
       static_cast<DataTypeManager::ConfigureResult>(result);
+  if (result.status == sync_driver::DataTypeManager::ABORTED)
+    error_callback.Run();
   service->OnConfigureDone(configure_result);
 }
 
@@ -67,7 +70,8 @@ class ProfileSyncServiceStartupTest : public testing::Test {
                        content::TestBrowserThreadBundle::REAL_FILE_THREAD |
                        content::TestBrowserThreadBundle::REAL_IO_THREAD),
         profile_manager_(TestingBrowserProcess::GetGlobal()),
-        sync_(NULL) {}
+        sync_(NULL),
+        failed_data_types_handler_(NULL) {}
 
   virtual ~ProfileSyncServiceStartupTest() {
   }
@@ -127,6 +131,16 @@ class ProfileSyncServiceStartupTest : public testing::Test {
     return static_cast<FakeSigninManagerForTesting*>(sync_->signin());
   }
 
+  void SetError() {
+    sync_driver::FailedDataTypesHandler::TypeErrorMap errors;
+    errors[syncer::BOOKMARKS] =
+        syncer::SyncError(FROM_HERE,
+                          syncer::SyncError::UNRECOVERABLE_ERROR,
+                          "Error",
+                          syncer::BOOKMARKS);
+    failed_data_types_handler_->UpdateFailedDataTypes(errors);
+  }
+
  protected:
   void SimulateTestUserSignin() {
     profile_->GetPrefs()->SetString(prefs::kGoogleServicesUsername,
@@ -143,7 +157,8 @@ class ProfileSyncServiceStartupTest : public testing::Test {
     DataTypeManagerMock* data_type_manager = new DataTypeManagerMock();
     EXPECT_CALL(*components_factory_mock(),
                 CreateDataTypeManager(_, _, _, _, _, _)).
-        WillOnce(Return(data_type_manager));
+        WillOnce(DoAll(SaveArg<5>(&failed_data_types_handler_),
+                       Return(data_type_manager)));
     return data_type_manager;
   }
 
@@ -161,6 +176,7 @@ class ProfileSyncServiceStartupTest : public testing::Test {
   TestingProfile* profile_;
   ProfileSyncService* sync_;
   ProfileSyncServiceObserverMock observer_;
+  sync_driver::FailedDataTypesHandler* failed_data_types_handler_;
 };
 
 class ProfileSyncServiceStartupCrosTest : public ProfileSyncServiceStartupTest {
@@ -508,23 +524,16 @@ TEST_F(ProfileSyncServiceStartupTest, StartFailure) {
   SetUpSyncBackendHost();
   DataTypeManagerMock* data_type_manager = SetUpDataTypeManager();
   DataTypeManager::ConfigureStatus status = DataTypeManager::ABORTED;
-  syncer::SyncError error(
-      FROM_HERE,
-      syncer::SyncError::DATATYPE_ERROR,
-      "Association failed.",
-      syncer::BOOKMARKS);
-  std::map<syncer::ModelType, syncer::SyncError> errors;
-  errors[syncer::BOOKMARKS] = error;
   DataTypeManager::ConfigureResult result(
       status,
-      syncer::ModelTypeSet(),
-      errors,
-      syncer::ModelTypeSet(),
       syncer::ModelTypeSet());
-  EXPECT_CALL(*data_type_manager, Configure(_, _)).
-      WillRepeatedly(
-          DoAll(InvokeOnConfigureStart(sync_),
-                InvokeOnConfigureDone(sync_, result)));
+  EXPECT_CALL(*data_type_manager, Configure(_, _)).WillRepeatedly(
+      DoAll(InvokeOnConfigureStart(sync_),
+            InvokeOnConfigureDone(
+                sync_,
+                base::Bind(&ProfileSyncServiceStartupTest::SetError,
+                           base::Unretained(this)),
+                result)));
   EXPECT_CALL(*data_type_manager, state()).
       WillOnce(Return(DataTypeManager::STOPPED));
   EXPECT_CALL(observer_, OnStateChanged()).Times(AnyNumber());

components/sync_driver/data_type_controller.cc

@@ -23,11 +23,11 @@ DataTypeController::DataTypeController(
 DataTypeController::~DataTypeController() {
 }
 
-bool DataTypeController::IsUnrecoverableResult(StartResult result) {
+bool DataTypeController::IsUnrecoverableResult(ConfigureResult result) {
   return (result == UNRECOVERABLE_ERROR);
 }
 
-bool DataTypeController::IsSuccessfulResult(StartResult result) {
+bool DataTypeController::IsSuccessfulResult(ConfigureResult result) {
   return (result == OK || result == OK_FIRST_RUN);
 }