[Web Payments] Add Spinners and timeout while waiting for UpdateWith

[Web Payments] Add spinners while waiting for UpdateWith

BUG=710568
Review-Url: https://codereview.chromium.org/2815723002
Cr-Commit-Position: refs/heads/master@{#464575}
Committed: https://chromium.googlesource.com/chromium/src/+/2f30baa13c1136a5188b6ebdae22b52699d687df

[anthonyvd, 2017-04-11 19:09:08]

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

[anthonyvd, 2017-04-11 19:09:09]

Hi Math, PTAL! Thanks :)

[please use gerrit instead, 2017-04-11 19:15:09]

rouslan@chromium.org changed reviewers:
+ rouslan@chromium.org

[please use gerrit instead, 2017-04-11 19:15:09]

https://codereview.chromium.org/2815723002/diff/1/components/payments/content...
File components/payments/content/payment_request.h (right):

https://codereview.chromium.org/2815723002/diff/1/components/payments/content...
components/payments/content/payment_request.h:108: base::OneShotTimer
wait_timer_;
Not necessary. There's already a timer in Blink:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen...

[anthonyvd, 2017-04-13 14:43:16]

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

[commit-bot: I haz the power, 2017-04-13 14:43:28]

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

[please use gerrit instead, 2017-04-13 14:53:00]

Description was changed from

==========
[Web Payments] Add Spinners and timeout while waiting for UpdateWith

BUG=710568
==========

to

==========
[Web Payments] Add Spinners and timeout while waiting for UpdateWith

BUG=710568
==========

[please use gerrit instead, 2017-04-13 14:53:00]

rouslan@chromium.org changed reviewers:
- rouslan@chromium.org

[anthonyvd, 2017-04-13 14:55:07]

Description was changed from

==========
[Web Payments] Add Spinners and timeout while waiting for UpdateWith

BUG=710568
==========

to

==========
[Web Payments] Add spinners while waiting for UpdateWith

BUG=710568
==========

[anthonyvd, 2017-04-13 14:55:07]

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

[anthonyvd, 2017-04-13 14:56:22]

Removed the timeout since there's already one in Blink. PTAL again!

https://codereview.chromium.org/2815723002/diff/1/components/payments/content...
File components/payments/content/payment_request.h (right):

https://codereview.chromium.org/2815723002/diff/1/components/payments/content...
components/payments/content/payment_request.h:108: base::OneShotTimer
wait_timer_;
On 2017/04/11 at 19:15:09, ಠ_ಠ wrote:
> Not necessary. There's already a timer in Blink:
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen...

Ah! Thanks :)

[please use gerrit instead, 2017-04-13 15:05:00]

LGTM % comment

https://codereview.chromium.org/2815723002/diff/20001/components/payments/con...
File components/payments/content/payment_request_spec.cc (right):

https://codereview.chromium.org/2815723002/diff/20001/components/payments/con...
components/payments/content/payment_request_spec.cc:132: for (auto& observer :
observers_)
Need {}.

[Mathieu, 2017-04-13 15:35:30]

looks good! just a few comments.

I'm a bit worried that there's no test for the spinners, as on Android we had
several regression cases where the UI would spin indefinitely. Perhaps the
payment_sheet_view_controller could call something like
dialog()->ShippingOptionSpinnerIsShowing(true/false) which would in turn bubble
an |observer_for_testing| event. In the browsertests, you could now check for
this event sequence

ResetEventObserver({DialogEvent::SHIPPING_OPTION_SPINNER_SHOWN,
DialogEvent::SHIPPING_OPTION_SPINNER_REMOVED, DialogEvent::SPEC_DONE_UPDATING})

Open to ideas... just trying to avoid regressions. Thanks

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

