diff --git a/components/autofill/core/browser/personal_data_manager.cc b/components/autofill/core/browser/personal_data_manager.cc index b00583d6e1c008..08afb7e35e4ea8 100644 --- a/components/autofill/core/browser/personal_data_manager.cc +++ b/components/autofill/core/browser/personal_data_manager.cc @@ -796,13 +796,28 @@ std::vector PersonalDataManager::GetCreditCardSuggestions( if (IsInAutofillSuggestionsDisabledExperiment()) return std::vector(); - std::list cards_to_suggest; - - GetOrderedCardsToSuggest(type, field_contents, &cards_to_suggest); + const std::vector credit_cards = GetCreditCards(); + std::list cards_to_suggest(credit_cards.begin(), + credit_cards.end()); DedupeCreditCardToSuggest(&cards_to_suggest); - return GetSuggestionsForCards(cards_to_suggest, type); + // Rank the cards by frecency (see AutofillDataModel for details). All expired + // cards should be suggested last, also by frecency. + base::Time comparison_time = base::Time::Now(); + cards_to_suggest.sort( + [comparison_time](const CreditCard* a, const CreditCard* b) { + bool a_has_valid_expiration = IsValidCreditCardExpirationDate( + a->expiration_year(), a->expiration_month(), comparison_time); + if (a_has_valid_expiration != + IsValidCreditCardExpirationDate( + b->expiration_year(), b->expiration_month(), comparison_time)) + return a_has_valid_expiration; + + return a->CompareFrecency(b, comparison_time); + }); + + return GetSuggestionsForCards(type, field_contents, cards_to_suggest); } bool PersonalDataManager::IsAutofillEnabled() const { @@ -1424,13 +1439,14 @@ const std::vector& PersonalDataManager::GetProfiles( return profiles_; } -void PersonalDataManager::GetOrderedCardsToSuggest( +std::vector PersonalDataManager::GetSuggestionsForCards( const AutofillType& type, const base::string16& field_contents, - std::list* cards_to_suggest) const { + const std::list& cards_to_suggest) const { + std::vector suggestions; std::list substring_matched_cards; base::string16 field_contents_lower = base::i18n::ToLower(field_contents); - for (const CreditCard* credit_card : GetCreditCards()) { + for (const CreditCard* credit_card : cards_to_suggest) { // The value of the stored data for this field type in the |credit_card|. base::string16 creditcard_field_value = credit_card->GetInfo(type, app_locale_); @@ -1444,69 +1460,51 @@ void PersonalDataManager::GetOrderedCardsToSuggest( creditcard_field_lower, field_contents_lower, type, credit_card->record_type() == CreditCard::MASKED_SERVER_CARD, &prefix_matched_suggestion)) { - if (prefix_matched_suggestion) { - cards_to_suggest->push_back(credit_card); + // Make a new suggestion. + suggestions.push_back(Suggestion()); + Suggestion* suggestion = &suggestions.back(); + + suggestion->value = credit_card->GetInfo(type, app_locale_); + suggestion->icon = base::UTF8ToUTF16(credit_card->type()); + suggestion->backend_id = credit_card->guid(); + suggestion->match = prefix_matched_suggestion + ? Suggestion::PREFIX_MATCH + : Suggestion::SUBSTRING_MATCH; + + // If the value is the card number, the label is the expiration date. + // Otherwise the label is the card number, or if that is empty the + // cardholder name. The label should never repeat the value. + if (type.GetStorableType() == CREDIT_CARD_NUMBER) { + suggestion->value = credit_card->TypeAndLastFourDigits(); + suggestion->label = credit_card->GetInfo( + AutofillType(CREDIT_CARD_EXP_DATE_2_DIGIT_YEAR), app_locale_); + } else if (credit_card->number().empty()) { + if (type.GetStorableType() != CREDIT_CARD_NAME_FULL) { + suggestion->label = credit_card->GetInfo( + AutofillType(CREDIT_CARD_NAME_FULL), app_locale_); + } } else { - substring_matched_cards.push_back(credit_card); +#if defined(OS_ANDROID) + // Since Android places the label on its own row, there's more + // horizontal + // space to work with. Show "Amex - 1234" rather than desktop's "*1234". + suggestion->label = credit_card->TypeAndLastFourDigits(); +#else + suggestion->label = base::ASCIIToUTF16("*"); + suggestion->label.append(credit_card->LastFourDigits()); +#endif } } } - // Rank the cards by frecency (see AutofillDataModel for details). - base::Time comparison_time = base::Time::Now(); - cards_to_suggest->sort([comparison_time](const AutofillDataModel* a, - const AutofillDataModel* b) { - return a->CompareFrecency(b, comparison_time); - }); - // Prefix matches should precede other token matches. if (IsFeatureSubstringMatchEnabled()) { - substring_matched_cards.sort([comparison_time](const AutofillDataModel* a, - const AutofillDataModel* b) { - return a->CompareFrecency(b, comparison_time); - }); - cards_to_suggest->insert(cards_to_suggest->end(), - substring_matched_cards.begin(), - substring_matched_cards.end()); + std::stable_sort(suggestions.begin(), suggestions.end(), + [](const Suggestion& a, const Suggestion& b) { + return a.match < b.match; + }); } -} -std::vector PersonalDataManager::GetSuggestionsForCards( - const std::list& cards_to_suggest, - const AutofillType& type) const { - std::vector suggestions; - for (const CreditCard* credit_card : cards_to_suggest) { - // Make a new suggestion. - suggestions.push_back(Suggestion()); - Suggestion* suggestion = &suggestions.back(); - - suggestion->value = credit_card->GetInfo(type, app_locale_); - suggestion->icon = base::UTF8ToUTF16(credit_card->type()); - suggestion->backend_id = credit_card->guid(); - - // If the value is the card number, the label is the expiration date. - // Otherwise the label is the card number, or if that is empty the - // cardholder name. The label should never repeat the value. - if (type.GetStorableType() == CREDIT_CARD_NUMBER) { - suggestion->value = credit_card->TypeAndLastFourDigits(); - suggestion->label = credit_card->GetInfo( - AutofillType(CREDIT_CARD_EXP_DATE_2_DIGIT_YEAR), app_locale_); - } else if (credit_card->number().empty()) { - if (type.GetStorableType() != CREDIT_CARD_NAME_FULL) { - suggestion->label = credit_card->GetInfo( - AutofillType(CREDIT_CARD_NAME_FULL), app_locale_); - } - } else { -#if defined(OS_ANDROID) - // Since Android places the label on its own row, there's more horizontal - // space to work with. Show "Amex - 1234" rather than desktop's "*1234". - suggestion->label = credit_card->TypeAndLastFourDigits(); -#else - suggestion->label = base::ASCIIToUTF16("*"); - suggestion->label.append(credit_card->LastFourDigits()); -#endif - } - } return suggestions; } diff --git a/components/autofill/core/browser/personal_data_manager.h b/components/autofill/core/browser/personal_data_manager.h index 73201933cb320a..d1fb48caa2777f 100644 --- a/components/autofill/core/browser/personal_data_manager.h +++ b/components/autofill/core/browser/personal_data_manager.h @@ -390,19 +390,12 @@ class PersonalDataManager : public KeyedService, const std::vector& GetProfiles( bool record_metrics) const; - // Fills |cards_to_suggest| with valid credit cards to suggest based on the - // |type| and |field_contents| of the credit card field. The cards are ordered - // by frecency. - void GetOrderedCardsToSuggest( + // Returns credit card suggestions based on the |cards_to_suggest| and the + // |type| and |field_contents| of the credit card field. + std::vector GetSuggestionsForCards( const AutofillType& type, const base::string16& field_contents, - std::list* cards_to_suggest) const; - - // Returns the suggestions to display for the |cards_to_suggest| based on the - // |type| of the credit card field. - std::vector GetSuggestionsForCards( - const std::list& cards_to_suggest, - const AutofillType& type) const; + const std::list& cards_to_suggest) const; const std::string app_locale_; diff --git a/components/autofill/core/browser/personal_data_manager_unittest.cc b/components/autofill/core/browser/personal_data_manager_unittest.cc index 4309097041b644..f58e77a9843b85 100644 --- a/components/autofill/core/browser/personal_data_manager_unittest.cc +++ b/components/autofill/core/browser/personal_data_manager_unittest.cc @@ -250,7 +250,7 @@ class PersonalDataManagerTest : public testing::Test { credit_card2.set_use_count(1); credit_card2.set_use_date(base::Time::Now() - base::TimeDelta::FromDays(1)); test::SetCreditCardInfo(&credit_card2, "Bonnie Parker", - "518765432109" /* Mastercard */, "", ""); + "518765432109" /* Mastercard */, "12", "3999"); personal_data_->AddCreditCard(credit_card2); EXPECT_CALL(personal_data_observer_, OnPersonalDataChanged()) @@ -3340,6 +3340,56 @@ TEST_F(PersonalDataManagerTest, EXPECT_EQ(ASCIIToUTF16("Bonnie Parker"), suggestions[4].value); } +// Test that expired cards are ordered by frecency and are always suggested +// after non expired cards even if they have a higher frecency score. +TEST_F(PersonalDataManagerTest, GetCreditCardSuggestions_ExpiredCards) { + ASSERT_EQ(0U, personal_data_->GetCreditCards().size()); + + // Add a never used non expired credit card. + CreditCard credit_card0("002149C1-EE28-4213-A3B9-DA243FFF021B", + "https://www.example.com"); + test::SetCreditCardInfo(&credit_card0, "Bonnie Parker", + "518765432109" /* Mastercard */, "04", "2999"); + personal_data_->AddCreditCard(credit_card0); + + // Add an expired card with a higher frecency score. + CreditCard credit_card1("287151C8-6AB1-487C-9095-28E80BE5DA15", + "https://www.example.com"); + test::SetCreditCardInfo(&credit_card1, "Clyde Barrow", + "347666888555" /* American Express */, "04", "1999"); + credit_card1.set_use_count(300); + credit_card1.set_use_date(base::Time::Now() - base::TimeDelta::FromDays(10)); + personal_data_->AddCreditCard(credit_card1); + + // Add an expired card with a lower frecency score. + CreditCard credit_card2("1141084B-72D7-4B73-90CF-3D6AC154673B", + "https://www.example.com"); + credit_card2.set_use_count(3); + credit_card2.set_use_date(base::Time::Now() - base::TimeDelta::FromDays(1)); + test::SetCreditCardInfo(&credit_card2, "John Dillinger", + "423456789012" /* Visa */, "01", "1998"); + personal_data_->AddCreditCard(credit_card2); + + EXPECT_CALL(personal_data_observer_, OnPersonalDataChanged()) + .WillOnce(QuitMainMessageLoop()); + base::MessageLoop::current()->Run(); + + ASSERT_EQ(3U, personal_data_->GetCreditCards().size()); + + std::vector suggestions = + personal_data_->GetCreditCardSuggestions( + AutofillType(CREDIT_CARD_NAME_FULL), + /* field_contents= */ base::string16()); + ASSERT_EQ(3U, suggestions.size()); + + // The never used non expired card should be suggested first. + EXPECT_EQ(ASCIIToUTF16("Bonnie Parker"), suggestions[0].value); + + // The expired cards should be sorted by frecency + EXPECT_EQ(ASCIIToUTF16("Clyde Barrow"), suggestions[1].value); + EXPECT_EQ(ASCIIToUTF16("John Dillinger"), suggestions[2].value); +} + // Test that a card that doesn't have a number is not shown in the suggestions // when querying credit cards by their number. TEST_F(PersonalDataManagerTest, GetCreditCardSuggestions_NumberMissing) {