[Autofill] Implemented PersonalDataManagerCleaner

PersonalDataManagerCleaner is responsible for applying address and
credit card fixes and cleanups once on browser startup provided that
the sync is enabled or when the sync starts.

This CL factors out everything related to fixes and cleanups of
address and credit card from the PersonalDataManager and moves them to
the newly created PersonalDataManagerCleaner class.


Change-Id: Ib6ed35b1237576c04dc6fe3fce8d8792cc07f3c1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2584811Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Reviewed-by: Matthias Körber <koerber@google.com>
Commit-Queue: Vidhan Jain <vidhanj@google.com>
Cr-Commit-Position: refs/heads/master@{#837708}

chrome/browser/autofill/android/personal_data_manager_android.cc

@@ -557,7 +557,7 @@ void PersonalDataManagerAndroid::UpdateServerCardBillingAddress(
   CreditCard card;
   PopulateNativeCreditCardFromJava(jcard, env, &card);
 
-  personal_data_manager_->UpdateServerCardMetadata(card);
+  personal_data_manager_->UpdateServerCardsMetadata({card});
 }
 
 ScopedJavaLocalRef<jstring>

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

@@ -427,8 +427,8 @@ bool CreditCardEditorViewController::ValidateModelAndSave() {
     credit_card_to_edit_->set_billing_address_id(
         model->GetItemIdentifierAt(address_combobox->GetSelectedIndex()));
     if (!is_incognito()) {
-      state()->GetPersonalDataManager()->UpdateServerCardMetadata(
-          *credit_card_to_edit_);
+      state()->GetPersonalDataManager()->UpdateServerCardsMetadata(
+          {*credit_card_to_edit_});
     }
     return true;
   }

components/autofill/core/browser/BUILD.gn

@@ -246,6 +246,8 @@ static_library("browser") {
     "payments/webauthn_callback_types.h",
     "personal_data_manager.cc",
     "personal_data_manager.h",
+    "personal_data_manager_cleaner.cc",
+    "personal_data_manager_cleaner.h",
     "personal_data_manager_observer.h",
     "randomized_encoder.cc",
     "randomized_encoder.h",

components/autofill/core/browser/personal_data_manager.h

@@ -24,10 +24,10 @@
 #include "components/autofill/core/browser/data_model/autofill_profile.h"
 #include "components/autofill/core/browser/data_model/credit_card.h"
 #include "components/autofill/core/browser/data_model/credit_card_cloud_token_data.h"
-#include "components/autofill/core/browser/data_model/test_data_creator.h"
 #include "components/autofill/core/browser/field_types.h"
 #include "components/autofill/core/browser/payments/account_info_getter.h"
 #include "components/autofill/core/browser/payments/payments_customer_data.h"
+#include "components/autofill/core/browser/personal_data_manager_cleaner.h"
 #include "components/autofill/core/browser/proto/server.pb.h"
 #include "components/autofill/core/browser/sync_utils.h"
 #include "components/autofill/core/browser/ui/suggestion.h"
@@ -201,9 +201,10 @@ class PersonalDataManager : public KeyedService,
   // status can be changed. Looks up the card by server ID.
   virtual void UpdateServerCreditCard(const CreditCard& credit_card);
 
-  // Updates the use stats and billing address id for the server |credit_card|.
-  // Looks up the card by server_id.
-  virtual void UpdateServerCardMetadata(const CreditCard& credit_card);
+  // Updates the use stats and billing address id for the server |credit_cards|.
+  // Looks up the cards by server_id.
+  virtual void UpdateServerCardsMetadata(
+      const std::vector<CreditCard>& credit_cards);
 
   // Resets the card for |guid| to the masked state.
   void ResetFullServerCard(const std::string& guid);
@@ -293,10 +294,6 @@ class PersonalDataManager : public KeyedService,
       bool field_is_autofilled,
       const std::vector<ServerFieldType>& field_types);
 
-  // Tries to delete disused addresses once per major version if the
-  // feature is enabled.
-  bool DeleteDisusedAddresses();
-
   // Returns the credit cards to suggest to the user. Those have been deduped
   // and ordered by frecency with the expired cards put at the end of the
   // vector. If |include_server_cards| is false, server side cards should not
@@ -321,10 +318,6 @@ class PersonalDataManager : public KeyedService,
       const base::string16& field_contents,
       bool include_server_cards);
 
