[AF] Reload from database to sort profiles

Profiles are sorted in the DB and not in the Personal Data Manager,
therefore, we need to reload them when the changes are done. =>
Call Refresh() each times there is no more change in the waiting
in the scheduling queue.

Bug: 920529
Change-Id: I7bc4337b881bac67185ed9ec279857fa6b207da4
Reviewed-on: https://chromium-review.googlesource.com/c/1406178
Commit-Queue: Parastoo Geranmayeh <parastoog@google.com>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Tatiana Gornak <melandory@chromium.org>
Reviewed-by: Mathieu Perreault <mathp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#625339}

chrome/browser/sync/test/integration/autofill_helper.cc

@@ -279,7 +279,7 @@ void SetProfiles(int profile, std::vector<AutofillProfile>* autofill_profiles) {
 
   pdm->AddObserver(&personal_data_observer);
   EXPECT_CALL(personal_data_observer, OnPersonalDataFinishedProfileTasks())
-      .WillOnce(QuitMessageLoop(&run_loop));
+      .WillRepeatedly(QuitMessageLoop(&run_loop));
   EXPECT_CALL(personal_data_observer, OnPersonalDataChanged())
       .Times(testing::AnyNumber());
 

chrome/browser/ui/views/payments/payment_request_browsertest_base.cc

@@ -460,8 +460,10 @@ void PaymentRequestBrowserTestBase::AddAutofillProfile(
   PersonalDataLoadedObserverMock personal_data_observer;
   personal_data_manager->AddObserver(&personal_data_observer);
   base::RunLoop data_loop;
-  EXPECT_CALL(personal_data_observer, OnPersonalDataChanged())
+  EXPECT_CALL(personal_data_observer, OnPersonalDataFinishedProfileTasks())
       .WillOnce(QuitMessageLoop(&data_loop));
+  EXPECT_CALL(personal_data_observer, OnPersonalDataChanged())
+      .Times(testing::AnyNumber());
   personal_data_manager->AddProfile(profile);
   data_loop.Run();
 
@@ -482,8 +484,11 @@ void PaymentRequestBrowserTestBase::AddCreditCard(
   PersonalDataLoadedObserverMock personal_data_observer;
   personal_data_manager->AddObserver(&personal_data_observer);
   base::RunLoop data_loop;
-  EXPECT_CALL(personal_data_observer, OnPersonalDataChanged())
+  EXPECT_CALL(personal_data_observer, OnPersonalDataFinishedProfileTasks())
       .WillOnce(QuitMessageLoop(&data_loop));
+  EXPECT_CALL(personal_data_observer, OnPersonalDataChanged())
+      .Times(testing::AnyNumber());
+
   personal_data_manager->AddCreditCard(card);
   data_loop.Run();
 
@@ -491,6 +496,18 @@ void PaymentRequestBrowserTestBase::AddCreditCard(
   EXPECT_EQ(card_count + 1, personal_data_manager->GetCreditCards().size());
 }
 
+void PaymentRequestBrowserTestBase::WaitForOnPersonalDataChanged() {
+  autofill::PersonalDataManager* personal_data_manager = GetDataManager();
+  PersonalDataLoadedObserverMock personal_data_observer;
+  personal_data_manager->AddObserver(&personal_data_observer);
+  base::RunLoop run_loop;
+  EXPECT_CALL(personal_data_observer, OnPersonalDataFinishedProfileTasks())
+      .WillOnce(QuitMessageLoop(&run_loop));
+  EXPECT_CALL(personal_data_observer, OnPersonalDataChanged())
+      .Times(testing::AnyNumber());
+  run_loop.Run();
+}
+
 void PaymentRequestBrowserTestBase::CreatePaymentRequestForTest(
     payments::mojom::PaymentRequestRequest request,
     content::RenderFrameHost* render_frame_host) {

chrome/browser/ui/views/payments/payment_request_browsertest_base.h

@@ -55,6 +55,7 @@ class PersonalDataLoadedObserverMock
   ~PersonalDataLoadedObserverMock() override;
 
   MOCK_METHOD0(OnPersonalDataChanged, void());
+  MOCK_METHOD0(OnPersonalDataFinishedProfileTasks, void());
 };
 
 // Base class for any interactive PaymentRequest test that will need to open
@@ -178,6 +179,7 @@ class PaymentRequestBrowserTestBase
   // are added close to each other.
   void AddAutofillProfile(const autofill::AutofillProfile& profile);
   void AddCreditCard(const autofill::CreditCard& card);
+  void WaitForOnPersonalDataChanged();
 
   void CreatePaymentRequestForTest(
       payments::mojom::PaymentRequestRequest request,

chrome/browser/ui/views/payments/payment_request_use_stats_browsertest.cc

@@ -62,6 +62,7 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestAutofillInstrumentUseStatsTest,
   InvokePaymentRequestUI();
   ResetEventWaiter(DialogEvent::DIALOG_CLOSED);
   PayWithCreditCardAndWait(base::ASCIIToUTF16("123"));
+  WaitForOnPersonalDataChanged();
 
   // Check that the usage of the card was recorded.
   autofill::CreditCard* updated_card =
@@ -111,6 +112,7 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestShippingAddressUseStatsTest, RecordUse) {
   InvokePaymentRequestUI();
   ResetEventWaiter(DialogEvent::DIALOG_CLOSED);
   PayWithCreditCardAndWait(base::ASCIIToUTF16("123"));
+  WaitForOnPersonalDataChanged();
 
   // Check that the usage of the profile was recorded.
   autofill::AutofillProfile* updated_shipping =
@@ -159,6 +161,7 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestContactAddressUseStatsTest, RecordUse) {
   InvokePaymentRequestUI();
   ResetEventWaiter(DialogEvent::DIALOG_CLOSED);
   PayWithCreditCardAndWait(base::ASCIIToUTF16("123"));
+  WaitForOnPersonalDataChanged();
 
   // Check that the usage of the profile was recorded.
   autofill::AutofillProfile* updated_contact =
@@ -209,6 +212,7 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestSameShippingAndContactAddressUseStatsTest,
   InvokePaymentRequestUI();
   ResetEventWaiter(DialogEvent::DIALOG_CLOSED);
   PayWithCreditCardAndWait(base::ASCIIToUTF16("123"));
+  WaitForOnPersonalDataChanged();
 
   // Check that the usage of the profile was only recorded once.
   autofill::AutofillProfile* updated_multi =

components/autofill/core/browser/autofill_profile.cc

@@ -457,6 +457,13 @@ bool AutofillProfile::EqualsForSyncPurposes(const AutofillProfile& profile)
          UseDateEqualsInSeconds(&profile) && EqualsSansGuid(profile);
 }
 
+bool AutofillProfile::EqualsForUpdatePurposes(
+    const AutofillProfile& profile) const {
+  return use_count() == profile.use_count() &&
+         UseDateEqualsInSeconds(&profile) &&
+         language_code() == profile.language_code() && Compare(profile) == 0;
+}
+
 bool AutofillProfile::EqualsForClientValidationPurpose(
     const AutofillProfile& profile) const {
   for (ServerFieldType type : kSupportedTypesByClientForValidation) {

components/autofill/core/browser/autofill_profile.h

@@ -126,6 +126,8 @@ class AutofillProfile : public AutofillDataModel {
   // differences in usage stats.
   bool EqualsForSyncPurposes(const AutofillProfile& profile) const;
 
+  bool EqualsForUpdatePurposes(const AutofillProfile& profile) const;
+
   // Compares the values of kSupportedTypesByClientForValidation fields.
   bool EqualsForClientValidationPurpose(const AutofillProfile& profile) const;
 

components/autofill/core/browser/personal_data_manager.cc

@@ -630,17 +630,20 @@ void PersonalDataManager::RecordUseOf(const AutofillDataModel& data_model) {
 
   AutofillProfile* profile = GetProfileByGUID(data_model.guid());
   if (profile) {
-    profile->RecordAndLogUse();
-
     if (profile->record_type() == AutofillProfile::LOCAL_PROFILE) {
-      UpdateProfileInDB(*profile);
+      // We can't make the change directly on the web_profiles_, the update
+      // should happen in the database first.
+      AutofillProfile updated_profile(*profile);
+      updated_profile.RecordAndLogUse();
+      UpdateProfileInDB(updated_profile);
     } else if (profile->record_type() == AutofillProfile::SERVER_PROFILE) {
+      profile->RecordAndLogUse();
       // TODO(crbug.com/864519): Update this once addresses support account
       // storage, and also use the server database.
       database_helper_->GetLocalDatabase()->UpdateServerAddressMetadata(
           *profile);
+      Refresh();
     }
-    Refresh();
   }
 }
 
@@ -1972,7 +1975,8 @@ void PersonalDataManager::OnAutofillProfileChanged(
       break;
     case AutofillProfileChange::UPDATE:
       profiles_server_validities_need_update_ = true;
-      if (profile_exists && !existing_profile->EqualsSansOrigin(profile)) {
+      if (profile_exists &&
+          !existing_profile->EqualsForUpdatePurposes(profile)) {
         web_profiles_.erase(
             FindElementByGUID<AutofillProfile>(web_profiles_, guid));
         web_profiles_.push_back(std::make_unique<AutofillProfile>(profile));
@@ -1988,10 +1992,7 @@ void PersonalDataManager::OnAutofillProfileChanged(
       NOTREACHED();
   }
 
-  ongoing_profile_changes_[guid].pop();
-
-  NotifyPersonalDataChanged();
-  HandleNextProfileChange(guid);
+  OnProfileChangeDone(guid);
 }
 
 void PersonalDataManager::LogServerCardLinkClicked() const {
@@ -2120,23 +2121,31 @@ bool PersonalDataManager::ApplyDedupingRoutine() {
   }
 
   DVLOG(1) << "Starting autofill profile de-duplication.";
-  std::unordered_set<AutofillProfile*> profiles_to_delete;
+  std::unordered_set<std::string> profiles_to_delete;
   profiles_to_delete.reserve(web_profiles_.size());
 
   // Create the map used to update credit card's billing addresses after the
   // dedupe.
   std::unordered_map<std::string, std::string> guids_merge_map;
 
-  DedupeProfiles(&web_profiles_, &profiles_to_delete, &guids_merge_map);
+  // The changes can't happen directly on the web_profiles_, but need to be
+  // updated in the database at first, and then updated on the web_profiles_.
+  // Therefore, we need a copy of web_profiles_ to keep track of the changes.
+  std::vector<std::unique_ptr<AutofillProfile>> new_profiles;
+  for (const auto& it : web_profiles_) {
+    new_profiles.push_back(std::make_unique<AutofillProfile>(*(it.get())));
+  }
+
+  DedupeProfiles(&new_profiles, &profiles_to_delete, &guids_merge_map);
 
   // Apply the profile changes to the database.
-  for (const auto& profile : web_profiles_) {
-    // If the profile was set to be deleted, remove it from the database.
-    if (profiles_to_delete.count(profile.get())) {
+  for (const auto& profile : new_profiles) {
+    // If the profile was set to be deleted, remove it from the database,
+    // otherwise update it.
+    if (profiles_to_delete.count(profile->guid())) {
       RemoveProfileFromDB(profile->guid());
     } else {
-      // Otherwise, update the profile in the database.
-      UpdateProfileInDB(*profile);
+      UpdateProfileInDB(*(profile.get()));
     }
   }
 
@@ -2150,7 +2159,7 @@ bool PersonalDataManager::ApplyDedupingRoutine() {
 
 void PersonalDataManager::DedupeProfiles(
     std::vector<std::unique_ptr<AutofillProfile>>* existing_profiles,
-    std::unordered_set<AutofillProfile*>* profiles_to_delete,
+    std::unordered_set<std::string>* profiles_to_delete,
     std::unordered_map<std::string, std::string>* guids_merge_map) const {
   AutofillMetrics::LogNumberOfProfilesConsideredForDedupe(
       existing_profiles->size());
@@ -2178,7 +2187,7 @@ void PersonalDataManager::DedupeProfiles(
 
     // If the profile was set to be deleted, skip it. It has already been
     // merged into another profile.
-    if (profiles_to_delete->count(profile_to_merge))
+    if (profiles_to_delete->count(profile_to_merge->guid()))
       continue;
 
     // If we have reached the verified profiles, stop trying to merge. Verified
@@ -2192,7 +2201,7 @@ void PersonalDataManager::DedupeProfiles(
       AutofillProfile* existing_profile = (*existing_profiles)[j].get();
 
       // Don't try to merge a profile that was already set for deletion.
-      if (profiles_to_delete->count(existing_profile))
+      if (profiles_to_delete->count(existing_profile->guid()))
         continue;
 
       // Move on if the profiles are not mergeable.
@@ -2212,7 +2221,7 @@ void PersonalDataManager::DedupeProfiles(
 
         // Since |profile_to_merge| was a duplicate of |existing_profile|
         // and was merged successfully, it can now be deleted.
-        profiles_to_delete->insert(profile_to_merge);
+        profiles_to_delete->insert(profile_to_merge->guid());
 
         // Now try to merge the new resulting profile with the rest of the
         // existing profiles.
@@ -2577,7 +2586,6 @@ void PersonalDataManager::AddProfileToDB(const AutofillProfile& profile) {
   }
 
   if (!ProfileChangesAreOnGoing(profile.guid())) {
-    // Don't add an existing/empty profile.
     if (FindByGUID<AutofillProfile>(web_profiles_, profile.guid()) ||
         FindByContents(web_profiles_, profile)) {
       NotifyPersonalDataChanged();
@@ -2585,16 +2593,22 @@ void PersonalDataManager::AddProfileToDB(const AutofillProfile& profile) {
     }
     database_helper_->GetLocalDatabase()->AddAutofillProfile(profile);
   }
-
   ongoing_profile_changes_[profile.guid()].push(
       AutofillProfileDeepChange(AutofillProfileChange::ADD, profile));
 }
 
 void PersonalDataManager::UpdateProfileInDB(const AutofillProfile& profile) {
-  // Update the profile in the web database.
   if (!ProfileChangesAreOnGoing(profile.guid())) {
-    database_helper_->GetLocalDatabase()->UpdateAutofillProfile(profile);
+    const auto* existing_profile = GetProfileByGUID(profile.guid());
+    bool profile_exists = (existing_profile != nullptr);
+    if (profile_exists && !existing_profile->EqualsForUpdatePurposes(profile)) {
+      database_helper_->GetLocalDatabase()->UpdateAutofillProfile(profile);
+    } else {
+      NotifyPersonalDataChanged();
+      return;
+    }
   }
+
   ongoing_profile_changes_[profile.guid()].push(
       AutofillProfileDeepChange(AutofillProfileChange::UPDATE, profile));
 }
@@ -2616,7 +2630,8 @@ void PersonalDataManager::HandleNextProfileChange(const std::string& guid) {
   if (!ProfileChangesAreOnGoing(guid))
     return;
 
-  const auto& change_type = ongoing_profile_changes_[guid].front().type();
+  const auto& change = ongoing_profile_changes_[guid].front();
+  const auto& change_type = change.type();
   const auto* existing_profile = GetProfileByGUID(guid);
   const bool profile_exists = (existing_profile != nullptr);
   const auto& profile = ongoing_profile_changes_[guid].front().profile();
@@ -2625,9 +2640,7 @@ void PersonalDataManager::HandleNextProfileChange(const std::string& guid) {
 
   if (change_type == AutofillProfileChange::REMOVE) {
     if (!profile_exists) {
-      ongoing_profile_changes_[guid].pop();
-      NotifyPersonalDataChanged();
-      HandleNextProfileChange(guid);
+      OnProfileChangeDone(guid);
       return;
     }
     database_helper_->GetLocalDatabase()->RemoveAutofillProfile(guid);
@@ -2636,19 +2649,15 @@ void PersonalDataManager::HandleNextProfileChange(const std::string& guid) {
 
   if (change_type == AutofillProfileChange::ADD) {
     if (profile_exists || FindByContents(web_profiles_, profile)) {
-      ongoing_profile_changes_[guid].pop();
-      NotifyPersonalDataChanged();
-      HandleNextProfileChange(guid);
+      OnProfileChangeDone(guid);
       return;
     }
     database_helper_->GetLocalDatabase()->AddAutofillProfile(profile);
     return;
   }
 
-  if (!profile_exists || existing_profile->EqualsSansOrigin(profile)) {
-    ongoing_profile_changes_[guid].pop();
-    NotifyPersonalDataChanged();
-    HandleNextProfileChange(guid);
+  if (!profile_exists || existing_profile->EqualsForUpdatePurposes(profile)) {
+    OnProfileChangeDone(guid);
     return;
   }
   database_helper_->GetLocalDatabase()->UpdateAutofillProfile(profile);
@@ -2669,6 +2678,17 @@ bool PersonalDataManager::ProfileChangesAreOnGoing() {
   return false;
 }
 
+void PersonalDataManager::OnProfileChangeDone(const std::string& guid) {
+  ongoing_profile_changes_[guid].pop();
+
+  if (!ProfileChangesAreOnGoing()) {
+    Refresh();
+  } else {
+    NotifyPersonalDataChanged();
+    HandleNextProfileChange(guid);
+  }
+}
+
 void PersonalDataManager::ClearOnGoingProfileChanges() {
   ongoing_profile_changes_.clear();
 }

components/autofill/core/browser/personal_data_manager.h

@@ -652,7 +652,7 @@ class PersonalDataManager : public KeyedService,
   // testing purposes.
   void DedupeProfiles(
       std::vector<std::unique_ptr<AutofillProfile>>* existing_profiles,
-      std::unordered_set<AutofillProfile*>* profile_guids_to_delete,
+      std::unordered_set<std::string>* profile_guids_to_delete,
       std::unordered_map<std::string, std::string>* guids_merge_map) const;
 
   // Updates the credit cards' billing address reference based on the merges
@@ -735,6 +735,9 @@ class PersonalDataManager : public KeyedService,
   // returns true if there is any ongoing change for profile with guid = |guid|
   // that's still on going.
   bool ProfileChangesAreOnGoing(const std::string& guid);
+  // Remove the change from the |ongoing_profile_changes_|, handle next task or
+  // Refresh.
+  void OnProfileChangeDone(const std::string& guid);
   // Clear |ongoing_profile_changes_|.
   void ClearOnGoingProfileChanges();