requestAutocomplete - more elaborate suggestion labels

chrome/browser/autofill/credit_card.cc

Index: chrome/browser/autofill/credit_card.cc
diff --git a/chrome/browser/autofill/credit_card.cc b/chrome/browser/autofill/credit_card.cc
index 66d8105efef326142fb1436a0385e465c3345b43..7b1e986c2155fa72507c859c6ed7f05228dd649b 100644
--- a/chrome/browser/autofill/credit_card.cc
+++ b/chrome/browser/autofill/credit_card.cc
@@ -25,6 +25,7 @@
 #include "chrome/browser/autofill/validation.h"
 #include "chrome/common/form_field_data.h"
 #include "grit/generated_resources.h"
+#include "grit/webkit_resources.h"
 #include "third_party/icu/public/common/unicode/uloc.h"
 #include "third_party/icu/public/i18n/unicode/dtfmtsym.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -71,7 +72,7 @@ std::string GetCreditCardType(const string16& number) {
 
       break;
     case 14:
-      if (first_three_digits >= 300 && first_three_digits <=305)
+      if (first_three_digits >= 300 && first_three_digits <= 305)
         return kDinersCard;
 
       if (first_digit == 36)
@@ -117,34 +118,6 @@ std::string GetCreditCardType(const string16& number) {
   return kGenericCard;
 }
 
-string16 GetCreditCardTypeDisplayName(const std::string& card_type) {
-  if (card_type == kAmericanExpressCard)
-    return l10n_util::GetStringUTF16(IDS_AUTOFILL_CC_AMEX);
-
-  if (card_type == kDinersCard)
-    return l10n_util::GetStringUTF16(IDS_AUTOFILL_CC_DINERS);
-
-  if (card_type == kDiscoverCard)
-    return l10n_util::GetStringUTF16(IDS_AUTOFILL_CC_DISCOVER);
-
-  if (card_type == kJCBCard)
-    return l10n_util::GetStringUTF16(IDS_AUTOFILL_CC_JCB);
-
-  if (card_type == kMasterCard)
-    return l10n_util::GetStringUTF16(IDS_AUTOFILL_CC_MASTERCARD);
-
-  if (card_type == kSoloCard)
-    return l10n_util::GetStringUTF16(IDS_AUTOFILL_CC_SOLO);
-
-  if (card_type == kVisaCard)
-    return l10n_util::GetStringUTF16(IDS_AUTOFILL_CC_VISA);
-
-  // If you hit this DCHECK, the above list of cases needs to be updated to
-  // include a new card.
-  DCHECK_EQ(kGenericCard, card_type);
-  return string16();
-}
-
 bool ConvertYear(const string16& year, int* num) {
   // If the |year| is empty, clear the stored value.
   if (year.empty()) {
@@ -277,7 +250,7 @@ string16 CreditCard::GetRawInfo(AutofillFieldType type) const {
     }
 
     case CREDIT_CARD_TYPE:
-      return GetCreditCardTypeDisplayName(type_);
+      return TypeForDisplay();
 
     case CREDIT_CARD_NUMBER:
       return number_;
@@ -436,6 +409,63 @@ string16 CreditCard::LastFourDigits() const {
   return number.substr(number.size() - kNumLastDigits, kNumLastDigits);
 }
 
+string16 CreditCard::TypeForDisplay() const {
+  if (type_ == kAmericanExpressCard)
+    return l10n_util::GetStringUTF16(IDS_AUTOFILL_CC_AMEX);
+  if (type_ == kDinersCard)
+    return l10n_util::GetStringUTF16(IDS_AUTOFILL_CC_DINERS);
+  if (type_ == kDiscoverCard)
+    return l10n_util::GetStringUTF16(IDS_AUTOFILL_CC_DISCOVER);
+  if (type_ == kJCBCard)
+    return l10n_util::GetStringUTF16(IDS_AUTOFILL_CC_JCB);
+  if (type_ == kMasterCard)
+    return l10n_util::GetStringUTF16(IDS_AUTOFILL_CC_MASTERCARD);
+  if (type_ == kSoloCard)
+    return l10n_util::GetStringUTF16(IDS_AUTOFILL_CC_SOLO);
+  if (type_ == kVisaCard)
+    return l10n_util::GetStringUTF16(IDS_AUTOFILL_CC_VISA);
+
+  // If you hit this DCHECK, the above list of cases needs to be updated to
+  // include a new card.
+  DCHECK_EQ(kGenericCard, type_);
+  return string16();
+}
+
+string16 CreditCard::TypeAndLastFourDigits() const {
+  string16 type = TypeForDisplay();
+  // TODO(estade): type may be empty, we probably want to return
+  // "Card - 1234" or something in that case.
+
+  string16 digits = LastFourDigits();
+  if (digits.empty())
+    return type;
+
+  // TODO(estade): i18n.
+  return type + ASCIIToUTF16(" - ") + digits;
+}

[Ilya Sherman, 2013/01/24 04:27:27]

This method seems like it should be implemented by the UI code, rather than by
the CreditCard class.

[Evan Stade, 2013/01/24 23:34:25]

I wanted to keep this close to similar functions, i.e. all those functions which
are dealing with presentation and returning string16.

+
+int CreditCard::IconResourceId() const {
+  if (type_ == kAmericanExpressCard)
+    return IDR_AUTOFILL_CC_AMEX;
+  if (type_ == kDinersCard)
+    return IDR_AUTOFILL_CC_DINERS;
+  if (type_ == kDiscoverCard)
+    return IDR_AUTOFILL_CC_DISCOVER;
+  if (type_ == kJCBCard)
+    return IDR_AUTOFILL_CC_JCB;
+  if (type_ == kMasterCard)
+    return IDR_AUTOFILL_CC_MASTERCARD;
+  if (type_ == kSoloCard)
+    return IDR_AUTOFILL_CC_SOLO;
+  if (type_ == kVisaCard)
+    return IDR_AUTOFILL_CC_VISA;
+
+  // If you hit this DCHECK, the above list of cases needs to be updated to
+  // include a new card.
+  DCHECK_EQ(kGenericCard, type_);
+  return IDR_AUTOFILL_CC_GENERIC;
+}

[Ilya Sherman, 2013/01/24 04:27:27]

Can AutofillPopupControllerImpl::GetIconResourceID() be updated to use this
method as well, rather than re-defining the mapping?

[Evan Stade, 2013/01/24 23:34:25]

I actually spent a while looking at this, but I'd rather not. Eventually we
should be able to delete AutofillPopupControllerImpl's code, and pass around
IDRs for icons instead of string identifiers (string16 is especially weird given
that it's not something that's used for display). So for now, I'd prefer to
leave the code as is rather than adding new code that I plan to delete. I filed
a bug: crbug.com/172101

Currently this mapping exists 4 times in chrome. This patch reduces that to 3.
Fixing the bug I filed will reduce it to 1.

+
 void CreditCard::operator=(const CreditCard& credit_card) {
   if (this == &credit_card)
     return;