[Web Payments] Add a mechanism to build item lists in the PR dialog.

[Web Payments] Add a mechanism to build item lists in the PR dialog.

This CL adds the PaymentRequestItemList class and uses it in the
payment method selection screen to display the list of available credit
cards. This class is meant to be used in all places in the PR dialog
where there are lists (payment method, shipping address, billing address
selection screens).

BUG=679835
Review-Url: https://codereview.chromium.org/2695653004
Cr-Commit-Position: refs/heads/master@{#452224}
Committed: https://chromium.googlesource.com/chromium/src/+/75bc466a32b5f692de641e036712861c5a2f2390

[anthonyvd, 2017-02-13 19:25:05]

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

[anthonyvd, 2017-02-13 19:25:06]

Hi guys,

Can you please take a look at this patch? It a first pass at adding a way to
easily make PR dialog lists like the payment method/addresses selection screens.

Currently, the list only displays the item and clicking them does nothing.
However, this CL was getting pretty big so I figured I could get the ball
rolling and follow up on the other features.

Cheers and thanks!

[please use gerrit instead, 2017-02-14 00:29:14]

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

https://codereview.chromium.org/2695653004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:47: explicit
PaymentMethodListItem(autofill::CreditCard* card) : card_(card) {}
// Does not take ownership of |card|, which  should not be null and should
outlive this object.

(Is that right?)

https://codereview.chromium.org/2695653004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:51:
std::unique_ptr<views::View> CreateItemView() override {
// payments::PaymentRequestItemList::Item implementation.

https://codereview.chromium.org/2695653004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:110:
layout->AddView(CreateCreditCardIconView(card_).release());
Great to see use of std::uniqiue_ptr and .release(). This makes memory
management more reliable and easy to inspect.

https://codereview.chromium.org/2695653004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:117: 
Remove extra line.

https://codereview.chromium.org/2695653004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:131:
std::vector<autofill::CreditCard*> available_cards = request->credit_cards();
This makes a copy of the vector, which is expensive. Either use const-ref or
call request->credit_cards() directly from the for loop.

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

https://codereview.chromium.org/2695653004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_item_list.h:34: // view.
Excellent comment!

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

https://codereview.chromium.org/2695653004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_views_util.cc:196:
autofill::CreditCard* card) {
Since only card->type() is used, I'd prefer a tighter interface that takes only
the type. (I think "Code Complete" recommends this, too.)

std::unique_ptr<views::View> CreateCreditCardIconView(const std::string&
card_type) {

https://codereview.chromium.org/2695653004/diff/1/components/payments/payment...
File components/payments/payment_request.cc (right):

https://codereview.chromium.org/2695653004/diff/1/components/payments/payment...
components/payments/payment_request.cc:27: selected_contact_profile_(nullptr) {
Need to initialize selected_credit_card_ to nullptr here. Otherwise the pointer
is initialized to point to a random location in memory.

https://codereview.chromium.org/2695653004/diff/1/components/payments/payment...
components/payments/payment_request.cc:127: return
data_manager->GetCreditCardsToSuggest();
These cards are owned by data_manager and can change at any moment due to sync.
Please create a cache of them similar to profile_cache_, such that you have full
control of lifetimes of these objects.

https://codereview.chromium.org/2695653004/diff/1/components/payments/payment...
components/payments/payment_request.cc:168: first_complete_card == cards.end()
?nullptr : *first_complete_card;
need space after "?"

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

https://codereview.chromium.org/2695653004/diff/1/components/payments/payment...
components/payments/payment_request.h:93: const
std::vector<autofill::CreditCard*> credit_cards();
Please add a "&" (ref) to avoid copying the vector.

[anthonyvd, 2017-02-22 20:15:21]

Thanks for the review. Comments addressed.

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

https://codereview.chromium.org/2695653004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:47: explicit
PaymentMethodListItem(autofill::CreditCard* card) : card_(card) {}
On 2017/02/14 at 00:29:14, rouslan wrote:
> // Does not take ownership of |card|, which  should not be null and should
outlive this object.
> 
> (Is that right?)

Yep, done.

https://codereview.chromium.org/2695653004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:51:
std::unique_ptr<views::View> CreateItemView() override {
On 2017/02/14 at 00:29:14, rouslan wrote:
> // payments::PaymentRequestItemList::Item implementation.

Done.

https://codereview.chromium.org/2695653004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:110:
layout->AddView(CreateCreditCardIconView(card_).release());
On 2017/02/14 at 00:29:14, rouslan wrote:
> Great to see use of std::uniqiue_ptr and .release(). This makes memory
management more reliable and easy to inspect.

Yeah I'm glad we agreed on just using unique_ptr everywhere even when it's just
to release() it in AddView. Makes things much clearer.

https://codereview.chromium.org/2695653004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:117: 
On 2017/02/14 at 00:29:14, rouslan wrote:
> Remove extra line.

Done.

https://codereview.chromium.org/2695653004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:131:
std::vector<autofill::CreditCard*> available_cards = request->credit_cards();
On 2017/02/14 at 00:29:14, rouslan wrote:
> This makes a copy of the vector, which is expensive. Either use const-ref or
call request->credit_cards() directly from the for loop.

Good catch, done! (Although I would hope that the compiler be eliding the copy
in practice :) )

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

https://codereview.chromium.org/2695653004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_item_list.h:34: // view.
On 2017/02/14 at 00:29:14, rouslan wrote:
> Excellent comment!

Thanks!

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

https://codereview.chromium.org/2695653004/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_views_util.cc:196:
autofill::CreditCard* card) {
On 2017/02/14 at 00:29:14, rouslan wrote:
> Since only card->type() is used, I'd prefer a tighter interface that takes
only the type. (I think "Code Complete" recommends this, too.)
> 
> std::unique_ptr<views::View> CreateCreditCardIconView(const std::string&
card_type) {

Agreed. Turns out this was implemented while I was out.

https://codereview.chromium.org/2695653004/diff/1/components/payments/payment...
File components/payments/payment_request.cc (right):

https://codereview.chromium.org/2695653004/diff/1/components/payments/payment...
components/payments/payment_request.cc:27: selected_contact_profile_(nullptr) {
On 2017/02/14 at 00:29:14, rouslan wrote:
> Need to initialize selected_credit_card_ to nullptr here. Otherwise the
pointer is initialized to point to a random location in memory.

Done.

https://codereview.chromium.org/2695653004/diff/1/components/payments/payment...
components/payments/payment_request.cc:127: return
data_manager->GetCreditCardsToSuggest();
On 2017/02/14 at 00:29:14, rouslan wrote:
> These cards are owned by data_manager and can change at any moment due to
sync. Please create a cache of them similar to profile_cache_, such that you
have full control of lifetimes of these objects.

Ahh, didn't know that! Done. I imagine we'll need to listen for data manager
updates and update the cache as well.

https://codereview.chromium.org/2695653004/diff/1/components/payments/payment...
components/payments/payment_request.cc:168: first_complete_card == cards.end()
?nullptr : *first_complete_card;
On 2017/02/14 at 00:29:14, rouslan wrote:
> need space after "?"

Done.

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

https://codereview.chromium.org/2695653004/diff/1/components/payments/payment...
components/payments/payment_request.h:93: const
std::vector<autofill::CreditCard*> credit_cards();
On 2017/02/14 at 00:29:14, rouslan wrote:
> Please add a "&" (ref) to avoid copying the vector.

Done.

[please use gerrit instead, 2017-02-22 20:26:28]

nit: After addressing comments and uploading a new patch that is ready for
review, please request a re-review explicitly. For example, I write things like
"Alice, please take a another look at patch 3" or "bob, ptal patch 4." Something
similar will go a long way in making the life of code reviewers (and
consequently your life as well) much easier. Thanks! :-D

LGTM

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

https://codereview.chromium.org/2695653004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.h:55: //
enabled state). See comment on CreatePaymentView for an illustration of the
To ease the job of readers of code, it's a good idea to reduce the amount of
code to read in order to understand what this method does. This code returns a
button with either "pay" or "done" label. That should be sufficient to
understand this particular method. Therefore, there's no need to point to
another method that does overall layoyt.

https://codereview.chromium.org/2695653004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.h:65: //
layout.
In the spirit of making this method understable as a single unit of code, please
provide ASCII wireframe of the structure of the returned view. No need to point
to other methods.

[tmartino, 2017-02-22 20:36:28]

lgtm

Did a pass on structure only. Discussed naming concerns with anthonyvd offline,
agreed to table those for another CL. Otherwise lgtm

[anthonyvd, 2017-02-22 20:55:06]

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

https://codereview.chromium.org/2695653004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.h:55: //
enabled state). See comment on CreatePaymentView for an illustration of the
On 2017/02/22 at 20:26:28, rouslan wrote:
> To ease the job of readers of code, it's a good idea to reduce the amount of
code to read in order to understand what this method does. This code returns a
button with either "pay" or "done" label. That should be sufficient to
understand this particular method. Therefore, there's no need to point to
another method that does overall layoyt.

Done.

https://codereview.chromium.org/2695653004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.h:65: //
layout.
On 2017/02/22 at 20:26:28, rouslan wrote:
> In the spirit of making this method understable as a single unit of code,
please provide ASCII wireframe of the structure of the returned view. No need to
point to other methods.

Done.

[anthonyvd, 2017-02-22 20:55:13]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2017-02-22 20:55:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
tmartino@chromium.org, rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/2695653004/#ps60001
(title: "Fix up some comments.")

[commit-bot: I haz the power, 2017-02-22 20:55:45]

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-22 22:04:47]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487796913798420,
"parent_rev": "02b0b43a38dfc706a751f8983972e56ef6b81ccb", "commit_rev":
"75bc466a32b5f692de641e036712861c5a2f2390"}

[commit-bot: I haz the power, 2017-02-22 22:05:26]

Description was changed from

==========
[Web Payments] Add a mechanism to build item lists in the PR dialog.

This CL adds the PaymentRequestItemList class and uses it in the
payment method selection screen to display the list of available credit
cards. This class is meant to be used in all places in the PR dialog
where there are lists (payment method, shipping address, billing address
selection screens).

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

to

==========
[Web Payments] Add a mechanism to build item lists in the PR dialog.

This CL adds the PaymentRequestItemList class and uses it in the
payment method selection screen to display the list of available credit
cards. This class is meant to be used in all places in the PR dialog
where there are lists (payment method, shipping address, billing address
selection screens).

BUG=679835

Review-Url: https://codereview.chromium.org/2695653004
Cr-Commit-Position: refs/heads/master@{#452224}
Committed:
https://chromium.googlesource.com/chromium/src/+/75bc466a32b5f692de641e036712...
==========

[commit-bot: I haz the power, 2017-02-22 22:05:27]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/75bc466a32b5f692de641e036712...

[Mathieu, 2017-02-22 23:00:32]

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

[Mathieu, 2017-02-22 23:00:33]

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

https://codereview.chromium.org/2695653004/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.cc:100:
layout->AddView(CreateLeadingFooterView().release());
This accidentally removed The leading footer view, can we add it back in?

[Mathieu, 2017-02-23 00:39:07]

More after-the-fact comments!

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

https://codereview.chromium.org/2695653004/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.cc:95:
layout->SetInsets(kFooterVerticalInset, kFooterHorizontalInset,
also I don't think this should be removed, since it sets the padding for the
whole footer and the extra/leading view and the button container don't have to
worry how they are being positioned in the footer.

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

https://codereview.chromium.org/2695653004/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.h:38: virtual
std::unique_ptr<views::View> CreateLeadingFooterView();
Admittedly I should have made this protected, similar to CreatePrimaryButton

https://codereview.chromium.org/2695653004/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_sheet_controller.h:67: virtual
std::unique_ptr<views::View> CreateExtraView();
We should keep one of CreateExtraView or CreateLeadingFooterView

https://codereview.chromium.org/2695653004/diff/60001/components/payments/pay...
File components/payments/payment_request.cc (right):

https://codereview.chromium.org/2695653004/diff/60001/components/payments/pay...
components/payments/payment_request.cc:132: const
std::vector<autofill::CreditCard*>& PaymentRequest::credit_cards() {
this can go in the .h?