[Payments] Basic validation in the credit card editor.
Introducting the ValidatingTextfield class, which will be used to validate only on first blur, and on every subsequent contents change.
Created validation methods in autofill code, which will return the proper error messages (to be displayed in a follow-up CL).
BUG=687601
TEST=ValidatingTextfieldTest unit_tests, PaymentRequest* interactive_ui_tests, AutofillValidation components_unittests
Review-Url: https://codereview.chromium.org/2673753005
Cr-Commit-Position: refs/heads/master@{#449409}
Committed: https://chromium.googlesource.com/chromium/src/+/d4cfd8f80766dbadbbe6f04202304af5b4863e3d
Description was changed from ========== [Payments] Basic validation in the credit card editor. BUG= ========== ...
3 years, 10 months ago
(2017-02-06 22:05:53 UTC)
#8
Description was changed from
==========
[Payments] Basic validation in the credit card editor.
BUG=
==========
to
==========
[Payments] Basic validation in the credit card editor.
Introducting the ValidatingTextfield class, which will be used to validate only
on first blur, and on every subsequent contents change.
Created validation methods in autofill code, which will return the proper error
messages (to be displayed in a follow-up CL).
BUG=687601
TEST=ValidatingTextfieldTest unit_tests, PaymentRequest* interactive_ui_tests,
AutofillValidation components_unittests
==========
3 years, 10 months ago
(2017-02-06 23:35:32 UTC)
#12
Dry run: This issue passed the CQ dry run.
please use gerrit instead
https://codereview.chromium.org/2673753005/diff/120001/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (left): https://codereview.chromium.org/2673753005/diff/120001/chrome/app/generated_resources.grd#oldcode9989 chrome/app/generated_resources.grd:9989: Year Thank you for cleaning up unused strings. Would ...
3 years, 10 months ago
(2017-02-07 21:55:22 UTC)
#13
Thanks rouslan for components/payments anthony for c/b/ui/views/payments https://codereview.chromium.org/2673753005/diff/120001/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (left): https://codereview.chromium.org/2673753005/diff/120001/chrome/app/generated_resources.grd#oldcode9989 chrome/app/generated_resources.grd:9989: Year On ...
3 years, 10 months ago
(2017-02-08 02:03:25 UTC)
#14
Thanks, hope I addressed everything! https://codereview.chromium.org/2673753005/diff/140001/chrome/browser/ui/views/payments/editor_view_controller.cc File chrome/browser/ui/views/payments/editor_view_controller.cc (right): https://codereview.chromium.org/2673753005/diff/140001/chrome/browser/ui/views/payments/editor_view_controller.cc#newcode67 chrome/browser/ui/views/payments/editor_view_controller.cc:67: content_view->AddChildView(CreateInputField(field, &text_field).release()); On 2017/02/08 ...
3 years, 10 months ago
(2017-02-08 21:21:51 UTC)
#17
Thanks, hope I addressed everything!
https://codereview.chromium.org/2673753005/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/editor_view_controller.cc (right):
https://codereview.chromium.org/2673753005/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.cc:67:
content_view->AddChildView(CreateInputField(field, &text_field).release());
On 2017/02/08 15:28:28, anthonyvd wrote:
> 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.)
I'll still keep the map, for controllers to be able to iterate through
textfields
https://codereview.chromium.org/2673753005/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.cc:85: // TODO(mathp):
Display |error_message| around |textfield|.
On 2017/02/08 15:28:27, anthonyvd wrote:
> 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.
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.
https://codereview.chromium.org/2673753005/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/editor_view_controller.h (right):
https://codereview.chromium.org/2673753005/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.h:52:
std::unordered_map<ValidatingTextfield*, const EditorField>;
On 2017/02/08 18:50:54, rouslan wrote:
> This might make copies of EditorField. I'd prefer std::unique_ptr<EditorField>
> values instead.
Discussed offline, EditorField is cheap and copies are OK.
https://codereview.chromium.org/2673753005/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.h:66: //
ValidatingTextfield::Delegate implementation.
On 2017/02/08 15:28:28, anthonyvd wrote:
> For consistency, maybe just "// ValidatingTextfield::Delegate:"
Done.
https://codereview.chromium.org/2673753005/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.h:69: const
TextFieldsMap& text_fields() { return text_fields_; }
On 2017/02/08 18:50:54, rouslan wrote:
> Make this method const please.
Done.
https://codereview.chromium.org/2673753005/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/validating_textfield.cc (right):
https://codereview.chromium.org/2673753005/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/validating_textfield.cc:7: #include
"base/strings/utf_string_conversions.h"
On 2017/02/08 15:28:28, anthonyvd wrote:
> unused?
Done.
https://codereview.chromium.org/2673753005/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/validating_textfield.h (right):
https://codereview.chromium.org/2673753005/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/validating_textfield.h:15: public:
On 2017/02/08 18:50:55, rouslan wrote:
> Need an empty destructor:
>
> "To make sure all implementations of the interface can be destroyed correctly,
> the interface must also declare a virtual destructor (in an exception to the
> first rule, this should not be pure). See Stroustrup, The C++ Programming
> Language, 3rd edition, section 12.4 for details."
>
> https://google.github.io/styleguide/cppguide.html#Interfaces
Done.
https://codereview.chromium.org/2673753005/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/validating_textfield.h:18: };
On 2017/02/08 15:28:28, anthonyvd wrote:
> DISALLOW_COPY_AND_ASSIGN
Done.
https://codereview.chromium.org/2673753005/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/validating_textfield.h:23: void OnBlur()
override;
On 2017/02/08 15:28:28, anthonyvd wrote:
> nit: comment about where the override is from.
Done.
https://codereview.chromium.org/2673753005/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/validating_textfield.h:34: };
On 2017/02/08 15:28:28, anthonyvd wrote:
> DISALLOW_COPY_AND_ASSIGN
Done.
anthonyvd
lgtm, thanks! https://codereview.chromium.org/2673753005/diff/140001/chrome/browser/ui/views/payments/editor_view_controller.cc File chrome/browser/ui/views/payments/editor_view_controller.cc (right): https://codereview.chromium.org/2673753005/diff/140001/chrome/browser/ui/views/payments/editor_view_controller.cc#newcode67 chrome/browser/ui/views/payments/editor_view_controller.cc:67: content_view->AddChildView(CreateInputField(field, &text_field).release()); On 2017/02/08 at 21:21:50, Mathieu ...
3 years, 10 months ago
(2017-02-09 14:29:27 UTC)
#18
lgtm, thanks!
https://codereview.chromium.org/2673753005/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/editor_view_controller.cc (right):
https://codereview.chromium.org/2673753005/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.cc:67:
content_view->AddChildView(CreateInputField(field, &text_field).release());
On 2017/02/08 at 21:21:50, Mathieu Perreault wrote:
> On 2017/02/08 15:28:28, anthonyvd wrote:
> > 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.)
>
> I'll still keep the map, for controllers to be able to iterate through
textfields
sgtm
https://codereview.chromium.org/2673753005/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/editor_view_controller.cc:85: // TODO(mathp):
Display |error_message| around |textfield|.
On 2017/02/08 at 21:21:50, Mathieu Perreault wrote:
> On 2017/02/08 15:28:27, anthonyvd wrote:
> > 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.
>
> 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.
Ah, I didn't think about the linked validation use case. Good to know you know
how to solve it :)
Mathieu
The CQ bit was checked by mathp@chromium.org to run a CQ dry run
3 years, 10 months ago
(2017-02-09 18:08:24 UTC)
#19
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_tsan_rel_ng/builds/12170)
3 years, 10 months ago
(2017-02-09 18:29:31 UTC)
#22
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_compile_dbg_ng/builds/353129) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, ...
3 years, 10 months ago
(2017-02-09 19:36:57 UTC)
#27
CQ is committing da patch. Bot data: {"patchset_id": 240001, "attempt_start_ts": 1486670663203490, "parent_rev": "0227e4a51b9dd67d5735d53aff95b8fa70a9e291", "commit_rev": "d4cfd8f80766dbadbbe6f04202304af5b4863e3d"}
3 years, 10 months ago
(2017-02-09 21:17:03 UTC)
#31
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1486670663203490,
"parent_rev": "0227e4a51b9dd67d5735d53aff95b8fa70a9e291", "commit_rev":
"d4cfd8f80766dbadbbe6f04202304af5b4863e3d"}
commit-bot: I haz the power
Description was changed from ========== [Payments] Basic validation in the credit card editor. Introducting the ...
3 years, 10 months ago
(2017-02-09 21:17:38 UTC)
#32
Message was sent while issue was closed.
Description was changed from
==========
[Payments] Basic validation in the credit card editor.
Introducting the ValidatingTextfield class, which will be used to validate only
on first blur, and on every subsequent contents change.
Created validation methods in autofill code, which will return the proper error
messages (to be displayed in a follow-up CL).
BUG=687601
TEST=ValidatingTextfieldTest unit_tests, PaymentRequest* interactive_ui_tests,
AutofillValidation components_unittests
==========
to
==========
[Payments] Basic validation in the credit card editor.
Introducting the ValidatingTextfield class, which will be used to validate only
on first blur, and on every subsequent contents change.
Created validation methods in autofill code, which will return the proper error
messages (to be displayed in a follow-up CL).
BUG=687601
TEST=ValidatingTextfieldTest unit_tests, PaymentRequest* interactive_ui_tests,
AutofillValidation components_unittests
Review-Url: https://codereview.chromium.org/2673753005
Cr-Commit-Position: refs/heads/master@{#449409}
Committed:
https://chromium.googlesource.com/chromium/src/+/d4cfd8f80766dbadbbe6f0420230...
==========
commit-bot: I haz the power
Committed patchset #8 (id:240001) as https://chromium.googlesource.com/chromium/src/+/d4cfd8f80766dbadbbe6f04202304af5b4863e3d
3 years, 10 months ago
(2017-02-09 21:17:40 UTC)
#33
Issue 2673753005: [Payments] Basic validation in the credit card editor.
(Closed)
Created 3 years, 10 months ago by Mathieu
Modified 3 years, 10 months ago
Reviewers: anthonyvd, please use gerrit instead
Base URL:
Comments: 42
Chromium Code Reviews
Issue
2673753005:
[Payments] Basic validation in the credit card editor. (Closed)
