[Autofill] Dedupe profiles on each major version.

components/autofill/core/browser/personal_data_manager.cc

 // Copyright 2013 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 "components/autofill/core/browser/personal_data_manager.h"
 
 #include <stddef.h>
 #include <algorithm>
 #include <list>
 #include <map>
@@ skipping 21 matching lines @@
 #include "components/autofill/core/browser/phone_number_i18n.h"
 #include "components/autofill/core/browser/validation.h"
 #include "components/autofill/core/common/autofill_pref_names.h"
 #include "components/autofill/core/common/autofill_switches.h"
 #include "components/autofill/core/common/autofill_util.h"
 #include "components/prefs/pref_service.h"
 #include "components/signin/core/browser/account_tracker_service.h"
 #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"
 
 namespace autofill {
 namespace {
 
 using ::i18n::addressinput::AddressField;
 using ::i18n::addressinput::GetStreetAddressLinesAsSingleLine;
 using ::i18n::addressinput::STREET_ADDRESS;
 
@@ skipping 265 matching lines @@
       pending_profiles_query_ = 0;
     return;
   }
 
   switch (result->GetType()) {
     case AUTOFILL_PROFILES_RESULT:
       if (h == pending_profiles_query_) {
         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 query response.
+        if (!ApplyDedupingRoutine())
+          ApplyProfileUseDatesFix();
       } else {
         ReceiveLoadedDbValues(h, result, &pending_server_profiles_query_,
                               &server_profiles_);
 
         if (!server_profiles_.empty()) {
           std::string account_id = signin_manager_->GetAuthenticatedAccountId();
           base::string16 email =
               base::UTF8ToUTF16(
                   account_tracker_->GetAccountInfo(account_id).email);
 
@@ skipping 590 matching lines @@
     if (!matching_profile_found &&
         !new_profile.PrimaryValue(app_locale_).empty() &&
         existing_profile->SaveAdditionalInfo(new_profile, app_locale)) {
       // Unverified profiles should always be updated with the newer data,
       // whereas verified profiles should only ever be overwritten by verified
       // data.  If an automatically aggregated profile would overwrite a
       // verified profile, just drop it.
       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
       // the profile will have a very slightly newer time reflecting what's
       // actually stored in the database.
       existing_profile->set_modification_date(base::Time::Now());
     }
     merged_profiles->push_back(*existing_profile);
   }
 
@@ skipping 595 matching lines @@
   if (IsFeatureSubstringMatchEnabled()) {
     std::stable_sort(suggestions.begin(), suggestions.end(),
                      [](const Suggestion& a, const Suggestion& b) {
                        return a.match < b.match;
                      });
   }
 
   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))
     return;
 
   std::vector<AutofillProfile> profiles;
   bool has_changed_data = false;
   for (AutofillProfile* profile : web_profiles()) {
     if (profile->use_date() == base::Time()) {
       profile->set_use_date(base::Time::Now() - base::TimeDelta::FromDays(14));
       has_changed_data = true;
     }
     profiles.push_back(*profile);
   }
 
   // Set the pref so that this fix is never run again.
   pref_service_->SetBoolean(prefs::kAutofillProfileUseDatesFixed, true);
 
   if (has_changed_data)
     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.

[Roger McFarlane (Chromium), 2016/06/20 18:40:04]

nit:

...was already run...

or, better yet,

...has already been run...

[sebsg, 2016/06/20 19:12:19]

Done.

+  if (!(pref_service_->GetInteger(prefs::kAutofillLastVersionDeduped) <

[Mathieu, 2016/06/20 15:48:54]

I think I would prefer >=, it's cleaner.

[sebsg, 2016/06/20 19:12:19]

Done.

+        current_major_version))
+    return false;
+
+  std::vector<AutofillProfile*> existing_profiles = web_profiles_.get();
+  std::set<std::string> profile_guids_to_delete;

[Roger McFarlane (Chromium), 2016/06/20 18:40:04]

micro-optimizations:

* Why reference the profiles by guid? Why not a set of pointers?
* std::unordered_set

[sebsg, 2016/06/20 19:12:19]

Done.

+
+  DedupeProfiles(&existing_profiles, &profile_guids_to_delete);
+
+  // Set the pref to the current major version.
+  pref_service_->SetInteger(prefs::kAutofillLastVersionDeduped,

[Roger McFarlane (Chromium), 2016/06/20 18:40:04]

unless there is no possibility of failure beyond this point, do this at the end,
right before the refresh.

[sebsg, 2016/06/20 19:12:19]

Done.

+                            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;
+}
+
+void PersonalDataManager::DedupeProfiles(
+    std::vector<AutofillProfile*>* existing_profiles,
+    std::set<std::string>* profile_guids_to_delete) {

[Roger McFarlane (Chromium), 2016/06/20 18:40:04]

unordered_set, reserved existing_profiles.size()

?

[sebsg, 2016/06/20 19:12:19]

Done.

+  // 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(

[Roger McFarlane (Chromium), 2016/06/20 18:40:04]

move the log to the beginning... since it's not a part of the actual dedup algo.

[sebsg, 2016/06/20 19:12:19]

Done.

+      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 into another profile.
+    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()) {

[Roger McFarlane (Chromium), 2016/06/20 18:40:04]

this is redundant with the bounds in the enclosed for

[sebsg, 2016/06/20 19:12:19]

Done.

+      // 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. Also
+        // don't 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_)) {
+          // If the profiles are found to be similar, |profile_to_merge|'s non
+          // empty data will overwrite the |existing_profile|'s data.
+          // |existing_profile| will have added the complementary data from
+          // |profile_to_merge| and will keep the most relevant syntax for data
+          // present in both profiles.
+          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())

[Roger McFarlane (Chromium), 2016/06/20 18:40:04]

this check seems too late.

I think you need to check if existing_profile->IsVerified before trying to save
additional info into it, no?

[sebsg, 2016/06/20 19:12:19]

The |existing_profile| can (and should sometimes) be verified. If that's the
case, SaveAdditionnalInfo will not call OverwriteWith. So the verified profile
will not be changed but the similar |profile_to_merge| will be deleted. However,
|profile_to_merge| should never be verified, because verified profiles do not
merge into other profiles.

When entering the inner loop at line 1655 we are sure that |profile_to_merge| is
not verified. The only moment where it can become verified is right after the
assignation at line 1676. That's why that condition is there.

[Roger McFarlane (Chromium), 2016/06/20 19:18:36]

Acknowledged.

+              break;
+          }
+        }
+      }
+    }
+  }
+  AutofillMetrics::LogNumberOfProfilesRemovedDuringDedupe(
+      profile_guids_to_delete->size());
+}
+
 }  // namespace autofill