[Payments] Add Ship. Addr. & Contact Info in Payment Response on Desktop.

[Payments] Add Ship. Addr. & Contact Info in Payment Response on Desktop.

BUG=705606
Review-Url: https://codereview.chromium.org/2779813003
Cr-Commit-Position: refs/heads/master@{#460422}
Committed: https://chromium.googlesource.com/chromium/src/+/ad86bd0055e9755857af84d5216b3c81ddd23e9e

[sebsg, 2017-03-27 22:20:54]

Description was changed from

==========
DS

BUG=
==========

to

==========
[Payments] Send Shipping Address in Payment Response on Desktop

BUG=705606
==========

[sebsg, 2017-03-27 22:26:59]

Patchset #2 (id:20001) has been deleted

[sebsg, 2017-03-27 22:27:04]

Patchset #1 (id:1) has been deleted

[Mathieu, 2017-03-27 23:42:42]

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

[Mathieu, 2017-03-27 23:42:43]

Looks great, only have some nits and TODO's to explain why the
PaymentResponseHelper class even though it's not really used now.

https://codereview.chromium.org/2779813003/diff/40001/components/payments/con...
File components/payments/content/payment_request_state_unittest.cc (right):

https://codereview.chromium.org/2779813003/diff/40001/components/payments/con...
components/payments/content/payment_request_state_unittest.cc:353: //
"basic-card" is specified so it is returned as the method name.
remove this line

https://codereview.chromium.org/2779813003/diff/40001/components/payments/con...
File components/payments/content/payment_response_helper.cc (right):

https://codereview.chromium.org/2779813003/diff/40001/components/payments/con...
components/payments/content/payment_response_helper.cc:45:
payment_address->phone =
TODO(crbug.com/xxxxxx): Format phone number according to spec.

https://codereview.chromium.org/2779813003/diff/40001/components/payments/con...
File components/payments/content/payment_response_helper.h (right):

https://codereview.chromium.org/2779813003/diff/40001/components/payments/con...
components/payments/content/payment_response_helper.h:9: #include
"components/autofill/core/browser/autofill_profile.h"
I think you can fwd declare this

https://codereview.chromium.org/2779813003/diff/40001/components/payments/con...
components/payments/content/payment_response_helper.h:17:
PaymentResponseHelper();
TODO(sebsg): Accept PaymentInstrument* and handle generating the payment aspect
of the PaymentResponse in this class.

[sebsg, 2017-03-28 16:57:54]

Description was changed from

==========
[Payments] Send Shipping Address in Payment Response on Desktop

BUG=705606
==========

to

==========
[Payments] Add Ship. Addr. & Contact Info in Payment Response on Desktop.

BUG=705606
==========

[sebsg, 2017-03-28 20:31:10]

Patchset #3 (id:80001) has been deleted

[sebsg, 2017-03-28 20:31:15]

Patchset #2 (id:60001) has been deleted

[sebsg, 2017-03-28 20:32:57]

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

[sebsg, 2017-03-28 20:33:06]

Patchset #2 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-03-28 20:33:38]

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

[sebsg, 2017-03-28 20:33:52]

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

[sebsg, 2017-03-28 20:33:53]

Hi guys, could you please take a look?

Thanks!

https://codereview.chromium.org/2779813003/diff/40001/components/payments/con...
File components/payments/content/payment_request_state_unittest.cc (right):

https://codereview.chromium.org/2779813003/diff/40001/components/payments/con...
components/payments/content/payment_request_state_unittest.cc:353: //
"basic-card" is specified so it is returned as the method name.
On 2017/03/27 23:42:42, Mathieu wrote:
> remove this line

Done.

https://codereview.chromium.org/2779813003/diff/40001/components/payments/con...
File components/payments/content/payment_response_helper.cc (right):

https://codereview.chromium.org/2779813003/diff/40001/components/payments/con...
components/payments/content/payment_response_helper.cc:45:
payment_address->phone =
On 2017/03/27 23:42:42, Mathieu wrote:
> TODO(crbug.com/xxxxxx): Format phone number according to spec.

Done.

https://codereview.chromium.org/2779813003/diff/40001/components/payments/con...
File components/payments/content/payment_response_helper.h (right):

https://codereview.chromium.org/2779813003/diff/40001/components/payments/con...
components/payments/content/payment_response_helper.h:9: #include
"components/autofill/core/browser/autofill_profile.h"
On 2017/03/27 23:42:42, Mathieu wrote:
> I think you can fwd declare this

Done.

https://codereview.chromium.org/2779813003/diff/40001/components/payments/con...
components/payments/content/payment_response_helper.h:17:
PaymentResponseHelper();
On 2017/03/27 23:42:42, Mathieu wrote:
> TODO(sebsg): Accept PaymentInstrument* and handle generating the payment
aspect
> of the PaymentResponse in this class.

Done.

[Mathieu, 2017-03-28 21:01:33]

lgtm

https://codereview.chromium.org/2779813003/diff/120001/components/payments/co...
File components/payments/content/payment_request_state.cc (right):

https://codereview.chromium.org/2779813003/diff/120001/components/payments/co...
components/payments/content/payment_request_state.cc:61: // TODO(sebsg): Move
this to the PaymentRepsonseHelper.
*PaymentResponseHelper

https://codereview.chromium.org/2779813003/diff/120001/components/payments/co...
components/payments/content/payment_request_state.cc:78:
DCHECK(selected_shipping_option_);
this line shows up twice

[Mathieu, 2017-03-28 21:01:36]

lgtm

[commit-bot: I haz the power, 2017-03-28 21:14:47]

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

[commit-bot: I haz the power, 2017-03-28 21:14:48]

Dry run: CQ has no permission to schedule in bucket
master.tryserver.chromium.linux

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

lgtm % mathp's comments. I'll trust you guys that there's more than a single
static function coming to PaymentResponseHelper :)

