[Sync] Have SyncableService's take ownership of their SyncChangeProcessor.

chrome/browser/extensions/app_notification_manager_sync_unittest.cc

Index: chrome/browser/extensions/app_notification_manager_sync_unittest.cc
diff --git a/chrome/browser/extensions/app_notification_manager_sync_unittest.cc b/chrome/browser/extensions/app_notification_manager_sync_unittest.cc
index bf822342859bd366b9a4ec05943f38a1acca011b..56fe2b6db280c50b088eb16080abfefff82a98a5 100644
--- a/chrome/browser/extensions/app_notification_manager_sync_unittest.cc
+++ b/chrome/browser/extensions/app_notification_manager_sync_unittest.cc
@@ -61,13 +61,35 @@ class TestChangeProcessor : public SyncChangeProcessor {
   DISALLOW_COPY_AND_ASSIGN(TestChangeProcessor);
 };
 
+class SyncChangeProcessorDelegate : public SyncChangeProcessor {
+ public:
+  explicit SyncChangeProcessorDelegate(SyncChangeProcessor* recipient)
+      : recipient_(recipient) {
+    DCHECK(recipient_);
+  }

[akalin, 2012/03/21 01:23:19]

virtual destructor

[Nicolas Zea, 2012/03/21 17:38:27]

Done.

+
+  // SyncChangeProcessor implementation.
+  virtual SyncError ProcessSyncChanges(
+      const tracked_objects::Location& from_here,
+      const SyncChangeList& change_list) OVERRIDE {
+    return recipient_->ProcessSyncChanges(from_here, change_list);
+  }
+
+ private:
+  // The recipient of all sync changes.
+  SyncChangeProcessor* recipient_;
+};
+
 }  // namespace
 
 class AppNotificationManagerSyncTest : public testing::Test {
  public:
   AppNotificationManagerSyncTest()
       : ui_thread_(BrowserThread::UI, &ui_loop_),
-        file_thread_(BrowserThread::FILE) { }
+        file_thread_(BrowserThread::FILE),
+        sync_processor_(new TestChangeProcessor),
+        sync_processor_delegate_(new SyncChangeProcessorDelegate(
+            sync_processor_.get())) {}
 
   ~AppNotificationManagerSyncTest() {
     model_ = NULL;
@@ -104,7 +126,11 @@ class AppNotificationManagerSyncTest : public testing::Test {
   }
 
   AppNotificationManager* model() { return model_.get(); }
-  TestChangeProcessor* processor() { return &processor_; }
+  TestChangeProcessor* processor() { return sync_processor_.get(); }
+
+  scoped_ptr<SyncChangeProcessor> PassProcessor() {
+    return sync_processor_delegate_.PassAs<SyncChangeProcessor>();

[akalin, 2012/03/21 01:23:19]

Perhaps instead of creating a delegate above, you could instead only create the
TestChangeProcessor when this is called and keep a (non-scoped) pointer to it? 
With comments saying that it's owned by model() and lives as long as it does,
etc.

[Nicolas Zea, 2012/03/21 17:38:27]

I prefer this approach as it lets you verify the behavior after a StopSyncing
call as well. If I create the TestChangeProcessor on demand, it will get
destroyed at StopSyncing time, so we're more limited.

Additionally, this approach lets you have two delegates to the same processor
(which some tests use to verify two syncable services match). Eventually I plan
to pull the SyncChangeProcessorDelegate and a FakeSyncChangeProcessor into their
own class and have all tests use those.

+  }
 
   // Creates a notification whose properties are set from the given integer.
   static AppNotification* CreateNotification(int suffix) {
@@ -194,7 +220,8 @@ class AppNotificationManagerSyncTest : public testing::Test {
   scoped_ptr<TestingProfile> profile_;
   scoped_refptr<AppNotificationManager> model_;
 
-  TestChangeProcessor processor_;
+  scoped_ptr<TestChangeProcessor> sync_processor_;
+  scoped_ptr<SyncChangeProcessorDelegate> sync_processor_delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(AppNotificationManagerSyncTest);
 };
@@ -273,7 +300,7 @@ TEST_F(AppNotificationManagerSyncTest, ModelAssocBothEmpty) {
   model()->MergeDataAndStartSyncing(
       syncable::APP_NOTIFICATIONS,
       SyncDataList(),  // Empty.
-      processor());
+      PassProcessor());
 
   EXPECT_EQ(0U, model()->GetAllSyncData(syncable::APP_NOTIFICATIONS).size());
   EXPECT_EQ(0, processor()->change_list_size());
@@ -290,7 +317,7 @@ TEST_F(AppNotificationManagerSyncTest, ModelAssocModelEmpty) {
   model()->MergeDataAndStartSyncing(
       syncable::APP_NOTIFICATIONS,
       initial_data,
-      processor());
+      PassProcessor());
 
   EXPECT_EQ(4U, model()->GetAllSyncData(syncable::APP_NOTIFICATIONS).size());
   // Model should all of the initial sync data.
@@ -327,7 +354,7 @@ TEST_F(AppNotificationManagerSyncTest, ModelAssocBothNonEmptyNoOverlap) {
   model()->MergeDataAndStartSyncing(
       syncable::APP_NOTIFICATIONS,
       initial_data,
-      processor());
+      PassProcessor());
 
   EXPECT_EQ(6U, model()->GetAllSyncData(syncable::APP_NOTIFICATIONS).size());
   for (SyncDataList::const_iterator iter = initial_data.begin();
@@ -378,7 +405,7 @@ TEST_F(AppNotificationManagerSyncTest, ModelAssocBothNonEmptySomeOverlap) {
   model()->MergeDataAndStartSyncing(
       syncable::APP_NOTIFICATIONS,
       initial_data,
-      processor());
+      PassProcessor());
 
   EXPECT_EQ(6U, model()->GetAllSyncData(syncable::APP_NOTIFICATIONS).size());
   for (SyncDataList::const_iterator iter = initial_data.begin();
@@ -426,7 +453,7 @@ TEST_F(AppNotificationManagerSyncTest, ModelAssocBothNonEmptyTitleMismatch) {
   SyncError sync_error = model()->MergeDataAndStartSyncing(
       syncable::APP_NOTIFICATIONS,
       initial_data,
-      processor());
+      PassProcessor());
 
   EXPECT_TRUE(sync_error.IsSet());
   EXPECT_EQ(syncable::APP_NOTIFICATIONS, sync_error.type());
@@ -450,7 +477,7 @@ TEST_F(AppNotificationManagerSyncTest, ModelAssocBothNonEmptyMatchesLocal) {
   SyncError sync_error = model()->MergeDataAndStartSyncing(
       syncable::APP_NOTIFICATIONS,
       initial_data,
-      processor());
+      PassProcessor());
 
   EXPECT_TRUE(sync_error.IsSet());
   EXPECT_EQ(syncable::APP_NOTIFICATIONS, sync_error.type());
@@ -463,7 +490,7 @@ TEST_F(AppNotificationManagerSyncTest, ProcessSyncChangesEmptyModel) {
   model()->MergeDataAndStartSyncing(
       syncable::APP_NOTIFICATIONS,
       SyncDataList(),
-      processor());
+      PassProcessor());
 
   // Set up a bunch of ADDs.
   SyncChangeList changes;
@@ -489,7 +516,7 @@ TEST_F(AppNotificationManagerSyncTest, ProcessSyncChangesNonEmptyModel) {
   model()->MergeDataAndStartSyncing(
       syncable::APP_NOTIFICATIONS,
       SyncDataList(),
-      processor());
+      PassProcessor());
 
   // Some adds and some deletes.
   SyncChangeList changes;
@@ -514,7 +541,7 @@ TEST_F(AppNotificationManagerSyncTest, ProcessSyncChangesIgnoreBadAdd) {
   model()->MergeDataAndStartSyncing(
       syncable::APP_NOTIFICATIONS,
       SyncDataList(),
-      processor());
+      PassProcessor());
 
   // Some adds and some deletes.
   SyncChangeList changes;
@@ -537,7 +564,7 @@ TEST_F(AppNotificationManagerSyncTest, ProcessSyncChangesIgnoreBadDelete) {
   model()->MergeDataAndStartSyncing(
       syncable::APP_NOTIFICATIONS,
       SyncDataList(),
-      processor());
+      PassProcessor());
 
   // Some adds and some deletes.
   SyncChangeList changes;
@@ -560,7 +587,7 @@ TEST_F(AppNotificationManagerSyncTest, ProcessSyncChangesIgnoreBadUpdates) {
   model()->MergeDataAndStartSyncing(
       syncable::APP_NOTIFICATIONS,
       SyncDataList(),
-      processor());
+      PassProcessor());
 
   // Some adds and some deletes.
   SyncChangeList changes;
@@ -584,7 +611,7 @@ TEST_F(AppNotificationManagerSyncTest, ProcessSyncChangesEmptyModelWithMax) {
   model()->MergeDataAndStartSyncing(
       syncable::APP_NOTIFICATIONS,
       SyncDataList(),
-      processor());
+      PassProcessor());
   for (unsigned int i = 0;
        i < AppNotificationManager::kMaxNotificationPerApp * 2; i++) {
     SyncChangeList changes;
@@ -619,19 +646,19 @@ TEST_F(AppNotificationManagerSyncTest,
 
 // Stop syncing sets state correctly.
 TEST_F(AppNotificationManagerSyncTest, StopSyncing) {
-  EXPECT_FALSE(model()->sync_processor_);
+  EXPECT_FALSE(model()->sync_processor_.get());
   EXPECT_FALSE(model()->models_associated_);
 
   model()->MergeDataAndStartSyncing(
       syncable::APP_NOTIFICATIONS,
       SyncDataList(),
-      processor());
+      PassProcessor());
 
-  EXPECT_TRUE(model()->sync_processor_);
+  EXPECT_TRUE(model()->sync_processor_.get());
   EXPECT_TRUE(model()->models_associated_);
 
   model()->StopSyncing(syncable::APP_NOTIFICATIONS);
-  EXPECT_FALSE(model()->sync_processor_);
+  EXPECT_FALSE(model()->sync_processor_.get());
   EXPECT_FALSE(model()->models_associated_);
 }
 
@@ -640,7 +667,7 @@ TEST_F(AppNotificationManagerSyncTest, AddsGetsSynced) {
   model()->MergeDataAndStartSyncing(
       syncable::APP_NOTIFICATIONS,
       SyncDataList(),
-      processor());
+      PassProcessor());
 
   AppNotification* n1 = CreateNotification(1);
   model()->Add(n1);
@@ -677,7 +704,7 @@ TEST_F(AppNotificationManagerSyncTest, ClearAllGetsSynced) {
   model()->MergeDataAndStartSyncing(
       syncable::APP_NOTIFICATIONS,
       initial_data,
-      processor());
+      PassProcessor());
 
   model()->ClearAll(ext_id);