[Payments] Add the pay button, and control its enabled state

[Payments] Add the pay button, and control its enabled state

Depending on the data requested by the merchant, the pay button may or
may not be disabled. More validation and logic to follow.

BUG=696733
TEST=PaymentSheet... interactive_ui_tests

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

[Mathieu, 2017-02-27 21:51:32]

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

[Mathieu, 2017-02-27 21:54:08]

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

[Mathieu, 2017-02-27 21:54:09]

Hey, it's 5PM review mailing time!

PTAL ;)

[Mathieu, 2017-02-28 01:45:50]

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

[commit-bot: I haz the power, 2017-02-28 01:46: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-28 02:59:57]

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

[commit-bot: I haz the power, 2017-02-28 02:59:58]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2017-02-28 17:16:35]

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

https://codereview.chromium.org/2715213005/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view.cc:146: void
PaymentRequestDialogView::UpdatePayButtonState(bool enabled) {
I don't think this pattern is the way to go:

1. The Payment Sheet isn't the only one with a pay button
2. Multiple Sheets layered on top of each other might need to update their pay
button at the same time
3. Traversing the view hierarchy isn't expensive but it's also not free.
4. As your comment mentions, there are weird states where the dialog is closed
and PR calls this function

I would instead add an PaymentRequest::Observer that raises a
PaymentInformationChanged() event. View controllers can query PaymentRequest in
their implementation of the observer to determine the state of the pay button
and subscribe/unsubscribe themselves to avoid lifetime issues.

WDYT?

[Mathieu, 2017-02-28 19:42:39]

Have a look at this version?

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

https://codereview.chromium.org/2715213005/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view.cc:146: void
PaymentRequestDialogView::UpdatePayButtonState(bool enabled) {
On 2017/02/28 17:16:32, anthonyvd wrote:
> I don't think this pattern is the way to go:
> 
> 1. The Payment Sheet isn't the only one with a pay button
> 2. Multiple Sheets layered on top of each other might need to update their pay
> button at the same time
> 3. Traversing the view hierarchy isn't expensive but it's also not free.
> 4. As your comment mentions, there are weird states where the dialog is closed
> and PR calls this function
> 
> I would instead add an PaymentRequest::Observer that raises a
> PaymentInformationChanged() event. View controllers can query PaymentRequest
in
> their implementation of the observer to determine the state of the pay button
> and subscribe/unsubscribe themselves to avoid lifetime issues.
> 
> WDYT?

OMG yes, it's much better with observers. Good catch about the pay button on
multiple screens, too.

[Mathieu, 2017-02-28 19:49:31]

Patchset #5 (id:80001) has been deleted

[Mathieu, 2017-02-28 19:49:38]

Patchset #3 (id:40001) has been deleted

[anthonyvd, 2017-02-28 21:07:18]

lgtm % a couple comments. Thanks for the changes!

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

https://codereview.chromium.org/2715213005/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc:188:
void PaymentRequestInteractiveTestBase::AddCreditCard(
Do we want to set_use_count() incrementally here so that we can depend on cards
being returned in the order they were added in tests?

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

https://codereview.chromium.org/2715213005/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:242: case
static_cast<int>(PaymentSheetViewControllerTags::PAY_BUTTON):
You can handle this in the base PaymentRequestSheetController since multiple
screens have the button and will use the same tag.

https://codereview.chromium.org/2715213005/diff/120001/components/payments/co...
File components/payments/content/payment_request.cc (right):

https://codereview.chromium.org/2715213005/diff/120001/components/payments/co...
components/payments/content/payment_request.cc:286: if (selected_credit_card_ ==
nullptr || !selected_credit_card_->IsValid())
Isn't there a caveat here that server cards aren't necessarily going to have
IsValid() return true? If that's the case, can you add a TODO?

https://codereview.chromium.org/2715213005/diff/120001/components/payments/co...
File components/payments/content/payment_request.h (right):

https://codereview.chromium.org/2715213005/diff/120001/components/payments/co...
components/payments/content/payment_request.h:35: class Observer {
Awesome, we can hook up a bunch more events through here as needed to keep the
views in sync with the model!

[Mathieu, 2017-02-28 21:43:27]

Thanks, will submit

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

https://codereview.chromium.org/2715213005/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_request_interactive_uitest_base.cc:188:
void PaymentRequestInteractiveTestBase::AddCreditCard(
On 2017/02/28 21:07:18, anthonyvd wrote:
> Do we want to set_use_count() incrementally here so that we can depend on
cards
> being returned in the order they were added in tests

Let's keep the ability for tests to specify a use_count, and possibly regret it
later. I'll add comments

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

https://codereview.chromium.org/2715213005/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:242: case
static_cast<int>(PaymentSheetViewControllerTags::PAY_BUTTON):
On 2017/02/28 21:07:18, anthonyvd wrote:
> You can handle this in the base PaymentRequestSheetController since multiple
> screens have the button and will use the same tag.

Done.

https://codereview.chromium.org/2715213005/diff/120001/components/payments/co...
File components/payments/content/payment_request.cc (right):

https://codereview.chromium.org/2715213005/diff/120001/components/payments/co...
components/payments/content/payment_request.cc:286: if (selected_credit_card_ ==
nullptr || !selected_credit_card_->IsValid())
On 2017/02/28 21:07:18, anthonyvd wrote:
> Isn't there a caveat here that server cards aren't necessarily going to have
> IsValid() return true? If that's the case, can you add a TODO?

oh wow good catch

https://codereview.chromium.org/2715213005/diff/120001/components/payments/co...
File components/payments/content/payment_request.h (right):

https://codereview.chromium.org/2715213005/diff/120001/components/payments/co...
components/payments/content/payment_request.h:35: class Observer {
On 2017/02/28 21:07:18, anthonyvd wrote:
> Awesome, we can hook up a bunch more events through here as needed to keep the
> views in sync with the model!

Ya!

[Mathieu, 2017-02-28 21:43:33]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2017-02-28 21:43:34]

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/2715213005/#ps100002
(title: "addressed comments")

[commit-bot: I haz the power, 2017-02-28 21:44:14]

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-28 22:11:35]

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

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

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Mathieu, 2017-02-28 23:20:35]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2017-02-28 23:21:57]

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 00:51:52]

CQ is committing da patch.

Bot data: {"patchset_id": 100002, "attempt_start_ts": 1488324035668540,
"parent_rev": "15db143218636b7f14ef38f9e5eb61479daa4b84", "commit_rev":
"d4be8de802d4fd6e2b8ec7a85253eae7db69c15e"}

[commit-bot: I haz the power, 2017-03-01 00:52:54]

Description was changed from

==========
[Payments] Add the pay button, and control its enabled state

Depending on the data requested by the merchant, the pay button may or
may not be disabled. More validation and logic to follow.

BUG=696733
TEST=PaymentSheet... interactive_ui_tests
==========

to

==========
[Payments] Add the pay button, and control its enabled state

Depending on the data requested by the merchant, the pay button may or
may not be disabled. More validation and logic to follow.

BUG=696733
TEST=PaymentSheet... interactive_ui_tests

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

[commit-bot: I haz the power, 2017-03-01 00:52:55]

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