[Sync] Services can now always assume processor exists.

components/sync/model/model_type_service_unittest.cc

Index: components/sync/model/model_type_service_unittest.cc
diff --git a/components/sync/model/model_type_service_unittest.cc b/components/sync/model/model_type_service_unittest.cc
index 15db0d7cfeae956e1020e8cc0e1963b574e5394a..55ec72e95270975f323f630e743e2b16e51a155f 100644
--- a/components/sync/model/model_type_service_unittest.cc
+++ b/components/sync/model/model_type_service_unittest.cc
@@ -4,11 +4,15 @@
 
 #include "components/sync/model/model_type_service.h"
 
+#include <utility>
+
 #include "base/bind.h"
 #include "base/memory/ptr_util.h"
 #include "components/sync/model/data_type_error_handler_mock.h"
 #include "components/sync/model/fake_model_type_change_processor.h"
+#include "components/sync/model/metadata_batch.h"
 #include "components/sync/model/stub_model_type_service.h"
+#include "components/sync/protocol/model_type_state.pb.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace syncer {
@@ -22,8 +26,24 @@ class MockModelTypeChangeProcessor : public FakeModelTypeChangeProcessor {
 
   void DisableSync() override { disabled_callback_.Run(); }
 
+  void OnMetadataLoaded(SyncError error,
+                        std::unique_ptr<MetadataBatch> batch) override {
+    on_metadata_loaded_error_ = error;
+    on_metadata_loaded_batch_ = std::move(batch);
+  }
+
+  const SyncError& on_metadata_loaded_error() const {
+    return on_metadata_loaded_error_;
+  }
+  MetadataBatch* on_metadata_loaded_batch() {
+    return on_metadata_loaded_batch_.get();
+  }
+
  private:
   base::Closure disabled_callback_;
+
+  SyncError on_metadata_loaded_error_;
+  std::unique_ptr<MetadataBatch> on_metadata_loaded_batch_;
 };
 
 class MockModelTypeService : public StubModelTypeService {
@@ -33,15 +53,11 @@ class MockModelTypeService : public StubModelTypeService {
                                         base::Unretained(this))) {}
   ~MockModelTypeService() override {}
 
-  void CreateChangeProcessor() { ModelTypeService::CreateChangeProcessor(); }
-
   MockModelTypeChangeProcessor* change_processor() const {
     return static_cast<MockModelTypeChangeProcessor*>(
         ModelTypeService::change_processor());
   }
 
-  bool on_processor_set_called() const { return on_processor_set_called_; }
-  void clear_on_processor_set_called() { on_processor_set_called_ = false; }
   bool processor_disable_sync_called() const {
     return processor_disable_sync_called_;
   }
@@ -54,11 +70,8 @@ class MockModelTypeService : public StubModelTypeService {
         &MockModelTypeService::OnProcessorDisableSync, base::Unretained(this)));
   }
 
-  void OnChangeProcessorSet() override { on_processor_set_called_ = true; }
-
   void OnProcessorDisableSync() { processor_disable_sync_called_ = true; }
 
-  bool on_processor_set_called_ = false;
   bool processor_disable_sync_called_ = false;
 };
 
@@ -84,32 +97,15 @@ class ModelTypeServiceTest : public ::testing::Test {
     start_callback_called_ = true;
   }
 
-  bool start_callback_called_;
+  bool start_callback_called_ = false;
   MockModelTypeService service_;
 };
 
-// CreateChangeProcessor should construct a processor and call
-// OnChangeProcessorSet if and only if one doesn't already exist.
-TEST_F(ModelTypeServiceTest, CreateChangeProcessor) {
-  EXPECT_FALSE(service()->change_processor());
-  EXPECT_FALSE(service()->on_processor_set_called());
-  service()->CreateChangeProcessor();
-  ModelTypeChangeProcessor* processor = service()->change_processor();
-  EXPECT_TRUE(processor);
-  EXPECT_TRUE(service()->on_processor_set_called());
-
-  // A second call shouldn't make a new processor.
-  service()->clear_on_processor_set_called();
-  service()->CreateChangeProcessor();
-  EXPECT_EQ(processor, service()->change_processor());
-  EXPECT_FALSE(service()->on_processor_set_called());
-}
-
 // OnSyncStarting should create a processor and call OnSyncStarting on it.
 TEST_F(ModelTypeServiceTest, OnSyncStarting) {
-  EXPECT_FALSE(service()->change_processor());
+  EXPECT_FALSE(start_callback_called());
   OnSyncStarting();
-  EXPECT_TRUE(service()->change_processor());
+
   // FakeModelTypeProcessor is the one that calls the callback, so if it was
   // called then we know the call on the processor was made.
   EXPECT_TRUE(start_callback_called());
@@ -117,13 +113,20 @@ TEST_F(ModelTypeServiceTest, OnSyncStarting) {
 
 // DisableSync should call DisableSync on the processor and then delete it.
 TEST_F(ModelTypeServiceTest, DisableSync) {
-  service()->CreateChangeProcessor();
-  EXPECT_TRUE(service()->change_processor());
   EXPECT_FALSE(service()->processor_disable_sync_called());
-
   service()->DisableSync();
-  EXPECT_FALSE(service()->change_processor());
+
+  // Disabling also wipes out metadata, and the service should have told the new
+  // processor about this.
   EXPECT_TRUE(service()->processor_disable_sync_called());

[maxbogue, 2016/10/12 23:40:35]

I'd move this line right under 117.

[skym, 2016/10/13 19:21:38]

Why? I like this formatting because it's broken into precondition checks ->
actions -> post condition checks. Since precondition and actions are so small,
they're grouped. Post conditions are also grouped.

[maxbogue, 2016/10/13 20:11:04]

My suggestion was for symmetry and because the comment describes the whole block
except this line. Either way is fine.

[skym, 2016/10/13 20:36:27]

Good point, added a space after the EXPECT_TRUE so it's clear that the comment
isn't applying to the rest of the lines.

+  EXPECT_FALSE(
+      service()->change_processor()->on_metadata_loaded_error().IsSet());
+  MetadataBatch* batch =
+      service()->change_processor()->on_metadata_loaded_batch();
+  EXPECT_NE(nullptr, batch);
+  EXPECT_EQ(sync_pb::ModelTypeState().SerializeAsString(),
+            batch->GetModelTypeState().SerializeAsString());
+  EXPECT_EQ(0U, batch->TakeAllMetadata().size());
 }
 
 // ResolveConflicts should return USE_REMOTE unless the remote data is deleted.