Retroactively addressed sky@'s comments from CL http://codereview.chromium.org/7566036/

chrome/browser/search_engines/template_url_service_sync_unittest.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/memory/scoped_ptr.h"
 #include "base/string_util.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/search_engines/template_url.h"
 #include "chrome/browser/search_engines/template_url_service.h"
@@ skipping 305 matching lines @@
   string16 original_turl_keyword = original_turl->keyword();
   model()->Add(original_turl);
 
   // Create a key that does not conflict with something in the model.
   scoped_ptr<TemplateURL> sync_turl(CreateTestTemplateURL("unique",
                                                           "http://new.com"));
   string16 sync_keyword = sync_turl->keyword();
   SyncChangeList changes;
 
   // No conflict, no TURLs changed, no changes.
-  EXPECT_FALSE(model()->ResolveSyncKeywordConflict(sync_turl.get(), changes));
+  EXPECT_FALSE(model()->ResolveSyncKeywordConflict(sync_turl.get(), &changes));
   EXPECT_EQ(original_turl_keyword, original_turl->keyword());
   EXPECT_EQ(sync_keyword, sync_turl->keyword());
   EXPECT_EQ(0U, changes.size());
 
   // Change sync keyword to something that conflicts, and make it older.
   // Conflict, sync keyword is uniquified, and a SyncChange is added.
   sync_turl->set_keyword(original_turl->keyword());
   sync_turl->set_last_modified(Time::FromTimeT(8999));
-  EXPECT_TRUE(model()->ResolveSyncKeywordConflict(sync_turl.get(), changes));
+  EXPECT_TRUE(model()->ResolveSyncKeywordConflict(sync_turl.get(), &changes));
   EXPECT_NE(sync_keyword, sync_turl->keyword());
   EXPECT_EQ(original_turl_keyword, original_turl->keyword());
   EXPECT_EQ(NULL, model()->GetTemplateURLForKeyword(sync_turl->keyword()));
   EXPECT_EQ(1U, changes.size());
   changes.clear();
 
   // Sync is newer. Original TemplateURL keyword is uniquified, no SyncChange
   // is added.
   sync_turl->set_keyword(original_turl->keyword());
   sync_keyword = sync_turl->keyword();
   sync_turl->set_last_modified(Time::FromTimeT(9001));
-  EXPECT_TRUE(model()->ResolveSyncKeywordConflict(sync_turl.get(), changes));
+  EXPECT_TRUE(model()->ResolveSyncKeywordConflict(sync_turl.get(), &changes));
   EXPECT_EQ(sync_keyword, sync_turl->keyword());
   EXPECT_NE(original_turl_keyword, original_turl->keyword());
   EXPECT_EQ(NULL, model()->GetTemplateURLForKeyword(sync_turl->keyword()));
   EXPECT_EQ(0U, changes.size());
 
   // Equal times. Same result as above. Sync left alone, original uniquified so
   // sync_turl can fit.
   sync_turl->set_keyword(original_turl->keyword());
   sync_keyword = sync_turl->keyword();
   // Note that we have to reset original_turl's last_modified time as it was
   // modified above.
   original_turl->set_last_modified(Time::FromTimeT(9000));
   sync_turl->set_last_modified(Time::FromTimeT(9000));
