[Autofill] Dedupe profiles on each major version.

components/autofill/core/browser/personal_data_manager.cc

Index: components/autofill/core/browser/personal_data_manager.cc
diff --git a/components/autofill/core/browser/personal_data_manager.cc b/components/autofill/core/browser/personal_data_manager.cc
index 525f808c1b0bcd823eb9fbc0ae0109668c43ff52..be42fb731472eb9c98ff477cf0086331064854ef 100644
--- a/components/autofill/core/browser/personal_data_manager.cc
+++ b/components/autofill/core/browser/personal_data_manager.cc
@@ -39,6 +39,7 @@
 #include "components/signin/core/browser/signin_manager.h"
 #include "components/signin/core/common/signin_pref_names.h"
 #include "components/variations/variations_associated_data.h"
+#include "components/version_info/version_info.h"
 #include "third_party/libaddressinput/src/cpp/include/libaddressinput/address_data.h"
 #include "third_party/libaddressinput/src/cpp/include/libaddressinput/address_formatter.h"
 
@@ -324,7 +325,10 @@ void PersonalDataManager::OnWebDataServiceRequestDone(
         ReceiveLoadedDbValues(h, result, &pending_profiles_query_,
                               &web_profiles_);
         LogProfileCount();  // This only logs local profiles.
-        ApplyProfileUseDatesFix();
+        // Since these two routines both re-launch the database queries, don't
+        // run them on the same on the same query response.

[Mathieu, 2016/06/20 12:10:07]

fix sentence

[sebsg, 2016/06/20 14:57:18]

Done.

+        if (ApplyDedupingRoutine())

[Mathieu, 2016/06/20 12:10:07]

do you mean if (!ApplyDedupingRoutine())?

[sebsg, 2016/06/20 14:57:18]

Done.

+          ApplyProfileUseDatesFix();
       } else {
         ReceiveLoadedDbValues(h, result, &pending_server_profiles_query_,
                               &server_profiles_);
@@ -935,12 +939,6 @@ std::string PersonalDataManager::MergeProfile(
       matching_profile_found = true;
       guid = existing_profile->guid();
 
-      if (IsAutofillProfileCleanupEnabled()) {
-        // Look for duplicates of |existing_profile| to merge into.
-        FindMergeAndDeleteDuplicateProfiles(existing_profiles,
-                                            existing_profile);
-      }
-
       // We set the modification date so that immediate requests for profiles
       // will properly reflect the fact that this profile has been modified
       // recently. After writing to the database and refreshing the local copies
@@ -1556,54 +1554,6 @@ std::vector<Suggestion> PersonalDataManager::GetSuggestionsForCards(
   return suggestions;
 }
 
-void PersonalDataManager::FindMergeAndDeleteDuplicateProfiles(
-    const std::vector<AutofillProfile*>& existing_profiles,
-    AutofillProfile* profile_to_merge) {
-  std::vector<std::string> profile_guids_to_delete;
-
-  FindAndMergeDuplicateProfiles(existing_profiles, profile_to_merge,
-                                &profile_guids_to_delete);
-
-  // Delete the duplicate profiles.
-  for (const std::string& profile_guid_to_delete : profile_guids_to_delete) {
-    RemoveByGUID(profile_guid_to_delete);
-  }
-}
-
-void PersonalDataManager::FindAndMergeDuplicateProfiles(
-    const std::vector<AutofillProfile*>& existing_profiles,
-    AutofillProfile* profile_to_merge,
-    std::vector<std::string>* profile_guids_to_delete) {
-  AutofillMetrics::LogNumberOfProfilesConsideredForDedupe(
-      existing_profiles.size());
-  for (AutofillProfile* existing_profile : existing_profiles) {
-    // Don't try to merge a profile with itself or with any profile with a
-    // different PrimaryValue.
-    if (existing_profile->guid() != profile_to_merge->guid() &&
-        AutofillProfile::CanonicalizeProfileString(
-            existing_profile->PrimaryValue(app_locale_)) ==
-            AutofillProfile::CanonicalizeProfileString(
-                profile_to_merge->PrimaryValue(app_locale_))) {
-      if (existing_profile->SaveAdditionalInfo(*profile_to_merge,
-                                               app_locale_)) {
-        // Since |profile_to_merge| was a duplicate of |existing_profile| and
-        // was merged sucessfully, it can now be deleted. The only exception is
-        // if |profile_to_merge| is verified and |existing_profile| is not.
-        // Verified profiles only merge with other verified profiles.
-        if (!profile_to_merge->IsVerified() || existing_profile->IsVerified())
-          profile_guids_to_delete->push_back(profile_to_merge->guid());
-
-        // Now try to merge the new resulting profile with the rest of the
-        // existing profiles.
-        profile_to_merge = existing_profile;
-      }
-    }
-  }
-
-  AutofillMetrics::LogNumberOfProfilesRemovedDuringDedupe(
-      profile_guids_to_delete->size());
-}
-
 void PersonalDataManager::ApplyProfileUseDatesFix() {
   // Don't run if the fix has already been applied.
   if (pref_service_->GetBoolean(prefs::kAutofillProfileUseDatesFixed))
@@ -1626,4 +1576,114 @@ void PersonalDataManager::ApplyProfileUseDatesFix() {
     SetProfiles(&profiles);
 }
 
+bool PersonalDataManager::ApplyDedupingRoutine() {
+  if (!IsAutofillProfileCleanupEnabled())
+    return false;
+
+  int current_major_version = atoi(version_info::GetVersionNumber().c_str());
+
+  // Check if the deduping routine was run on this major version.
+  if (pref_service_->GetInteger(prefs::kAutofillLastVersionDeduped) <

[Mathieu, 2016/06/20 12:10:07]

let's return early for the inverse condition here

[sebsg, 2016/06/20 14:57:17]

Done.

+      current_major_version) {
+    std::vector<AutofillProfile*> existing_profiles = web_profiles_.get();
+    std::set<std::string> profile_guids_to_delete;
+
+    DedupeProfiles(&existing_profiles, &profile_guids_to_delete);
+
+    // Set the pref to the current major version.
+    pref_service_->SetInteger(prefs::kAutofillLastVersionDeduped,
+                              current_major_version);
+
+    // Apply the changes to the database.
+    for (AutofillProfile* profile : existing_profiles) {
+      // If the profile was set to be deleted, remove it from the database.
+      if (profile_guids_to_delete.count(profile->guid())) {
+        database_->RemoveAutofillProfile(profile->guid());
+      } else {
+        // Otherwise, update the profile in the database.
+        database_->UpdateAutofillProfile(*profile);
+      }
+    }
+
+    // Refresh the local cache and send notifications to observers.
+    Refresh();
+
+    return true;
+  }
+
+  return false;
+}
+
+void PersonalDataManager::DedupeProfiles(
+    std::vector<AutofillProfile*>* existing_profiles,
+    std::set<std::string>* profile_guids_to_delete) {
+  // Sort the profiles by frecency with all the verified profiles at the end.
+  // That way the most relevant profiles will get merged into the less relevant
+  // profiles, which keeps the syntax of the most relevant profiles data.
+  // Verified profiles are put at the end because they do not merge into other
+  // profiles, so the loop can be stopped when we reach those. However they need
+  // to be in the vector because an unverified profile trying to merge into a
+  // similar verified profile will be discarded.
+  base::Time comparison_time = base::Time::Now();
+  std::sort(existing_profiles->begin(), existing_profiles->end(),
+            [comparison_time](const AutofillDataModel* a,
+                              const AutofillDataModel* b) {
+              if (a->IsVerified() != b->IsVerified())
+                return !a->IsVerified();
+              return a->CompareFrecency(b, comparison_time);
+            });
+
+  AutofillMetrics::LogNumberOfProfilesConsideredForDedupe(
+      existing_profiles->size());
+
+  for (size_t i = 0; i < existing_profiles->size(); ++i) {
+    AutofillProfile* profile_to_merge = (*existing_profiles)[i];
+
+    // If the profile was set to be deleted, skip it. It has already been
+    // merged.

[Mathieu, 2016/06/20 12:10:07]

nit: "merged into another profile".

[sebsg, 2016/06/20 14:57:18]

Done.

+    if (profile_guids_to_delete->count(profile_to_merge->guid()))
+      continue;
+
+    // If we have reached the verified profiles, stop trying to merge. Verified
+    // profiles do not get merged.
+    if (profile_to_merge->IsVerified())
+      break;
+
+    // If we have not reached the last profile.
+    if (i + 1 < existing_profiles->size()) {
+      // Try to merge |profile_to_merge| with all the less relevant
+      // |existing_profiles|.
+      for (size_t j = i + 1; j < existing_profiles->size(); ++j) {
+        // Don't try to merge a profile that was already set for deletion
+        // because it means that the profile has already been merged. Also don't

[Mathieu, 2016/06/20 12:10:07]

this line is clear enough, I think we can remove.

[sebsg, 2016/06/20 14:57:18]

Done.

+        // try to merge with profiles with a different PrimaryValue because they
+        // are not similar.
+        AutofillProfile* existing_profile = (*existing_profiles)[j];
+        if (!profile_guids_to_delete->count(existing_profile->guid()) &&
+            existing_profile->PrimaryValue(app_locale_) ==
+                profile_to_merge->PrimaryValue(app_locale_)) {
+          // Try to merge the two profiles.

[Mathieu, 2016/06/20 12:10:07]

Can you be more specific? I thought |profile_to_merge| was the most relevant due
to frecency, and now it's being deleted and the less relevant is kept. Expand a
little bit on what SaveAdditionalInfo does with the most relevant information,
so that the reader is not confused.

[sebsg, 2016/06/20 14:57:18]

Done.

+          if (existing_profile->SaveAdditionalInfo(*profile_to_merge,
+                                                   app_locale_)) {
+            // Since |profile_to_merge| was a duplicate of |existing_profile|
+            // and was merged sucessfully, it can now be deleted.
+            profile_guids_to_delete->insert(profile_to_merge->guid());
+
+            // Now try to merge the new resulting profile with the rest of the
+            // existing profiles.
+            profile_to_merge = existing_profile;
+
+            // Verified profiles do not get merged. Save some time by not
+            // trying.
+            if (profile_to_merge->IsVerified())
+              break;
+          }
+        }
+      }
+    }
+  }
+  AutofillMetrics::LogNumberOfProfilesRemovedDuringDedupe(
+      profile_guids_to_delete->size());
+}
+
 }  // namespace autofill