Sync: Display non-severe errors on about:sync in gray color rather th…

…an red.

This fix changes representation of some disabled datatypes on about:sync page.
There are two special cases supported by this fix:
1) When a datatype needs to be disabled due to configuration constraints,
   for example, when sync to need to disable delete directives when
   encryption is enabled.
2) When a datatype isn't ready to start yet (was disabled with
   SyncError::UNREADY_ERROR).

To implement the fix, SyncError class was extended with severity() property,
which is based on the error type. The purpose of severity is to determine
logging severity of the error and the representation on about:sync page.
Sync errors of UNREADY_ERROR and DATATYPE_POLICY_ERROR types are logged with
LOG_INFO level rather than LOG_ERROR. In the UI errors of these types are
displayed on gray background rather than red.

DATATYPE_POLICY_ERROR is a new error type used for a few cases when a type
is disabled by policy (i.e. configuration constraints). Other than its
severity it is treated the same as DATATYPE_ERROR.

I added an overload for DisableDatatype function which allows to specify
an error type. By default, the error type is DATATYPE_ERROR which is
consistent with the current implementation.

BUG=380446,392110

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

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

chrome/browser/resources/sync_internals/about.css

@@ -44,6 +44,10 @@
  background: rgb(255, 255, 204);
 }
 
+#typeInfo .disabled {
+ background: rgb(224, 224, 224);
+}
+
 #typeInfo .ok {
  background: rgb(204, 255, 204);
 }

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_, DisableDatatype(_,_,_)).
+  EXPECT_CALL(service_, DisableDatatype(_)).
       WillOnce(InvokeWithoutArgs(bookmark_dtc_.get(),
                                  &BookmarkDataTypeController::Stop));
   SetStopExpectations();

chrome/browser/sync/glue/extension_backed_data_type_controller.cc

@@ -83,9 +83,12 @@ void ExtensionBackedDataTypeController::OnExtensionUnloaded(
     // unloads (e.g. for permission changes). If that becomes a large enough
     // issue, we should consider using the extension unload reason to just
     // trigger a reconfiguration without disabling (type will be unready).
-    sync_service->DisableDatatype(type(),
-                                  FROM_HERE,
-                                  "Extension unloaded");
+    syncer::SyncError error(
+        FROM_HERE,
+        syncer::SyncError::DATATYPE_ERROR,
+        "Extension unloaded",
+        type());
+    sync_service->DisableDatatype(error);
   }
 }
 

chrome/browser/sync/glue/frontend_data_type_controller.cc

@@ -147,7 +147,9 @@ DataTypeController::State FrontendDataTypeController::state() const {
 void FrontendDataTypeController::OnSingleDatatypeUnrecoverableError(
     const tracked_objects::Location& from_here, const std::string& message) {
   RecordUnrecoverableError(from_here, message);
-  sync_service_->DisableDatatype(type(), from_here, message);
+  syncer::SyncError error(
+      from_here, syncer::SyncError::DATATYPE_ERROR, message, type());
+  sync_service_->DisableDatatype(error);
 }
 
 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_, DisableDatatype(_, _, _))
+  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,9 @@ void NonFrontendDataTypeController::DisableImpl(
     const tracked_objects::Location& from_here,
     const std::string& message) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  profile_sync_service_->DisableDatatype(type(), from_here, message);
+  syncer::SyncError error(
+      from_here, syncer::SyncError::DATATYPE_ERROR, message, type());
+  profile_sync_service_->DisableDatatype(error);
 }
 
 void NonFrontendDataTypeController::RecordAssociationTime(

chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc

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

chrome/browser/sync/glue/typed_url_data_type_controller.cc

@@ -121,10 +121,12 @@ 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()->DisableDatatype(
-          syncer::TYPED_URLS,
+      syncer::SyncError error(
           FROM_HERE,
-          "History saving is now disabled by policy.");
+          syncer::SyncError::DATATYPE_POLICY_ERROR,
+          "History saving is now disabled by policy.",
+          syncer::TYPED_URLS);
+      profile_sync_service()->DisableDatatype(error);
     }
   }
 }

