Focus first invalid field of payment request editor

chrome/browser/ui/views/payments/validation_delegate.h

Index: chrome/browser/ui/views/payments/validation_delegate.h
diff --git a/chrome/browser/ui/views/payments/validation_delegate.h b/chrome/browser/ui/views/payments/validation_delegate.h
index 4065398a85785949bab0b93d3800c264d016804a..84c9b93239c03ad3b19ddf16fae8b28c1e716829 100644
--- a/chrome/browser/ui/views/payments/validation_delegate.h
+++ b/chrome/browser/ui/views/payments/validation_delegate.h
@@ -16,10 +16,13 @@ class ValidationDelegate {
  public:
   virtual ~ValidationDelegate() {}
 
-  // Only the delegate knows how to validate the textfield.
-  virtual bool ValidateTextfield(views::Textfield* textfield) = 0;
+  // Only the delegate knows how to validate the textfield. Only display an
+  // error in the UI when |display_error| is true.
+  virtual bool ValidateTextfield(views::Textfield* textfield,

[anthonyvd, 2017/05/12 14:19:44]

I think we should separate the validation logic from the error display here.
This would allow us to call the validation function alone as needed and remove
the need for this |display_error|.

Maybe have Validate{Textfield|Combobox} return whether the field is valid or not
and take a string16* (that defaults to nullptr) as an argument to fill with the
error string. Then, add DisplayError that takes the error string as an argument
and updates the UI with it.

I think this would also make testing the validation functions easier. WDYT?

[MAD, 2017/05/18 16:01:46]

As discussed... OK?

[anthonyvd, 2017/05/18 18:12:24]

Yeah, I like this way better :) Thanks!

+                                 bool display_error) = 0;
 
-  virtual bool ValidateCombobox(views::Combobox* combobox) = 0;
+  virtual bool ValidateCombobox(views::Combobox* combobox,
+                                bool display_error) = 0;
   virtual void ComboboxModelChanged(views::Combobox* combobox) = 0;
 };