-  // Tries to delete disused credit cards once per major version if the
-  // feature is enabled.
-  bool DeleteDisusedCreditCards();
-
   // Re-loads profiles and credit cards from the WebDatabase asynchronously.
   // In the general case, this is a no-op and will re-create the same
   // in-memory model as existed prior to the call.  If any change occurred to
@@ -349,6 +342,14 @@ class PersonalDataManager : public KeyedService,
   void set_variations_country_code_for_testing(std::string country_code) {
     variations_country_code_ = country_code;
   }
+
+  // Returns the raw pointer to PersonalDataManagerCleaner used for testing
+  // purposes.
+  PersonalDataManagerCleaner* personal_data_manager_cleaner_for_testing()
+      const {
+    DCHECK(personal_data_manager_cleaner_);
+    return personal_data_manager_cleaner_.get();
+  }
 #endif
 
   // Returns our best guess for the country a user is likely to use when
@@ -490,16 +491,13 @@ class PersonalDataManager : public KeyedService,
   FRIEND_TEST_ALL_PREFIXES(
       PersonalDataManagerTest,
       GetCreditCardSuggestions_NoCreditCardsAddedIfDisabled);
-  FRIEND_TEST_ALL_PREFIXES(PersonalDataManagerTest,
-                           ClearProfileNonSettingsOrigins);
-  FRIEND_TEST_ALL_PREFIXES(PersonalDataManagerTest,
-                           ClearCreditCardNonSettingsOrigins);
   FRIEND_TEST_ALL_PREFIXES(PersonalDataManagerTest,
                            RequestProfileServerValidity);
   FRIEND_TEST_ALL_PREFIXES(PersonalDataManagerTest,
                            GetProfileSuggestions_Validity);
 
   friend class autofill::AutofillInteractiveTest;
+  friend class autofill::PersonalDataManagerCleaner;
   friend class autofill::PersonalDataManagerFactory;
   friend class AutofillMetricsTest;
   friend class FormDataImporterTest;
@@ -578,11 +576,6 @@ class PersonalDataManager : public KeyedService,
   // this class and must outlive |this|.
   void SetPrefService(PrefService* pref_service);
 
-  // Clears the value of the origin field of the autofill profiles or cards that
-  // were not created from the settings page.
-  void ClearProfileNonSettingsOrigins();
-  void ClearCreditCardNonSettingsOrigins();
-
   // Called when the |profile| is validated by the AutofillProfileValidator,
   // updates the profiles on the |ongoing_profile_changes_| and the DB.
   virtual void OnValidated(const AutofillProfile* profile);
@@ -664,36 +657,6 @@ class PersonalDataManager : public KeyedService,
       const base::string16& field_contents,
       const std::vector<CreditCard*>& cards_to_suggest) const;
 