[sebsg, 2017-03-29 15:04:18]

Thanks! Sending to CQ

https://codereview.chromium.org/2779813003/diff/120001/components/payments/co...
File components/payments/content/payment_request_state.cc (right):

https://codereview.chromium.org/2779813003/diff/120001/components/payments/co...
components/payments/content/payment_request_state.cc:61: // TODO(sebsg): Move
this to the PaymentRepsonseHelper.
On 2017/03/28 21:01:32, Mathieu wrote:
> *PaymentResponseHelper

Done.

https://codereview.chromium.org/2779813003/diff/120001/components/payments/co...
components/payments/content/payment_request_state.cc:78:
DCHECK(selected_shipping_option_);
On 2017/03/28 21:01:32, Mathieu wrote:
> this line shows up twice

Done.

[sebsg, 2017-03-29 15:04:21]

The CQ bit was checked by sebsg@chromium.org

[sebsg, 2017-03-29 15:04:21]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2779813003/#ps140001
(title: "Addressed comments")

[commit-bot: I haz the power, 2017-03-29 15:04:47]

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-29 16:39:22]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490799861518250,
"parent_rev": "95e4983f85e5afb97b37df59b5666b2f3d2949a1", "commit_rev":
"ad86bd0055e9755857af84d5216b3c81ddd23e9e"}

[commit-bot: I haz the power, 2017-03-29 16:40:06]

Description was changed from

==========
[Payments] Add Ship. Addr. & Contact Info in Payment Response on Desktop.

BUG=705606
==========

to

==========
[Payments] Add Ship. Addr. & Contact Info in Payment Response on Desktop.

BUG=705606

Review-Url: https://codereview.chromium.org/2779813003
Cr-Commit-Position: refs/heads/master@{#460422}
Committed:
https://chromium.googlesource.com/chromium/src/+/ad86bd0055e9755857af84d5216b...
==========

[commit-bot: I haz the power, 2017-03-29 16:40:07]

Committed patchset #3 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/ad86bd0055e9755857af84d5216b...