[WebPayments] Add the Payment Method section in the Payment Sheet

[WebPayments] Add the Payment Method section in the Payment Sheet

BUG=679835
Review-Url: https://codereview.chromium.org/2621153002
Cr-Commit-Position: refs/heads/master@{#444162}
Committed: https://chromium.googlesource.com/chromium/src/+/045303a3e7169ff8f56477510fd7f352060dc9a1

[anthonyvd, 2017-01-12 19:06:56]

Patchset #1 (id:1) has been deleted

[anthonyvd, 2017-01-12 19:18:58]

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

[anthonyvd, 2017-01-12 19:18:59]

Hi guys, can you PTAL at this patch that adds the Payment Method section to the
PR dialog?

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

https://codereview.chromium.org/2621153002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:36: // A column
for the close button.
New mocks don't have the X but a "Cancel" button. I'm removing the X here so
it's easier to match the mocks with our layouts and we can implement "Cancel" in
separate patches.

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

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:261: // The
name colums in each row should all have the same width, large enough to
If anyone has a better idea to do this that doesn't feel as dirty, I'm all ears.

[anthonyvd, 2017-01-12 19:19:38]

Patchset #1 (id:20001) has been deleted

[please use gerrit instead, 2017-01-12 19:34:03]

lgtm % comments

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

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:25:
PaymentRequest* request, PaymentRequestDialog* dialog)
Parameter names should match in header and source.

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_method_view_controller.h (right):

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_method_view_controller.h:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
Here and elsewhere: 2017

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_sheet_view_controller.h (right):

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.h:38: void
InitWidestNameColumnViewWidth();
newline between methods and member variables.

https://codereview.chromium.org/2621153002/diff/40001/components/autofill/cor...
File components/autofill/core/browser/credit_card.cc (right):

https://codereview.chromium.org/2621153002/diff/40001/components/autofill/cor...
components/autofill/core/browser/credit_card.cc:160: if (type ==
kAmericanExpressCard)
return GetPaymentRequestData(type).icon_resource_id;

https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil...

[Mathieu, 2017-01-12 21:27:11]

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/payments...
File chrome/browser/payments/chrome_payment_request_delegate.h (right):

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.h:17:
ChromePaymentRequestDelegate() {}
Let's create the ChromePaymentRequestDelegate with a WebContents* that it keeps
as member, and can reuse in GetPersonalDataManager.

