[autofill] Move fixups to be invoked after initial database load

Bug: 868533
Change-Id: Ieb2d140b7293de20e6c53f6ab497308b4df7dc0f
Reviewed-on: https://chromium-review.googlesource.com/1153665
Commit-Queue: Roger McFarlane <rogerm@chromium.org>
Commit-Queue: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579414}

components/autofill/core/browser/personal_data_manager.cc

@@ -520,18 +520,6 @@ void PersonalDataManager::Shutdown() {
 
 void PersonalDataManager::OnSyncServiceInitialized(
     syncer::SyncService* sync_service) {
-  // If the sync service is not enabled for autofill address profiles then run
-  // address cleanup/startup code here. Otherwise, defer until after sync has
-  // started.
-  if (!IsSyncEnabledFor(sync_service, syncer::AUTOFILL_PROFILE))
-    ApplyAddressFixesAndCleanups();
-
-  // Similarly, if the sync service is not enabled for autofill credit cards
-  // then run credit card address cleanup/startup code here. Otherwise, defer
-  // until after sync has started.
-  if (!IsSyncEnabledFor(sync_service, syncer::AUTOFILL_WALLET_DATA))
-    ApplyCardFixesAndCleanups();
-
   if (sync_service_ != sync_service) {
     // Before the sync service pointer gets changed, remove the observer.
     if (sync_service_)
@@ -633,9 +621,24 @@ void PersonalDataManager::OnWebDataServiceRequestDone(
       pending_server_profiles_query_ == 0 &&
       pending_server_creditcards_query_ == 0 &&
       database_helper_->GetServerDatabase()) {
+    // On initial data load, is_data_loaded_ will be false here.
+    if (!is_data_loaded_) {
+      // If sync is enabled for addresses, defer running cleanups until address
+      // sync has started; otherwise, do it now.
+      if (!IsSyncEnabledFor(sync_service_, syncer::AUTOFILL_PROFILE))
+        ApplyAddressFixesAndCleanups();
+
+      // If sync is enabled for credit cards, defer running cleanups until card
+      // sync has started; otherwise, do it now.
+      if (!IsSyncEnabledFor(sync_service_, syncer::AUTOFILL_WALLET_DATA))
+        ApplyCardFixesAndCleanups();
+
+      // Log address and credit card startup metrics.
+      LogStoredProfileMetrics();
+      LogStoredCreditCardMetrics();
+    }
+
     is_data_loaded_ = true;
-    LogStoredProfileMetrics();
-    LogStoredCreditCardMetrics();
     NotifyPersonalDataChanged();
   }
 }
@@ -2463,6 +2466,7 @@ bool PersonalDataManager::IsAddressDeletable(
 
 bool PersonalDataManager::DeleteDisusedAddresses() {
   if (!base::FeatureList::IsEnabled(kAutofillDeleteDisusedAddresses)) {
+    DVLOG(1) << "Deletion is disabled";
     return false;
   }
 
@@ -2484,6 +2488,7 @@ bool PersonalDataManager::DeleteDisusedAddresses() {
 
   // Early exit when there are no profiles.
   if (profiles.empty()) {
+    DVLOG(1) << "There are no profiles";
     return true;
   }
 

components/autofill/core/browser/personal_data_manager_unittest.cc

@@ -180,7 +180,7 @@ class PersonalDataManagerTestBase {
 
     // Verify that the web database has been updated and the notification sent.
     EXPECT_CALL(personal_data_observer_, OnPersonalDataChanged())
-        .WillOnce(QuitMainMessageLoop());
+        .WillRepeatedly(QuitMainMessageLoop());
     base::RunLoop().Run();
   }
 
@@ -349,7 +349,7 @@ class PersonalDataManagerTestBase {
   // Verifies that the web database has been updated and the notification sent.
   void WaitForOnPersonalDataChanged() {
     EXPECT_CALL(personal_data_observer_, OnPersonalDataChanged())
-        .WillOnce(QuitMainMessageLoop());
+        .WillRepeatedly(QuitMainMessageLoop());
     base::RunLoop().Run();
   }
 
@@ -6272,7 +6272,7 @@ TEST_F(PersonalDataManagerTest, ClearCreditCardNonSettingsOrigins) {
 }
 
 // Tests that all the non settings origins of autofill profiles are cleared even
-// if Autofill is disabled.
+// if sync is disabled.
 TEST_F(
     PersonalDataManagerTest,
     SyncServiceInitializedWithAutofillDisabled_ClearProfileNonSettingsOrigins) {
@@ -6285,14 +6285,18 @@ TEST_F(
   personal_data_->AddProfile(profile);
   WaitForOnPersonalDataChanged();
 
-  EXPECT_CALL(personal_data_observer_, OnPersonalDataChanged()).Times(1);
-  prefs_->SetBoolean(prefs::kAutofillEnabled, false);
+  // Turn off autofill profile sync.
+  auto model_type_set = sync_service_.GetActiveDataTypes();
+  model_type_set.Remove(syncer::AUTOFILL_PROFILE);
+  sync_service_.SetDataTypes(model_type_set);
 
+  // The data should still exist.
   ASSERT_EQ(1U, personal_data_->GetProfiles().size());
 
-  personal_data_->OnSyncServiceInitialized(nullptr);
+  // Reload the personal data manager.
+  ResetPersonalDataManager(USER_MODE_NORMAL);
 
-  WaitForOnPersonalDataChanged();
+  // The profile should still exist.
   ASSERT_EQ(1U, personal_data_->GetProfiles().size());
 
   // The profile's origin should be cleared
@@ -6300,7 +6304,7 @@ TEST_F(
 }
 
 // Tests that all the non settings origins of autofill credit cards are cleared
-// even if Autofill is disabled.
+// even if sync is disabled.
 TEST_F(
     PersonalDataManagerTest,
     SyncServiceInitializedWithAutofillDisabled_ClearCreditCardNonSettingsOrigins) {
@@ -6312,14 +6316,19 @@ TEST_F(
   personal_data_->AddCreditCard(credit_card);
   WaitForOnPersonalDataChanged();
 
-  EXPECT_CALL(personal_data_observer_, OnPersonalDataChanged()).Times(1);
-  prefs_->SetBoolean(prefs::kAutofillEnabled, false);
+  // Turn off autofill profile sync.
+  auto model_type_set = sync_service_.GetActiveDataTypes();
+  model_type_set.Remove(syncer::AUTOFILL_WALLET_DATA);
+  model_type_set.Remove(syncer::AUTOFILL_WALLET_METADATA);
+  sync_service_.SetDataTypes(model_type_set);
 
+  // The credit card should still exist.
   ASSERT_EQ(1U, personal_data_->GetCreditCards().size());
 
-  personal_data_->OnSyncServiceInitialized(nullptr);
+  // Reload the personal data manager.
+  ResetPersonalDataManager(USER_MODE_NORMAL);
 
-  WaitForOnPersonalDataChanged();
+  // The credit card should still exist.
   ASSERT_EQ(1U, personal_data_->GetCreditCards().size());
 
   // The card's origin should be cleared