[Payments] Basic validation in the credit card editor.

chrome/browser/ui/views/payments/editor_view_controller.cc

Index: chrome/browser/ui/views/payments/editor_view_controller.cc
diff --git a/chrome/browser/ui/views/payments/editor_view_controller.cc b/chrome/browser/ui/views/payments/editor_view_controller.cc
index 8d77ce38c48c04dbf905631a3f9705be2807bb9e..a9355e083c0ead519352e9ee839c56c7c17c66a7 100644
--- a/chrome/browser/ui/views/payments/editor_view_controller.cc
+++ b/chrome/browser/ui/views/payments/editor_view_controller.cc
@@ -4,13 +4,16 @@
 
 #include "chrome/browser/ui/views/payments/editor_view_controller.h"
 
+#include <memory>
 #include <utility>
 
+#include "base/memory/ptr_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/ui/views/payments/payment_request_dialog_view.h"
 #include "chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h"
 #include "chrome/browser/ui/views/payments/payment_request_views_util.h"
 #include "chrome/grit/generated_resources.h"
+#include "components/autofill/core/browser/validation.h"
 #include "components/payments/payment_request.h"
 #include "components/strings/grit/components_strings.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -36,7 +39,7 @@ enum class EditorViewControllerTags : int {
   SAVE_BUTTON = kFirstTagValue,
 };
 
-constexpr int kNumCharactersInShortField = 6;
+constexpr int kNumCharactersInShortField = 8;
 constexpr int kNumCharactersInLongField = 20;
 
 }  // namespace
@@ -60,20 +63,13 @@ std::unique_ptr<views::View> EditorViewController::CreateView() {
   // Create an input label/textfield for each field definition.
   std::vector<EditorField> fields = GetFieldDefinitions();
   for (const auto& field : fields) {
-    views::Textfield* text_field = nullptr;
+    ValidatingTextfield* text_field = nullptr;
     content_view->AddChildView(CreateInputField(field, &text_field).release());

[anthonyvd, 2017/02/08 15:28:28]

I'd remove the ValidatingTextField** parameter and just insert inside the map in
the CreateInputField() function.

(Note that the suggestion below would remove the need for that map altogether.)

[Mathieu, 2017/02/08 21:21:50]

I'll still keep the map, for controllers to be able to iterate through
textfields

[anthonyvd, 2017/02/09 14:29:27]

sgtm

     // |field| is moved out of the |fields| structure and should not be
     // referenced after the following line.
     text_fields_.insert(std::make_pair(text_field, std::move(field)));
   }
 
-  // TODO(mathp): Use the save button in the footer once it's built.
-  views::LabelButton* button = views::MdTextButton::CreateSecondaryUiButton(
-      this, l10n_util::GetStringUTF16(IDS_PASSWORD_MANAGER_SAVE_BUTTON));
-  button->set_tag(static_cast<int>(EditorViewControllerTags::SAVE_BUTTON));
-  button->set_id(static_cast<int>(DialogViewID::EDITOR_SAVE_BUTTON));
-  content_view->AddChildView(button);
-
   return CreatePaymentView(
       CreateSheetHeaderView(
           true, l10n_util::GetStringUTF16(
@@ -82,6 +78,24 @@ std::unique_ptr<views::View> EditorViewController::CreateView() {
       std::move(content_view));
 }
 
+bool EditorViewController::ValidateTextfield(ValidatingTextfield* textfield) {
+  base::string16 error_message;
+  const auto field = text_fields_.find(textfield);
+  DCHECK(field != text_fields_.end());
+  // TODO(mathp): Display |error_message| around |textfield|.

[anthonyvd, 2017/02/08 15:28:27]

So the issue I see happening here in the future is the following:

When the textfield doesn't pass validation and you have to display an error
message, you'll have to figure out which field it is, and traverse the view
hierarchy to find the view used to display the message. Since this is the
function called for every text field, it might get a little messy to rebuild all
that information.

WDYT of changing this such as:

1. Make ValidatingTextField::delegate_ a std::unique_ptr
2. Instead of having the controller subclass Delegate, we can have something
like EditorValidatingDelegate that does it and has members for the bits of UI it
needs to update
3. Since the delegate is now owned by the ValidatingTextField, we have one for
each field and they can have their own context

I think this way the code that updates the UI post-validation is going to be
much simpler (for example, the delegate would simply have a pointer to the error
label it needs to update). It'll also avoid multiple searches through maps. I
think the text_field_map_ will not even be needed anymore after that change.

[Mathieu, 2017/02/08 21:21:50]

OK you make good points, I've changed it. To explain a bit the initial idea, I
see a future where there will be the need to have "linked" validation between
two fields (i.e. MM and YY) hence this centralized location. But with your
design, we can still achieve that by giving a pointer back to the controller to
these delegates to warn during such events, triggering full validation. 

[anthonyvd, 2017/02/09 14:29:27]

Ah, I didn't think about the linked validation use case. Good to know you know
how to solve it :)

+  return autofill::IsValidForType(textfield->text(), field->second.type,
+                                  &error_message);
+}
+
+std::unique_ptr<views::Button> EditorViewController::CreatePrimaryButton() {
+  std::unique_ptr<views::Button> button(
+      views::MdTextButton::CreateSecondaryUiBlueButton(
+          this, l10n_util::GetStringUTF16(IDS_PASSWORD_MANAGER_SAVE_BUTTON)));
+  button->set_tag(static_cast<int>(EditorViewControllerTags::SAVE_BUTTON));
+  button->set_id(static_cast<int>(DialogViewID::EDITOR_SAVE_BUTTON));
+  return button;
+}
+
 void EditorViewController::ButtonPressed(views::Button* sender,
                                          const ui::Event& event) {
   switch (sender->tag()) {
@@ -97,12 +111,12 @@ void EditorViewController::ButtonPressed(views::Button* sender,
 
 void EditorViewController::ContentsChanged(views::Textfield* sender,
                                            const base::string16& new_contents) {
-  // TODO(mathp): Validate the |sender| textfield and display errors.
+  static_cast<ValidatingTextfield*>(sender)->OnContentsChanged();
 }
 
 std::unique_ptr<views::View> EditorViewController::CreateInputField(
     const EditorField& field,
-    views::Textfield** text_field) {
+    ValidatingTextfield** text_field) {
   std::unique_ptr<views::View> row = base::MakeUnique<views::View>();
 
   row->SetBorder(payments::CreatePaymentRequestRowBorder());
@@ -127,8 +141,10 @@ std::unique_ptr<views::View> EditorViewController::CreateInputField(
   layout->StartRow(0, 0);
   layout->AddView(new views::Label(field.label));
 
-  *text_field = new views::Textfield();
+  *text_field = new ValidatingTextfield(this);
   (*text_field)->set_controller(this);
+  // Using autofill field type as a view ID (for testing).
+  (*text_field)->set_id(static_cast<int>(field.type));
   (*text_field)
       ->set_default_width_in_chars(field.length_hint ==
                                            EditorField::LengthHint::HINT_SHORT