[Payments] Fix up field widths in desktop editors.

[Payments] Fix up field widths in desktop editors.

Width is now conformant to spec. See bug for screenshots.

Moved the billing address combobox to be handled by the base class now.
The credit card editor only adds the Add button as an extra view.
Modified the address combobox model to accept a default selected guid.

BUG=718548, 720593
TEST=visual

Review-Url: https://codereview.chromium.org/2871873003
Cr-Commit-Position: refs/heads/master@{#470980}
Committed: https://chromium.googlesource.com/chromium/src/+/d6449a52e98ecf9dcc888c0868935d9944375e7a

[Mathieu, 2017-05-09 17:53:32]

Description was changed from

==========
[Payments] Fix up field widths in desktop editors.

Width is now conformant to spec.

BUG=718548
TEST=visual
==========

to

==========
[Payments] Fix up field widths in desktop editors.

Width is now conformant to spec. See bug for screenshots.

BUG=718548
TEST=visual
==========

[Mathieu, 2017-05-09 17:53:32]

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

[Mathieu, 2017-05-09 17:54:11]

Anthony PTAL!

[anthonyvd, 2017-05-09 21:01:21]

Thanks! One comment.

https://codereview.chromium.org/2871873003/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/editor_view_controller.cc (right):

https://codereview.chromium.org/2871873003/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/editor_view_controller.cc:205:
views::GridLayout::FIXED, kShortInputFieldMinimumWidth, 0);
These variables are named "minimum width" but you're passing them to the fixed
width argument. They should either be renamed or passed to the min_width
argument.

Also, the dialog might have different widths soon in non-english locales. You
might want to make sure this works *without* a fixed width for the field (fixed
for the labels is fine).

[Mathieu, 2017-05-10 12:30:35]

PTAL!

https://codereview.chromium.org/2871873003/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/editor_view_controller.cc (right):

https://codereview.chromium.org/2871873003/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/editor_view_controller.cc:205:
views::GridLayout::FIXED, kShortInputFieldMinimumWidth, 0);
On 2017/05/09 21:01:21, anthonyvd wrote:
> These variables are named "minimum width" but you're passing them to the fixed
> width argument. They should either be renamed or passed to the min_width
> argument.
> 
> Also, the dialog might have different widths soon in non-english locales. You
> might want to make sure this works *without* a fixed width for the field
(fixed
> for the labels is fine).

Done. 

The fixed width for input fields (176/272px) is a requirement of the spec. Are
you saying the dialog could get narrower? I think there is still a little space
on the right to allow for that. We could also make kLabelWidth a bit shorter.
Can we open a bug for non-english locales? The UI is kinda broken right now
without this change.

[anthonyvd, 2017-05-10 13:26:02]

https://codereview.chromium.org/2871873003/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/editor_view_controller.cc (right):

https://codereview.chromium.org/2871873003/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/editor_view_controller.cc:205:
views::GridLayout::FIXED, kShortInputFieldMinimumWidth, 0);
On 2017/05/10 at 12:30:35, Mathieu wrote:
> On 2017/05/09 21:01:21, anthonyvd wrote:
> > These variables are named "minimum width" but you're passing them to the
fixed
> > width argument. They should either be renamed or passed to the min_width
> > argument.
> > 
> > Also, the dialog might have different widths soon in non-english locales.
You
> > might want to make sure this works *without* a fixed width for the field
(fixed
> > for the labels is fine).
> 
> Done. 
> 
> The fixed width for input fields (176/272px) is a requirement of the spec. Are
you saying the dialog could get narrower? I think there is still a little space
on the right to allow for that. We could also make kLabelWidth a bit shorter. 

It's the opposite: if the strings in buttons are long, the dialog will have to
grow in 16px increments according the Harmony. Having fixed fields in that case
will make it so there's potentially a lot of white space to the right of the
fields.

I think the spec gives fixed values that assume a fixed width dialog, which
isn't the case. We should probably have either

1. Fixed width expressed as fixed_width = (current_dialog_width - label_width -
trailing_padding) for a value of trailing_padding that matches the current spec
for the base dialog width (512px)
2. Preserve the short/long field width ratio by not specifying a fixed width but
specifying the proper resize_percent on the field's column and maybe a trailing
padding column

FWIW I think 1. makes more sense (and is probably simpler).

> Can we open a bug for non-english locales? The UI is kinda broken right now
without this change.

I mean we haven't shipped yet, might as well do it right from the start ;)

[Mathieu, 2017-05-10 14:39:41]

Anthony, adjusted the width according to UX decision, PTAL!

[anthonyvd, 2017-05-10 15:03:56]

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

https://codereview.chromium.org/2871873003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.cc:205: int
short_input_width =
Ok so one last thing to simplify this code. Currently the UX spec says:

Dialog width: 512px
Label width: 140px
Padding between label and field: 16px
Insets: 2x16px

