[Web Payments] Add the contact info screen.

[Web Payments] Add the contact info screen.

This CL implements the bare minimum Contact Info Sheet. The list will display
all available contact info profiles and a checkmark next to the selected one.

Functionality of actually selecting items will be coming in a follow up change.

BUG=697084
Review-Url: https://codereview.chromium.org/2725623004
Cr-Commit-Position: refs/heads/master@{#454014}
Committed: https://chromium.googlesource.com/chromium/src/+/f4dc12111a0349f4b9ecb2ee43220044f4c401a8

[anthonyvd, 2017-02-28 16:50:44]

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

[anthonyvd, 2017-02-28 16:50:44]

Hi guys,

Can you PTAL at this first pass of the Contact Info Sheet?

Thanks!

[Mathieu, 2017-02-28 17:36:00]

I see an observer for test added but no test, did you forget to add it to the
change?

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

https://codereview.chromium.org/2725623004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/contact_info_view_controller.cc:37:
std::unique_ptr<views::View> CreateItemView() override {
This will be similar to the shipping addresses list items, right? We should keep
that in mind.

https://codereview.chromium.org/2725623004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/contact_info_view_controller.cc:61:
AddressStyleType::DETAILED, std::string(), *profile_, true, true, true);
Put a TODO to hook this up with the request options - request_payer_name,
request_payer_phone, request_payer_email (I am adding this to PaymentRequest
now)

Actually since the PaymentRequest knows the selected_contact profile and the
options, GetcontactInfoLabel could be a function on PaymentRequest which would
only need SUMMARY/DETAILED to be supplied.

This would allow it to be reused for the payment_sheet_view_controller

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

https://codereview.chromium.org/2725623004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_views_util.cc:194: int
missing_fields = 0;
unused?

https://codereview.chromium.org/2725623004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_views_util.cc:219:
label->SetHorizontalAlignment(gfx::HorizontalAlignment::ALIGN_LEFT);
appears to be the default?

[anthonyvd, 2017-02-28 18:36:27]

Thanks for the review Math, addressed some comments and followed up on the
others.

As for the test Observer: I added it because I was touching the code that calls
it, but the functionality isn't there yet (only the layout) so there's not much
to test. My following CL will have actual selection and stuff and include the
related tests.

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

https://codereview.chromium.org/2725623004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/contact_info_view_controller.cc:37:
std::unique_ptr<views::View> CreateItemView() override {
On 2017/02/28 at 17:35:59, Mathieu Perreault wrote:
> This will be similar to the shipping addresses list items, right? We should
keep that in mind.

Yeah. I can refactor all of this once this CL and Tommy's land.

https://codereview.chromium.org/2725623004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/contact_info_view_controller.cc:61:
AddressStyleType::DETAILED, std::string(), *profile_, true, true, true);
On 2017/02/28 at 17:35:59, Mathieu Perreault wrote:
> Put a TODO to hook this up with the request options - request_payer_name,
request_payer_phone, request_payer_email (I am adding this to PaymentRequest
now)
> 
> Actually since the PaymentRequest knows the selected_contact profile and the
options, GetcontactInfoLabel could be a function on PaymentRequest which would
only need SUMMARY/DETAILED to be supplied.
> 
> This would allow it to be reused for the payment_sheet_view_controller

The issue with that is that we're also calling GetContactInfoLabel for profiles
that aren't the selected one. We could have it be a function of the profile,
SUMMARY/DETAILED, and PaymentRequest for the options. WDYT?

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

https://codereview.chromium.org/2725623004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_views_util.cc:194: int
missing_fields = 0;
On 2017/02/28 at 17:36:00, Mathieu Perreault wrote:
> unused?

Not used yet, added a TODO about that below.

https://codereview.chromium.org/2725623004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_views_util.cc:219:
label->SetHorizontalAlignment(gfx::HorizontalAlignment::ALIGN_LEFT);
On 2017/02/28 at 17:35:59, Mathieu Perreault wrote:
> appears to be the default?

The BoxLayout was changing it. I added a call to
BoxLayout::set_cross_axis_alignment and this isn't needed anymore.

[Mathieu, 2017-02-28 21:15:17]

lgtm with nits

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

https://codereview.chromium.org/2725623004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/contact_info_view_controller.cc:61:
AddressStyleType::DETAILED, std::string(), *profile_, true, true, true);
On 2017/02/28 18:36:27, anthonyvd wrote:
> On 2017/02/28 at 17:35:59, Mathieu Perreault wrote:
> > Put a TODO to hook this up with the request options - request_payer_name,
> request_payer_phone, request_payer_email (I am adding this to PaymentRequest
> now)
> > 
> > Actually since the PaymentRequest knows the selected_contact profile and the
> options, GetcontactInfoLabel could be a function on PaymentRequest which would
> only need SUMMARY/DETAILED to be supplied.
> > 
> > This would allow it to be reused for the payment_sheet_view_controller
> 
> The issue with that is that we're also calling GetContactInfoLabel for
profiles
> that aren't the selected one. We could have it be a function of the profile,
> SUMMARY/DETAILED, and PaymentRequest for the options. WDYT?