-  EXPECT_TRUE(model()->ResolveSyncKeywordConflict(sync_turl.get(), changes));
+  EXPECT_TRUE(model()->ResolveSyncKeywordConflict(sync_turl.get(), &changes));
   EXPECT_EQ(sync_keyword, sync_turl->keyword());
   EXPECT_NE(original_turl_keyword, original_turl->keyword());
   EXPECT_EQ(NULL, model()->GetTemplateURLForKeyword(sync_turl->keyword()));
   EXPECT_EQ(0U, changes.size());
 }
 
 TEST_F(TemplateURLServiceSyncTest, FindDuplicateOfSyncTemplateURL) {
   TemplateURL* original_turl =
       CreateTestTemplateURL("key1", "http://key1.com");
   model()->Add(original_turl);
@@ skipping 24 matching lines @@
 TEST_F(TemplateURLServiceSyncTest, MergeSyncAndLocalURLDuplicates) {
   TemplateURL* original_turl =
       CreateTestTemplateURL("key1", "http://key1.com", std::string(), 9000);
   model()->Add(original_turl);
   TemplateURL* sync_turl =
       CreateTestTemplateURL("key1", "http://key1.com", std::string(), 9001);
   SyncChangeList changes;
 
   // The sync TemplateURL is newer. It should replace the original TemplateURL.
   // Note that MergeSyncAndLocalURLDuplicates takes ownership of sync_turl.
-  model()->MergeSyncAndLocalURLDuplicates(sync_turl, original_turl, changes);
+  model()->MergeSyncAndLocalURLDuplicates(sync_turl, original_turl, &changes);
   const TemplateURL* result =
       model()->GetTemplateURLForKeyword(UTF8ToUTF16("key1"));
   ASSERT_TRUE(result);
   EXPECT_EQ(9001, result->last_modified().ToTimeT());
   EXPECT_EQ(0U, changes.size());
 
   // The sync TemplateURL is older. The existing TemplateURL should win and a
   // SyncChange should be added to the list.
   TemplateURL* sync_turl2 =
       CreateTestTemplateURL("key1", "http://key1.com", std::string(), 8999);
-  model()->MergeSyncAndLocalURLDuplicates(sync_turl2, sync_turl, changes);
+  model()->MergeSyncAndLocalURLDuplicates(sync_turl2, sync_turl, &changes);
   result = model()->GetTemplateURLForKeyword(UTF8ToUTF16("key1"));
   ASSERT_TRUE(result);
   EXPECT_EQ(9001, result->last_modified().ToTimeT());
   EXPECT_EQ(1U, changes.size());
 }
 
 TEST_F(TemplateURLServiceSyncTest, StartSyncEmpty) {
   model()->MergeDataAndStartSyncing(
       syncable::SEARCH_ENGINES,
       SyncDataList(),  // Empty.
@@ skipping 544 matching lines @@
   ASSERT_FALSE(error.IsSet());
 
   SyncChangeList changes;
   changes.push_back(CreateTestSyncChange(
       SyncChange::ACTION_UPDATE,
       CreateTestTemplateURL("newkeyword", "http://new.com", "key2")));
   processor()->set_erroneous(true);
   error = model()->ProcessSyncChanges(FROM_HERE, changes);
   EXPECT_TRUE(error.IsSet());
 }
+
+TEST_F(TemplateURLServiceSyncTest, MergeTwiceWithSameSyncData) {
+  // Ensure that a second merge with the same data as the first does not
+  // actually update the local data.
+  SyncDataList initial_data;
+  initial_data.push_back(CreateInitialSyncData().at(0));
+  TemplateURL* turl = TemplateURLService::CreateTemplateURLFromSyncData(
+      initial_data.at(0));
+  ASSERT_TRUE(turl);
+  turl->set_last_modified(Time::FromTimeT(10));  // earlier
+  model()->Add(turl);
+
+  SyncError error = model()->MergeDataAndStartSyncing(
+      syncable::SEARCH_ENGINES,
+      initial_data,
+      processor());
+  ASSERT_FALSE(error.IsSet());
+
+  // We should have updated the original TemplateURL with Sync's version.
+  // Keep a copy of it so we can compare it after we re-merge.
+  ASSERT_TRUE(model()->GetTemplateURLForGUID("key1"));
+  TemplateURL updated_turl(*model()->GetTemplateURLForGUID("key1"));
+  EXPECT_EQ(Time::FromTimeT(90), updated_turl.last_modified());
+
+  // Modify a single field of the initial data. This should not be updated in
+  // the second merge, as the last_modified timestamp remains the same.
+  scoped_ptr<TemplateURL> temp_turl(
+      TemplateURLService::CreateTemplateURLFromSyncData(initial_data.at(0)));
+  temp_turl->set_short_name(UTF8ToUTF16("SomethingDifferent"));
+  initial_data.clear();
+  initial_data.push_back(
+      TemplateURLService::CreateSyncDataFromTemplateURL(*temp_turl));
+
+  // Remerge the data again. This simulates shutting down and syncing again
+  // at a different time, but the cloud data has not changed.
+  model()->StopSyncing(syncable::SEARCH_ENGINES);
+  error = model()->MergeDataAndStartSyncing(syncable::SEARCH_ENGINES,
+                                            initial_data,
+                                            processor());
+  ASSERT_FALSE(error.IsSet());
+
+  // Check that the TemplateURL was not modified.
+  const TemplateURL* reupdated_turl = model()->GetTemplateURLForGUID("key1");
+  ASSERT_TRUE(reupdated_turl);
+  AssertEquals(updated_turl, *reupdated_turl);
+}