[Sync] Add support for dynamically enabling/disabling types

The notion of Unready types is also introduced for those types which control whether
they should run, and therefore for which a configuration (which might
involve network access) should not even be performed.

BUG=368834

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

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

chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc

@@ -342,7 +342,7 @@ TEST_F(SyncBookmarkDataTypeControllerTest, OnSingleDatatypeUnrecoverableError) {
   SetAssociateExpectations();
   EXPECT_CALL(*model_associator_, SyncModelHasUserCreatedNodes(_)).
       WillRepeatedly(DoAll(SetArgumentPointee<0>(true), Return(true)));
-  EXPECT_CALL(service_, DisableBrokenDatatype(_,_,_)).
+  EXPECT_CALL(service_, DisableDatatype(_,_,_)).
       WillOnce(InvokeWithoutArgs(bookmark_dtc_.get(),
                                  &BookmarkDataTypeController::Stop));
   SetStopExpectations();

chrome/browser/sync/glue/frontend_data_type_controller.cc

@@ -147,7 +147,7 @@ DataTypeController::State FrontendDataTypeController::state() const {
 void FrontendDataTypeController::OnSingleDatatypeUnrecoverableError(
     const tracked_objects::Location& from_here, const std::string& message) {
   RecordUnrecoverableError(from_here, message);
-  sync_service_->DisableBrokenDatatype(type(), from_here, message);
+  sync_service_->DisableDatatype(type(), from_here, message);
 }
 
 FrontendDataTypeController::FrontendDataTypeController()

chrome/browser/sync/glue/frontend_data_type_controller_unittest.cc

@@ -261,7 +261,7 @@ TEST_F(SyncFrontendDataTypeControllerTest, OnSingleDatatypeUnrecoverableError) {
   SetAssociateExpectations();
   SetActivateExpectations(DataTypeController::OK);
   EXPECT_CALL(*dtc_mock_.get(), RecordUnrecoverableError(_, "Test"));
-  EXPECT_CALL(service_, DisableBrokenDatatype(_, _, _))
+  EXPECT_CALL(service_, DisableDatatype(_, _, _))
       .WillOnce(InvokeWithoutArgs(frontend_dtc_.get(),
                                   &FrontendDataTypeController::Stop));
   SetStopExpectations();

chrome/browser/sync/glue/non_frontend_data_type_controller.cc

@@ -383,7 +383,7 @@ void NonFrontendDataTypeController::DisableImpl(
     const tracked_objects::Location& from_here,
     const std::string& message) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  profile_sync_service_->DisableBrokenDatatype(type(), from_here, message);
+  profile_sync_service_->DisableDatatype(type(), from_here, message);
 }
 
 void NonFrontendDataTypeController::RecordAssociationTime(

chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc

@@ -375,7 +375,7 @@ TEST_F(SyncNonFrontendDataTypeControllerTest,
   SetAssociateExpectations();
   SetActivateExpectations(DataTypeController::OK);
   EXPECT_CALL(*dtc_mock_.get(), RecordUnrecoverableError(_, "Test"));
-  EXPECT_CALL(service_, DisableBrokenDatatype(_, _, _))
+  EXPECT_CALL(service_, DisableDatatype(_, _, _))
       .WillOnce(InvokeWithoutArgs(non_frontend_dtc_.get(),
                                   &NonFrontendDataTypeController::Stop));
   SetStopExpectations();

chrome/browser/sync/glue/sync_backend_host_impl.cc

@@ -347,8 +347,15 @@ void SyncBackendHostImpl::ConfigureDataTypes(
       GetDataTypesInState(FATAL, config_state_map);
   syncer::ModelTypeSet crypto_types =
       GetDataTypesInState(CRYPTO, config_state_map);
+  syncer::ModelTypeSet unready_types =
+      GetDataTypesInState(UNREADY, config_state_map);
   disabled_types.PutAll(fatal_types);
+
+  // TODO(zea): These types won't be fully purged if they are subsequently
+  // disabled by the user. Fix that. See crbug.com/386778
   disabled_types.PutAll(crypto_types);
+  disabled_types.PutAll(unready_types);
+
   syncer::ModelTypeSet active_types =
       GetDataTypesInState(CONFIGURE_ACTIVE, config_state_map);
   syncer::ModelTypeSet clean_first_types =
@@ -392,6 +399,7 @@ void SyncBackendHostImpl::ConfigureDataTypes(
   syncer::ModelTypeSet inactive_types =
       GetDataTypesInState(CONFIGURE_INACTIVE, config_state_map);
   types_to_purge.RemoveAll(inactive_types);
+  types_to_purge.RemoveAll(unready_types);
 
   // If a type has already been disabled and unapplied or journaled, it will
   // not be part of the |types_to_purge| set, and therefore does not need

chrome/browser/sync/glue/typed_url_data_type_controller.cc

@@ -121,7 +121,7 @@ void TypedUrlDataTypeController::OnSavingBrowserHistoryDisabledChanged() {
     // generate an unrecoverable error. This can be fixed by restarting
     // Chrome (on restart, typed urls will not be a registered type).
     if (state() != NOT_RUNNING && state() != STOPPING) {
-      profile_sync_service()->DisableBrokenDatatype(
+      profile_sync_service()->DisableDatatype(
           syncer::TYPED_URLS,
           FROM_HERE,
           "History saving is now disabled by policy.");

chrome/browser/sync/profile_sync_components_factory_impl.cc

@@ -193,7 +193,7 @@ void ProfileSyncComponentsFactoryImpl::DisableBrokenType(
     const tracked_objects::Location& from_here,
     const std::string& message) {
   ProfileSyncService* p = ProfileSyncServiceFactory::GetForProfile(profile_);
-  p->DisableBrokenDatatype(type, from_here, message);
+  p->DisableDatatype(type, from_here, message);
 }
 
 DataTypeController::DisableTypeCallback

chrome/browser/sync/profile_sync_service.cc

@@ -942,7 +942,7 @@ void ProfileSyncService::OnUnrecoverableErrorImpl(
 }
 
 // TODO(zea): Move this logic into the DataTypeController/DataTypeManager.
-void ProfileSyncService::DisableBrokenDatatype(
+void ProfileSyncService::DisableDatatype(
     syncer::ModelType type,
     const tracked_objects::Location& from_here,
     std::string message) {
@@ -967,6 +967,24 @@ void ProfileSyncService::DisableBrokenDatatype(
                  weak_factory_.GetWeakPtr()));
 }
 
+void ProfileSyncService::ReenableDatatype(syncer::ModelType type) {
+  // Only reconfigure if the type actually had a data type or unready error.
+  if (!failed_data_types_handler_.ResetDataTypeErrorFor(type) &&
+      !failed_data_types_handler_.ResetUnreadyErrorFor(type)) {
+    return;
+  }
+
+  // If the type is no longer enabled, don't bother reconfiguring.
+  // TODO(zea): something else should encapsulate the notion of "whether a type
+  // should be enabled".
+  if (!syncer::CoreTypes().Has(type) && !GetPreferredDataTypes().Has(type))
+    return;
+
+  base::MessageLoop::current()->PostTask(FROM_HERE,
+      base::Bind(&ProfileSyncService::ReconfigureDatatypeManager,
+                 weak_factory_.GetWeakPtr()));
+}
+
 void ProfileSyncService::UpdateBackendInitUMA(bool success) {
   if (backend_mode_ != SYNC)
     return;
@@ -1342,9 +1360,9 @@ void ProfileSyncService::OnEncryptedTypesChanged(
   // delete directives are unnecessary.
   if (GetActiveDataTypes().Has(syncer::HISTORY_DELETE_DIRECTIVES) &&
       encrypted_types_.Has(syncer::SESSIONS)) {
-    DisableBrokenDatatype(syncer::HISTORY_DELETE_DIRECTIVES,
-                          FROM_HERE,
-                          "Delete directives not supported with encryption.");
+    DisableDatatype(syncer::HISTORY_DELETE_DIRECTIVES,
+                    FROM_HERE,
+                    "Delete directives not supported with encryption.");
   }
 }
 
@@ -1748,9 +1766,9 @@ void ProfileSyncService::OnUserChoseDatatypes(
   failed_data_types_handler_.Reset();
   if (GetActiveDataTypes().Has(syncer::HISTORY_DELETE_DIRECTIVES) &&
       encrypted_types_.Has(syncer::SESSIONS)) {
-    DisableBrokenDatatype(syncer::HISTORY_DELETE_DIRECTIVES,
-                          FROM_HERE,
-                          "Delete directives not supported with encryption.");
+    DisableDatatype(syncer::HISTORY_DELETE_DIRECTIVES,
+                    FROM_HERE,
+                    "Delete directives not supported with encryption.");
   }
   ChangePreferredDataTypes(chosen_types);
   AcknowledgeSyncedTypes();

chrome/browser/sync/profile_sync_service.h

@@ -60,10 +60,6 @@ namespace base {
 class CommandLine;
 };
 
-namespace extensions {
-struct Event;
-}
-
 namespace browser_sync {
 class BackendMigrator;
 class ChangeProcessor;
@@ -553,12 +549,16 @@ class ProfileSyncService : public ProfileSyncServiceBase,
       const tracked_objects::Location& from_here,
       const std::string& message) OVERRIDE;
 
-  // Called when a datatype wishes to disable itself due to having hit an
-  // unrecoverable error.
-  virtual void DisableBrokenDatatype(
-      syncer::ModelType type,
-      const tracked_objects::Location& from_here,
-      std::string message);
+  // Called when a datatype wishes to disable itself. Note, this does not change
+  // preferred state of a datatype and is not persisted across restarts.
+  virtual void DisableDatatype(syncer::ModelType type,
+                               const tracked_objects::Location& from_here,
+                               std::string message);
+
+  // Called to re-enable a type disabled by DisableDatatype(..). Note, this does
+  // not change the preferred state of a datatype, and is not persisted across
+  // restarts.
+  void ReenableDatatype(syncer::ModelType type);
 
   // The functions below (until ActivateDataType()) should only be
   // called if sync_initialized() is true.

chrome/browser/sync/profile_sync_service_mock.h

@@ -57,7 +57,7 @@ class ProfileSyncServiceMock : public ProfileSyncService {
   MOCK_METHOD2(OnUnrecoverableError,
                void(const tracked_objects::Location& location,
                const std::string& message));
-  MOCK_METHOD3(DisableBrokenDatatype, void(syncer::ModelType,
+  MOCK_METHOD3(DisableDatatype, void(syncer::ModelType,
                const tracked_objects::Location&,
                std::string message));
   MOCK_CONST_METHOD0(GetUserShare, syncer::UserShare*());

components/sync_driver/backend_data_type_configurer.h

@@ -32,6 +32,7 @@ class BackendDataTypeConfigurer {
     DISABLED,             // Not syncing. Disabled by user.
     FATAL,                // Not syncing due to unrecoverable error.
     CRYPTO,               // Not syncing due to a cryptographer error.
+    UNREADY,              // Not syncing due to transient error.
   };
   typedef std::map<syncer::ModelType, DataTypeConfigState>
       DataTypeConfigStateMap;

components/sync_driver/data_type_controller.cc

@@ -56,4 +56,8 @@ DataTypeController::disable_callback() {
   return disable_callback_;
 }
 
+bool DataTypeController::ReadyForStart() const {
+  return true;
+}
+
 }  // namespace browser_sync

components/sync_driver/data_type_controller.h

@@ -136,6 +136,13 @@ class DataTypeController
   // UserShare handle to associate model data with.
   void OnUserShareReady(syncer::UserShare* share);
 
+  // Whether the DataTypeController is ready to start. This is useful if the
+  // datatype itself must make the decision about whether it should be enabled
+  // at all (and therefore whether the initial download of the sync data for
+  // the type should be performed).
+  // Returns true by default.
+  virtual bool ReadyForStart() const;
+
  protected:
   friend class base::RefCountedDeleteOnMessageLoop<DataTypeController>;
   friend class base::DeleteHelper<DataTypeController>;

components/sync_driver/data_type_manager.cc

@@ -29,9 +29,6 @@ DataTypeManager::ConfigureResult::ConfigureResult(
       failed_data_types(failed_data_types),
       unfinished_data_types(unfinished_data_types),
       needs_crypto(needs_crypto) {
-  if (!failed_data_types.empty() || !needs_crypto.Empty()) {
-    DCHECK_NE(OK, status);
-  }
 }
 
 DataTypeManager::ConfigureResult::~ConfigureResult() {

components/sync_driver/data_type_manager_impl.cc

@@ -83,8 +83,26 @@ void DataTypeManagerImpl::Configure(syncer::ModelTypeSet desired_types,
   syncer::ModelTypeSet filtered_desired_types;
   for (syncer::ModelTypeSet::Iterator type = desired_types.First();
       type.Good(); type.Inc()) {
+    DataTypeController::TypeMap::const_iterator iter =
+        controllers_->find(type.Get());
     if (syncer::IsControlType(type.Get()) ||
-        controllers_->find(type.Get()) != controllers_->end()) {
+        iter != controllers_->end()) {
+      if (iter != controllers_->end()) {
+        if (!iter->second->ReadyForStart()) {
+          // Add the type to the unready types set to prevent purging it. It's
+          // up to the datatype controller to, if necessary, explicitly
+          // mark the type as broken to trigger a purge.
+          syncer::SyncError error(FROM_HERE,
+                                  syncer::SyncError::UNREADY_ERROR,
+                                  "Datatype not ready at config time.",
+                                  type.Get());
+          std::map<syncer::ModelType, syncer::SyncError> errors;
+          errors[type.Get()] = error;
+          failed_data_types_handler_->UpdateFailedDataTypes(errors);
+        } else {
+          failed_data_types_handler_->ResetUnreadyErrorFor(type.Get());
+        }
+      }
       filtered_desired_types.Put(type.Get());
     }
   }
@@ -111,19 +129,9 @@ void DataTypeManagerImpl::ConfigureImpl(
     return;
   }
 
-  if (state_ == CONFIGURED &&
-      last_requested_types_.Equals(desired_types) &&
-      reason == syncer::CONFIGURE_REASON_RECONFIGURATION &&
-      syncer::Intersection(failed_data_types_handler_->GetFailedTypes(),
-                           last_requested_types_).Empty()) {
-    // If the set of enabled types hasn't changed and there are no failing
-    // types, we can exit out early.
-    DVLOG(1) << "Reconfigure with same types, bypassing confguration.";
-    NotifyStart();
-    ConfigureResult result(OK, last_requested_types_);
-    NotifyDone(result);
-    return;
-  }
+  // TODO(zea): consider not performing a full configuration once there's a
+  // reliable way to determine if the requested set of enabled types matches the
+  // current set.
 
   last_requested_types_ = desired_types;
   last_configure_reason_ = reason;
@@ -141,25 +149,30 @@ void DataTypeManagerImpl::ConfigureImpl(
 BackendDataTypeConfigurer::DataTypeConfigStateMap
 DataTypeManagerImpl::BuildDataTypeConfigStateMap(
     const syncer::ModelTypeSet& types_being_configured) const {
-  // 1. Get the failed types (both due to fatal and crypto errors).
+  // 1. Get the failed types (due to fatal, crypto, and unready errors).
   // 2. Add the difference between last_requested_types_ and the failed types
   //    as CONFIGURE_INACTIVE.
   // 3. Flip |types_being_configured| to CONFIGURE_ACTIVE.
   // 4. Set non-enabled user types as DISABLED.
-  // 5. Set the fatal and crypto types to their respective states.
+  // 5. Set the fatal, crypto, and unready types to their respective states.
   syncer::ModelTypeSet error_types =
       failed_data_types_handler_->GetFailedTypes();
   syncer::ModelTypeSet fatal_types =
       failed_data_types_handler_->GetFatalErrorTypes();
   syncer::ModelTypeSet crypto_types =
       failed_data_types_handler_->GetCryptoErrorTypes();
+  syncer::ModelTypeSet unready_types=
+      failed_data_types_handler_->GetUnreadyErrorTypes();
 
   // Types with persistence errors are only purged/resynced when they're
   // actively being configured.
   syncer::ModelTypeSet persistence_types =
       failed_data_types_handler_->GetPersistenceErrorTypes();
   persistence_types.RetainAll(types_being_configured);
 
+  // Types with unready errors do not count as unready if they've been disabled.
+  unready_types.RetainAll(last_requested_types_);
+
   syncer::ModelTypeSet enabled_types = last_requested_types_;
   enabled_types.RemoveAll(error_types);
   syncer::ModelTypeSet disabled_types =
@@ -191,6 +204,9 @@ DataTypeManagerImpl::BuildDataTypeConfigStateMap(
   BackendDataTypeConfigurer::SetDataTypesState(
       BackendDataTypeConfigurer::CRYPTO, crypto_types,
         &config_state_map);
+  BackendDataTypeConfigurer::SetDataTypesState(
+      BackendDataTypeConfigurer::UNREADY, unready_types,
+        &config_state_map);
   return config_state_map;
 }
 

components/sync_driver/data_type_manager_impl_unittest.cc

@@ -1154,4 +1154,46 @@ TEST_F(SyncDataTypeManagerImplTest, ConfigureForBackupRollback) {
                   syncer::CONFIGURE_REASON_BACKUP_ROLLBACK);
 }
 
+TEST_F(SyncDataTypeManagerImplTest, ReenableAfterDataTypeError) {
+  syncer::SyncError error(FROM_HERE,
+                          syncer::SyncError::DATATYPE_ERROR,
+                          "Datatype disabled",
+                          syncer::BOOKMARKS);
+  std::map<syncer::ModelType, syncer::SyncError> errors;
+  errors[syncer::BOOKMARKS] = error;
+  failed_data_types_handler_.UpdateFailedDataTypes(errors);
+
+  AddController(PREFERENCES);  // Will succeed.
+  AddController(BOOKMARKS);    // Will be disabled due to datatype error.
+
+  SetConfigureStartExpectation();
+  SetConfigureDoneExpectation(DataTypeManager::PARTIAL_SUCCESS);
+
+  Configure(dtm_.get(), ModelTypeSet(BOOKMARKS, PREFERENCES));
+  FinishDownload(*dtm_, ModelTypeSet(), ModelTypeSet());
+  FinishDownload(*dtm_, ModelTypeSet(PREFERENCES), ModelTypeSet());
+  GetController(PREFERENCES)->FinishStart(DataTypeController::OK);
+  EXPECT_EQ(DataTypeManager::CONFIGURED, dtm_->state());
+  EXPECT_EQ(DataTypeController::RUNNING, GetController(PREFERENCES)->state());
+  EXPECT_EQ(DataTypeController::NOT_RUNNING, GetController(BOOKMARKS)->state());
+
+  Mock::VerifyAndClearExpectations(&observer_);
+
+  // Re-enable bookmarks.
+  failed_data_types_handler_.ResetDataTypeErrorFor(syncer::BOOKMARKS);
+
+  SetConfigureStartExpectation();
+  SetConfigureDoneExpectation(DataTypeManager::OK);
+
+  Configure(dtm_.get(), ModelTypeSet(BOOKMARKS, PREFERENCES));
+  EXPECT_EQ(DataTypeManager::DOWNLOAD_PENDING, dtm_->state());
+  FinishDownload(*dtm_, ModelTypeSet(), ModelTypeSet());
+  FinishDownload(*dtm_, ModelTypeSet(BOOKMARKS), ModelTypeSet());
+  EXPECT_EQ(DataTypeManager::CONFIGURING, dtm_->state());
+  GetController(BOOKMARKS)->FinishStart(DataTypeController::OK);
+  EXPECT_EQ(DataTypeManager::CONFIGURED, dtm_->state());
+  EXPECT_EQ(DataTypeController::RUNNING, GetController(PREFERENCES)->state());
+  EXPECT_EQ(DataTypeController::RUNNING, GetController(BOOKMARKS)->state());
+}
+
 }  // namespace browser_sync