chrome/browser/sync/glue/typed_url_model_associator_unittest.cc

@@ -80,8 +80,7 @@ static void CreateModelAssociatorAsync(base::WaitableEvent* startup,
   aborted->Wait();
   syncer::SyncError error = (*associator)->AssociateModels(NULL, NULL);
   EXPECT_TRUE(error.IsSet());
-  EXPECT_EQ("datatype error was encountered: Association was aborted.",
-            error.message());
+  EXPECT_EQ("Association was aborted.", error.message());
   delete *associator;
   done->Signal();
 }

chrome/browser/sync/profile_sync_components_factory_impl.cc

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

chrome/browser/sync/profile_sync_service.cc

@@ -957,21 +957,13 @@ void ProfileSyncService::OnUnrecoverableErrorImpl(
 }
 
 // TODO(zea): Move this logic into the DataTypeController/DataTypeManager.
-void ProfileSyncService::DisableDatatype(
-    syncer::ModelType type,
-    const tracked_objects::Location& from_here,
-    std::string message) {
+void ProfileSyncService::DisableDatatype(const syncer::SyncError& error) {
   // First deactivate the type so that no further server changes are
   // passed onto the change processor.
-  DeactivateDataType(type);
-
-  syncer::SyncError error(from_here,
-                          syncer::SyncError::DATATYPE_ERROR,
-                          message,
-                          type);
+  DeactivateDataType(error.model_type());
 
   std::map<syncer::ModelType, syncer::SyncError> errors;
-  errors[type] = error;
+  errors[error.model_type()] = error;
 
   // Update this before posting a task. So if a configure happens before
   // the task that we are going to post, this type would still be disabled.
@@ -1382,9 +1374,12 @@ void ProfileSyncService::OnEncryptedTypesChanged(
   // delete directives are unnecessary.
   if (GetActiveDataTypes().Has(syncer::HISTORY_DELETE_DIRECTIVES) &&
       encrypted_types_.Has(syncer::SESSIONS)) {
-    DisableDatatype(syncer::HISTORY_DELETE_DIRECTIVES,
-                    FROM_HERE,
-                    "Delete directives not supported with encryption.");
+    syncer::SyncError error(
+        FROM_HERE,
+        syncer::SyncError::DATATYPE_POLICY_ERROR,
+        "Delete directives not supported with encryption.",
+        syncer::HISTORY_DELETE_DIRECTIVES);
+    DisableDatatype(error);
   }
 }
 
@@ -1790,9 +1785,12 @@ void ProfileSyncService::OnUserChoseDatatypes(
   failed_data_types_handler_.Reset();
   if (GetActiveDataTypes().Has(syncer::HISTORY_DELETE_DIRECTIVES) &&
       encrypted_types_.Has(syncer::SESSIONS)) {
-    DisableDatatype(syncer::HISTORY_DELETE_DIRECTIVES,
-                    FROM_HERE,
-                    "Delete directives not supported with encryption.");
+    syncer::SyncError error(
+        FROM_HERE,
+        syncer::SyncError::DATATYPE_POLICY_ERROR,
+        "Delete directives not supported with encryption.",
+        syncer::HISTORY_DELETE_DIRECTIVES);
+    DisableDatatype(error);
   }
   ChangePreferredDataTypes(chosen_types);
   AcknowledgeSyncedTypes();
@@ -2037,10 +2035,22 @@ base::Value* ProfileSyncService::GetTypeStatusMap() const {
     if (error_map.find(type) != error_map.end()) {
       const syncer::SyncError &error = error_map.find(type)->second;
       DCHECK(error.IsSet());
-      std::string error_text = "Error: " + error.location().ToString() +
-          ", " + error.message();
-      type_status->SetString("status", "error");
-      type_status->SetString("value", error_text);
+      switch (error.GetSeverity()) {
+        case syncer::SyncError::SYNC_ERROR_SEVERITY_ERROR: {
+            std::string error_text = "Error: " + error.location().ToString() +
+                ", " + error.GetMessagePrefix() + error.message();
+            type_status->SetString("status", "error");
+            type_status->SetString("value", error_text);
+          }
+          break;
+        case syncer::SyncError::SYNC_ERROR_SEVERITY_INFO:
+          type_status->SetString("status", "disabled");
+          type_status->SetString("value", error.message());
+          break;
+        default:
+          NOTREACHED() << "Unexpected error severity.";
+          break;
+      }
     } else if (syncer::IsProxyType(type) && passive_types.Has(type)) {
       // Show a proxy type in "ok" state unless it is disabled by user.
       DCHECK(!throttled_types.Has(type));

chrome/browser/sync/profile_sync_service.h

@@ -549,11 +549,10 @@ class ProfileSyncService : public ProfileSyncServiceBase,
       const tracked_objects::Location& from_here,
       const std::string& message) OVERRIDE;
 
-  // Called when a datatype wishes to disable itself. Note, this does not change
+  // Called when a datatype wishes to disable itself. The datatype to be
+  // disabled is passed via |error.model_type()|. 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);
+  virtual void DisableDatatype(const syncer::SyncError& error);
 
   // 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

chrome/browser/sync/profile_sync_service_mock.h

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

chrome/browser/themes/theme_syncable_service_unittest.cc

@@ -535,9 +535,7 @@ TEST_F(ThemeSyncableServiceTest, StopSync) {
   error = theme_sync_service_->ProcessSyncChanges(FROM_HERE, changes);
   EXPECT_TRUE(error.IsSet());
   EXPECT_EQ(syncer::THEMES, error.model_type());
-  EXPECT_EQ("datatype error was encountered: Theme syncable service is not "
-                "started.",
-            error.message());
+  EXPECT_EQ("Theme syncable service is not started.", error.message());
 }
 
 TEST_F(ThemeSyncableServiceTest, RestoreSystemThemeBitWhenChangeToCustomTheme) {

components/sync_driver/failed_data_types_handler.cc

@@ -45,6 +45,7 @@ bool FailedDataTypesHandler::UpdateFailedDataTypes(const TypeErrorMap& errors) {
         unrecoverable_errors_.insert(*iter);
         break;
       case syncer::SyncError::DATATYPE_ERROR:
+      case syncer::SyncError::DATATYPE_POLICY_ERROR:
         data_type_errors_.insert(*iter);
         break;
       case syncer::SyncError::CRYPTO_ERROR:

sync/api/sync_error.cc

@@ -18,30 +18,10 @@ SyncError::SyncError() {
 
 SyncError::SyncError(const tracked_objects::Location& location,
                      ErrorType error_type,
-                     const std::string& custom_message,
+                     const std::string& message,
                      ModelType model_type) {
-  std::string type_message;
-  switch (error_type) {
-    case UNRECOVERABLE_ERROR:
-      type_message = "unrecoverable error was encountered: ";
-      break;
-    case DATATYPE_ERROR:
-      type_message = "datatype error was encountered: ";
-      break;
-    case PERSISTENCE_ERROR:
-      type_message = "persistence error was encountered: ";
-      break;
-    case CRYPTO_ERROR:
-      type_message = "cryptographer error was encountered: ";
-      break;
-    case UNREADY_ERROR:
-      type_message = "unready error was encountered: ";
-      break;
-    case UNSET:
-      NOTREACHED() << "Invalid error type";
-      return;
-  }
-  Init(location, type_message + custom_message, model_type, error_type);
+  DCHECK(error_type != UNSET);
+  Init(location, message, model_type, error_type);
   PrintLogError();
 }
 
@@ -120,20 +100,63 @@ SyncError::ErrorType SyncError::error_type() const {
   return error_type_;
 }
 
+SyncError::Severity SyncError::GetSeverity() const {
+  switch (error_type_) {
+    case UNREADY_ERROR:
+    case DATATYPE_POLICY_ERROR:
+      return SYNC_ERROR_SEVERITY_INFO;
+    default:
+      return SYNC_ERROR_SEVERITY_ERROR;
+  }
+}
+
+std::string SyncError::GetMessagePrefix() const {
+  std::string type_message;
+  switch (error_type_) {
+    case UNRECOVERABLE_ERROR:
+      type_message = "unrecoverable error was encountered: ";
+      break;
+    case DATATYPE_ERROR:
+      type_message = "datatype error was encountered: ";
+      break;
+    case PERSISTENCE_ERROR:
+      type_message = "persistence error was encountered: ";
+      break;
+    case CRYPTO_ERROR:
+      type_message = "cryptographer error was encountered: ";
+      break;
+    case UNREADY_ERROR:
+      type_message = "unready error was encountered: ";
+      break;
+    case DATATYPE_POLICY_ERROR:
+      type_message = "disabled due to configuration constraints: ";
+      break;
+    case UNSET:
+      NOTREACHED() << "Invalid error type";
+      break;
+  }
+  return type_message;
+}
+
 std::string SyncError::ToString() const {
   if (!IsSet()) {
     return std::string();
   }
   return location_->ToString() + ", " + ModelTypeToString(model_type_) +
-      " " + message_;
+      " " + GetMessagePrefix() + message_;
 }
 
 void SyncError::PrintLogError() const {
+  logging::LogSeverity logSeverity =
+      (GetSeverity() == SYNC_ERROR_SEVERITY_INFO)
+          ? logging::LOG_INFO : logging::LOG_ERROR;
+
   LAZY_STREAM(logging::LogMessage(location_->file_name(),
                                   location_->line_number(),
-                                  logging::LOG_ERROR).stream(),
-              LOG_IS_ON(ERROR))
-      << ModelTypeToString(model_type_) << " " << message_;
+                                  logSeverity).stream(),
+              logSeverity >= ::logging::GetMinLogLevel())
+      << ModelTypeToString(model_type_) << " "
+      << GetMessagePrefix() << message_;
 }
 
 void PrintTo(const SyncError& sync_error, std::ostream* os) {

sync/api/sync_error.h

@@ -38,8 +38,18 @@ class SYNC_EXPORT SyncError {
                           // datataype should be associated after a sync update.
     CRYPTO_ERROR,         // A cryptographer error was detected, and the
                           // datatype should be associated after it is resolved.
-    UNREADY_ERROR,        // The type is not ready to start yet, so should be
+    UNREADY_ERROR,        // A datatype is not ready to start yet, so should be
                           // neither purged nor enabled until it is ready.
+    DATATYPE_POLICY_ERROR // A datatype should be disabled and purged due
+                          // to configuration constraints.
+  };
+
+  // Severity is used to indicate how an error should be logged and
+  // represented to an end user.
+  enum Severity {
+    SYNC_ERROR_SEVERITY_ERROR,  // Severe unrecoverable error.
+    SYNC_ERROR_SEVERITY_INFO    // Low-severity recoverable error or
+                                // configuration policy issue.
   };
 
   // Default constructor refers to "no error", and IsSet() will return false.
@@ -76,6 +86,11 @@ class SYNC_EXPORT SyncError {
   ModelType model_type() const;
   ErrorType error_type() const;
 
+  // Error severity for logging and UI purposes.
+  Severity GetSeverity() const;
+  // Type specific message prefix for logging and UI purposes.
+  std::string GetMessagePrefix() const;
+
   // Returns empty string is IsSet() is false.
   std::string ToString() const;
  private: