[Web Payments] Implement item selection in lists.

[Web Payments] Implement item selection in lists.

This change implements item selection in all lists in PaymentRequest
dialog sheets. It also implements updating the Payment Sheet after a
selection changes.

BUG=703302
TEST=unit_tests,browsertests

Review-Url: https://codereview.chromium.org/2759253002
Cr-Commit-Position: refs/heads/master@{#458553}
Committed: https://chromium.googlesource.com/chromium/src/+/0116ce334c021757db311c867d96bb56faeda1b7

[anthonyvd, 2017-03-20 19:29:54]

Patchset #1 (id:1) has been deleted

[anthonyvd, 2017-03-20 19:30:47]

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

[anthonyvd, 2017-03-20 19:30:47]

Hi Math, PTAL?

With this change, it actually becomes possible to change selected values from
the UI.

Thanks!

[Mathieu, 2017-03-21 13:56:59]

Looks great, few questions.

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

https://codereview.chromium.org/2759253002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_item_list.cc:117:
list()->SelectItem(this, /*go_back=*/true);
Do you think we could do 

if (CanBeSelected()) {
  list()->SelectItem(this);
}
PerformSelectionConfirmed(/*item_was_selected=*/CanBeSelected());

Each subclass would know what to do in case of item selection. I prefer each
subclass having to decide whether to call dialog_->GoBack().

Let me know what you think.

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

https://codereview.chromium.org/2759253002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:209:
std::unique_ptr<views::View> content_view = base::MakeUnique<views::View>();
As discussed, let's own the content view instead of an additional
container_view.

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

https://codereview.chromium.org/2759253002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_option_view_controller.cc:34: void
SelectedStateChanged() override {
We always seem to use SelectedStateChanged + selected() - unless I'm wrong. If
this is true perhaps the call should be "item is now selected"?

[Mathieu, 2017-03-21 14:05:47]

Also, I see mentions of "unit_tests, browsertests" but no files are present.
Would be great to have those kinds of tests :)

[anthonyvd, 2017-03-21 14:56:07]

Feedback addressed, thanks!

Re: tests. The list code is already tested, hence the mention. The only thing
this CL adds is that the dialog navigates back, which I didn't feel was super
relevant to test. Basically since this is a refactor, existing tests should not
stop passing. Let me know if your opinion differs :)

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

https://codereview.chromium.org/2759253002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_item_list.cc:117:
list()->SelectItem(this, /*go_back=*/true);
On 2017/03/21 at 13:56:59, Mathieu Perreault wrote:
> Do you think we could do 
> 
> if (CanBeSelected()) {
>   list()->SelectItem(this);
> }
> PerformSelectionConfirmed(/*item_was_selected=*/CanBeSelected());
> 
> Each subclass would know what to do in case of item selection. I prefer each
subclass having to decide whether to call dialog_->GoBack().
> 
> Let me know what you think.

I changed the code to call GoBack in subclasses implementation of
SelectedStateChanged(). This seemed a bit cleaner than adding another function
that would only call GoBack, yet as flexible.

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

https://codereview.chromium.org/2759253002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:209:
std::unique_ptr<views::View> content_view = base::MakeUnique<views::View>();
On 2017/03/21 at 13:56:59, Mathieu Perreault wrote:
> As discussed, let's own the content view instead of an additional
container_view.

Yep will refactor in a follow up, as discussed.

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

