Skip to content

Commit

Permalink
They are currently sorted by frecency with the rest of the cards.
Browse files Browse the repository at this point in the history 
BUG=601165
TEST=PersonalDataManagerTest

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

Cr-Commit-Position: refs/heads/master@{#386098}
  • Loading branch information
@sebsg
sebsg authored and Commit bot committed Apr 8, 2016
1 parent d6385a7 commit 037e1d8
Show file tree
Hide file tree
Showing 3 changed files with 114 additions and 73 deletions.
120 changes: 59 additions & 61 deletions components/autofill/core/browser/personal_data_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -796,13 +796,28 @@ std::vector<Suggestion> PersonalDataManager::GetCreditCardSuggestions(
if (IsInAutofillSuggestionsDisabledExperiment())
return std::vector<Suggestion>();

std::list<const CreditCard*> cards_to_suggest;

GetOrderedCardsToSuggest(type, field_contents, &cards_to_suggest);
const std::vector<CreditCard*> credit_cards = GetCreditCards();
std::list<const CreditCard*> 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 {
Expand Down Expand Up @@ -1424,13 +1439,14 @@ const std::vector<AutofillProfile*>& PersonalDataManager::GetProfiles(
return profiles_;
}

void PersonalDataManager::GetOrderedCardsToSuggest(
std::vector<Suggestion> PersonalDataManager::GetSuggestionsForCards(
const AutofillType& type,
const base::string16& field_contents,
std::list<const CreditCard*>* cards_to_suggest) const {
const std::list<const CreditCard*>& cards_to_suggest) const {
std::vector<Suggestion> suggestions;
std::list<const CreditCard*> 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_);
Expand All @@ -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<Suggestion> PersonalDataManager::GetSuggestionsForCards(
const std::list<const CreditCard*>& cards_to_suggest,
const AutofillType& type) const {
std::vector<Suggestion> 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;
}

Expand Down
15 changes: 4 additions & 11 deletions components/autofill/core/browser/personal_data_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -390,19 +390,12 @@ class PersonalDataManager : public KeyedService,
const std::vector<AutofillProfile*>& 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<Suggestion> GetSuggestionsForCards(
const AutofillType& type,
const base::string16& field_contents,
std::list<const CreditCard*>* 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<Suggestion> GetSuggestionsForCards(
const std::list<const CreditCard*>& cards_to_suggest,
const AutofillType& type) const;
const std::list<const CreditCard*>& cards_to_suggest) const;

const std::string app_locale_;

Expand Down
52 changes: 51 additions & 1 deletion components/autofill/core/browser/personal_data_manager_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down Expand Up @@ -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<Suggestion> 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) {
Expand Down

0 comments on commit 037e1d8

Please sign in to comment.