[Autofill] Run autofill-profile de-dupe after sync starts if sync is enabled.

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 d6c311f22c2a7d11bf4cf4b410de6cab85c11d23..9d980e772fb33eef194e69f71eb07c22c657e353 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/account_tracker_service.h"
 #include "components/signin/core/browser/signin_manager.h"
 #include "components/signin/core/common/signin_pref_names.h"
+#include "components/sync_driver/sync_service.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"
@@ -287,6 +288,7 @@ void PersonalDataManager::Init(scoped_refptr<AutofillWebDataService> database,
   LoadCreditCards();
 
   database_->AddObserver(this);
+  is_autofill_profile_dedupe_pending_ = IsAutofillProfileCleanupEnabled();
 }
 
 PersonalDataManager::~PersonalDataManager() {
@@ -299,6 +301,27 @@ PersonalDataManager::~PersonalDataManager() {
     database_->RemoveObserver(this);
 }
 
+void PersonalDataManager::OnSyncServiceConfigured(
+    sync_driver::SyncService* sync_service) {
+  // We want to know when, if at all, we need to run autofill profile de-
+  // duplication: now or after waiting until sync has started.
+  if (!is_autofill_profile_dedupe_pending_) {
+    // De-duplication isn't enabled.
+    return;
+  }
+
+  // If the sync service is configured to start and to sync autofill profiles,
+  // then we can just let the notification that sync has started trigger the
+  // de-duplication.
+  if (sync_service && sync_service->CanSyncStart() &&
+      sync_service->GetPreferredDataTypes().Has(syncer::AUTOFILL_PROFILE)) {
+    return;
+  }
+
+  // Otherwise, run the de-duplication now.
+  ApplyDedupingRoutine();
+}
+
 void PersonalDataManager::OnWebDataServiceRequestDone(
     WebDataServiceBase::Handle h,
     const WDTypedResult* result) {
@@ -326,10 +349,7 @@ void PersonalDataManager::OnWebDataServiceRequestDone(
         ReceiveLoadedDbValues(h, result, &pending_profiles_query_,
                               &web_profiles_);
         LogProfileCount();  // This only logs local profiles.
-        // Since these two routines both re-launch the database queries, don't
-        // run them on the same query response.
-        if (!ApplyDedupingRoutine())
-          ApplyProfileUseDatesFix();
+        ApplyProfileUseDatesFix();
       } else {
         ReceiveLoadedDbValues(h, result, &pending_server_profiles_query_,
                               &server_profiles_);
@@ -380,6 +400,9 @@ void PersonalDataManager::OnWebDataServiceRequestDone(
 }
 
 void PersonalDataManager::AutofillMultipleChanged() {
+  if (is_autofill_profile_dedupe_pending_)

[Nicolas Zea, 2016/07/12 20:04:47]

Could this short circuit the delay from sync you set above?

[Roger McFarlane (Chromium), 2016/07/12 21:23:26]

Not that I've seen

From what I can tell, this doesn't get called for the first time until the end
of MergeDataAndStartSyncing(). and if sync isn't active, it doesn't get called
at all.

That said, this is based on observational data. I don't have the full picture of
all the corner cases of how this works.

[Nicolas Zea, 2016/07/12 23:16:17]

I'm worried this might be a bit brittle. I think it would be better to have a
OnSyncMergeFinished method for any post-sync work you want to perform, and have
the MergeDataAndStartSyncing method call it directly.

+    ApplyDedupingRoutine();
+
   Refresh();
 }
 
@@ -1606,16 +1629,22 @@ void PersonalDataManager::ApplyProfileUseDatesFix() {
 }
 
 bool PersonalDataManager::ApplyDedupingRoutine() {
-  if (!IsAutofillProfileCleanupEnabled())
+  if (!is_autofill_profile_dedupe_pending_)
     return false;
 
-  int current_major_version = atoi(version_info::GetVersionNumber().c_str());
+  DCHECK(IsAutofillProfileCleanupEnabled());
+  is_autofill_profile_dedupe_pending_ = false;
 
   // Check if the deduping routine has already been run on this major version.
+  int current_major_version = atoi(version_info::GetVersionNumber().c_str());
   if (pref_service_->GetInteger(prefs::kAutofillLastVersionDeduped) >=
-      current_major_version)
+      current_major_version) {
+    VLOG(1)

[Nicolas Zea, 2016/07/12 20:04:47]

nit: did you mean to do DVLOG, rather than VLOG? (VLOG statements should
typically be limited to debugging existing issues, as they introduce unnecessary
strings into the binary).

[Roger McFarlane (Chromium), 2016/07/12 21:23:26]

Yes, thanks!

Done.

+        << "Autofill profile de-duplication already performed for this version";
     return false;
+  }
 
+  VLOG(1) << "Starting autofill profile de-duplication.";
   std::vector<AutofillProfile*> existing_profiles = web_profiles_.get();
   std::unordered_set<AutofillProfile*> profiles_to_delete;
   profiles_to_delete.reserve(existing_profiles.size());