Provide shipping in PaymentRequest response

Provide shipping in PaymentRequest result

If the merchant provides non-NULL shipping options, the merchant does
not need shipping address to calculate shipping costs. The UI selects
the default shipping address, but does not send it to the merchant. This
patch sends the shipping address to the merchant when the user clicks
"Pay".

BUG=608222
Committed: https://crrev.com/f63c3bce0928c9534671fb325d45fd1524ac6b37
Cr-Commit-Position: refs/heads/master@{#391133}

[please use gerrit instead, 2016-05-01 22:39:08]

rouslan@chromium.org changed reviewers:
+ dfalcantara@chromium.org, palmer@chromium.org

[please use gerrit instead, 2016-05-01 22:39:09]

dfalcantara@, ptal PaymentRequestImpl.java

mek@, ptal third_party/WebKit/Source/modules/payments/*.

palmer@, ptal payment_request.mojom

[please use gerrit instead, 2016-05-01 22:39:29]

Description was changed from

==========
Provide shipping in PaymentRequest result

If the merchant provides non-NULL shipping options, the merchant does
not need shipping address to calculate shipping costs. The UI selects
the default shipping address, but does not send it to the merchant. This
patch sends the shipping address to the merchant when the user clicks
"Pay".

BUG=608222
==========

to

==========
Provide shipping in PaymentRequest result

If the merchant provides non-NULL shipping options, the merchant does
not need shipping address to calculate shipping costs. The UI selects
the default shipping address, but does not send it to the merchant. This
patch sends the shipping address to the merchant when the user clicks
"Pay".

BUG=608222
==========

[please use gerrit instead, 2016-05-01 22:39:29]

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

[please use gerrit instead, 2016-05-01 22:39:53]

mek@, ptal third_party/WebKit/Source/modules/payments/*.

[gone, 2016-05-02 17:45:37]

android bits lgtm

[Marijn Kruisselbrink, 2016-05-02 19:02:27]

other than some of the helper code in your unit test really needing to be in an
anonymous namespace, this lgtm

https://codereview.chromium.org/1937783002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/payments/PaymentsValidatorsTest.cpp
(right):

https://codereview.chromium.org/1937783002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentsValidatorsTest.cpp:13: struct
TestCase {
Here you really need an anonymous namespace to not run into conflicts with any
other code that happens to have a type named TestCase (presumably other code is
doing the right thing so such conflicts won't happen as long as this is the only
test trying to squat the global namespace like this, but still...)

[please use gerrit instead, 2016-05-02 20:34:45]

https://codereview.chromium.org/1937783002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/payments/PaymentsValidatorsTest.cpp
(right):

https://codereview.chromium.org/1937783002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentsValidatorsTest.cpp:13: struct
TestCase {
On 2016/05/02 19:02:27, Marijn Kruisselbrink wrote:
> Here you really need an anonymous namespace to not run into conflicts with any
> other code that happens to have a type named TestCase (presumably other code
is
> doing the right thing so such conflicts won't happen as long as this is the
only
> test trying to squat the global namespace like this, but still...)

Done.

[palmer, 2016-05-02 21:58:41]

https://codereview.chromium.org/1937783002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/1937783002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:506:

Nit: Don't need a blank line here.

https://codereview.chromium.org/1937783002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:512:
assert selectedShippingAddress instanceof AutofillAddress;
Is crashing the browser process the right thing to do here? I.e. are we sure
this assert will only fire in case of programmer error, not bad data?

[please use gerrit instead, 2016-05-02 22:08:19]

palmer@, ptal patch 3.

https://codereview.chromium.org/1937783002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/1937783002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:506:

On 2016/05/02 21:58:41, palmer wrote:
> Nit: Don't need a blank line here.

Done.

https://codereview.chromium.org/1937783002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:512:
assert selectedShippingAddress instanceof AutofillAddress;
On 2016/05/02 21:58:41, palmer wrote:
> Is crashing the browser process the right thing to do here? I.e. are we sure
> this assert will only fire in case of programmer error, not bad data?

Yes, this can be a programmer error only. The only place where shipping
addresses are instantiated is line 192 in this file:

  addresses.add(new AutofillAddress(profile));

[palmer, 2016-05-02 22:09:15]

Great, thanks. LGTM.

[please use gerrit instead, 2016-05-02 22:12:05]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2016-05-02 22:12:05]

The patchset sent to the CQ was uploaded after l-g-t-m from mek@chromium.org,
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/1937783002/#ps40001
(title: "Address comments")

[commit-bot: I haz the power, 2016-05-02 22:12:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1937783002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1937783002/40001

[commit-bot: I haz the power, 2016-05-03 01:03:00]

Description was changed from

==========
Provide shipping in PaymentRequest result

If the merchant provides non-NULL shipping options, the merchant does
not need shipping address to calculate shipping costs. The UI selects
the default shipping address, but does not send it to the merchant. This
patch sends the shipping address to the merchant when the user clicks
"Pay".

BUG=608222
==========

to

==========
Provide shipping in PaymentRequest result

If the merchant provides non-NULL shipping options, the merchant does
not need shipping address to calculate shipping costs. The UI selects
the default shipping address, but does not send it to the merchant. This
patch sends the shipping address to the merchant when the user clicks
"Pay".

BUG=608222
==========

[commit-bot: I haz the power, 2016-05-03 01:03:01]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-03 01:04:00]

Description was changed from

==========
Provide shipping in PaymentRequest result

If the merchant provides non-NULL shipping options, the merchant does
not need shipping address to calculate shipping costs. The UI selects
the default shipping address, but does not send it to the merchant. This
patch sends the shipping address to the merchant when the user clicks
"Pay".

BUG=608222
==========

to

==========
Provide shipping in PaymentRequest result

If the merchant provides non-NULL shipping options, the merchant does
not need shipping address to calculate shipping costs. The UI selects
the default shipping address, but does not send it to the merchant. This
patch sends the shipping address to the merchant when the user clicks
"Pay".

BUG=608222

Committed: https://crrev.com/f63c3bce0928c9534671fb325d45fd1524ac6b37
Cr-Commit-Position: refs/heads/master@{#391133}
==========

[commit-bot: I haz the power, 2016-05-03 01:04:01]

Patchset 3 (id:??) landed as
https://crrev.com/f63c3bce0928c9534671fb325d45fd1524ac6b37
Cr-Commit-Position: refs/heads/master@{#391133}