Don't offer to save/upload credit cards that matched masked cards.

Historically, we've offered to save cards when they match a masked card because it's possible that it's actually a different card. However, these circumstances are unlikely and the same circumstances arise now that we allow upload of local cards. If the user has either already uploaded a local card or they have the same card saved locally and entered in Payments from another source, they will be asked to upload this card even though it's already present as a masked card.

The crash regression tested for in DontOfferToSavePaymentsCard is no longer possible since we don't check recently_unmasked_cards_ so I've removed that.

BUG=535784

Review URL: https://codereview.chromium.org/1698543002

Cr-Commit-Position: refs/heads/master@{#375368}

5 files changed, 10 insertions, 45 deletions

diff --git a/components/autofill/core/browser/autofill_manager.cc b/components/autofill/core/browser/autofill_manager.cc
index f767e77..acc92f3 100644
--- a/components/autofill/core/browser/autofill_manager.cc
+++ b/components/autofill/core/browser/autofill_manager.cc
@@ -278,8 +278,6 @@ bool AutofillManager::OnFormSubmitted(const FormData& form) {
   if (submitted_form->IsAutofillable())
     ImportFormData(*submitted_form);
 
-  recently_unmasked_cards_.clear();
-
   return true;
 }
 
@@ -835,7 +833,6 @@ void AutofillManager::OnDidGetRealPan(AutofillClient::PaymentsRpcResult result,
   if (!real_pan.empty()) {
     DCHECK_EQ(AutofillClient::SUCCESS, result);
     credit_card_form_event_logger_->OnDidFillSuggestion(unmask_request_.card);
-    recently_unmasked_cards_.push_back(unmask_request_.card);
     unmask_request_.card.set_record_type(CreditCard::FULL_SERVER_CARD);
     unmask_request_.card.SetNumber(base::UTF8ToUTF16(real_pan));
     if (!unmask_request_.user_response.exp_month.empty()) {
@@ -990,13 +987,6 @@ void AutofillManager::ImportFormData(const FormStructure& submitted_form) {
   if (!imported_credit_card)
     return;
 
-  // Don't offer to save any cards that were recently unmasked.
-  for (const CreditCard& unmasked_card : recently_unmasked_cards_) {
-    if (unmasked_card.TypeAndLastFourDigits() ==
-        imported_credit_card->TypeAndLastFourDigits())
-      return;
-  }
-
   if (!IsCreditCardUploadEnabled()) {
     // This block will only be reached if we have observed a new card. In this
     // case, ImportFormData will return false if the card matches one already
diff --git a/components/autofill/core/browser/autofill_manager.h b/components/autofill/core/browser/autofill_manager.h
index 7f21268..26d8cee 100644
--- a/components/autofill/core/browser/autofill_manager.h
+++ b/components/autofill/core/browser/autofill_manager.h
@@ -493,10 +493,6 @@ class AutofillManager : public AutofillDownloadManager::Observer,
   payments::PaymentsClient::UploadRequestDetails upload_request_;
   bool user_did_accept_upload_prompt_;
 
-  // Masked copies of recently unmasked cards, to help avoid double-asking to
-  // save the card (in the unmask prompt and in the save prompt after submit).
-  std::vector<CreditCard> recently_unmasked_cards_;
-
 #ifdef ENABLE_FORM_DEBUG_DUMP
   // The last few autofilled forms (key/value pairs) submitted, for debugging.
   // TODO(brettw) this should be removed. See DumpAutofillData.
diff --git a/components/autofill/core/browser/autofill_manager_unittest.cc b/components/autofill/core/browser/autofill_manager_unittest.cc
index 7e83d99..7d77bf5 100644
--- a/components/autofill/core/browser/autofill_manager_unittest.cc
+++ b/components/autofill/core/browser/autofill_manager_unittest.cc
@@ -901,7 +901,6 @@ class AutofillManagerTest : public testing::Test {
                             "2017");
     card->SetTypeForMaskedCard(kVisaCard);
 
-    EXPECT_CALL(autofill_client_, ConfirmSaveCreditCardLocally(_, _)).Times(0);
     EXPECT_CALL(*autofill_driver_, SendFormDataToRenderer(_, _, _))
         .Times(AtLeast(1));
     autofill_manager_->FillOrPreviewCreditCardForm(
@@ -3833,25 +3832,6 @@ TEST_F(AutofillManagerTest, DontOfferToSavePaymentsCard) {
   autofill_manager_->OnDidGetRealPan(AutofillClient::SUCCESS,
                                      "4012888888881881");
   autofill_manager_->OnFormSubmitted(form);
-
-  // The rest of this test is a regression test for http://crbug.com/483602.
-  // The goal is not to crash.
-  EXPECT_CALL(*autofill_driver_, SendFormDataToRenderer(_, _, _));
-  for (size_t i = 0; i < form.fields.size(); ++i) {
-    form.fields[i].value.clear();
-  }
-  autofill_manager_->FillOrPreviewCreditCardForm(
-      AutofillDriver::FORM_DATA_ACTION_FILL, kDefaultPageID, form,
-      form.fields[1], card);
-  autofill_manager_->OnUnmaskResponse(response);
-  autofill_manager_->OnDidGetRealPan(AutofillClient::SUCCESS,
-                                     "4012888888881881");
-
-  form = FormData();
-  test::CreateTestAddressFormData(&form);
-  FormsSeen(std::vector<FormData>(1, form));
-  ManuallyFillAddressForm("Flo", "Master", "77401", "US", &form);
-  autofill_manager_->OnFormSubmitted(form);
 }
 
 TEST_F(AutofillManagerTest, FillInUpdatedExpirationDate) {
diff --git a/components/autofill/core/browser/personal_data_manager.cc b/components/autofill/core/browser/personal_data_manager.cc
index 315cec1..d2e69a1 100644
--- a/components/autofill/core/browser/personal_data_manager.cc
+++ b/components/autofill/core/browser/personal_data_manager.cc
@@ -1394,13 +1394,9 @@ bool PersonalDataManager::ImportCreditCard(
     }
   }
 
-  // Also don't offer to save if we already have this stored as a full wallet
-  // card. Note that we will offer to save masked server cards, as long as the
-  // user re-typed the info by hand. See AutofillManager's
-  // |recently_unmasked_cards_|.
+  // Also don't offer to save if we already have this stored as a server card.
   for (const CreditCard* card : server_credit_cards_) {
-    if (card->record_type() == CreditCard::FULL_SERVER_CARD &&
-        candidate_credit_card.IsLocalDuplicateOfServerCard(*card))
+    if (candidate_credit_card.IsLocalDuplicateOfServerCard(*card))
       return false;
   }
 
diff --git a/components/autofill/core/browser/personal_data_manager_unittest.cc b/components/autofill/core/browser/personal_data_manager_unittest.cc
index 33358a5..28deb80 100644
--- a/components/autofill/core/browser/personal_data_manager_unittest.cc
+++ b/components/autofill/core/browser/personal_data_manager_unittest.cc
@@ -3581,8 +3581,10 @@ TEST_F(PersonalDataManagerTest, DontDuplicateServerCard) {
       .WillOnce(QuitMainMessageLoop());
   base::MessageLoop::current()->Run();
 
-  // A valid credit card form. A user re-types one of their masked cards.
-  // We should offer to save.
+  // A valid credit card form. A user re-enters one of their masked cards.
+  // We shouldn't offer to save. It's possible this is actually a different card
+  // but it's very unlikely. And these circumstances will also arise if the user
+  // has the same card available locally and synced from payments.
   FormData form1;
   FormFieldData field;
   test::CreateTestFormField("Name on card:", "name_on_card", "John Dillinger",
@@ -3599,12 +3601,13 @@ TEST_F(PersonalDataManagerTest, DontDuplicateServerCard) {
   FormStructure form_structure1(form1);
   form_structure1.DetermineHeuristicTypes();
   scoped_ptr<CreditCard> imported_credit_card;
-  EXPECT_TRUE(personal_data_->ImportFormData(form_structure1, false,
+  EXPECT_FALSE(personal_data_->ImportFormData(form_structure1, false,
                                              &imported_credit_card));
-  EXPECT_TRUE(imported_credit_card);
+  EXPECT_FALSE(imported_credit_card);
 
   // A user re-types (or fills with) an unmasked card. Don't offer to save
-  // again.
+  // here, either. Since it's unmasked, we know for certain that it's the same
+  // card.
   FormData form2;
   test::CreateTestFormField("Name on card:", "name_on_card", "Clyde Barrow",
                             "text", &field);