[Payments] Show what's missing for incomplete payment methods.

components/autofill/core/browser/validation.cc

Index: components/autofill/core/browser/validation.cc
diff --git a/components/autofill/core/browser/validation.cc b/components/autofill/core/browser/validation.cc
index e7237f7b3b73d3fab1ad1a963a3cb245a0e0df3f..854c9d4c1d2983553f56e9b514e266eb804a70b3 100644
--- a/components/autofill/core/browser/validation.cc
+++ b/components/autofill/core/browser/validation.cc
@@ -125,6 +125,54 @@ bool IsValidCreditCardNumberForBasicCardNetworks(
   return false;
 }
 
+bool CreditCardHasNumberAndName(const CreditCard& card,
+                                const std::string& app_locale) {
+  return !card.number().empty() &&
+         !card.GetInfo(autofill::AutofillType(autofill::CREDIT_CARD_NAME_FULL),
+                       app_locale)
+              .empty();
+}
+
+bool IsCompleteForPaymentRequest(const CreditCard& card,

[anthonyvd, 2017/04/13 19:50:55]

I think this could return a bitfield enum instead of a bool and remove the
|missing_info| param. This would decouple this from UI strings (so it's easier
to reuse it in a place where we do care about what's missing but not the
specific strings), letting the UI do the enum -> string mapping.

[Mathieu, 2017/04/13 20:00:15]

This is intentional because we want this file to be the state -> string mapping,
so that it can be used on other platforms (mainly iOS but I could see android
also calling this). I had discussed with Moe and it's useful for him to have a
method such as this one. For example, he used IsValidForType from this file to
get the proper error for the credit card editor [1].

[1] https://cs.chromium.org/search/?q=IsValidForType+file:ios&type=cs

[anthonyvd, 2017/04/13 20:48:27]

Totally understand the need to have a common function to get that string but I
still think determining which field is missing should be separate from the one
generating the string. I'd do a function returning an enum and extract the
string generation logic from this function here in a separate one.

I also think the resulting code will be cleaner. This function currently repeats
if (!message.empty()) { if (missing_info) {}} 3 times and potentially overwrites
message a bunch of times, which makes it harder to follow.

Something like:

ValidationResult IsCompleteForPayment(const CreditCard& cc) {
  ValidationResult res = VALID;
  if (cc.IsExpired()) res |= EXPIRED;
  if (/* name is empty */) res |= NAME_MISSING;

  return res;
}

base::string16 GetCCValidationString(ValidationResult res) {
  if (res == VALID) return "";
  if (res == EXPIRED) return "expired";
  if (res == NAME_MISSING) return "name_missing";
  return "Multiple things wrong";
}

[Mathieu, 2017/04/18 17:26:01]

Cool, it's better now, PTAL

+                                 const std::string& app_locale,
+                                 base::string16* missing_info) {
+  base::string16 message;
+  if (card.IsExpired(autofill::AutofillClock::Now())) {
+    message = l10n_util::GetStringUTF16(
+        IDS_PAYMENTS_VALIDATION_INVALID_CREDIT_CARD_EXPIRED);
+  }
+  if (card.number().empty()) {
+    if (!message.empty()) {
+      if (missing_info) {
+        *missing_info =
+            l10n_util::GetStringUTF16(IDS_PAYMENTS_MORE_INFORMATION_REQUIRED);
+      }
+      return false;
+    }
+    message = l10n_util::GetStringUTF16(
+        IDS_PAYMENTS_CARD_NUMBER_INVALID_VALIDATION_MESSAGE);
+  }
+  if (card.GetInfo(autofill::AutofillType(autofill::CREDIT_CARD_NAME_FULL),
+                   app_locale)
+          .empty()) {
+    if (!message.empty()) {
+      if (missing_info) {
+        *missing_info =
+            l10n_util::GetStringUTF16(IDS_PAYMENTS_MORE_INFORMATION_REQUIRED);
+      }
+      return false;
+    }
+    message = l10n_util::GetStringUTF16(IDS_PAYMENTS_NAME_ON_CARD_REQUIRED);
+  }
+
+  if (!message.empty()) {
+    if (missing_info)
+      *missing_info = std::move(message);
+    return false;
+  }
+  return true;
+}
+
 bool IsValidEmailAddress(const base::string16& text) {
   // E-Mail pattern as defined by the WhatWG. (4.10.7.1.5 E-Mail state)
   const base::string16 kEmailPattern = base::ASCIIToUTF16(