Yeah ok you're right, it's more unit-testable if we pass three booleans, let's
leave it as is

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

https://codereview.chromium.org/2725623004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_views_util.cc:194: int
missing_fields = 0;
On 2017/02/28 18:36:27, anthonyvd wrote:
> On 2017/02/28 at 17:36:00, Mathieu Perreault wrote:
> > unused?
> 
> Not used yet, added a TODO about that below.

nit: I would remove |missing_fields| if it's not used, even though it's used
shortly.

https://codereview.chromium.org/2725623004/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_views_util.cc (right):

https://codereview.chromium.org/2725623004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:180:
std::unique_ptr<views::View> GetContactInfoLabel(
Add a TODO: there needs to be a unit test for how we layout labels, wdyt?

[anthonyvd, 2017-03-01 15:27:15]

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

[commit-bot: I haz the power, 2017-03-01 15:27:27]

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

[anthonyvd, 2017-03-01 15:27:55]

Thanks Math, comments addressed.

+rouslan@, can you please do an OWNERs review of this patch? Thanks!

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

https://codereview.chromium.org/2725623004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/contact_info_view_controller.cc:61:
AddressStyleType::DETAILED, std::string(), *profile_, true, true, true);
On 2017/02/28 at 21:15:16, Mathieu Perreault wrote:
> On 2017/02/28 18:36:27, anthonyvd wrote:
> > On 2017/02/28 at 17:35:59, Mathieu Perreault wrote:
> > > Put a TODO to hook this up with the request options - request_payer_name,
> > request_payer_phone, request_payer_email (I am adding this to PaymentRequest
> > now)
> > > 
> > > Actually since the PaymentRequest knows the selected_contact profile and
the
> > options, GetcontactInfoLabel could be a function on PaymentRequest which
would
> > only need SUMMARY/DETAILED to be supplied.
> > > 
> > > This would allow it to be reused for the payment_sheet_view_controller
> > 
> > The issue with that is that we're also calling GetContactInfoLabel for
profiles
> > that aren't the selected one. We could have it be a function of the profile,
> > SUMMARY/DETAILED, and PaymentRequest for the options. WDYT?
> 
> Yeah ok you're right, it's more unit-testable if we pass three booleans, let's
leave it as is

Ack.

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

https://codereview.chromium.org/2725623004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_views_util.cc:194: int
missing_fields = 0;
On 2017/02/28 at 21:15:16, Mathieu Perreault wrote:
> On 2017/02/28 18:36:27, anthonyvd wrote:
> > On 2017/02/28 at 17:36:00, Mathieu Perreault wrote:
> > > unused?
> > 
> > Not used yet, added a TODO about that below.
> 
> nit: I would remove |missing_fields| if it's not used, even though it's used
shortly.

Fair point, done.

https://codereview.chromium.org/2725623004/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_views_util.cc (right):

https://codereview.chromium.org/2725623004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:180:
std::unique_ptr<views::View> GetContactInfoLabel(
On 2017/02/28 at 21:15:16, Mathieu Perreault wrote:
> Add a TODO: there needs to be a unit test for how we layout labels, wdyt?

Good point. Done.

[commit-bot: I haz the power, 2017-03-01 15:40:13]

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

[commit-bot: I haz the power, 2017-03-01 15:40:13]

Dry run: Try jobs failed on following builders:
  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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mathieu, 2017-03-01 15:52:39]

