Make AutofillManagerTest and CreditCardSaveManagerTest use a central TestPersonalDataManager

Bug: 778436
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I8ba2a695880a8594a145ecfa9724907253f5ad7f
Reviewed-on: https://chromium-review.googlesource.com/752074
Commit-Queue: Jared Saul <jsaul@google.com>
Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Mathieu Perreault <mathp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522218}

components/autofill/core/browser/address_combobox_model_unittest.cc

@@ -28,7 +28,7 @@ TEST(AddressComboboxModelTest, Empty) {
 TEST(AddressComboboxModelTest, OneAddress) {
   TestPersonalDataManager test_personal_data_manager;
   AutofillProfile profile1(test::GetFullProfile());
-  test_personal_data_manager.AddTestingProfile(&profile1);
+  test_personal_data_manager.AddProfile(profile1);
 
   AddressComboboxModel model(test_personal_data_manager, kAppLocale,
                              profile1.guid());
@@ -50,8 +50,8 @@ TEST(AddressComboboxModelTest, TwoAddresses) {
 
   // Force |profile1| to be shown first in the combobox.
   profile1.set_use_count(100);
-  test_personal_data_manager.AddTestingProfile(&profile1);
-  test_personal_data_manager.AddTestingProfile(&profile2);
+  test_personal_data_manager.AddProfile(profile1);
+  test_personal_data_manager.AddProfile(profile2);
 
   AddressComboboxModel model(test_personal_data_manager, kAppLocale,
                              profile2.guid());
@@ -71,7 +71,7 @@ TEST(AddressComboboxModelTest, TwoAddresses) {
 TEST(AddressComboboxModelTest, AddAnAddress) {
   TestPersonalDataManager test_personal_data_manager;
   AutofillProfile profile1(test::GetFullProfile());
-  test_personal_data_manager.AddTestingProfile(&profile1);
+  test_personal_data_manager.AddProfile(profile1);
 
   AddressComboboxModel model(test_personal_data_manager, kAppLocale, "");
   EXPECT_EQ(3, model.GetItemCount());

components/autofill/core/browser/country_combobox_model_unittest.cc

@@ -21,14 +21,14 @@ class CountryComboboxModelTest : public testing::Test {
  public:
   CountryComboboxModelTest()
       : pref_service_(autofill::test::PrefServiceForTesting()) {
-    manager_.SetTestingPrefService(pref_service_.get());
+    manager_.SetPrefService(pref_service_.get());
     manager_.set_timezone_country_code("KR");
     model_.reset(new CountryComboboxModel());
     model_->SetCountries(manager_, base::Callback<bool(const std::string&)>(),
                          "en-US");
   }
 
-  void TearDown() override { manager_.SetTestingPrefService(nullptr); }
+  void TearDown() override { manager_.SetPrefService(nullptr); }
 
   TestPersonalDataManager* manager() { return &manager_; }
   CountryComboboxModel* model() { return model_.get(); }

components/autofill/core/browser/test_personal_data_manager.cc

@@ -4,7 +4,6 @@
 
 #include "components/autofill/core/browser/test_personal_data_manager.h"
 
-#include "base/memory/ptr_util.h"
 #include "components/autofill/core/browser/personal_data_manager_observer.h"
 
 namespace autofill {
@@ -14,43 +13,100 @@ TestPersonalDataManager::TestPersonalDataManager()
 
 TestPersonalDataManager::~TestPersonalDataManager() {}
 
-void TestPersonalDataManager::SetTestingPrefService(PrefService* pref_service) {
-  SetPrefService(pref_service);
+void TestPersonalDataManager::RecordUseOf(const AutofillDataModel& data_model) {
+  CreditCard* credit_card = GetCreditCardWithGUID(data_model.guid().c_str());
+  if (credit_card)
+    credit_card->RecordAndLogUse();
+
+  AutofillProfile* profile = GetProfileWithGUID(data_model.guid().c_str());
+  if (profile)
+    profile->RecordAndLogUse();
+}
+
+std::string TestPersonalDataManager::SaveImportedProfile(
+    const AutofillProfile& imported_profile) {
+  num_times_save_imported_profile_called_++;
+  AddProfile(imported_profile);
+  return imported_profile.guid();
+}
+
+std::string TestPersonalDataManager::SaveImportedCreditCard(
+    const CreditCard& imported_credit_card) {
+  AddCreditCard(imported_credit_card);
+  return imported_credit_card.guid();
 }
 
-void TestPersonalDataManager::AddTestingProfile(AutofillProfile* profile) {
-  profiles_.push_back(profile);
+void TestPersonalDataManager::AddProfile(const AutofillProfile& profile) {
+  std::unique_ptr<AutofillProfile> profile_ptr =
+      std::make_unique<AutofillProfile>(profile);
+  web_profiles_.push_back(std::move(profile_ptr));
   NotifyPersonalDataChanged();
 }
 
-void TestPersonalDataManager::AddTestingCreditCard(CreditCard* credit_card) {
-  credit_cards_.push_back(credit_card);
+void TestPersonalDataManager::RemoveByGUID(const std::string& guid) {
+  CreditCard* credit_card = GetCreditCardWithGUID(guid.c_str());
+  if (credit_card) {
+    local_credit_cards_.erase(
+        std::find_if(local_credit_cards_.begin(), local_credit_cards_.end(),
+                     [credit_card](const std::unique_ptr<CreditCard>& ptr) {
+                       return ptr.get() == credit_card;
+                     }));
+  }
+
+  AutofillProfile* profile = GetProfileWithGUID(guid.c_str());
+  if (profile) {
+    web_profiles_.erase(
+        std::find_if(web_profiles_.begin(), web_profiles_.end(),
+                     [profile](const std::unique_ptr<AutofillProfile>& ptr) {
+                       return ptr.get() == profile;
+                     }));
+  }
+}
+
+void TestPersonalDataManager::AddCreditCard(const CreditCard& credit_card) {
+  std::unique_ptr<CreditCard> local_credit_card =
+      std::make_unique<CreditCard>(credit_card);
+  local_credit_cards_.push_back(std::move(local_credit_card));
   NotifyPersonalDataChanged();
 }
 
-void TestPersonalDataManager::AddTestingServerCreditCard(
+void TestPersonalDataManager::AddFullServerCreditCard(
     const CreditCard& credit_card) {
-  server_credit_cards_.push_back(base::MakeUnique<CreditCard>(credit_card));
+  // Though the name is AddFullServerCreditCard, this test class treats masked
+  // and full server cards equally, relying on their preset RecordType to
+  // differentiate them.
+  AddServerCreditCard(credit_card);
 }
 
 std::vector<AutofillProfile*> TestPersonalDataManager::GetProfiles() const {
-  return profiles_;
+  std::vector<AutofillProfile*> result;
+  result.reserve(web_profiles_.size());
+  for (const auto& profile : web_profiles_)
+    result.push_back(profile.get());
+  return result;
 }
 
 std::vector<CreditCard*> TestPersonalDataManager::GetCreditCards() const {
-  return credit_cards_;
+  // TODO(crbug.com/778436): The real PersonalDataManager relies on its
+  // |pref_service_| to decide what to return. Since the lack of a pref_service_
+  // makes this fake class crash, it might be useful to refactor the real
+  // GetCreditCards()'s logic into overrideable methods and then remove this
+  // function.
+  std::vector<CreditCard*> result;
+  result.reserve(local_credit_cards_.size() + server_credit_cards_.size());
+  for (const auto& card : local_credit_cards_)
+    result.push_back(card.get());
+  for (const auto& card : server_credit_cards_)
+    result.push_back(card.get());
+  return result;
 }
 
-std::string TestPersonalDataManager::SaveImportedProfile(
-    const AutofillProfile& imported_profile) {
-  imported_profile_ = imported_profile;
-  return imported_profile.guid();
-}
+const std::string& TestPersonalDataManager::GetDefaultCountryCodeForNewAddress()
+    const {
+  if (default_country_code_.empty())
+    return PersonalDataManager::GetDefaultCountryCodeForNewAddress();
 
-std::string TestPersonalDataManager::SaveImportedCreditCard(
-    const CreditCard& imported_credit_card) {
-  imported_credit_card_ = imported_credit_card;
-  return imported_credit_card.guid();
+  return default_country_code_;
 }
 
 std::string TestPersonalDataManager::CountryCodeForCurrentTimezone()
@@ -58,12 +114,37 @@ std::string TestPersonalDataManager::CountryCodeForCurrentTimezone()
   return timezone_country_code_;
 }
 
-const std::string& TestPersonalDataManager::GetDefaultCountryCodeForNewAddress()
-    const {
-  if (default_country_code_.empty())
-    return PersonalDataManager::GetDefaultCountryCodeForNewAddress();
+void TestPersonalDataManager::ClearProfiles() {
+  web_profiles_.clear();
+}
 
-  return default_country_code_;
+void TestPersonalDataManager::ClearCreditCards() {
+  local_credit_cards_.clear();
+  server_credit_cards_.clear();
+}
+
+AutofillProfile* TestPersonalDataManager::GetProfileWithGUID(const char* guid) {
+  for (AutofillProfile* profile : GetProfiles()) {
+    if (!profile->guid().compare(guid))
+      return profile;
+  }
+  return nullptr;
+}
+
+CreditCard* TestPersonalDataManager::GetCreditCardWithGUID(const char* guid) {
+  for (CreditCard* card : GetCreditCards()) {
+    if (!card->guid().compare(guid))
+      return card;
+  }
+  return nullptr;
+}
+
+void TestPersonalDataManager::AddServerCreditCard(
+    const CreditCard& credit_card) {
+  std::unique_ptr<CreditCard> server_credit_card =
+      std::make_unique<CreditCard>(credit_card);
+  server_credit_cards_.push_back(std::move(server_credit_card));
+  NotifyPersonalDataChanged();
 }
 
 }  // namespace autofill

components/autofill/core/browser/test_personal_data_manager.h

@@ -11,10 +11,6 @@
 #include "components/autofill/core/browser/credit_card.h"
 #include "components/autofill/core/browser/personal_data_manager.h"
 
-namespace syncer {
-class SyncService;
-}
-
 namespace autofill {
 
 // A simplistic PersonalDataManager used for testing.
@@ -23,33 +19,44 @@ class TestPersonalDataManager : public PersonalDataManager {
   TestPersonalDataManager();
   ~TestPersonalDataManager() override;
 
-  // PersonalDataManager:
+  using PersonalDataManager::set_database;
+  using PersonalDataManager::SetPrefService;
+
+  // PersonalDataManager overrides.  These functions are overridden as needed
+  // for various tests, whether to skip calls to uncreated databases/services,
+  // or to make things easier in general to toggle.
   void OnSyncServiceInitialized(syncer::SyncService* sync_service) override {}
+  void RecordUseOf(const AutofillDataModel& data_model) override;
+  std::string SaveImportedProfile(
+      const AutofillProfile& imported_profile) override;
+  std::string SaveImportedCreditCard(
+      const CreditCard& imported_credit_card) override;
+  void AddProfile(const AutofillProfile& profile) override;
+  void RemoveByGUID(const std::string& guid) override;
+  void AddCreditCard(const CreditCard& credit_card) override;
+  void AddFullServerCreditCard(const CreditCard& credit_card) override;
+  std::vector<AutofillProfile*> GetProfiles() const override;
+  std::vector<CreditCard*> GetCreditCards() const override;
+  const std::string& GetDefaultCountryCodeForNewAddress() const override;
+  std::string CountryCodeForCurrentTimezone() const override;
 
-  // Sets which PrefService to use and observe. |pref_service| is not owned by
-  // this class and must outlive |this|.
-  void SetTestingPrefService(PrefService* pref_service);
+  // Unique to TestPersonalDataManager:
 
-  // Adds |profile| to |profiles_|. This does not take ownership of |profile|.
-  void AddTestingProfile(AutofillProfile* profile);
+  // Clears |web_profiles_|.
+  void ClearProfiles();
 
-  // Adds |credit_card| to |credit_cards_|. This does not take ownership of
-  // |credit_card|.
-  void AddTestingCreditCard(CreditCard* credit_card);
+  // Clears |local_credit_cards_| and |server_credit_cards_|.
+  void ClearCreditCards();
 
-  // Adds |credit_card| to |server_credit_cards_| by copying.
-  void AddTestingServerCreditCard(const CreditCard& credit_card);
+  // Gets a profile based on the provided |guid|.
+  AutofillProfile* GetProfileWithGUID(const char* guid);
 
-  std::vector<AutofillProfile*> GetProfiles() const override;
-  std::vector<CreditCard*> GetCreditCards() const override;
-
-  std::string SaveImportedProfile(
-      const AutofillProfile& imported_profile) override;
-  std::string SaveImportedCreditCard(
-      const CreditCard& imported_credit_card) override;
+  // Gets a credit card based on the provided |guid| (local or server).
+  CreditCard* GetCreditCardWithGUID(const char* guid);
 
-  std::string CountryCodeForCurrentTimezone() const override;
-  const std::string& GetDefaultCountryCodeForNewAddress() const override;
+  // Adds a card to |server_credit_cards_|.  Functionally identical to
+  // AddFullServerCreditCard().
+  void AddServerCreditCard(const CreditCard& credit_card);
 
   void set_timezone_country_code(const std::string& timezone_country_code) {
     timezone_country_code_ = timezone_country_code;
@@ -58,16 +65,16 @@ class TestPersonalDataManager : public PersonalDataManager {
     default_country_code_ = default_country_code;
   }
 
-  const AutofillProfile& imported_profile() { return imported_profile_; }
-  const CreditCard& imported_credit_card() { return imported_credit_card_; }
+  int num_times_save_imported_profile_called() const {
+    return num_times_save_imported_profile_called_;
+  }
 
  private:
-  std::vector<AutofillProfile*> profiles_;
-  std::vector<CreditCard*> credit_cards_;
-  AutofillProfile imported_profile_;
-  CreditCard imported_credit_card_;
   std::string timezone_country_code_;
   std::string default_country_code_;
+  int num_times_save_imported_profile_called_ = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(TestPersonalDataManager);
 };
 
 }  // namespace autofill

components/payments/content/payment_request_state_unittest.cc

@@ -33,10 +33,10 @@ class PaymentRequestStateTest : public testing::Test,
                         test_payment_request_delegate_.GetUkmRecorder()),
         address_(autofill::test::GetFullProfile()),
         credit_card_visa_(autofill::test::GetCreditCard()) {
-    test_personal_data_manager_.AddTestingProfile(&address_);
+    test_personal_data_manager_.AddProfile(address_);
     credit_card_visa_.set_billing_address_id(address_.guid());
     credit_card_visa_.set_use_count(5u);
-    test_personal_data_manager_.AddTestingCreditCard(&credit_card_visa_);
+    test_personal_data_manager_.AddCreditCard(credit_card_visa_);
   }
   ~PaymentRequestStateTest() override {}
 

components/payments/content/payment_response_helper_unittest.cc

@@ -30,9 +30,9 @@ class PaymentResponseHelperTest : public testing::Test,
       : test_payment_request_delegate_(&test_personal_data_manager_),
         address_(autofill::test::GetFullProfile()),
         billing_addresses_({&address_}) {
-    test_personal_data_manager_.AddTestingProfile(&address_);
+    test_personal_data_manager_.AddProfile(address_);
 
-    // Setup the autofill payment instrument.
+    // Set up the autofill payment instrument.
     autofill::CreditCard visa_card = autofill::test::GetCreditCard();
     visa_card.set_billing_address_id(address_.guid());
     visa_card.set_use_count(5u);

ios/chrome/browser/payments/payment_request_unittest_base.h

@@ -42,8 +42,8 @@ class PaymentRequestUnitTestBase {
   // and/or AddCreditCard.
   void CreateTestPaymentRequest();
 
-  void AddAutofillProfile(autofill::AutofillProfile profile);
-  void AddCreditCard(autofill::CreditCard card);
+  void AddAutofillProfile(const autofill::AutofillProfile& profile);
+  void AddCreditCard(const autofill::CreditCard& card);
 
   SigninManager* GetSigninManager();
 
@@ -58,19 +58,14 @@ class PaymentRequestUnitTestBase {
   TestChromeBrowserState* browser_state() {
     return chrome_browser_state_.get();
   }
-  const std::vector<std::unique_ptr<autofill::AutofillProfile>>& profiles()
-      const {
-    return profiles_;
+  std::vector<autofill::AutofillProfile*> profiles() const {
+    return personal_data_manager_.GetProfiles();
   }
-  const std::vector<std::unique_ptr<autofill::CreditCard>>& credit_cards()
-      const {
-    return cards_;
+  std::vector<autofill::CreditCard*> credit_cards() const {
+    return personal_data_manager_.GetCreditCards();
   }
 
  private:
-  std::vector<std::unique_ptr<autofill::AutofillProfile>> profiles_;
-  std::vector<std::unique_ptr<autofill::CreditCard>> cards_;
-
   web::TestWebThreadBundle web_thread_bundle_;
   web::TestWebState web_state_;
   std::unique_ptr<PrefService> pref_service_;

ios/chrome/browser/payments/payment_request_unittest_base.mm

@@ -27,11 +27,11 @@ void PaymentRequestUnitTestBase::SetUp() {
                                      &ios::BuildFakeSigninManager);
   chrome_browser_state_ = test_cbs_builder.Build();
   web_state_.SetBrowserState(chrome_browser_state_.get());
-  personal_data_manager_.SetTestingPrefService(pref_service_.get());
+  personal_data_manager_.SetPrefService(pref_service_.get());
 }
 
 void PaymentRequestUnitTestBase::TearDown() {
-  personal_data_manager_.SetTestingPrefService(nullptr);
+  personal_data_manager_.SetPrefService(nullptr);
 }
 
 void PaymentRequestUnitTestBase::CreateTestPaymentRequest() {
@@ -42,15 +42,13 @@ void PaymentRequestUnitTestBase::CreateTestPaymentRequest() {
 }
 
 void PaymentRequestUnitTestBase::AddAutofillProfile(
-    autofill::AutofillProfile profile) {
-  profiles_.push_back(
-      base::MakeUnique<autofill::AutofillProfile>(std::move(profile)));
-  personal_data_manager_.AddTestingProfile(profiles_.back().get());
+    const autofill::AutofillProfile& profile) {
+  personal_data_manager_.AddProfile(profile);
 }
 
-void PaymentRequestUnitTestBase::AddCreditCard(autofill::CreditCard card) {
-  cards_.push_back(base::MakeUnique<autofill::CreditCard>(std::move(card)));
-  personal_data_manager_.AddTestingCreditCard(cards_.back().get());
+void PaymentRequestUnitTestBase::AddCreditCard(
+    const autofill::CreditCard& card) {
+  personal_data_manager_.AddCreditCard(card);
 }
 
 SigninManager* PaymentRequestUnitTestBase::GetSigninManager() {

ios/chrome/browser/payments/payment_response_helper_unittest.mm

@@ -66,7 +66,7 @@ class PaymentRequestPaymentResponseHelperTest : public PlatformTest {
       : profile_(autofill::test::GetFullProfile()),
         credit_card_(autofill::test::GetCreditCard()),
         chrome_browser_state_(TestChromeBrowserState::Builder().Build()) {
-    personal_data_manager_.AddTestingProfile(&profile_);
+    personal_data_manager_.AddProfile(profile_);
     payment_request_ = base::MakeUnique<TestPaymentRequest>(
         payment_request_test_util::CreateTestWebPaymentRequest(),
         chrome_browser_state_.get(), &web_state_, &personal_data_manager_);

ios/chrome/browser/ui/payments/address_edit_coordinator_unittest.mm

@@ -114,7 +114,7 @@ class PaymentRequestAddressEditCoordinatorTest
     PaymentRequestUnitTestBase::SetUp();
 
     autofill::CountryNames::SetLocaleString("en-US");
-    personal_data_manager_.SetTestingPrefService(pref_service());
+    personal_data_manager_.SetPrefService(pref_service());
 
     payment_request_ = base::MakeUnique<MockTestPaymentRequest>(
         payment_request_test_util::CreateTestWebPaymentRequest(),
@@ -129,7 +129,7 @@ class PaymentRequestAddressEditCoordinatorTest
   }
 
   void TearDown() override {
-    personal_data_manager_.SetTestingPrefService(nullptr);
+    personal_data_manager_.SetPrefService(nullptr);
 
     PaymentRequestUnitTestBase::TearDown();
   }

ios/chrome/browser/ui/payments/contact_info_edit_coordinator_unittest.mm

@@ -103,7 +103,7 @@ class PaymentRequestContactInfoEditCoordinatorTest
   void SetUp() override {
     PaymentRequestUnitTestBase::SetUp();
 
-    personal_data_manager_.SetTestingPrefService(pref_service());
+    personal_data_manager_.SetPrefService(pref_service());
 
     payment_request_ = base::MakeUnique<MockTestPaymentRequest>(
         payment_request_test_util::CreateTestWebPaymentRequest(),
@@ -118,7 +118,7 @@ class PaymentRequestContactInfoEditCoordinatorTest
   }
 
   void TearDown() override {
-    personal_data_manager_.SetTestingPrefService(nullptr);
+    personal_data_manager_.SetPrefService(nullptr);
 
     PaymentRequestUnitTestBase::TearDown();
   }

ios/chrome/browser/ui/payments/payment_request_coordinator_unittest.mm

@@ -170,7 +170,7 @@ TEST_F(PaymentRequestCoordinatorTest, DidSelectShippingAddress) {
 
   // Call the ShippingAddressSelectionCoordinator delegate method.
   [coordinator shippingAddressSelectionCoordinator:nil
-                          didSelectShippingAddress:profiles().back().get()];
+                          didSelectShippingAddress:profiles().back()];
 }
 
 // Tests that calling the ShippingOptionSelectionCoordinator delegate method