Focus first invalid field of payment request editor

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

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_UI_VIEWS_PAYMENTS_VALIDATION_DELEGATE_H_
 #define CHROME_BROWSER_UI_VIEWS_PAYMENTS_VALIDATION_DELEGATE_H_
 
 namespace views {
 class Combobox;
 class Textfield;
 }  // namespace views
 
 namespace payments {
 
 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;
 };
 
 }  // namespace payments
 
 #endif  // CHROME_BROWSER_UI_VIEWS_PAYMENTS_VALIDATION_DELEGATE_H_