I don't think the PaymentRequestDelegate interface needs to be specific to
WebContents (i.e. someone could provide a FooBarPaymentRequestDelegate that
didn't require WebContents to get a PersonalDataManager)

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.h:21:
autofill::PersonalDataManager* GetDataManagerFromWebContents(
nit: would call GetPersonalDataManager()

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

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:18: #include
"ui/views/controls/label.h"
need?

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:35: return
payments::CreatePaymentView(
do you need payments:: ?

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

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:210:
PaymentSheetViewController::CreatePaymentMethodRow() {
I really like the ASCII art in the Java Impl, could we have some?

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:261: // The
name colums in each row should all have the same width, large enough to
On 2017/01/12 19:18:59, anthonyvd wrote:
> If anyone has a better idea to do this that doesn't feel as dirty, I'm all
ears.

Idea 1: https://cs.chromium.org/chromium/src/ui/gfx/text_utils.h?rcl=0&l=29

Idea 2: RenderText thingy here
https://cs.chromium.org/chromium/src/ui/views/controls/label.cc?rcl=0&l=908

https://codereview.chromium.org/2621153002/diff/40001/components/autofill/cor...
File components/autofill/core/browser/credit_card.cc (right):

https://codereview.chromium.org/2621153002/diff/40001/components/autofill/cor...
components/autofill/core/browser/credit_card.cc:159: int
CreditCard::PaymentRequestIconResourceId(const std::string& type) {
Would this fit alongside these:
https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil...

https://codereview.chromium.org/2621153002/diff/40001/components/payments/pay...
File components/payments/payment_request.h (right):

https://codereview.chromium.org/2621153002/diff/40001/components/payments/pay...
components/payments/payment_request.h:51: // Returns nullptr if there is no
selected card.
wrap lines

[anthonyvd, 2017-01-13 16:09:19]

Thanks! Comments addressed.

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/payments...
File chrome/browser/payments/chrome_payment_request_delegate.h (right):

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.h:21:
autofill::PersonalDataManager* GetDataManagerFromWebContents(
On 2017/01/12 at 21:27:10, Mathieu Perreault wrote:
> nit: would call GetPersonalDataManager()

Done.

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

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:18: #include
"ui/views/controls/label.h"
On 2017/01/12 at 21:27:10, Mathieu Perreault wrote:
> need?

Nope, done.

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:25:
PaymentRequest* request, PaymentRequestDialog* dialog)
On 2017/01/12 at 19:34:02, rouslan wrote:
> Parameter names should match in header and source.

Done.

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:35: return
payments::CreatePaymentView(
On 2017/01/12 at 21:27:10, Mathieu Perreault wrote:
> do you need payments:: ?

Nope, done.

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_method_view_controller.h (right):

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_method_view_controller.h:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/01/12 at 19:34:02, rouslan wrote:
> Here and elsewhere: 2017

Done.

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

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:210:
PaymentSheetViewController::CreatePaymentMethodRow() {
On 2017/01/12 at 21:27:11, Mathieu Perreault wrote:
> I really like the ASCII art in the Java Impl, could we have some?

Done here and above.

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:261: // The
name colums in each row should all have the same width, large enough to
On 2017/01/12 at 21:27:11, Mathieu Perreault wrote:
> On 2017/01/12 19:18:59, anthonyvd wrote:
> > If anyone has a better idea to do this that doesn't feel as dirty, I'm all
ears.
> 
> Idea 1: https://cs.chromium.org/chromium/src/ui/gfx/text_utils.h?rcl=0&l=29
> 
> Idea 2: RenderText thingy here
https://cs.chromium.org/chromium/src/ui/views/controls/label.cc?rcl=0&l=908

It looks like neither of those would take into account the rest of the label
(insets, borders, etc) as they only measure the text itself. #2 also seems
equivalent to my implementation since it requires creating a label :(

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_sheet_view_controller.h (right):

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.h:38: void
InitWidestNameColumnViewWidth();
On 2017/01/12 at 19:34:02, rouslan wrote:
> newline between methods and member variables.

Done.

https://codereview.chromium.org/2621153002/diff/40001/components/autofill/cor...
File components/autofill/core/browser/credit_card.cc (right):

https://codereview.chromium.org/2621153002/diff/40001/components/autofill/cor...
components/autofill/core/browser/credit_card.cc:159: int
CreditCard::PaymentRequestIconResourceId(const std::string& type) {
On 2017/01/12 at 21:27:11, Mathieu Perreault wrote:
> Would this fit alongside these:
https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil...

See Rouslan's comment below and my reply :)

https://codereview.chromium.org/2621153002/diff/40001/components/autofill/cor...
components/autofill/core/browser/credit_card.cc:160: if (type ==
kAmericanExpressCard)
On 2017/01/12 at 19:34:02, rouslan wrote:
> return GetPaymentRequestData(type).icon_resource_id;
> 
>
https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil...

Ahh, just what I needed! Removed this new function and just inlined the
GetPaymentRequestData(type).icon_resource_id call.

https://codereview.chromium.org/2621153002/diff/40001/components/payments/pay...
File components/payments/payment_request.h (right):

https://codereview.chromium.org/2621153002/diff/40001/components/payments/pay...
components/payments/payment_request.h:51: // Returns nullptr if there is no
selected card.
On 2017/01/12 at 21:27:11, Mathieu Perreault wrote:
> wrap lines

Done.

[anthonyvd, 2017-01-13 16:36:06]

Code Review sneakily hid a comment from me. It's now addressed!

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/payments...
File chrome/browser/payments/chrome_payment_request_delegate.h (right):

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.h:17:
ChromePaymentRequestDelegate() {}
On 2017/01/12 at 21:27:10, Mathieu Perreault wrote:
> Let's create the ChromePaymentRequestDelegate with a WebContents* that it
keeps as member, and can reuse in GetPersonalDataManager.
> 
> I don't think the PaymentRequestDelegate interface needs to be specific to
WebContents (i.e. someone could provide a FooBarPaymentRequestDelegate that
didn't require WebContents to get a PersonalDataManager)

Done.

[Mathieu, 2017-01-13 16:40:16]

lgtm

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

https://codereview.chromium.org/2621153002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:261: // The
name colums in each row should all have the same width, large enough to
On 2017/01/13 16:09:19, anthonyvd wrote:
> On 2017/01/12 at 21:27:11, Mathieu Perreault wrote:
> > On 2017/01/12 19:18:59, anthonyvd wrote:
> > > If anyone has a better idea to do this that doesn't feel as dirty, I'm all
> ears.
> > 
> > Idea 1: https://cs.chromium.org/chromium/src/ui/gfx/text_utils.h?rcl=0&l=29
> > 
> > Idea 2: RenderText thingy here
> https://cs.chromium.org/chromium/src/ui/views/controls/label.cc?rcl=0&l=908
> 
> It looks like neither of those would take into account the rest of the label
> (insets, borders, etc) as they only measure the text itself. #2 also seems
> equivalent to my implementation since it requires creating a label :(

Thanks for looking into it! Perhaps a views expert would have more insight into
this.

[anthonyvd, 2017-01-13 16:41:33]

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

[anthonyvd, 2017-01-13 16:41:34]

Hi Scott, can you PTAL at the c/b/ui/views/* code in this CL that adds a new
section to the PaymentRequest dialog?

Thanks a ton!

[sky, 2017-01-13 20:18:09]

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

https://codereview.chromium.org/2621153002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:291:
views::Label label(l10n_util::GetStringUTF16(name));
I wouldn't create a Label each time through. Also, IMO this code is a bit on the
unreadable side, but I leave that decision to you.

[anthonyvd, 2017-01-13 21:01:32]

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

https://codereview.chromium.org/2621153002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:291:
views::Label label(l10n_util::GetStringUTF16(name));
On 2017/01/13 at 20:18:08, sky wrote:
> I wouldn't create a Label each time through. Also, IMO this code is a bit on
the unreadable side, but I leave that decision to you.

Good point. Changed the code to reuse the label and made it a little more
readable with a plain-old for loop.

[sky, 2017-01-13 23:42:52]

One minor suggestion.

https://codereview.chromium.org/2621153002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right):

https://codereview.chromium.org/2621153002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:271: void
PaymentSheetViewController::InitWidestNameColumnViewWidth() {
Effective c++ recommends preferring non-member functions where possible. WDYT of
moving this function to the anonymous namespace at the top of the file and
having it return the size (so that it doesn't need to modify
widget_name_column_view_width_)? That way you could call from the constructor
and widget_name_column_view_width_ could be const.

https://codereview.chromium.org/2621153002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:288: for (int
name_id: section_names) {
So much more readable!

[anthonyvd, 2017-01-16 15:33:07]

https://codereview.chromium.org/2621153002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right):

https://codereview.chromium.org/2621153002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:271: void
PaymentSheetViewController::InitWidestNameColumnViewWidth() {
On 2017/01/13 at 23:42:52, sky wrote:
> Effective c++ recommends preferring non-member functions where possible. WDYT
of moving this function to the anonymous namespace at the top of the file and
having it return the size (so that it doesn't need to modify
widget_name_column_view_width_)? That way you could call from the constructor
and widget_name_column_view_width_ could be const.

Great point! Done.

[sky, 2017-01-17 17:40:07]

LGTM

https://codereview.chromium.org/2621153002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right):

https://codereview.chromium.org/2621153002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:148: for (int
name_id: section_names) {
nit: space before and after ':', e.g. "name_id : section_names"

[anthonyvd, 2017-01-17 19:37:32]

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

[commit-bot: I haz the power, 2017-01-17 19:38:28]

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-01-17 19:40:56]

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

[commit-bot: I haz the power, 2017-01-17 19:40:59]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[anthonyvd, 2017-01-17 19:43:51]

https://codereview.chromium.org/2621153002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc (right):

https://codereview.chromium.org/2621153002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:148: for (int
name_id: section_names) {
On 2017/01/17 at 17:40:07, sky wrote:
> nit: space before and after ':', e.g. "name_id : section_names"

Done.

[anthonyvd, 2017-01-17 20:49:26]

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

[commit-bot: I haz the power, 2017-01-17 20:50:04]

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-01-17 20:54:02]

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

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

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[anthonyvd, 2017-01-17 21:10:52]

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

[commit-bot: I haz the power, 2017-01-17 21:11:15]

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-01-17 22:09:14]

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

[commit-bot: I haz the power, 2017-01-17 22:09:15]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2017-01-17 22:10:19]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2017-01-17 22:10:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, mathp@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2621153002/#ps180001
(title: "Fix BUILD.gn deps.")

[commit-bot: I haz the power, 2017-01-17 22:10:58]

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-01-17 22:19:23]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1484691019337660,
"parent_rev": "30f0712eba825108e870f76270cee95f8c2102b9", "commit_rev":
"045303a3e7169ff8f56477510fd7f352060dc9a1"}

[commit-bot: I haz the power, 2017-01-17 22:19:53]

Description was changed from

==========
[WebPayments] Add the Payment Method section in the Payment Sheet

BUG=679835
==========

to

==========
[WebPayments] Add the Payment Method section in the Payment Sheet

BUG=679835

Review-Url: https://codereview.chromium.org/2621153002
Cr-Commit-Position: refs/heads/master@{#444162}
Committed:
https://chromium.googlesource.com/chromium/src/+/045303a3e7169ff8f56477510fd7...
==========

[commit-bot: I haz the power, 2017-01-17 22:19:54]

Committed patchset #8 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/045303a3e7169ff8f56477510fd7...