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

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.h"
 #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/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"
 #include "third_party/libaddressinput/src/cpp/include/libaddressinput/address_formatter.h"
 
 namespace autofill {
 namespace {
 
 using ::i18n::addressinput::AddressField;
 using ::i18n::addressinput::GetStreetAddressLinesAsSingleLine;
@@ skipping 228 matching lines @@
     AutofillMetrics::LogIsAutofillEnabledAtStartup(IsAutofillEnabled());
 
   // WebDataService may not be available in tests.
   if (!database_.get())
     return;
 
   LoadProfiles();
   LoadCreditCards();
 
   database_->AddObserver(this);
+  is_autofill_profile_dedupe_pending_ = IsAutofillProfileCleanupEnabled();
 }
 
 PersonalDataManager::~PersonalDataManager() {
   CancelPendingQuery(&pending_profiles_query_);
   CancelPendingQuery(&pending_server_profiles_query_);
   CancelPendingQuery(&pending_creditcards_query_);
   CancelPendingQuery(&pending_server_creditcards_query_);
 
   if (database_.get())
     database_->RemoveObserver(this);
 }
 
+void PersonalDataManager::OnSyncServiceInitialized(
+    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) {
   DCHECK(pending_profiles_query_ || pending_server_profiles_query_ ||
          pending_creditcards_query_ || pending_server_creditcards_query_);
 
   // TODO(robliao): Remove ScopedTracker below once https://crbug.com/422460 is
   // fixed.
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "422460 PersonalDataManager::OnWebDataServiceRequestDone"));
 
   if (!result) {
     // Error from the web database.
     if (h == pending_creditcards_query_)
       pending_creditcards_query_ = 0;
     else if (h == pending_profiles_query_)
       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.
-        // 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_);
 
         if (!server_profiles_.empty()) {
           std::string account_id = signin_manager_->GetAuthenticatedAccountId();
           base::string16 email =
               base::UTF8ToUTF16(
                   account_tracker_->GetAccountInfo(account_id).email);
 
@@ skipping 33 matching lines @@
       pending_server_creditcards_query_ == 0) {
     is_data_loaded_ = true;
     NotifyPersonalDataChanged();
   }
 }
 
 void PersonalDataManager::AutofillMultipleChanged() {
   Refresh();
 }
 
+void PersonalDataManager::SyncStarted(syncer::ModelType model_type) {
+  if (model_type == syncer::AUTOFILL_PROFILE &&
+      is_autofill_profile_dedupe_pending_) {
+    ApplyDedupingRoutine();
+  }
+}
+
 void PersonalDataManager::AddObserver(PersonalDataManagerObserver* observer) {
   observers_.AddObserver(observer);
 }
 
 void PersonalDataManager::RemoveObserver(
     PersonalDataManagerObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
 bool PersonalDataManager::ImportFormData(
@@ skipping 1203 matching lines @@
   }
 
   // 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())
+  if (!is_autofill_profile_dedupe_pending_)

[Mathieu, 2016/07/18 14:53:19]

are these two lines necessary? Seems like callers have already checked before we
got here.

[Roger McFarlane (Chromium), 2016/07/18 15:29:18]

there's a slight mismatch between how the tests expect this to be called and how
it is actually called.

The tests treat the function as if it was called "MaybeApplyDedupingRoutine" but
the real call sites subsume the "maybe" and call it only when it will actually
be applied.

In a follow-on CL, I'll fix the tests and turn this into a DCHECK.

     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) {
+    DVLOG(1)
+        << "Autofill profile de-duplication already performed for this version";
     return false;
+  }
 
+  DVLOG(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());
 
   DedupeProfiles(&existing_profiles, &profiles_to_delete);
 
   // 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 (profiles_to_delete.count(profile)) {
@@ skipping 83 matching lines @@
         if (profile_to_merge->IsVerified())
           break;
       }
     }
   }
   AutofillMetrics::LogNumberOfProfilesRemovedDuringDedupe(
       profiles_to_delete->size());
 }
 
 }  // namespace autofill