Revert 60083 - Fix bug "Autofill: A large number of unnecessary sync updates

generated when no syncable action was performed".

Reason for revert: breaks sync integration tests.

BUG=54461,56305
TEST=bug repro steps

Review URL: http://codereview.chromium.org/3426007

TBR=nick@chromium.org
Review URL: http://codereview.chromium.org/3436025

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

5 files changed, 68 insertions, 116 deletions

diff --git a/chrome/browser/sync/glue/autofill_change_processor.cc b/chrome/browser/sync/glue/autofill_change_processor.cc
index 629c745..149d266 100644
--- a/chrome/browser/sync/glue/autofill_change_processor.cc
+++ b/chrome/browser/sync/glue/autofill_change_processor.cc
@@ -78,44 +78,34 @@ void AutofillChangeProcessor::Observe(NotificationType type,
   }
 }
 
-void AutofillChangeProcessor::ChangeProfileLabelIfAlreadyTaken(
-    sync_api::BaseTransaction* trans,
-    const string16& pre_update_label,
-    AutoFillProfile* profile,
+void AutofillChangeProcessor::HandleMoveAsideIfNeeded(
+    sync_api::BaseTransaction* trans, AutoFillProfile* profile,
     std::string* tag) {
   DCHECK_EQ(AutofillModelAssociator::ProfileLabelToTag(profile->Label()),
                                                        *tag);
   sync_api::ReadNode read_node(trans);
-  if (pre_update_label != profile->Label() &&
-      read_node.InitByClientTagLookup(syncable::AUTOFILL, *tag)) {
+  if (read_node.InitByClientTagLookup(syncable::AUTOFILL, *tag)) {
     // Handle the edge case of duplicate labels.
-    string16 new_label(AutofillModelAssociator::MakeUniqueLabel(
-        profile->Label(), pre_update_label, trans));
-    if (new_label.empty()) {
+    string16 label(AutofillModelAssociator::MakeUniqueLabel(profile->Label(),
+        trans));
+    if (label.empty()) {
       error_handler()->OnUnrecoverableError(FROM_HERE,
           "No unique label; can't move aside");
       return;
     }
-    OverrideProfileLabel(new_label, profile, tag);
-  }
-}
+    tag->assign(AutofillModelAssociator::ProfileLabelToTag(label));
 
-void AutofillChangeProcessor::OverrideProfileLabel(
-    const string16& new_label,
-    AutoFillProfile* profile_to_update,
-    std::string* tag_to_update) {
-  tag_to_update->assign(AutofillModelAssociator::ProfileLabelToTag(new_label));
+    profile->set_label(label);
+    if (!web_database_->UpdateAutoFillProfile(*profile)) {
+      std::string err = "Failed to overwrite label for node ";
+      err += UTF16ToUTF8(label);
+      error_handler()->OnUnrecoverableError(FROM_HERE, err);
+      return;
+    }
 
-  profile_to_update->set_label(new_label);
-  if (!web_database_->UpdateAutoFillProfile(*profile_to_update)) {
-    std::string err = "Failed to overwrite label for node ";
-    err += UTF16ToUTF8(new_label);
-    error_handler()->OnUnrecoverableError(FROM_HERE, err);
-    return;
+    // Notify the PersonalDataManager that it's out of date.
+    PostOptimisticRefreshTask();
   }
-
-  // Notify the PersonalDataManager that it's out of date.
-  PostOptimisticRefreshTask();
 }
 
 void AutofillChangeProcessor::PostOptimisticRefreshTask() {
@@ -147,24 +137,24 @@ void AutofillChangeProcessor::ObserveAutofillProfileChanged(
     case AutofillProfileChange::ADD: {
       scoped_ptr<AutoFillProfile> clone(
           static_cast<AutoFillProfile*>(change->profile()->Clone()));
-      DCHECK_EQ(clone->Label(), change->key());
-      ChangeProfileLabelIfAlreadyTaken(trans, string16(), clone.get(), &tag);
+      DCHECK_EQ(AutofillModelAssociator::ProfileLabelToTag(clone->Label()),
+                tag);
+      HandleMoveAsideIfNeeded(trans, clone.get(), &tag);
       AddAutofillProfileSyncNode(trans, autofill_root, tag, clone.get());
       break;
     }
     case AutofillProfileChange::UPDATE: {
       scoped_ptr<AutoFillProfile> clone(
           static_cast<AutoFillProfile*>(change->profile()->Clone()));
-      std::string pre_update_tag = AutofillModelAssociator::ProfileLabelToTag(
-          change->pre_update_label());
-      DCHECK_EQ(clone->Label(), change->key());
       sync_api::WriteNode sync_node(trans);
-      ChangeProfileLabelIfAlreadyTaken(trans, change->pre_update_label(),
-                                       clone.get(), &tag);
-      if (pre_update_tag != tag) {
-        // If the label changes, replace the node instead of updating it.
-        RemoveSyncNode(pre_update_tag, trans);
-        AddAutofillProfileSyncNode(trans, autofill_root, tag, clone.get());
+      if (change->pre_update_label() != change->profile()->Label()) {
+        // A re-labelling: we need to remove + add on the sync side.
+        RemoveSyncNode(AutofillModelAssociator::ProfileLabelToTag(
+            change->pre_update_label()), trans);
+        // Watch out! Could be relabelling to an existing label!
+        HandleMoveAsideIfNeeded(trans, clone.get(), &tag);
+        AddAutofillProfileSyncNode(trans, autofill_root, tag,
+                                   clone.get());
         return;
       }
       int64 sync_id = model_associator_->GetSyncIdFromChromeId(tag);
@@ -178,7 +168,7 @@ void AutofillChangeProcessor::ObserveAutofillProfileChanged(
               "Autofill node lookup failed.");
           return;
         }
-        WriteAutofillProfile(*clone.get(), &sync_node);
+        WriteAutofillProfile(*change->profile(), &sync_node);
       }
       break;
     }
diff --git a/chrome/browser/sync/glue/autofill_change_processor.h b/chrome/browser/sync/glue/autofill_change_processor.h
index bcc2d88..a9af80e 100644
--- a/chrome/browser/sync/glue/autofill_change_processor.h
+++ b/chrome/browser/sync/glue/autofill_change_processor.h
@@ -107,22 +107,10 @@ class AutofillChangeProcessor : public ChangeProcessor,
   // This method should be called on an ADD notification from the chrome model.
   // |tag| contains the unique sync client tag identifier for |profile|, which
   // is derived from the profile label using ProfileLabelToTag.
-  // |existing_unique_label| is the current label of the object, if any; this
-  // is an allowed value, because it's taken by the item in question.
-  // For new items, set |existing_unique_label| to the empty string.
-  void ChangeProfileLabelIfAlreadyTaken(
-      sync_api::BaseTransaction* trans,
-      const string16& existing_unique_label,
-      AutoFillProfile* profile,
+  void HandleMoveAsideIfNeeded(
+      sync_api::BaseTransaction* trans, AutoFillProfile* profile,
       std::string* tag);
 
-  // Reassign the label of the profile, write this back to the web database,
-  // and update |tag| with the tag corresponding to the new label.
-  void OverrideProfileLabel(
-      const string16& new_label,
-      AutoFillProfile* profile_to_update,
-      std::string* tag_to_update);
-
   // Helper to create a sync node with tag |tag|, storing |profile| as
   // the node's AutofillSpecifics.
   void AddAutofillProfileSyncNode(
diff --git a/chrome/browser/sync/glue/autofill_model_associator.cc b/chrome/browser/sync/glue/autofill_model_associator.cc
index f2ed463..c4a83ca 100644
--- a/chrome/browser/sync/glue/autofill_model_associator.cc
+++ b/chrome/browser/sync/glue/autofill_model_associator.cc
@@ -125,7 +125,7 @@ bool AutofillModelAssociator::TraverseAndAssociateChromeAutoFillProfiles(
       int64 sync_id = node.GetId();
       if (id_map_.find(tag) != id_map_.end()) {
         // We just looked up something we already associated.  Move aside.
-        label = MakeUniqueLabel(label, string16(), write_trans);
+        label = MakeUniqueLabel(label, write_trans);
         if (label.empty()) {
           return false;
         }
@@ -159,18 +159,11 @@ bool AutofillModelAssociator::TraverseAndAssociateChromeAutoFillProfiles(
 
 // static
 string16 AutofillModelAssociator::MakeUniqueLabel(
-    const string16& non_unique_label,
-    const string16& existing_unique_label,
-    sync_api::BaseTransaction* trans) {
-  if (non_unique_label == existing_unique_label) {
-    return existing_unique_label;
-  }
+    const string16& non_unique_label, sync_api::BaseTransaction* trans) {
   int unique_id = 1;  // Priming so we start by appending "2".
   while (unique_id++ < kMaxNumAttemptsToFindUniqueLabel) {
     string16 suffix(base::IntToString16(unique_id));
     string16 unique_label = non_unique_label + suffix;
-    if (unique_label == existing_unique_label)
-      return unique_label;  // We'll use the one we already have.
     sync_api::ReadNode node(trans);
     if (node.InitByClientTagLookup(syncable::AUTOFILL,
                                    ProfileLabelToTag(unique_label))) {
diff --git a/chrome/browser/sync/glue/autofill_model_associator.h b/chrome/browser/sync/glue/autofill_model_associator.h
index bc07bc3..07c3a58 100644
--- a/chrome/browser/sync/glue/autofill_model_associator.h
+++ b/chrome/browser/sync/glue/autofill_model_associator.h
@@ -121,14 +121,7 @@ class AutofillModelAssociator
   // Returns sync service instance.
   ProfileSyncService* sync_service() { return sync_service_; }
 
-  // Compute and apply suffix to a label so that the resulting label is
-  // unique in the sync database.
-  // |new_non_unique_label| is the colliding label which is to be uniquified.
-  // |existing_unique_label| is the current label of the object, if any; this
-  // is treated as a unique label even if colliding.  If no such label is
-  // available, |existing_unique_label| may be empty.
-  static string16 MakeUniqueLabel(const string16& new_non_unique_label,
-                                  const string16& existing_unique_label,
+  static string16 MakeUniqueLabel(const string16& non_unique_label,
                                   sync_api::BaseTransaction* trans);
 
  private:
diff --git a/chrome/browser/sync/profile_sync_service_autofill_unittest.cc b/chrome/browser/sync/profile_sync_service_autofill_unittest.cc
index 3691905..d0884cc 100644
--- a/chrome/browser/sync/profile_sync_service_autofill_unittest.cc
+++ b/chrome/browser/sync/profile_sync_service_autofill_unittest.cc
@@ -49,7 +49,6 @@ using testing::DoDefault;
 using testing::ElementsAre;
 using testing::Eq;
 using testing::Invoke;
-using testing::Mock;
 using testing::Return;
 using testing::SaveArg;
 using testing::SetArgumentPointee;
@@ -842,19 +841,21 @@ TEST_F(ProfileSyncServiceAutofillTest, ProcessUserChangeUpdateProfileRelabel) {
 
 TEST_F(ProfileSyncServiceAutofillTest,
        ProcessUserChangeUpdateProfileRelabelConflict) {
-  std::vector<AutoFillProfile*> native_profiles;
-  native_profiles.push_back(new AutoFillProfile(string16(), 0));
-  native_profiles.push_back(new AutoFillProfile(string16(), 0));
-  autofill_unittest::SetProfileInfo(native_profiles[0],
+  AutoFillProfile* native_profile = new AutoFillProfile(string16(), 0);
+  autofill_unittest::SetProfileInfo(native_profile,
       "Billing", "Josephine", "Alicia", "Saenz",
       "joewayne@me.xyz", "Fox", "1212 Center.", "Bld. 5", "Orlando", "FL",
       "32801", "US", "19482937549", "13502849239");
-  autofill_unittest::SetProfileInfo(native_profiles[1],
+  AutoFillProfile* native_profile2 = new AutoFillProfile(string16(), 0);
+  autofill_unittest::SetProfileInfo(native_profile2,
       "ExistingLabel", "Marion", "Mitchell", "Morrison",
       "johnwayne@me.xyz", "Fox", "123 Zoo St.", "unit 5", "Hollywood", "CA",
       "91601", "US", "12345678910", "01987654321");
-  AutoFillProfile marion(*native_profiles[1]);
-  AutoFillProfile josephine(*native_profiles[0]);
+  std::vector<AutoFillProfile*> native_profiles;
+  native_profiles.push_back(native_profile);
+  native_profiles.push_back(native_profile2);
+  AutoFillProfile marion(*native_profile2);
+  AutoFillProfile josephine_update(*native_profile);
 
   EXPECT_CALL(web_database_, GetAllAutofillEntries(_)). WillOnce(Return(true));
   EXPECT_CALL(web_database_, GetAutoFillProfiles(_)).
@@ -863,45 +864,32 @@ TEST_F(ProfileSyncServiceAutofillTest,
   CreateRootTask task(this, syncable::AUTOFILL);
   StartSyncService(&task, false);
   ASSERT_TRUE(task.success());
-  MessageLoop::current()->RunAllPending();
-  Mock::VerifyAndClearExpectations(&web_database_);
-  native_profiles.clear();  // Contents freed.
-
-  // Update josephine twice with marion's label.  The second time ought to be
-  // idempotent, settling on the same name and not triggering a sync upload.
-  for (int pass = 0; pass < 2; ++pass) {
-    AutoFillProfile josephine_update(josephine);
-    josephine_update.set_label(ASCIIToUTF16("ExistingLabel"));
-
-    AutoFillProfile relabelled_profile;
-    EXPECT_CALL(web_database_, UpdateAutoFillProfile(
-        ProfileMatchesExceptLabel(josephine_update))).
-        WillOnce(DoAll(SaveArg<0>(&relabelled_profile), Return(true)));
-    EXPECT_CALL(*personal_data_manager_, Refresh());
-
-    AutofillProfileChange change(AutofillProfileChange::UPDATE,
-                                 josephine_update.Label(), &josephine_update,
-                                 josephine.Label());
-    scoped_refptr<ThreadNotifier> notifier = new ThreadNotifier(&db_thread_);
-    notifier->Notify(NotificationType::AUTOFILL_PROFILE_CHANGED,
-                     Source<WebDataService>(web_data_service_.get()),
-                     Details<AutofillProfileChange>(&change));
-    MessageLoop::current()->RunAllPending();  // Run the Refresh task.
-    Mock::VerifyAndClearExpectations(&web_database_);
-
-    std::vector<AutofillEntry> new_sync_entries;
-    std::vector<AutoFillProfile> new_sync_profiles;
-    ASSERT_TRUE(GetAutofillEntriesFromSyncDB(&new_sync_entries,
-                                             &new_sync_profiles));
-    ASSERT_EQ(2U, new_sync_profiles.size());
-    marion.set_unique_id(0);  // The sync DB doesn't store IDs.
-    EXPECT_EQ(marion, new_sync_profiles[1]);
-    EXPECT_TRUE(ProfilesMatchExceptLabelImpl(josephine_update,
-                                             new_sync_profiles[0]));
-    EXPECT_EQ(ASCIIToUTF16("ExistingLabel2"), new_sync_profiles[0].Label());
-    EXPECT_EQ(ASCIIToUTF16("ExistingLabel2"), relabelled_profile.Label());
-    josephine = relabelled_profile;
-  }
+
+  josephine_update.set_label(ASCIIToUTF16("ExistingLabel"));
+  AutoFillProfile relabelled_profile;
+  EXPECT_CALL(web_database_, UpdateAutoFillProfile(
+      ProfileMatchesExceptLabel(josephine_update))).
+      WillOnce(DoAll(SaveArg<0>(&relabelled_profile), Return(true)));
+  EXPECT_CALL(*personal_data_manager_, Refresh());
+
+  AutofillProfileChange change(AutofillProfileChange::UPDATE,
+                               josephine_update.Label(), &josephine_update,
+                               ASCIIToUTF16("Billing"));
+  scoped_refptr<ThreadNotifier> notifier = new ThreadNotifier(&db_thread_);
+  notifier->Notify(NotificationType::AUTOFILL_PROFILE_CHANGED,
+                   Source<WebDataService>(web_data_service_.get()),
+                   Details<AutofillProfileChange>(&change));
+
+  std::vector<AutofillEntry> new_sync_entries;
+  std::vector<AutoFillProfile> new_sync_profiles;
+  ASSERT_TRUE(GetAutofillEntriesFromSyncDB(&new_sync_entries,
+                                           &new_sync_profiles));
+  ASSERT_EQ(2U, new_sync_profiles.size());
+  marion.set_unique_id(0);  // The sync DB doesn't store IDs.
+  EXPECT_EQ(marion, new_sync_profiles[1]);
+  EXPECT_TRUE(ProfilesMatchExceptLabelImpl(josephine_update,
+                                           new_sync_profiles[0]));
+  EXPECT_EQ(new_sync_profiles[0].Label(), relabelled_profile.Label());
 }
 
 TEST_F(ProfileSyncServiceAutofillTest, ProcessUserChangeRemoveEntry) {