https://codereview.chromium.org/2759253002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_option_view_controller.cc:34: void
SelectedStateChanged() override {
On 2017/03/21 at 13:56:59, Mathieu Perreault wrote:
> We always seem to use SelectedStateChanged + selected() - unless I'm wrong. If
this is true perhaps the call should be "item is now selected"?

Yeah I thought about this too. I think it just happens that *now* we only do
stuff when the item is selected but conceptually it makes sense to notify items
when they're deselected as well (for example, some items might want to modify
how they look depending on whether they're selected or not).

[Mathieu, 2017-03-21 15:07:15]

On 2017/03/21 14:56:07, anthonyvd wrote:
> Feedback addressed, thanks!
> 
> Re: tests. The list code is already tested, hence the mention. The only thing
> this CL adds is that the dialog navigates back, which I didn't feel was super
> relevant to test. Basically since this is a refactor, existing tests should
not
> stop passing. Let me know if your opinion differs :)
> 
>
https://codereview.chromium.org/2759253002/diff/20001/chrome/browser/ui/views...
> File chrome/browser/ui/views/payments/payment_request_item_list.cc (right):
> 
>
https://codereview.chromium.org/2759253002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/payments/payment_request_item_list.cc:117:
> list()->SelectItem(this, /*go_back=*/true);
> On 2017/03/21 at 13:56:59, Mathieu Perreault wrote:
> > Do you think we could do 
> > 
> > if (CanBeSelected()) {
> >   list()->SelectItem(this);
> > }
> > PerformSelectionConfirmed(/*item_was_selected=*/CanBeSelected());
> > 
> > Each subclass would know what to do in case of item selection. I prefer each
> subclass having to decide whether to call dialog_->GoBack().
> > 
> > Let me know what you think.
> 
> I changed the code to call GoBack in subclasses implementation of
> SelectedStateChanged(). This seemed a bit cleaner than adding another function
> that would only call GoBack, yet as flexible.
> 
>
https://codereview.chromium.org/2759253002/diff/20001/chrome/browser/ui/views...
> File chrome/browser/ui/views/payments/payment_sheet_view_controller.cc
(right):
> 
>
https://codereview.chromium.org/2759253002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:209:
> std::unique_ptr<views::View> content_view = base::MakeUnique<views::View>();
> On 2017/03/21 at 13:56:59, Mathieu Perreault wrote:
> > As discussed, let's own the content view instead of an additional
> container_view.
> 
> Yep will refactor in a follow up, as discussed.
> 
>
https://codereview.chromium.org/2759253002/diff/20001/chrome/browser/ui/views...
> File chrome/browser/ui/views/payments/shipping_option_view_controller.cc
> (right):
> 
>
https://codereview.chromium.org/2759253002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/payments/shipping_option_view_controller.cc:34: void
> SelectedStateChanged() override {
> On 2017/03/21 at 13:56:59, Mathieu Perreault wrote:
> > We always seem to use SelectedStateChanged + selected() - unless I'm wrong.
If
> this is true perhaps the call should be "item is now selected"?
> 
> Yeah I thought about this too. I think it just happens that *now* we only do
> stuff when the item is selected but conceptually it makes sense to notify
items
> when they're deselected as well (for example, some items might want to modify
> how they look depending on whether they're selected or not).

In the browsertests, you can add an event observer for BACK_NAVIGATION, should
be pretty straightforward!

Examples:
https://cs.chromium.org/search/?q=BACK_NAVIGATION+file:payments&sq=package:ch...

[anthonyvd, 2017-03-21 19:04:12]

Relevant browser test updated. PTAL!

[Mathieu, 2017-03-21 19:27:36]

lgtm with a UX question

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

https://codereview.chromium.org/2759253002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_method_view_controller_browsertest.cc:105:
ClickOnDialogViewAndWait(list_view->child_at(1));
Do we not expect it to go back to the payment method screen? In a way, you're
"confirming" your choice...

[anthonyvd, 2017-03-21 20:29:14]

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

https://codereview.chromium.org/2759253002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_method_view_controller_browsertest.cc:105:
ClickOnDialogViewAndWait(list_view->child_at(1));
On 2017/03/21 at 19:27:36, Mathieu Perreault wrote:
> Do we not expect it to go back to the payment method screen? In a way, you're
"confirming" your choice...

Good question, we can ask Bruno!

[anthonyvd, 2017-03-21 20:29:17]

The CQ bit was checked by anthonyvd@chromium.org

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

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-21 21:29:05]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490128157438190,
"parent_rev": "cb791d2124ed2ea2a88e3bdbef1c5cb66dd3e86a", "commit_rev":
"0116ce334c021757db311c867d96bb56faeda1b7"}

[commit-bot: I haz the power, 2017-03-21 21:29:54]

Description was changed from

==========
[Web Payments] Implement item selection in lists.

This change implements item selection in all lists in PaymentRequest
dialog sheets. It also implements updating the Payment Sheet after a
selection changes.

BUG=703302
TEST=unit_tests,browsertests
==========

to

==========
[Web Payments] Implement item selection in lists.

This change implements item selection in all lists in PaymentRequest
dialog sheets. It also implements updating the Payment Sheet after a
selection changes.

BUG=703302
TEST=unit_tests,browsertests

Review-Url: https://codereview.chromium.org/2759253002
Cr-Commit-Position: refs/heads/master@{#458553}
Committed:
https://chromium.googlesource.com/chromium/src/+/0116ce334c021757db311c867d96...
==========

[commit-bot: I haz the power, 2017-03-21 21:29:55]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/0116ce334c021757db311c867d96...