[Web Payments] Add the shipping method section to the payment sheet

[Web Payments] Add the shipping method section to the payment sheet

This change adds a shipping method section to the initial Payment
Request dialog view. It's empty if no shipping method is selected, otherwise
it displays the last entry marked selected in the Shipping Options array
sent by the website.

BUG=700019
Review-Url: https://codereview.chromium.org/2744633002
Cr-Commit-Position: refs/heads/master@{#455860}
Committed: https://chromium.googlesource.com/chromium/src/+/8de8fe9639e0bf1e5c4dcc32cfb076e7d6eeca94

[anthonyvd, 2017-03-09 15:57:38]

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

[anthonyvd, 2017-03-09 15:57:39]

Hey Math, can you PTAL?

In particular, I'd like you to double check that the way this change stores the
selected shipping method (as a pointer to the PaymentShippingOption object)
makes sense.

Thanks!

[Mathieu, 2017-03-09 19:07:03]

Thanks lookin' gewd

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

https://codereview.chromium.org/2744633002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:516: if
(request()->selected_shipping_option()) {
pull this out first since it's being reused 3 times?

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

https://codereview.chromium.org/2744633002/diff/1/components/payments/content...
components/payments/content/payment_request.cc:403: selected_shipping_option_ =
selected_shipping_option_it->get();
I think it's fine to keep the pointer because |details_| is owned by this
PaymentRequest and will outlive it. Maybe you can write that comment in the .h

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

https://codereview.chromium.org/2744633002/diff/1/components/payments/content...
components/payments/content/payment_request.h:187: void
UpdateSelectedShippingOptionFromDetails();
Let's add a comment

https://codereview.chromium.org/2744633002/diff/1/components/payments/content...
components/payments/content/payment_request.h:217:
payments::mojom::PaymentShippingOption* selected_shipping_option_;
Should we initialize this (and maybe other pointers?) to nullptr in the
constructor?

https://codereview.chromium.org/2744633002/diff/1/components/payments_strings...
File components/payments_strings.grdp (right):

https://codereview.chromium.org/2744633002/diff/1/components/payments_strings...
components/payments_strings.grdp:189: Shipping method
Can we reuse strings? I wouldn't like to add a string that only gets used on
desktop. I think Android has one of (1) Shipping Option (2) Delivery Option or
(3) Pickup option? We can have a todo to implement this and have Shipping Option
for now...

[anthonyvd, 2017-03-09 19:43:55]

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

https://codereview.chromium.org/2744633002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:516: if
(request()->selected_shipping_option()) {
On 2017/03/09 at 19:07:03, Mathieu Perreault wrote:
> pull this out first since it's being reused 3 times?

Done.

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

https://codereview.chromium.org/2744633002/diff/1/components/payments/content...
components/payments/content/payment_request.cc:403: selected_shipping_option_ =
selected_shipping_option_it->get();
On 2017/03/09 at 19:07:03, Mathieu Perreault wrote:
> I think it's fine to keep the pointer because |details_| is owned by this
PaymentRequest and will outlive it. Maybe you can write that comment in the .h

Done.

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

https://codereview.chromium.org/2744633002/diff/1/components/payments/content...
components/payments/content/payment_request.h:187: void
UpdateSelectedShippingOptionFromDetails();
On 2017/03/09 at 19:07:03, Mathieu Perreault wrote:
> Let's add a comment

Done.

https://codereview.chromium.org/2744633002/diff/1/components/payments/content...
components/payments/content/payment_request.h:217:
payments::mojom::PaymentShippingOption* selected_shipping_option_;
On 2017/03/09 at 19:07:03, Mathieu Perreault wrote:
> Should we initialize this (and maybe other pointers?) to nullptr in the
constructor?

Done.

https://codereview.chromium.org/2744633002/diff/1/components/payments_strings...
File components/payments_strings.grdp (right):

https://codereview.chromium.org/2744633002/diff/1/components/payments_strings...
components/payments_strings.grdp:189: Shipping method
On 2017/03/09 at 19:07:03, Mathieu Perreault wrote:
> Can we reuse strings? I wouldn't like to add a string that only gets used on
desktop. I think Android has one of (1) Shipping Option (2) Delivery Option or
(3) Pickup option? We can have a todo to implement this and have Shipping Option
for now...

Changed this to use the Android "Shipping Option" and added a TODO.

[Mathieu, 2017-03-09 19:48:43]

lgtm with rename

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

https://codereview.chromium.org/2744633002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.h:49:
std::unique_ptr<views::View> CreateShippingMethodContent();
Sorry I wasn't clear when we discussed this. I was hoping we would rename
ShippingMethod to ShippingOption everywhere it's used, including the enums. I
don't think "shipping method" is a concept that we want to introduce only here
on desktop.

[anthonyvd, 2017-03-09 19:56:58]

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

[commit-bot: I haz the power, 2017-03-09 19:57:34]

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

[anthonyvd, 2017-03-09 20:15:44]

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

https://codereview.chromium.org/2744633002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.h:49:
std::unique_ptr<views::View> CreateShippingMethodContent();
On 2017/03/09 at 19:48:42, Mathieu Perreault wrote:
> Sorry I wasn't clear when we discussed this. I was hoping we would rename
ShippingMethod to ShippingOption everywhere it's used, including the enums. I
don't think "shipping method" is a concept that we want to introduce only here
on desktop.

Done everywhere

[commit-bot: I haz the power, 2017-03-09 20:37:31]

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

[commit-bot: I haz the power, 2017-03-09 20:37:32]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2017-03-09 20:44:23]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2017-03-09 20:44:23]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org
Link to the patchset: https://codereview.chromium.org/2744633002/#ps60001
(title: "Rename shipping method to shipping option.")

[commit-bot: I haz the power, 2017-03-09 20:44:52]

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-09 20:55:12]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489092263400320,
"parent_rev": "84f245311f5d8c0de90215222478f0c462591cb9", "commit_rev":
"8de8fe9639e0bf1e5c4dcc32cfb076e7d6eeca94"}

[commit-bot: I haz the power, 2017-03-09 20:55:52]

Description was changed from

==========
[Web Payments] Add the shipping method section to the payment sheet

This change adds a shipping method section to the initial Payment
Request dialog view. It's empty if no shipping method is selected, otherwise
it displays the last entry marked selected in the Shipping Options array
sent by the website.

BUG=700019
==========

to

==========
[Web Payments] Add the shipping method section to the payment sheet

This change adds a shipping method section to the initial Payment
Request dialog view. It's empty if no shipping method is selected, otherwise
it displays the last entry marked selected in the Shipping Options array
sent by the website.

BUG=700019

Review-Url: https://codereview.chromium.org/2744633002
Cr-Commit-Position: refs/heads/master@{#455860}
Committed:
https://chromium.googlesource.com/chromium/src/+/8de8fe9639e0bf1e5c4dcc32cfb0...
==========

[commit-bot: I haz the power, 2017-03-09 20:55:54]

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