Description was changed from

==========
[Web Payments] Add the contact info screen.

This CL implements the bare minimum Contact Info Sheet. The list will display
all available contact info profiles and a checkmark next to the selected one.

Functionality of actually selecting items will be coming in a follow up change.

BUG=697084
==========

to

==========
[Web Payments] Add the contact info screen.

This CL implements the bare minimum Contact Info Sheet. The list will display
all available contact info profiles and a checkmark next to the selected one.

Functionality of actually selecting items will be coming in a follow up change.

BUG=697084
==========

[Mathieu, 2017-03-01 15:52:39]

mathp@chromium.org changed reviewers:
+ rouslan@chromium.org
- tmartino@chromium.org

[please use gerrit instead, 2017-03-01 17:10:14]

lgtm % nits

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

https://codereview.chromium.org/2725623004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/contact_info_view_controller.cc:28:
ContactInfoListItem(autofill::AutofillProfile* profile,
Specify in a comment the ownership relationship for this object and these raw
pointers.

https://codereview.chromium.org/2725623004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/contact_info_view_controller.cc:37:
std::unique_ptr<views::View> CreateItemView() override {
Put the name of the interface in a comment for this method.

https://codereview.chromium.org/2725623004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/contact_info_view_controller.cc:80: void
SelectedStateChanged() override {}
Put the name of the interface in a comment for this method.

https://codereview.chromium.org/2725623004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/contact_info_view_controller.cc:82: void
ButtonPressed(views::Button* sender, const ui::Event& event) override {}
Put the name of the interface in a comment for this method.

[anthonyvd, 2017-03-01 18:55:11]

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

[commit-bot: I haz the power, 2017-03-01 18:55:44]

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

[anthonyvd, 2017-03-01 20:08:58]

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

https://codereview.chromium.org/2725623004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/contact_info_view_controller.cc:28:
ContactInfoListItem(autofill::AutofillProfile* profile,
On 2017/03/01 at 17:10:14, rouslan wrote:
> Specify in a comment the ownership relationship for this object and these raw
pointers.

Done.

https://codereview.chromium.org/2725623004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/contact_info_view_controller.cc:37:
std::unique_ptr<views::View> CreateItemView() override {
On 2017/03/01 at 17:10:14, rouslan wrote:
> Put the name of the interface in a comment for this method.

Done.

https://codereview.chromium.org/2725623004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/contact_info_view_controller.cc:80: void
SelectedStateChanged() override {}
On 2017/03/01 at 17:10:14, rouslan wrote:
> Put the name of the interface in a comment for this method.

Done.

https://codereview.chromium.org/2725623004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/contact_info_view_controller.cc:82: void
ButtonPressed(views::Button* sender, const ui::Event& event) override {}
On 2017/03/01 at 17:10:14, rouslan wrote:
> Put the name of the interface in a comment for this method.

Done.

[commit-bot: I haz the power, 2017-03-01 20:10:18]

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

[commit-bot: I haz the power, 2017-03-01 20:10:19]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2017-03-01 20:10:24]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2017-03-01 20:10:25]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/2725623004/#ps80001
(title: "Address comments.")

[commit-bot: I haz the power, 2017-03-01 20:11:27]

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-03-01 20:19:42]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488399024981970,
"parent_rev": "a46956a61e7e7c02ffcbd595f40f300f31932076", "commit_rev":
"f4dc12111a0349f4b9ecb2ee43220044f4c401a8"}

[commit-bot: I haz the power, 2017-03-01 20:20:21]

Description was changed from

==========
[Web Payments] Add the contact info screen.

This CL implements the bare minimum Contact Info Sheet. The list will display
all available contact info profiles and a checkmark next to the selected one.

Functionality of actually selecting items will be coming in a follow up change.

BUG=697084
==========

to

==========
[Web Payments] Add the contact info screen.

This CL implements the bare minimum Contact Info Sheet. The list will display
all available contact info profiles and a checkmark next to the selected one.

Functionality of actually selecting items will be coming in a follow up change.

BUG=697084

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

[commit-bot: I haz the power, 2017-03-01 20:20:22]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/f4dc12111a0349f4b9ecb2ee4322...