-  // Runs the routine that removes the orphan rows in the autofill tables if
-  // it's never been done.
-  void RemoveOrphanAutofillTableRows();
-
-  // Applies the deduping routine once per major version if the feature is
-  // enabled. Calls DedupeProfiles with the content of |web_profiles_| as a
-  // parameter. Removes the profiles to delete from the database and updates the
-  // others. Also updates the credit cards' billing address references. Returns
-  // true if the routine was run.
-  bool ApplyDedupingRoutine();
-
-  // Goes through all the |existing_profiles| and merges all similar unverified
-  // profiles together. Also discards unverified profiles that are similar to a
-  // verified profile. All the profiles except the results of the merges will be
-  // added to |profile_guids_to_delete|. This routine should be run once per
-  // major version. Records all the merges into the |guids_merge_map|.
-  //
-  // This method should only be called by ApplyDedupingRoutine. It is split for
-  // testing purposes.
-  void DedupeProfiles(
-      std::vector<std::unique_ptr<AutofillProfile>>* existing_profiles,
-      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
-  // that happened during the dedupe, as defined in |guids_merge_map|. Also
-  // updates the cards entries in the database.
-  void UpdateCardsBillingAddressReference(
-      const std::unordered_map<std::string, std::string>& guids_merge_map);
-
   // Converts the Wallet addresses to local autofill profiles. This should be
   // called after all the syncable data has been processed (local cards and
   // profiles, Wallet data and metadata). Also updates Wallet cards' billing
@@ -728,12 +691,6 @@ class PersonalDataManager : public KeyedService,
   void RemoveAutofillProfileByGUIDAndBlankCreditCardReference(
       const std::string& guid);
 
-  // Applies various fixes and cleanups on autofill addresses.
-  void ApplyAddressFixesAndCleanups();
-
-  // Applies various fixes and cleanups on autofill credit cards.
-  void ApplyCardFixesAndCleanups();
-
   // Resets |synced_profile_validity_|.
   void ResetProfileValidity();
 
@@ -773,6 +730,12 @@ class PersonalDataManager : public KeyedService,
   // one copy has a nickname, take that.
   base::string16 GetDisplayNicknameForCreditCard(const CreditCard& card) const;
 
+  // Returns true if the sync is enabled for |model_type|.
+  bool IsSyncEnabledFor(syncer::ModelType model_type);
+
+  // Returns the database that is used for storing local data.
+  scoped_refptr<AutofillWebDataService> GetLocalDatabase();
+
   // Stores the |app_locale| supplied on construction.
   const std::string app_locale_;
 
@@ -799,6 +762,11 @@ class PersonalDataManager : public KeyedService,
   // |profile_validities_need_update_| whenever this is changed.
   std::unique_ptr<UserProfileValidityMap> synced_profile_validity_;
 
+  // PersonalDataManagerCleaner is used to apply various address and credit
+  // card fixes/cleanups one time at browser startup or when the sync starts.
+  // PersonalDataManagerCleaner is declared as a friend class.
+  std::unique_ptr<PersonalDataManagerCleaner> personal_data_manager_cleaner_;
+
   // A timely ordered list of ongoing changes for each profile.
   std::unordered_map<std::string, std::deque<AutofillProfileDeepChange>>
       ongoing_profile_changes_;
@@ -834,14 +802,6 @@ class PersonalDataManager : public KeyedService,
   // An observer to listen for changes to prefs::kAutofillWalletImportEnabled.
   std::unique_ptr<BooleanPrefMember> wallet_enabled_pref_;
 
-  // True if autofill profile cleanup needs to be performed.
-  bool is_autofill_profile_cleanup_pending_ = false;
-
-  // Used to create test data. If the AutofillCreateDataForTest feature is
-  // enabled, this helper creates autofill profiles and credit card data that
-  // would otherwise be difficult to create manually using the UI.
-  TestDataCreator test_data_creator_;
-
   // Whether sync should be considered on in a test.
   bool is_syncing_for_test_ = false;
 

components/autofill/core/browser/personal_data_manager_cleaner.h

+// Copyright 2020 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.
+
+#ifndef COMPONENTS_AUTOFILL_CORE_BROWSER_PERSONAL_DATA_MANAGER_CLEANER_H_
+#define COMPONENTS_AUTOFILL_CORE_BROWSER_PERSONAL_DATA_MANAGER_CLEANER_H_
+
+#include <unordered_set>
+
+#include "components/autofill/core/browser/data_model/autofill_profile.h"
+#include "components/autofill/core/browser/data_model/test_data_creator.h"
+#include "components/sync/base/model_type.h"
+
+class PrefService;
+
+namespace autofill {
+
+class PersonalDataManager;
+
+// PersonalDataManagerCleaner is responsible for applying address and credit
+// card cleanups once on browser startup provided that the sync is enabled or
+// when the sync starts.
+class PersonalDataManagerCleaner {
+ public:
+  PersonalDataManagerCleaner(PersonalDataManager* personal_data_manager,
+                             PrefService* pref_service);
+  ~PersonalDataManagerCleaner();
+  PersonalDataManagerCleaner(const PersonalDataManagerCleaner&) = delete;
+  PersonalDataManagerCleaner& operator=(const PersonalDataManagerCleaner&) =
+      delete;
+
+  // Applies address and credit card fixes and cleanups if the sync is enabled.
+  // Also, logs address, credit card and offer startup metrics.
+  void CleanupData();
+
+  // Applies address/credit card fixes and cleanups depending on the
+  // |model_type|.
+  void SyncStarted(syncer::ModelType model_type);
+
+#if defined(UNIT_TEST)
+  // A wrapper around |ApplyDedupingRoutine()| used for testing purposes.
+  bool ApplyDedupingRoutineForTesting() { return ApplyDedupingRoutine(); }
+
+  // A wrapper around |DedupeProfiles()| used for testing purposes.
+  void DedupeProfilesForTesting(
+      std::vector<std::unique_ptr<AutofillProfile>>* existing_profiles,
+      std::unordered_set<std::string>* profile_guids_to_delete,
+      std::unordered_map<std::string, std::string>* guids_merge_map) const {
+    DedupeProfiles(existing_profiles, profile_guids_to_delete, guids_merge_map);
+  }
+
+  // A wrapper around |UpdateCardsBillingAddressReference()| used for testing
+  // purposes.
+  void UpdateCardsBillingAddressReferenceForTesting(
+      const std::unordered_map<std::string, std::string>& guids_merge_map) {
+    UpdateCardsBillingAddressReference(guids_merge_map);
+  }
+
+  // A wrapper around |DeleteDisusedAddresses()| used for testing purposes.
+  bool DeleteDisusedAddressesForTesting() { return DeleteDisusedAddresses(); }
+
+  // A wrapper around |DeleteDisusedCreditCards()| used for testing purposes.
+  bool DeleteDisusedCreditCardsForTesting() {
+    return DeleteDisusedCreditCards();
+  }
+
+  // A wrapper around |ClearProfileNonSettingsOrigins()| used for testing
+  // purposes.
+  void ClearProfileNonSettingsOriginsForTesting() {
+    ClearProfileNonSettingsOrigins();
+  }
+
+  // A wrapper around |ClearCreditCardNonSettingsOrigins()| used for testing
+  // purposes.
+  void ClearCreditCardNonSettingsOriginsForTesting() {
+    ClearCreditCardNonSettingsOrigins();
+  }
+
+  // Setter method for |is_autofill_profile_cleanup_pending_| used for testing
+  // purposes.
+  void set_is_autofill_profile_cleanup_pending(bool value) {
+    is_autofill_profile_cleanup_pending_ = value;
+  }
+#endif  // defined(UNIT_TEST)
+
+ private:
+  // Applies various fixes and cleanups on autofill addresses.
+  void ApplyAddressFixesAndCleanups();
+
+  // Applies various fixes and cleanups on autofill credit cards.
+  void ApplyCardFixesAndCleanups();
+
+  // Runs the routine that removes the orphan rows in the autofill tables if
+  // it's never been done.
+  void RemoveOrphanAutofillTableRows();
+
+  // Applies the deduping routine once per major version if the feature is
+  // enabled. Calls DedupeProfiles with the content of
+  // |PersonalDataManager::GetProfiles()| as a parameter. Removes the profiles
+  // to delete from the database and updates the others. Also updates the credit
+  // cards billing address references. Returns true if the routine was run.
+  bool ApplyDedupingRoutine();
+
+  // Goes through all the |existing_profiles| and merges all similar unverified
+  // profiles together. Also discards unverified profiles that are similar to a
+  // verified profile. All the profiles except the results of the merges will be
+  // added to |profile_guids_to_delete|. This routine should be run once per
+  // major version. Records all the merges into the |guids_merge_map|.
+  //
+  // This method should only be called by ApplyDedupingRoutine. It is split for
+  // testing purposes.
+  void DedupeProfiles(
+      std::vector<std::unique_ptr<AutofillProfile>>* existing_profiles,
+      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 references based on the merges
+  // that happened during the dedupe, as defined in |guids_merge_map|. Also
+  // updates the cards entries in the database.
+  void UpdateCardsBillingAddressReference(
+      const std::unordered_map<std::string, std::string>& guids_merge_map);
+
+  // Tries to delete disused addresses once per major version if the
+  // feature is enabled.
+  bool DeleteDisusedAddresses();
+
+  // Tries to delete disused credit cards once per major version if the
+  // feature is enabled.
+  bool DeleteDisusedCreditCards();
+
+  // Clears the value of the origin field of the autofill profiles or cards that
+  // were not created from the settings page.
+  void ClearProfileNonSettingsOrigins();
+  void ClearCreditCardNonSettingsOrigins();
+
+  // Used for adding test data by |test_data_creator_|.
+  void AddProfileForTest(const AutofillProfile& profile);
+  void AddCreditCardForTest(const CreditCard& credit_card);
+
+  // True if autofill profile cleanup needs to be performed.
+  bool is_autofill_profile_cleanup_pending_ = false;
+
+  // Used to create test data. If the AutofillCreateDataForTest feature is
+  // enabled, this helper creates autofill profiles and credit card data that
+  // would otherwise be difficult to create manually using the UI.
+  TestDataCreator test_data_creator_;
+
+  // The personal data manager, used to load and update the personal data
+  // from/to the web database.
+  PersonalDataManager* const personal_data_manager_ = nullptr;
+
+  // The PrefService used by this instance.
+  PrefService* const pref_service_ = nullptr;
+};
+
+}  // namespace autofill
+
+#endif  // COMPONENTS_AUTOFILL_CORE_BROWSER_PERSONAL_DATA_MANAGER_CLEANER_H_