https://codereview.chromium.org/2815723002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:553:
PaymentRequestSpec::UpdateReason::SHIPPING_OPTION) {
so we'd have to check with UX, but if you select an address
(UpdateReason::SHIPPING_ADDRESS) it would probably make sense to spin this
section too? The updated spec can come back with a different selected option, in
which case it would magically change without a spinner.

https://codereview.chromium.org/2815723002/diff/20001/components/payments/con...
File components/payments/content/payment_request.cc (right):

https://codereview.chromium.org/2815723002/diff/20001/components/payments/con...
components/payments/content/payment_request.cc:128:
spec_->StartWaitingForUpdateWith(
What do you think about putting this in
PaymentRequestState::SetSelectedShippingOption (before delegate_-> is called)? I
would like to avoid too much UI-type logic in here, and just keep what's
required for the mojo interaction.

https://codereview.chromium.org/2815723002/diff/20001/components/payments/con...
components/payments/content/payment_request.cc:135:
spec_->StartWaitingForUpdateWith(
Similarly in PaymentRequestState::SetSelectedShippingProfile

[commit-bot: I haz the power, 2017-04-13 15:44:12]

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

[commit-bot: I haz the power, 2017-04-13 15:44:13]

Dry run: This issue passed the CQ dry run.

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

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

[commit-bot: I haz the power, 2017-04-13 21:02:45]

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

[anthonyvd, 2017-04-13 21:05:59]

Addressed comments, PTAL!

Math, re: browser tests. The Payment Sheet decides whether to show a spinner
when it gets an UpdateContentView call based only on the value of
current_update_reason_. Since that member is reset to NONE in the SpecUpdated
callback, testing it the way you suggest doesn't really add any more robustness
unfortunately (since we're already looking for SPEC_UPDATED events in tests). 

I thought about this a bit and couldn't come up with a way to test this short of
asserting that the view hierarchy contains or not a spinner, which I think is
exactly the kind of browser tests we should avoid (they're fickle and don't
provide particularly much value).

Do you have any suggestion?

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

https://codereview.chromium.org/2815723002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:553:
PaymentRequestSpec::UpdateReason::SHIPPING_OPTION) {
On 2017/04/13 at 15:35:29, Mathieu wrote:
> so we'd have to check with UX, but if you select an address
(UpdateReason::SHIPPING_ADDRESS) it would probably make sense to spin this
section too? The updated spec can come back with a different selected option, in
which case it would magically change without a spinner.

I'll ask UX and follow up.

https://codereview.chromium.org/2815723002/diff/20001/components/payments/con...
File components/payments/content/payment_request.cc (right):

https://codereview.chromium.org/2815723002/diff/20001/components/payments/con...
components/payments/content/payment_request.cc:128:
spec_->StartWaitingForUpdateWith(
On 2017/04/13 at 15:35:29, Mathieu wrote:
> What do you think about putting this in
PaymentRequestState::SetSelectedShippingOption (before delegate_-> is called)? I
would like to avoid too much UI-type logic in here, and just keep what's
required for the mojo interaction.

Yeah makes sense. It was here because of the timer, which doesn't exist anymore.

https://codereview.chromium.org/2815723002/diff/20001/components/payments/con...
components/payments/content/payment_request.cc:135:
spec_->StartWaitingForUpdateWith(
On 2017/04/13 at 15:35:29, Mathieu wrote:
> Similarly in PaymentRequestState::SetSelectedShippingProfile

Done.

https://codereview.chromium.org/2815723002/diff/20001/components/payments/con...
File components/payments/content/payment_request_spec.cc (right):

https://codereview.chromium.org/2815723002/diff/20001/components/payments/con...
components/payments/content/payment_request_spec.cc:132: for (auto& observer :
observers_)
On 2017/04/13 at 15:04:59, ಠ_ಠ wrote:
> Need {}.

Done.

[Mathieu, 2017-04-13 21:08:14]

lgtm

[commit-bot: I haz the power, 2017-04-13 21:57:18]

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

[commit-bot: I haz the power, 2017-04-13 21:57:19]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2017-04-13 22:06:49]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2017-04-13 22:06:50]

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

[commit-bot: I haz the power, 2017-04-13 22:07:44]

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-04-13 22:30:55]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492121209827140,
"parent_rev": "30fa4e4840a39609fb5f8093e74454fb18a1500b", "commit_rev":
"2f30baa13c1136a5188b6ebdae22b52699d687df"}

[commit-bot: I haz the power, 2017-04-13 22:31:46]

Description was changed from

==========
[Web Payments] Add spinners while waiting for UpdateWith

BUG=710568
==========

to

==========
[Web Payments] Add spinners while waiting for UpdateWith

BUG=710568

Review-Url: https://codereview.chromium.org/2815723002
Cr-Commit-Position: refs/heads/master@{#464575}
Committed:
https://chromium.googlesource.com/chromium/src/+/2f30baa13c1136a5188b6ebdae22...
==========

[commit-bot: I haz the power, 2017-04-13 22:31:47]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/2f30baa13c1136a5188b6ebdae22...