[Web Payments] Add Cancel button to all sheets

[Web Payments] Add Cancel button to all sheets

This CL refactors a bit of the Controller code to add a Cancel button
to all sheets. The base PaymentRequestSheetController class handles
clicks on the Cancel button (and other buttons that are common to
multiple sheets, like the back arrow).

This CL also adds a function to build the button row of the dialog.
Currently, this function only creates and lays out the Cancel button.

BUG=687339
Review-Url: https://codereview.chromium.org/2668063003
Cr-Commit-Position: refs/heads/master@{#448266}
Committed: https://chromium.googlesource.com/chromium/src/+/1b084b11b0404e4fec76c5ab372ef39226596dc0

[anthonyvd, 2017-01-31 22:33:10]

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

[anthonyvd, 2017-01-31 22:33:10]

Hi guys,

Can you PTAL at this CL to add the cancel button and centralize some of the
event handling code in PaymentRequestSheetController subclasses?

Thanks!

[Mathieu, 2017-01-31 22:53:12]

Very cool! I have a question

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

https://codereview.chromium.org/2668063003/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_views_util.h:54: //
|content_view| is displayed between |header_view| and the pay/cancel buttons.
Let's add a similar comment for buttons_view?

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

https://codereview.chromium.org/2668063003/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:202:
CreateButtonsView(this));
Perhaps CreateButtonsView could be part of CreatePaymentView, and instead we
just pass |this| here as the button listener? Or, moving CreatePaymentView into
PaymentRequestSheetController, so that we avoid passing |this|. 

I don't see each view creating it's own extra buttons in the future and passing
a different buttons view here. I could only see a view modifying the label of
the "Pay/Save/Done" button (achieved through a function override), and possibly
listening to the ButtonPressed event for the "Pay/Save/Done" button to override
default pay behavior, which we have the mechanism for below. But, perhaps I'm
missing some aspect of the mocks which would require extra buttons apart from
Cancel and "Pay/Save/Done".

[please use gerrit instead, 2017-02-01 14:33:35]

lgtm % comments and what Mathieu says.

https://codereview.chromium.org/2668063003/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/payment_request_sheet_controller.h
(right):

https://codereview.chromium.org/2668063003/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_sheet_controller.h:10: #include
"base/logging.h"
No longer used.

https://codereview.chromium.org/2668063003/diff/1/components/autofill_strings...
File components/autofill_strings.grdp (right):

https://codereview.chromium.org/2668063003/diff/1/components/autofill_strings...
components/autofill_strings.grdp:370: Cancel
Use
https://cs.chromium.org/chromium/src/components/components_strings.grd?rcl=df...
IDS_CANCEL instead.

[anthonyvd, 2017-02-01 15:35:23]

Thanks, addressed feedback and replied to comments!

https://codereview.chromium.org/2668063003/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/payment_request_sheet_controller.h
(right):

https://codereview.chromium.org/2668063003/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_sheet_controller.h:10: #include
"base/logging.h"
On 2017/02/01 at 14:33:35, rouslan wrote:
> No longer used.

Done.

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

https://codereview.chromium.org/2668063003/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_views_util.h:54: //
|content_view| is displayed between |header_view| and the pay/cancel buttons.
On 2017/01/31 at 22:53:12, Mathieu Perreault wrote:
> Let's add a similar comment for buttons_view?

Done.

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

https://codereview.chromium.org/2668063003/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:202:
CreateButtonsView(this));
On 2017/01/31 at 22:53:12, Mathieu Perreault wrote:
> Perhaps CreateButtonsView could be part of CreatePaymentView, and instead we
just pass |this| here as the button listener? Or, moving CreatePaymentView into
PaymentRequestSheetController, so that we avoid passing |this|. 
> 
> I don't see each view creating it's own extra buttons in the future and
passing a different buttons view here. I could only see a view modifying the
label of the "Pay/Save/Done" button (achieved through a function override), and
possibly listening to the ButtonPressed event for the "Pay/Save/Done" button to
override default pay behavior, which we have the mechanism for below. But,
perhaps I'm missing some aspect of the mocks which would require extra buttons
apart from Cancel and "Pay/Save/Done".

