[Payments] Add error messages to credit card editor.

[Payments] Add error messages to credit card editor.

Adjusts the UI to be closer to the mocks.
* No more border between rows.
* Added spacing between rows.
* New grid system to position an error label below the input field.

BUG=687601
TEST=manual

Review-Url: https://codereview.chromium.org/2715533002
Cr-Commit-Position: refs/heads/master@{#452235}
Committed: https://chromium.googlesource.com/chromium/src/+/6b35359be7e542dec28d4ae27ea83f861f6cc34a

[Mathieu, 2017-02-22 14:00:39]

The CQ bit was checked by mathp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 14:00:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mathieu, 2017-02-22 14:30:16]

Patchset #2 (id:20001) has been deleted

[Mathieu, 2017-02-22 14:30:21]

The CQ bit was checked by mathp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 14:30:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mathieu, 2017-02-22 14:33:03]

mathp@chromium.org changed reviewers:
+ anthonyvd@chromium.org

[Mathieu, 2017-02-22 14:33:04]

PTAL!

[Mathieu, 2017-02-22 14:33:20]

Patchset #2 (id:40001) has been deleted

[Mathieu, 2017-02-22 14:33:24]

The CQ bit was checked by mathp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 14:33:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 15:14:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 15:14:23]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2017-02-22 19:32:45]

Looks good, few comments/questions.

https://codereview.chromium.org/2715533002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/editor_view_controller.cc (right):

https://codereview.chromium.org/2715533002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.cc:193:
label->SetMultiLine(true);
I don't see where the max width of the label is set so I'd expect it to just be
sized to its content all the time. What triggers the label being multiline in
this case?

https://codereview.chromium.org/2715533002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.cc:232: field.type);
Are all fields guaranteed to have a different type? What's field.type here?

https://codereview.chromium.org/2715533002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/editor_view_controller.h (right):

https://codereview.chromium.org/2715533002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.h:106: virtual
std::unique_ptr<ValidationDelegate> CreateValidationDelegate(
Why not just use |this| in the function body instead of passing |this| as a
param?

https://codereview.chromium.org/2715533002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h (right):

https://codereview.chromium.org/2715533002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h:30:
ERROR_LABEL_OFFSET = 1000,
You can omit the 1000 here I think. It's guaranteed to be greater than all the
values above it so the general pattern is to just name it something that
indicates it should be the last element in the enum and add a comment to that
effect. What about DIALOG_VIEW_IDS_MAX?

[Mathieu, 2017-02-22 20:07:00]

Patchset #3 (id:80001) has been deleted

[Mathieu, 2017-02-22 20:08:59]

Have a look!

https://codereview.chromium.org/2715533002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/editor_view_controller.cc (right):

https://codereview.chromium.org/2715533002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.cc:193:
label->SetMultiLine(true);
On 2017/02/22 19:32:45, anthonyvd wrote:
> I don't see where the max width of the label is set so I'd expect it to just
be
> sized to its content all the time. What triggers the label being multiline in
> this case?

Correct! I've set a max width now.

https://codereview.chromium.org/2715533002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.cc:232: field.type);
On 2017/02/22 19:32:45, anthonyvd wrote:
> Are all fields guaranteed to have a different type? What's field.type here?

Yes they all have different type, it's the autofill type they represent (e.g.
autofill::CREDIT_CARD_NAME_FULL) . This is a way to get to this label during
testing.

https://codereview.chromium.org/2715533002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/editor_view_controller.h (right):

https://codereview.chromium.org/2715533002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.h:106: virtual
std::unique_ptr<ValidationDelegate> CreateValidationDelegate(
On 2017/02/22 19:32:45, anthonyvd wrote:
> Why not just use |this| in the function body instead of passing |this| as a
> param?

Doh!

https://codereview.chromium.org/2715533002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h (right):

https://codereview.chromium.org/2715533002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h:30:
ERROR_LABEL_OFFSET = 1000,
On 2017/02/22 19:32:45, anthonyvd wrote:
> You can omit the 1000 here I think. It's guaranteed to be greater than all the
> values above it so the general pattern is to just name it something that
> indicates it should be the last element in the enum and add a comment to that
> effect. What about DIALOG_VIEW_IDS_MAX?

You're right I can get rid of the 1000 here, I've set VIEW_ID_NONE to start at
the Autofill type's max. To explain a bit more, this is a slight hack that I'm
using. The input field will have set_id(field.type), (e.g. set_id(56)) and the
error label will have set_id(ERROR_LABEL_OFFSET + 56), so I can recall both
during tests. Here's the autofill types:
https://cs.chromium.org/chromium/src/components/autofill/core/browser/field_t...

Hey don't hate the player, hate the ViewID game!

[anthonyvd, 2017-02-22 21:04:47]

lgtm

[Mathieu, 2017-02-22 21:14:30]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2017-02-22 21:15:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mathieu, 2017-02-22 21:23:20]

The CQ bit was unchecked by mathp@chromium.org

[Mathieu, 2017-02-22 21:24:13]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2017-02-22 21:24:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2715533002/#ps120001
(title: "clean")

[commit-bot: I haz the power, 2017-02-22 21:24:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 22:25:27]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487798653767380,
"parent_rev": "55723a7098ece4916d203986b5f7d336a0c0a9f5", "commit_rev":
"6b35359be7e542dec28d4ae27ea83f861f6cc34a"}

[commit-bot: I haz the power, 2017-02-22 22:26:10]

Description was changed from

==========
[Payments] Add error messages to credit card editor.

Adjusts the UI to be closer to the mocks.
* No more border between rows.
* Added spacing between rows.
* New grid system to position an error label below the input field.

BUG=687601
TEST=manual
==========

to

==========
[Payments] Add error messages to credit card editor.

Adjusts the UI to be closer to the mocks.
* No more border between rows.
* Added spacing between rows.
* New grid system to position an error label below the input field.

BUG=687601
TEST=manual

Review-Url: https://codereview.chromium.org/2715533002
Cr-Commit-Position: refs/heads/master@{#452235}
Committed:
https://chromium.googlesource.com/chromium/src/+/6b35359be7e542dec28d4ae27ea8...
==========

[commit-bot: I haz the power, 2017-02-22 22:26:11]

Committed patchset #4 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/6b35359be7e542dec28d4ae27ea8...