Don't show the "Save credit card for Autofill?" infobar unnecessarily.

BUG=98968, 98991
TEST=unit_tests --gtest_filter=PersonalDataManager.*CreditCard*

Review URL: http://codereview.chromium.org/8114031

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@103994 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 116 insertions, 49 deletions

diff --git a/chrome/browser/autofill/credit_card.cc b/chrome/browser/autofill/credit_card.cc
index 30f477d..4764ba42 100644
--- a/chrome/browser/autofill/credit_card.cc
+++ b/chrome/browser/autofill/credit_card.cc
@@ -420,6 +420,26 @@ void CreditCard::operator=(const CreditCard& credit_card) {
   guid_ = credit_card.guid_;
 }
 
+bool CreditCard::UpdateFromImportedCard(const CreditCard& imported_card) {
+  if (this->GetCanonicalizedInfo(CREDIT_CARD_NUMBER) !=
+          imported_card.GetCanonicalizedInfo(CREDIT_CARD_NUMBER)) {
+    return false;
+  }
+
+  // Note that the card number is intentionally not updated, so as to preserve
+  // any formatting (i.e. separator characters).  Since the card number is not
+  // updated, there is no reason to update the card type, either.
+  if (!imported_card.name_on_card_.empty())
+    name_on_card_ = imported_card.name_on_card_;
+
+  // The expiration date for |imported_card| should always be set.
+  DCHECK(imported_card.expiration_month_ && imported_card.expiration_year_);
+  expiration_month_ = imported_card.expiration_month_;
+  expiration_year_ = imported_card.expiration_year_;
+
+  return true;
+}
+
 int CreditCard::Compare(const CreditCard& credit_card) const {
   // The following CreditCard field types are the only types we store in the
   // WebDB so far, so we're only concerned with matching these types in the
diff --git a/chrome/browser/autofill/credit_card.h b/chrome/browser/autofill/credit_card.h
index 965429b4..575573c 100644
--- a/chrome/browser/autofill/credit_card.h
+++ b/chrome/browser/autofill/credit_card.h
@@ -52,6 +52,12 @@ class CreditCard : public FormGroup {
   // For use in STL containers.
   void operator=(const CreditCard& credit_card);
 
+  // If the card numbers for |this| and |imported_card| match, overwrites |this|
+  // card's data with the data in |credit_card| and returns true.  Otherwise,
+  // returns false.
+  bool UpdateFromImportedCard(const CreditCard& imported_card)
+      WARN_UNUSED_RESULT;
+
   // Comparison for Sync.  Returns 0 if the credit card is the same as |this|,
   // or < 0, or > 0 if it is different.  The implied ordering can be used for
   // culling duplicates.  The ordering is based on collation order of the
diff --git a/chrome/browser/autofill/form_group.cc b/chrome/browser/autofill/form_group.cc
index ae7f316..b4d0efd 100644
--- a/chrome/browser/autofill/form_group.cc
+++ b/chrome/browser/autofill/form_group.cc
@@ -102,24 +102,15 @@ bool FormGroup::IntersectionOfTypesHasEqualValues(
 }
 
 void FormGroup::MergeWith(const FormGroup& form_group) {
-  FieldTypeSet a, b, intersection;
+  FieldTypeSet a, b, difference;
   GetNonEmptyTypes(&a);
   form_group.GetNonEmptyTypes(&b);
   std::set_difference(b.begin(), b.end(),
                       a.begin(), a.end(),
-                      std::inserter(intersection, intersection.begin()));
-
-  for (FieldTypeSet::const_iterator iter = intersection.begin();
-       iter != intersection.end(); ++iter) {
-    SetInfo(*iter, form_group.GetInfo(*iter));
-  }
-}
-
-void FormGroup::OverwriteWith(const FormGroup& form_group) {
-  FieldTypeSet a;;
-  form_group.GetNonEmptyTypes(&a);
+                      std::inserter(difference, difference.begin()));
 
-  for (FieldTypeSet::const_iterator iter = a.begin(); iter != a.end(); ++iter) {
+  for (FieldTypeSet::const_iterator iter = difference.begin();
+       iter != difference.end(); ++iter) {
     SetInfo(*iter, form_group.GetInfo(*iter));
   }
 }
diff --git a/chrome/browser/autofill/form_group.h b/chrome/browser/autofill/form_group.h
index d940682..1ce0430 100644
--- a/chrome/browser/autofill/form_group.h
+++ b/chrome/browser/autofill/form_group.h
@@ -63,9 +63,6 @@ class FormGroup {
   // Merges the field data in |form_group| with this FormGroup.
   void MergeWith(const FormGroup& form_group);
 
-  // Overwrites the field data in |form_group| with this FormGroup.
-  void OverwriteWith(const FormGroup& form_group);
-
  protected:
   // AutofillProfile needs to call into GetSupportedTypes() for objects of
   // non-AutofillProfile type, for which mere inheritance is insufficient.
diff --git a/chrome/browser/autofill/personal_data_manager.cc b/chrome/browser/autofill/personal_data_manager.cc
index b063b3a..db27ffb 100644
--- a/chrome/browser/autofill/personal_data_manager.cc
+++ b/chrome/browser/autofill/personal_data_manager.cc
@@ -307,11 +307,15 @@ bool PersonalDataManager::ImportFormData(
   }
 
   // Don't import if we already have this info.
+  // Don't present an infobar if we have already saved this card number.
+  bool merged_credit_card = false;
   if (local_imported_credit_card.get()) {
     for (std::vector<CreditCard*>::const_iterator iter = credit_cards_.begin();
          iter != credit_cards_.end();
          ++iter) {
-      if (local_imported_credit_card->IsSubsetOf(**iter)) {
+      if ((*iter)->UpdateFromImportedCard(*local_imported_credit_card.get())) {
+        merged_credit_card = true;
+        UpdateCreditCard(**iter);
         local_imported_credit_card.reset();
         break;
       }
@@ -324,7 +328,7 @@ bool PersonalDataManager::ImportFormData(
   }
   *imported_credit_card = local_imported_credit_card.release();
 
-  if (imported_profile.get() || *imported_credit_card) {
+  if (imported_profile.get() || *imported_credit_card || merged_credit_card) {
     return true;
   } else {
     FOR_EACH_OBSERVER(PersonalDataManagerObserver, observers_,
@@ -845,8 +849,12 @@ void PersonalDataManager::CancelPendingQuery(WebDataService::Handle* handle) {
 
 void PersonalDataManager::SaveImportedProfile(
     const AutofillProfile& imported_profile) {
-  if (profile_->IsOffTheRecord())
+  if (profile_->IsOffTheRecord()) {
+    // The |IsOffTheRecord| check should happen earlier in the import process,
+    // upon form submission.
+    NOTREACHED();
     return;
+  }
 
   // Don't save a web profile if the data in the profile is a subset of an
   // auxiliary profile.
@@ -865,34 +873,27 @@ void PersonalDataManager::SaveImportedProfile(
 
 void PersonalDataManager::SaveImportedCreditCard(
     const CreditCard& imported_credit_card) {
-  if (profile_->IsOffTheRecord())
+  DCHECK(!imported_credit_card.number().empty());
+  if (profile_->IsOffTheRecord()) {
+    // The |IsOffTheRecord| check should happen earlier in the import process,
+    // upon form submission.
+    NOTREACHED();
     return;
+  }
 
   // Set to true if |imported_credit_card| is merged into the credit card list.
   bool merged = false;
 
   std::vector<CreditCard> creditcards;
-  for (std::vector<CreditCard*>::const_iterator iter = credit_cards_.begin();
-       iter != credit_cards_.end();
-       ++iter) {
-    if (imported_credit_card.IsSubsetOf(**iter)) {
-      // In this case, the existing credit card already contains all of the data
-      // in |imported_credit_card|, so consider the credit cards already
-      // merged.
-      merged = true;
-    } else if ((*iter)->IntersectionOfTypesHasEqualValues(
-        imported_credit_card)) {
-      // |imported_credit_card| contains all of the data in this credit card,
-      // plus more.
+  for (std::vector<CreditCard*>::const_iterator card = credit_cards_.begin();
+       card != credit_cards_.end();
+       ++card) {
+    // If |imported_credit_card| has not yet been merged, check whether it
+    // should be with the current |card|.
+    if (!merged && (*card)->UpdateFromImportedCard(imported_credit_card))
       merged = true;
-      (*iter)->MergeWith(imported_credit_card);
-    } else if (!imported_credit_card.number().empty() &&
-               (*iter)->number() == imported_credit_card.number()) {
-      merged = true;
-      (*iter)->OverwriteWith(imported_credit_card);
-    }
 
-    creditcards.push_back(**iter);
+    creditcards.push_back(**card);
   }
 
   if (!merged)
diff --git a/chrome/browser/autofill/personal_data_manager_unittest.cc b/chrome/browser/autofill/personal_data_manager_unittest.cc
index 48bf5d2..4a7c43b 100644
--- a/chrome/browser/autofill/personal_data_manager_unittest.cc
+++ b/chrome/browser/autofill/personal_data_manager_unittest.cc
@@ -1386,9 +1386,7 @@ TEST_F(PersonalDataManagerTest, AggregateSameCreditCardWithConflict) {
   form_structure2.DetermineHeuristicTypes();
   EXPECT_TRUE(personal_data_->ImportFormData(form_structure2,
                                              &imported_credit_card));
-  ASSERT_TRUE(imported_credit_card);
-  personal_data_->SaveImportedCreditCard(*imported_credit_card);
-  delete imported_credit_card;
+  EXPECT_FALSE(imported_credit_card);
 
   // Verify that the web database has been updated and the notification sent.
   EXPECT_CALL(personal_data_observer_,
@@ -1528,11 +1526,14 @@ TEST_F(PersonalDataManagerTest, AggregateCreditCardWithMissingInfoInNew) {
 
   FormStructure form_structure2(form2);
   form_structure2.DetermineHeuristicTypes();
-  EXPECT_FALSE(personal_data_->ImportFormData(form_structure2,
-                                              &imported_credit_card));
-  ASSERT_FALSE(imported_credit_card);
+  EXPECT_TRUE(personal_data_->ImportFormData(form_structure2,
+                                             &imported_credit_card));
+  EXPECT_FALSE(imported_credit_card);
 
-  // Note: no refresh here.
+  // Wait for the refresh, which in this case is a no-op.
+  EXPECT_CALL(personal_data_observer_,
+              OnPersonalDataChanged()).WillOnce(QuitUIMessageLoop());
+  MessageLoop::current()->Run();
 
   // No change is expected.
   CreditCard expected2;
@@ -1609,9 +1610,7 @@ TEST_F(PersonalDataManagerTest, AggregateCreditCardWithMissingInfoInOld) {
   const CreditCard* imported_credit_card;
   EXPECT_TRUE(personal_data_->ImportFormData(form_structure,
                                              &imported_credit_card));
-  ASSERT_TRUE(imported_credit_card);
-  personal_data_->SaveImportedCreditCard(*imported_credit_card);
-  delete imported_credit_card;
+  EXPECT_FALSE(imported_credit_card);
 
   // Verify that the web database has been updated and the notification sent.
   EXPECT_CALL(personal_data_observer_,
@@ -1628,6 +1627,59 @@ TEST_F(PersonalDataManagerTest, AggregateCreditCardWithMissingInfoInOld) {
   EXPECT_EQ(0, expected2.Compare(*results2[0]));
 }
 
+// We allow the user to store a credit card number with separators via the UI.
+// We should not try to re-aggregate the same card with the separators stripped.
+TEST_F(PersonalDataManagerTest, AggregateSameCreditCardWithSeparators) {
+  // Start with a single valid credit card stored via the preferences.
+  // Note the separators in the credit card number.
+  CreditCard saved_credit_card;
+  autofill_test::SetCreditCardInfo(&saved_credit_card,
+      "Biggie Smalls", "4111 1111 1111 1111" /* Visa */, "01", "2011");
+  personal_data_->AddCreditCard(saved_credit_card);
+
+  // Verify that the web database has been updated and the notification sent.
+  EXPECT_CALL(personal_data_observer_,
+              OnPersonalDataChanged()).WillOnce(QuitUIMessageLoop());
+  MessageLoop::current()->Run();
+
+  const std::vector<CreditCard*>& results1 = personal_data_->credit_cards();
+  ASSERT_EQ(1U, results1.size());
+  EXPECT_EQ(0, saved_credit_card.Compare(*results1[0]));
+
+  // Import the same card info, but with different separators in the number.
+  FormData form;
+  webkit_glue::FormField field;
+  autofill_test::CreateTestFormField(
+      "Name on card:", "name_on_card", "Biggie Smalls", "text", &field);
+  form.fields.push_back(field);
+  autofill_test::CreateTestFormField(
+      "Card Number:", "card_number", "4111-1111-1111-1111", "text", &field);
+  form.fields.push_back(field);
+  autofill_test::CreateTestFormField(
+      "Exp Month:", "exp_month", "01", "text", &field);
+  form.fields.push_back(field);
+  autofill_test::CreateTestFormField(
+      "Exp Year:", "exp_year", "2011", "text", &field);
+  form.fields.push_back(field);
+
+  FormStructure form_structure(form);
+  form_structure.DetermineHeuristicTypes();
+  const CreditCard* imported_credit_card;
+  EXPECT_TRUE(personal_data_->ImportFormData(form_structure,
+                                             &imported_credit_card));
+  EXPECT_FALSE(imported_credit_card);
+
+  // Wait for the refresh, which in this case is a no-op.
+  EXPECT_CALL(personal_data_observer_,
+              OnPersonalDataChanged()).WillOnce(QuitUIMessageLoop());
+  MessageLoop::current()->Run();
+
+  // Expect that no new card is saved.
+  const std::vector<CreditCard*>& results2 = personal_data_->credit_cards();
+  ASSERT_EQ(1U, results2.size());
+  EXPECT_EQ(0, saved_credit_card.Compare(*results2[0]));
+}
+
 TEST_F(PersonalDataManagerTest, GetNonEmptyTypes) {
   // Check that there are no available types with no profiles stored.
   FieldTypeSet non_empty_types;