[Web Payments] Implement the credit card selection UI functionality.

[Web Payments] Implement the credit card selection UI functionality.

This patch adds the necessary logic to allow the user to use the
Payment Method Selection Screen to select the credit card to send to the
website.

BUG=679835
Review-Url: https://codereview.chromium.org/2711973002
Cr-Commit-Position: refs/heads/master@{#453239}
Committed: https://chromium.googlesource.com/chromium/src/+/4b014c7ebbe5ed0d2abf313d04699caf6c6ed18d

[anthonyvd, 2017-02-23 20:29:06]

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

[anthonyvd, 2017-02-23 20:29:06]

Hi Math, can you please take a look at this patch that enables credit card
selection?

Tommy, this is more of an FYI since you'll be implementing a similar mechanism
in the address selection screen, but all comments are welcome!

Thanks!

[Mathieu, 2017-02-23 21:22:53]

I originally wanted to suggest having the checkmark and the selection logic part
of the base class for item (IsComplete -> virtual, ButtonPressed shared) since
it's shared with other lists, but I'm having trouble suggesting a good way.
Let's keep this in mind for the 2nd user of this list? Especially as we add a
pencil that also jumps to the editor.

lgtm

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

https://codereview.chromium.org/2711973002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_item_list.cc:56: void
PaymentRequestItemList::SelectItem(PaymentRequestItemList::Item* item) {
As a newcomer to this code, I keep having double takes between
PaymentRequestItemList and PaymentRequestItemList::Item, but it's already better
after 10mn :)

https://codereview.chromium.org/2711973002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_item_list.cc:58: // First,
deselect the previously selected item
should we early abort if item->selected()

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

https://codereview.chromium.org/2711973002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_item_list.h:42: void
set_selected(bool selected);
nit: If it has side effects it probably needs to be CamelCased

[anthonyvd, 2017-02-23 21:49:20]

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

[anthonyvd, 2017-02-23 21:49:20]

It's funny you mention having more logic in the base: I started implementing
something in that direction but was having trouble managing some of the
uncertainty about a few things (namely how the editor screen would be created
and shown).

I'll definitely keep that in mind and refactor as this piece of code gets more
consumers.

+rouslan@, can you please do an OWNERs review on this patch? Thanks!

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

https://codereview.chromium.org/2711973002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_item_list.cc:56: void
PaymentRequestItemList::SelectItem(PaymentRequestItemList::Item* item) {
On 2017/02/23 at 21:22:53, Mathieu Perreault wrote:
> As a newcomer to this code, I keep having double takes between
PaymentRequestItemList and PaymentRequestItemList::Item, but it's already better
after 10mn :)

Fair point. There's a bigger, overarching discussing that Tommy and myself think
we need to have about naming (ie: all our code is in payments:: but we still
name all our classes PaymentRequest*) and that should probably be part of it.

https://codereview.chromium.org/2711973002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_item_list.cc:58: // First,
deselect the previously selected item
On 2017/02/23 at 21:22:53, Mathieu Perreault wrote:
> should we early abort if item->selected()

Done. Also realized naming both the parameter and the loop element |item| was a
little confusing, so changed that.

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

https://codereview.chromium.org/2711973002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_item_list.h:42: void
set_selected(bool selected);
On 2017/02/23 at 21:22:53, Mathieu Perreault wrote:
> nit: If it has side effects it probably needs to be CamelCased

Good point, done.

[please use gerrit instead, 2017-02-24 14:56:53]

LGTM % minor nits.

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

https://codereview.chromium.org/2711973002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:46: // Does
not take ownership of |card|, which  should not be null and should
Please update the comment to mention that |list| will own this object.

https://codereview.chromium.org/2711973002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:115: if
(!selected()) {
nit: No {} for single-line if body.

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

https://codereview.chromium.org/2711973002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_item_list.cc:61: // First,
deselect the previously selected item
nit: consider having a selected_item_ pointer in the list to avoid iterating
over items.

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

https://codereview.chromium.org/2711973002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_item_list.h:67: // Adds an item
to this list. |item->list()| should be equal to this object.
Please remove the words "equal to" to avoid confusion with operator==.

[anthonyvd, 2017-02-24 16:36:58]

While addressing Rouslan's comments, I realized it'd be trivial to add
set_selected_credit_card to PaymentRequest and have the selection be reflected
in the model.

Rouslan, could you please take a look at that, as well as the change based on
your comment to cache the selected item pointer in PaymentRequestItemList?

Thanks!

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

https://codereview.chromium.org/2711973002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:46: // Does
not take ownership of |card|, which  should not be null and should
On 2017/02/24 at 14:56:52, rouslan wrote:
> Please update the comment to mention that |list| will own this object.

Ah, missed that one. Done.

https://codereview.chromium.org/2711973002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:115: if
(!selected()) {
On 2017/02/24 at 14:56:52, rouslan wrote:
> nit: No {} for single-line if body.

Done.

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

https://codereview.chromium.org/2711973002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_item_list.cc:61: // First,
deselect the previously selected item
On 2017/02/24 at 14:56:53, rouslan wrote:
> nit: consider having a selected_item_ pointer in the list to avoid iterating
over items.

Done.

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

https://codereview.chromium.org/2711973002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_item_list.h:67: // Adds an item
to this list. |item->list()| should be equal to this object.
On 2017/02/24 at 14:56:53, rouslan wrote:
> Please remove the words "equal to" to avoid confusion with operator==.

Done.

[please use gerrit instead, 2017-02-24 16:40:59]

lgtm % comment

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

https://codereview.chromium.org/2711973002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_item_list.cc:77: }
selected_item_ = nullptr?

[anthonyvd, 2017-02-24 21:48:00]

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

[commit-bot: I haz the power, 2017-02-24 21:48:46]

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

[anthonyvd, 2017-02-24 21:49:42]

mathp@: as discussed offline, here's a patchset with unit and interactive ui
tests. Can you PTAL?
rouslan@: Addressed your latest comment. The latest patch doesn't touch anything
else other than test-related things so feel free to review it or not as you wish
:)

Thanks!

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

https://codereview.chromium.org/2711973002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_item_list.cc:77: }
On 2017/02/24 at 16:40:59, rouslan wrote:
> selected_item_ = nullptr?

Done.

[commit-bot: I haz the power, 2017-02-24 21:52:46]

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

[commit-bot: I haz the power, 2017-02-24 21:52:47]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[anthonyvd, 2017-02-24 21:59:37]

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

[commit-bot: I haz the power, 2017-02-24 22:00:40]

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

[Mathieu, 2017-02-24 22:51:24]

very cool tests lgtm

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

https://codereview.chromium.org/2711973002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_method_view_controller_interactive_uitest.cc:27:
IN_PROC_BROWSER_TEST_F(PaymentMethodViewControllerTest, TestEmptyEditor) {
EmptyList?

Also I would rename Test from the test name

https://codereview.chromium.org/2711973002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_method_view_controller_interactive_uitest.cc:42:
personal_data_manager->AddCreditCard(card);
I think I would move the Add call just before data_loop.Run()

same for others.

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

https://codereview.chromium.org/2711973002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_credit_card_editor_interactive_uitest.cc:262:
autofill::PersonalDataManager* personal_data_manager =
can you make this use the new GetDataManager

[anthonyvd, 2017-02-24 23:23:42]

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

[commit-bot: I haz the power, 2017-02-24 23:24:27]

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

[anthonyvd, 2017-02-24 23:26:28]

Addressed comments. Thanks for the review!

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

https://codereview.chromium.org/2711973002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_method_view_controller_interactive_uitest.cc:27:
IN_PROC_BROWSER_TEST_F(PaymentMethodViewControllerTest, TestEmptyEditor) {
On 2017/02/24 at 22:51:24, Mathieu Perreault wrote:
> EmptyList?
> 
> Also I would rename Test from the test name

Done here and below.

https://codereview.chromium.org/2711973002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_method_view_controller_interactive_uitest.cc:42:
personal_data_manager->AddCreditCard(card);
On 2017/02/24 at 22:51:24, Mathieu Perreault wrote:
> I think I would move the Add call just before data_loop.Run()
> 
> same for others.

Done.

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

https://codereview.chromium.org/2711973002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_credit_card_editor_interactive_uitest.cc:262:
autofill::PersonalDataManager* personal_data_manager =
On 2017/02/24 at 22:51:24, Mathieu Perreault wrote:
> can you make this use the new GetDataManager

Done throughout.

[commit-bot: I haz the power, 2017-02-24 23:27:33]

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

[commit-bot: I haz the power, 2017-02-24 23:27:34]

Dry run: Try jobs failed on following builders:
  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-02-24 23:34:25]

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

[commit-bot: I haz the power, 2017-02-24 23:35:03]

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-24 23:50:30]

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

[commit-bot: I haz the power, 2017-02-24 23:50:31]

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

[anthonyvd, 2017-02-25 00:18:52]

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

[commit-bot: I haz the power, 2017-02-25 00:19:14]

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-25 01:01:04]

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

[commit-bot: I haz the power, 2017-02-25 01:01:05]

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

[anthonyvd, 2017-02-27 16:00:07]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2017-02-27 16:00:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, mathp@chromium.org
Link to the patchset: https://codereview.chromium.org/2711973002/#ps160001
(title: "Fix test flakiness.")

[commit-bot: I haz the power, 2017-02-27 16:00:26]

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-27 17:08:33]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1488211207660070,
"parent_rev": "39484cbb5b2e2089054e2cd642e03bbe817f42af", "commit_rev":
"4b014c7ebbe5ed0d2abf313d04699caf6c6ed18d"}

[commit-bot: I haz the power, 2017-02-27 17:09:16]

Description was changed from

==========
[Web Payments] Implement the credit card selection UI functionality.

This patch adds the necessary logic to allow the user to use the
Payment Method Selection Screen to select the credit card to send to the
website.

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

to

==========
[Web Payments] Implement the credit card selection UI functionality.

This patch adds the necessary logic to allow the user to use the
Payment Method Selection Screen to select the credit card to send to the
website.

BUG=679835

Review-Url: https://codereview.chromium.org/2711973002
Cr-Commit-Position: refs/heads/master@{#453239}
Committed:
https://chromium.googlesource.com/chromium/src/+/4b014c7ebbe5ed0d2abf313d0469...
==========

[commit-bot: I haz the power, 2017-02-27 17:09:18]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/4b014c7ebbe5ed0d2abf313d0469...