[Sync] Introduce ModelError for USS error handling.

components/sync/model/fake_model_type_change_processor.cc

Index: components/sync/model/fake_model_type_change_processor.cc
diff --git a/components/sync/model/fake_model_type_change_processor.cc b/components/sync/model/fake_model_type_change_processor.cc
index e0df9d869a1ac6237a1a68b1f3e3a297102d2a54..cc45fb2d5b9c3bb896c33fc13ece74a7a11e7089 100644
--- a/components/sync/model/fake_model_type_change_processor.cc
+++ b/components/sync/model/fake_model_type_change_processor.cc
@@ -19,8 +19,11 @@ std::unique_ptr<ModelTypeChangeProcessor> FakeModelTypeChangeProcessor::Create(
   return base::WrapUnique(new FakeModelTypeChangeProcessor());
 }
 
-FakeModelTypeChangeProcessor::FakeModelTypeChangeProcessor() {}
-FakeModelTypeChangeProcessor::~FakeModelTypeChangeProcessor() {}
+FakeModelTypeChangeProcessor::FakeModelTypeChangeProcessor() = default;
+
+FakeModelTypeChangeProcessor::~FakeModelTypeChangeProcessor() {
+  DCHECK(!error_.IsSet());

[skym, 2017/01/09 18:20:28]

Can you add a comment here explaining why this is here? And what unittests have
done wrong if they hit this DCHECK?

[maxbogue, 2017/01/09 21:29:33]

Done.

+}
 
 void FakeModelTypeChangeProcessor::Put(
     const std::string& client_tag,
@@ -32,7 +35,6 @@ void FakeModelTypeChangeProcessor::Delete(
     MetadataChangeList* metadata_change_list) {}
 
 void FakeModelTypeChangeProcessor::OnMetadataLoaded(
-    SyncError error,
     std::unique_ptr<MetadataBatch> batch) {}
 
 void FakeModelTypeChangeProcessor::OnSyncStarting(
@@ -49,10 +51,20 @@ bool FakeModelTypeChangeProcessor::IsTrackingMetadata() {
   return true;
 }
 
-SyncError FakeModelTypeChangeProcessor::CreateAndUploadError(
+void FakeModelTypeChangeProcessor::ReportError(const ModelError& error) {
+  error_ = error;

[skym, 2017/01/09 18:20:28]

Is overriding a set error with an unset or set error a problem?

[maxbogue, 2017/01/09 21:29:33]

Changing how this works to just have a boolean expect_error_ that gets checked
here, instead of storing the error itself.

+}
+
+void FakeModelTypeChangeProcessor::ReportError(
     const tracked_objects::Location& location,
     const std::string& message) {
-  return SyncError();
+  ReportError(ModelError(location, message));

[skym, 2017/01/09 18:20:28]

One one hand, pure virtual classes are nice. On the other hand, is there really
a use case for the implementation of this method to not do exactly this?

[maxbogue, 2017/01/09 21:29:33]

Not necessarily, but pure virtual classes are nice and we'll never have more
than these couple implementations of it so I'm leaving it this way.

+}
+
+ModelError FakeModelTypeChangeProcessor::TakeError() {
+  ModelError error = error_;
+  error_ = ModelError();
+  return error;
 }
 
 }  // namespace syncer