Many of the screens in the mocks have extra/different things other than
Pay/Cancel in that footer:

- Shipping Address, Payment Method, and Contact Info sheets have "Add
{Address|Card|Info}", "Edit", "Cancel" (no "Pay"). Their "Edit" button also
becomes "Hide Edit" in some cases.
- The editors have reminder text in there instead of buttons: "* indicates
required fields"

I see a few options:

1- This one where the controller creating the view passes everything they want
in the footer that's not "Cancel". We can have utility functions that create the
"Pay", "Edit", etc buttons easily.

2- Pass arguments to CreateButtonsView indicating which buttons should be added.
I feel like this gets out of hand quickly and leaks the controller's
responsibility into the generic/common code. In addition, this option makes it
impossible for controllers to cleanly keep a reference to their buttons, for
example if they need to control their disabled state.

3- Since CreateButtonsView takes a PaymentRequestSheetController*, have it call
into a function of that controller that either signals which buttons are
required or returns the actual buttons. That's pretty much a "pull" version of
1- and 2-.

I prefer #1 but I could obviously be missing something. WDYT?

https://codereview.chromium.org/2668063003/diff/1/components/autofill_strings...
File components/autofill_strings.grdp (right):

https://codereview.chromium.org/2668063003/diff/1/components/autofill_strings...
components/autofill_strings.grdp:370: Cancel
On 2017/02/01 at 14:33:35, rouslan wrote:
> Use
https://cs.chromium.org/chromium/src/components/components_strings.grd?rcl=df...
IDS_CANCEL instead.

Makes sense. Done.

[Mathieu, 2017-02-01 17:17:36]

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

https://codereview.chromium.org/2668063003/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:202:
CreateButtonsView(this));
On 2017/02/01 15:35:23, anthonyvd wrote:
> On 2017/01/31 at 22:53:12, Mathieu Perreault wrote:
> > Perhaps CreateButtonsView could be part of CreatePaymentView, and instead we
> just pass |this| here as the button listener? Or, moving CreatePaymentView
into
> PaymentRequestSheetController, so that we avoid passing |this|. 
> > 
> > I don't see each view creating it's own extra buttons in the future and
> passing a different buttons view here. I could only see a view modifying the
> label of the "Pay/Save/Done" button (achieved through a function override),
and
> possibly listening to the ButtonPressed event for the "Pay/Save/Done" button
to
> override default pay behavior, which we have the mechanism for below. But,
> perhaps I'm missing some aspect of the mocks which would require extra buttons
> apart from Cancel and "Pay/Save/Done".
> 
> Many of the screens in the mocks have extra/different things other than
> Pay/Cancel in that footer:
> 
> - Shipping Address, Payment Method, and Contact Info sheets have "Add
> {Address|Card|Info}", "Edit", "Cancel" (no "Pay"). Their "Edit" button also
> becomes "Hide Edit" in some cases.
> - The editors have reminder text in there instead of buttons: "* indicates
> required fields"
> 
> I see a few options:
> 
> 1- This one where the controller creating the view passes everything they want
> in the footer that's not "Cancel". We can have utility functions that create
the
> "Pay", "Edit", etc buttons easily.
> 
> 2- Pass arguments to CreateButtonsView indicating which buttons should be
added.
> I feel like this gets out of hand quickly and leaks the controller's
> responsibility into the generic/common code. In addition, this option makes it
> impossible for controllers to cleanly keep a reference to their buttons, for
> example if they need to control their disabled state.
> 
> 3- Since CreateButtonsView takes a PaymentRequestSheetController*, have it
call
> into a function of that controller that either signals which buttons are
> required or returns the actual buttons. That's pretty much a "pull" version of
> 1- and 2-.
> 
> I prefer #1 but I could obviously be missing something. WDYT?

Wow, I didn't realize that there were so many buttons. I had a look at the mocks
along with your explanation.

For simplicity let's split into the "left view" (contains either a label, or an
"Add [blah]" button) and the right view (Ideally always "Cancel" and
"Done/Pay/Save" -- Zach has sent feedback on "Edit").