Form this, we can infer 324px left for the field itself, minus 176px for short
fields and 272px for long fields, the trailing padding should therefore always
be 148px/52px for short/long fields. Let's call that value |trailing_padding|.

Because of this, this code can be changed to:

1. A fixed size column for the label
2. A 16px padding column
3. A USE_PREF sized column for the field
4. A fixed size padding column, |trailing_padding| px in width

This cleans up the code a bit (it removes the width computation) and removes
uses of GetActualDialogWidth(), which is probably going away.

[anthonyvd, 2017-05-10 19:05:57]

Description was changed from

==========
[Payments] Fix up field widths in desktop editors.

Width is now conformant to spec. See bug for screenshots.

BUG=718548
TEST=visual
==========

to

==========
[Payments] Fix up field widths in desktop editors.

Width is now conformant to spec. See bug for screenshots.

BUG=718548,720593
TEST=visual
==========

[Mathieu, 2017-05-10 19:54:57]

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

[commit-bot: I haz the power, 2017-05-10 19:56:02]

Dry run: CQ is trying da patch.

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

[Mathieu, 2017-05-10 19:56:03]

Description was changed from

==========
[Payments] Fix up field widths in desktop editors.

Width is now conformant to spec. See bug for screenshots.

BUG=718548,720593
TEST=visual
==========

to

==========
[Payments] Fix up field widths in desktop editors.

Width is now conformant to spec. See bug for screenshots.

Moved the billing address combobox to be handled by the base class now.
The credit card editor only adds the Add button as an extra view.
Modified the address combobox model to accept a default selected guid.

BUG=718548,720593
TEST=visual
==========

[Mathieu, 2017-05-10 19:56:20]

Hi Anthony, this will probably need a full review again, sorry. PTAL

[commit-bot: I haz the power, 2017-05-10 20:05:59]

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

[commit-bot: I haz the power, 2017-05-10 20:06:00]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Mathieu, 2017-05-10 20:29:45]

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

[commit-bot: I haz the power, 2017-05-10 20:31:23]

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-05-10 22:17:56]

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

[commit-bot: I haz the power, 2017-05-10 22:17:57]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2017-05-11 00:32:14]

Looks great, thanks for bearing with me. Few minor comments!

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

https://codereview.chromium.org/2871873003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.cc:222: // |            
                                                            |
nit: remove this empty line?

https://codereview.chromium.org/2871873003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.cc:365:
layout->SkipColumns(1);
nit: I'm not sure this skip is required since this is the last column.

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

https://codereview.chromium.org/2871873003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.h:113: virtual
std::unique_ptr<views::View> CreateExtraViewForField(
nit: add comment

[Mathieu, 2017-05-11 01:16:24]

Thanks PTAL!

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

https://codereview.chromium.org/2871873003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.cc:222: // |            
                                                            |
On 2017/05/11 00:32:14, anthonyvd wrote:
> nit: remove this empty line? 

Done.

https://codereview.chromium.org/2871873003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.cc:365:
layout->SkipColumns(1);
On 2017/05/11 00:32:14, anthonyvd wrote:
> nit: I'm not sure this skip is required since this is the last column.

Done.

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

https://codereview.chromium.org/2871873003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/editor_view_controller.h:113: virtual
std::unique_ptr<views::View> CreateExtraViewForField(
On 2017/05/11 00:32:14, anthonyvd wrote:
> nit: add comment

Done.

[anthonyvd, 2017-05-11 15:19:11]

lgtm

[Mathieu, 2017-05-11 15:19:42]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2017-05-11 15:20:24]

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-05-11 16:59:50]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494515982281480,
"parent_rev": "1852efa6e4d9c3353cd43d1815929d46e0f19a70", "commit_rev":
"d6449a52e98ecf9dcc888c0868935d9944375e7a"}

[commit-bot: I haz the power, 2017-05-11 16:59:59]

Description was changed from

==========
[Payments] Fix up field widths in desktop editors.

Width is now conformant to spec. See bug for screenshots.

Moved the billing address combobox to be handled by the base class now.
The credit card editor only adds the Add button as an extra view.
Modified the address combobox model to accept a default selected guid.

BUG=718548,720593
TEST=visual
==========

to

==========
[Payments] Fix up field widths in desktop editors.

Width is now conformant to spec. See bug for screenshots.

Moved the billing address combobox to be handled by the base class now.
The credit card editor only adds the Add button as an extra view.
Modified the address combobox model to accept a default selected guid.

BUG=718548,720593
TEST=visual

Review-Url: https://codereview.chromium.org/2871873003
Cr-Commit-Position: refs/heads/master@{#470980}
Committed:
https://chromium.googlesource.com/chromium/src/+/d6449a52e98ecf9dcc888c086893...
==========

[commit-bot: I haz the power, 2017-05-11 17:00:00]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/d6449a52e98ecf9dcc888c086893...