The left view will be specific to each view controller, so they can pass the
whole view in CreatePaymentView (in a followup CL).

For the right view, since things are in flux and the Done/Pay/Save button is not
implemented now, we could defer? Though I still think my original suggestion
(each controller having to implement CreatePrimaryButton or
GetLabelForPrimaryButton, in line with your #3) makes sense... RE: the reference
to the button: you could get a reference through something like primary_button()
whenever you want to disable the button.

[anthonyvd, 2017-02-01 17:38:49]

On 2017/02/01 at 17:17:36, mathp wrote:
>
https://codereview.chromium.org/2668063003/diff/1/chrome/browser/ui/views/pay...
> File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc
(right):
> 
>
https://codereview.chromium.org/2668063003/diff/1/chrome/browser/ui/views/pay...
> chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:202:
CreateButtonsView(this));
> On 2017/02/01 15:35:23, anthonyvd wrote:
> > On 2017/01/31 at 22:53:12, Mathieu Perreault wrote:
> > > Perhaps CreateButtonsView could be part of CreatePaymentView, and instead
we
> > just pass |this| here as the button listener? Or, moving CreatePaymentView
into
> > PaymentRequestSheetController, so that we avoid passing |this|. 
> > > 
> > > I don't see each view creating it's own extra buttons in the future and
> > passing a different buttons view here. I could only see a view modifying the
> > label of the "Pay/Save/Done" button (achieved through a function override),
and
> > possibly listening to the ButtonPressed event for the "Pay/Save/Done" button
to
> > override default pay behavior, which we have the mechanism for below. But,
> > perhaps I'm missing some aspect of the mocks which would require extra
buttons
> > apart from Cancel and "Pay/Save/Done".
> > 
> > Many of the screens in the mocks have extra/different things other than
> > Pay/Cancel in that footer:
> > 
> > - Shipping Address, Payment Method, and Contact Info sheets have "Add
> > {Address|Card|Info}", "Edit", "Cancel" (no "Pay"). Their "Edit" button also
> > becomes "Hide Edit" in some cases.
> > - The editors have reminder text in there instead of buttons: "* indicates
> > required fields"
> > 
> > I see a few options:
> > 
> > 1- This one where the controller creating the view passes everything they
want
> > in the footer that's not "Cancel". We can have utility functions that create
the
> > "Pay", "Edit", etc buttons easily.
> > 
> > 2- Pass arguments to CreateButtonsView indicating which buttons should be
added.
> > I feel like this gets out of hand quickly and leaks the controller's
> > responsibility into the generic/common code. In addition, this option makes
it
> > impossible for controllers to cleanly keep a reference to their buttons, for
> > example if they need to control their disabled state.
> > 
> > 3- Since CreateButtonsView takes a PaymentRequestSheetController*, have it
call
> > into a function of that controller that either signals which buttons are
> > required or returns the actual buttons. That's pretty much a "pull" version
of
> > 1- and 2-.
> > 
> > I prefer #1 but I could obviously be missing something. WDYT?
> 
> Wow, I didn't realize that there were so many buttons. I had a look at the
mocks along with your explanation.
> 
> For simplicity let's split into the "left view" (contains either a label, or
an "Add [blah]" button) and the right view (Ideally always "Cancel" and
"Done/Pay/Save" -- Zach has sent feedback on "Edit").
> 
> The left view will be specific to each view controller, so they can pass the
whole view in CreatePaymentView (in a followup CL).
> 
> For the right view, since things are in flux and the Done/Pay/Save button is
not implemented now, we could defer? Though I still think my original suggestion
(each controller having to implement CreatePrimaryButton or
GetLabelForPrimaryButton, in line with your #3) makes sense... RE: the reference
to the button: you could get a reference through something like primary_button()
whenever you want to disable the button.

Makes sense especially if "Edit" is going away. I think having the controller
implement CreatePrimaryButton this way it can retain a non-owned pointer to the
primary button to control it's enabled/disabled state. It might have to be
coupled with a HasPrimaryButton() though, since not all screens have it. What do
you think of:

virtual bool HasPrimaryButton() const = 0;
virtual std::unique_ptr<views::Button> CreatePrimaryButton() = 0;

Then rename CreateButtonsView() to CreateFooterView() and change it to something
like:

std::unique_ptr<views::View> CreateFooterView(PaymentRequestSheetController*
controller, std::unique_ptr<views::View> extra_view) {
  // Add extra_view to the left
  if (controller->HasPrimaryButton()) {
    // Add controller->CreatePrimaryButton() to the right
  }
  // Add cancel button to the right of the primary button
  return footer;
}

[Mathieu, 2017-02-01 19:08:51]

On 2017/02/01 17:38:49, anthonyvd wrote:
> On 2017/02/01 at 17:17:36, mathp wrote:
> >
>
https://codereview.chromium.org/2668063003/diff/1/chrome/browser/ui/views/pay...
> > File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/2668063003/diff/1/chrome/browser/ui/views/pay...
> > chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:202:
> CreateButtonsView(this));
> > On 2017/02/01 15:35:23, anthonyvd wrote:
> > > On 2017/01/31 at 22:53:12, Mathieu Perreault wrote:
> > > > Perhaps CreateButtonsView could be part of CreatePaymentView, and
instead
> we
> > > just pass |this| here as the button listener? Or, moving CreatePaymentView
> into
> > > PaymentRequestSheetController, so that we avoid passing |this|. 
> > > > 
> > > > I don't see each view creating it's own extra buttons in the future and
> > > passing a different buttons view here. I could only see a view modifying
the
> > > label of the "Pay/Save/Done" button (achieved through a function
override),
> and
> > > possibly listening to the ButtonPressed event for the "Pay/Save/Done"
button
> to
> > > override default pay behavior, which we have the mechanism for below. But,
> > > perhaps I'm missing some aspect of the mocks which would require extra
> buttons
> > > apart from Cancel and "Pay/Save/Done".
> > > 
> > > Many of the screens in the mocks have extra/different things other than
> > > Pay/Cancel in that footer:
> > > 
> > > - Shipping Address, Payment Method, and Contact Info sheets have "Add
> > > {Address|Card|Info}", "Edit", "Cancel" (no "Pay"). Their "Edit" button
also
> > > becomes "Hide Edit" in some cases.
> > > - The editors have reminder text in there instead of buttons: "* indicates
> > > required fields"
> > > 
> > > I see a few options:
> > > 
> > > 1- This one where the controller creating the view passes everything they
> want
> > > in the footer that's not "Cancel". We can have utility functions that
create
> the
> > > "Pay", "Edit", etc buttons easily.
> > > 
> > > 2- Pass arguments to CreateButtonsView indicating which buttons should be
> added.
> > > I feel like this gets out of hand quickly and leaks the controller's
> > > responsibility into the generic/common code. In addition, this option
makes
> it
> > > impossible for controllers to cleanly keep a reference to their buttons,
for
> > > example if they need to control their disabled state.
> > > 
> > > 3- Since CreateButtonsView takes a PaymentRequestSheetController*, have it
> call
> > > into a function of that controller that either signals which buttons are
> > > required or returns the actual buttons. That's pretty much a "pull"
version
> of
> > > 1- and 2-.
> > > 
> > > I prefer #1 but I could obviously be missing something. WDYT?
> > 
> > Wow, I didn't realize that there were so many buttons. I had a look at the
> mocks along with your explanation.
> > 
> > For simplicity let's split into the "left view" (contains either a label, or
> an "Add [blah]" button) and the right view (Ideally always "Cancel" and
> "Done/Pay/Save" -- Zach has sent feedback on "Edit").
> > 
> > The left view will be specific to each view controller, so they can pass the
> whole view in CreatePaymentView (in a followup CL).
> > 
> > For the right view, since things are in flux and the Done/Pay/Save button is
> not implemented now, we could defer? Though I still think my original
suggestion
> (each controller having to implement CreatePrimaryButton or
> GetLabelForPrimaryButton, in line with your #3) makes sense... RE: the
reference
> to the button: you could get a reference through something like
primary_button()
> whenever you want to disable the button.
> 
> Makes sense especially if "Edit" is going away. I think having the controller
> implement CreatePrimaryButton this way it can retain a non-owned pointer to
the
> primary button to control it's enabled/disabled state. It might have to be
> coupled with a HasPrimaryButton() though, since not all screens have it. What
do
> you think of:
> 
> virtual bool HasPrimaryButton() const = 0;
> virtual std::unique_ptr<views::Button> CreatePrimaryButton() = 0;
> 
> Then rename CreateButtonsView() to CreateFooterView() and change it to
something
> like:
> 
> std::unique_ptr<views::View> CreateFooterView(PaymentRequestSheetController*
> controller, std::unique_ptr<views::View> extra_view) {
>   // Add extra_view to the left
>   if (controller->HasPrimaryButton()) {
>     // Add controller->CreatePrimaryButton() to the right
>   }
>   // Add cancel button to the right of the primary button
>   return footer;
> }

What if CreatePrimaryButton would return an empty std::unique_ptr<views::View>
in case of no button?

std::unique_ptr<views::View> primary = controller->CreatePrimaryButton();
if (primary) {
  // Retain a reference to primary.get();
  // add primary to the view
}

testing for std::unique_ptr:
http://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool

[anthonyvd, 2017-02-02 20:29:03]

Made the footer changes we discussed. PTAL!

[Mathieu, 2017-02-02 21:04:58]

Thanks for your patience! lgtm

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

https://codereview.chromium.org/2668063003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.cc:27:
PaymentRequestSheetController::CreatePrimaryButton() {
should we make this abstract and ask every controller to implement it? I feel
like it could lead to fewer bugs. I could be swayed though

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

https://codereview.chromium.org/2668063003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.h:57:
std::unique_ptr<views::View> CreatePaymentView(
Comment to say it will add the footer from below

[anthonyvd, 2017-02-02 21:33:23]

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

[anthonyvd, 2017-02-02 21:33:24]

Thanks to you for the feedback! Addressed comments.

Scott, can you please do an OWNERS review of this c/b/ui/views/ code? Thank you!

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

https://codereview.chromium.org/2668063003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.cc:27:
PaymentRequestSheetController::CreatePrimaryButton() {
On 2017/02/02 at 21:04:58, Mathieu Perreault wrote:
> should we make this abstract and ask every controller to implement it? I feel
like it could lead to fewer bugs. I could be swayed though

That's a good question, I was hesitating between that and having the base class
return the "Pay" button since that's the default one. Considering I wasn't ready
to implement a proper "Pay" yet, I went with the empty unique_ptr for now.

I think returning Pay here and Done in the base editor controller + empty
unique_ptr in the couple of screens that have no button would be ideal in terms
of not repeating ourselves. WDYT?

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

https://codereview.chromium.org/2668063003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.h:57:
std::unique_ptr<views::View> CreatePaymentView(
On 2017/02/02 at 21:04:58, Mathieu Perreault wrote:
> Comment to say it will add the footer from below

Oops, it was still in the utils file. Good catch, done.

[Mathieu, 2017-02-02 21:44:15]

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

https://codereview.chromium.org/2668063003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.cc:27:
PaymentRequestSheetController::CreatePrimaryButton() {
On 2017/02/02 21:33:24, anthonyvd wrote:
> On 2017/02/02 at 21:04:58, Mathieu Perreault wrote:
> > should we make this abstract and ask every controller to implement it? I
feel
> like it could lead to fewer bugs. I could be swayed though
> 
> That's a good question, I was hesitating between that and having the base
class
> return the "Pay" button since that's the default one. Considering I wasn't
ready
> to implement a proper "Pay" yet, I went with the empty unique_ptr for now.
> 
> I think returning Pay here and Done in the base editor controller + empty
> unique_ptr in the couple of screens that have no button would be ideal in
terms
> of not repeating ourselves. WDYT?

Yeah it certainly works and it's not a huge change if we ever regret it :) sgtm

[sky, 2017-02-03 02:36:50]

How about making you and rouslan@ owners of c/b/ui/views/payments?

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

https://codereview.chromium.org/2668063003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.cc:28: return
std::unique_ptr<views::Button>();
return nullptr.

https://codereview.chromium.org/2668063003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.cc:33: switch
(sender->tag()) {
Can you static_cast to the PaymentRequestCommonTags so you don't need a default
branch?

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

https://codereview.chromium.org/2668063003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.h:47: //
return an empty std::unique_ptr to indicate that no primary button should
optional: I would say null rather than empty std::unique_ptr, but you are
technically correct.

https://codereview.chromium.org/2668063003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.h:51: virtual
std::unique_ptr<views::Button> CreatePrimaryButton();
Can this be protected?

[anthonyvd, 2017-02-03 16:50:57]

Making rouslan@ and I OWNERS SGTM if you think we can handle our own views code
:)

Addressed comments and added an OWNERS file, thanks!

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

https://codereview.chromium.org/2668063003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.cc:28: return
std::unique_ptr<views::Button>();
On 2017/02/03 at 02:36:49, sky wrote:
> return nullptr.

Done.

https://codereview.chromium.org/2668063003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.cc:33: switch
(sender->tag()) {
On 2017/02/03 at 02:36:49, sky wrote:
> Can you static_cast to the PaymentRequestCommonTags so you don't need a
default branch?

Oh, much better. Thanks! I still need to NOTREACHED() for the
PAYMENT_REQUEST_COMMON_TAG_MAX value but this makes it less error prone to add
value to the enum.

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

https://codereview.chromium.org/2668063003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.h:47: //
return an empty std::unique_ptr to indicate that no primary button should
On 2017/02/03 at 02:36:50, sky wrote:
> optional: I would say null rather than empty std::unique_ptr, but you are
technically correct.

Clarified that empty std::unique_ptr here means nullptr. Thanks.

https://codereview.chromium.org/2668063003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.h:51: virtual
std::unique_ptr<views::Button> CreatePrimaryButton();
On 2017/02/03 at 02:36:49, sky wrote:
> Can this be protected?

Sure can, done!

[sky, 2017-02-03 23:42:12]

LGTM - I'm always happy to review tricky views related changes, but it seems
like the bulk of these changes aren't that. So, definitely add yourselves as
owners.

[sky, 2017-02-03 23:42:25]

Which you did, thanks!

[anthonyvd, 2017-02-06 14:52:14]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2017-02-06 14:52:14]

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/2668063003/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-06 14:52:25]

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-06 16:14:29]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486392734256390,
"parent_rev": "6ebe16e737195e35e4cb250dce734817bd3db339", "commit_rev":
"1b084b11b0404e4fec76c5ab372ef39226596dc0"}

[commit-bot: I haz the power, 2017-02-06 16:15:01]

Description was changed from

==========
[Web Payments] Add Cancel button to all sheets

This CL refactors a bit of the Controller code to add a Cancel button
to all sheets. The base PaymentRequestSheetController class handles
clicks on the Cancel button (and other buttons that are common to
multiple sheets, like the back arrow).

This CL also adds a function to build the button row of the dialog.
Currently, this function only creates and lays out the Cancel button.

BUG=687339
==========

to

==========
[Web Payments] Add Cancel button to all sheets

This CL refactors a bit of the Controller code to add a Cancel button
to all sheets. The base PaymentRequestSheetController class handles
clicks on the Cancel button (and other buttons that are common to
multiple sheets, like the back arrow).

This CL also adds a function to build the button row of the dialog.
Currently, this function only creates and lays out the Cancel button.

BUG=687339

Review-Url: https://codereview.chromium.org/2668063003
Cr-Commit-Position: refs/heads/master@{#448266}
Committed:
https://chromium.googlesource.com/chromium/src/+/1b084b11b0404e4fec76c5ab372e...
==========

[commit-bot: I haz the power, 2017-02-06 16:15:03]

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

[Mathieu, 2017-02-06 17:24:53]

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

https://codereview.chromium.org/2668063003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_request_sheet_controller.cc:35:
dialog()->CloseDialog();
I think for most screens, the desired behavior of the "Cancel" button should be
to GoBack() (until you are at the root, at which point you close). You may want
to double check that with UX