PaymentRequest: Provide shippingAddress in PaymentResponse.

PaymentRequest: Provide shippingAddress in PaymentResponse.

The shippingAddress attribute was already added by this issue[1] but current
implementation isn't supporting the attribute yet. So, we should provide the
attribute in PaymentResponse.

[1] https://github.com/w3c/browser-payment-api/pull/139

BUG=617194
Committed: https://crrev.com/11456cf3138b836726da2975e75f44005de2a266
Cr-Commit-Position: refs/heads/master@{#398058}

[zino, 2016-06-05 23:44:13]

Description was changed from

==========
PaymentRequest: Provide shippingAddress in PaymentResponse.

The shippingAddress attribute was already added by this issue[1] but current
implementation isn't supporting the attribute yet. So, we should provide the
attribute in PaymentResponse.

[1] https://github.com/w3c/browser-payment-api/pull/139

BUG=617194
==========

to

==========
PaymentRequest: Provide shippingAddress in PaymentResponse.

The shippingAddress attribute was already added by this issue[1] but current
implementation isn't supporting the attribute yet. So, we should provide the
attribute in PaymentResponse.

[1] https://github.com/w3c/browser-payment-api/pull/139

BUG=617194
==========

[zino, 2016-06-05 23:44:13]

jinho.bang@samsung.com changed reviewers:
+ rouslan@chromium.org

[zino, 2016-06-05 23:44:23]

PTAL

[please use gerrit instead, 2016-06-06 00:33:08]

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

[commit-bot: I haz the power, 2016-06-06 00:33:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2038333002/1

[please use gerrit instead, 2016-06-06 01:09:35]

Excellent work! Thank you for adding tests as well. These are very useful.

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

https://codereview.chromium.org/2038333002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:430: if
(response->shipping_address) {
void PaymentRequest::OnPaymentResponse(mojom::blink::PaymentResponsePtr
response)
{
    DCHECK(m_showResolver);
    DCHECK(!m_completeResolver);

    if (m_options.requestShipping()) {
        if (!response->shipping_address) {
            m_showResolver->reject(DOMException::create(SyntaxError));
            clearResolversAndCloseMojoConnection();
            return;
        }

        String errorMessage;
        if
(!PaymentsValidators::isValidShippingAddress(response->shipping_address,
&errorMessage)) {
            ...

The spec says "This should never occur", but I don't know how much I can trust
the IPC message. (This data is coming from an IPC message.) Therefore, let's
handle it and reject the promise.

https://codereview.chromium.org/2038333002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:438: if
(m_options.requestShipping())
Replace this block with:

m_shippingAddress = new PaymentAddress(response->shipping_address->Clone());

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

https://codereview.chromium.org/2038333002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentResponse.cpp:17: ,
m_shippingAddress(nullptr)
, m_shippingAddress(response->shipping_address ? new
PaymentAddress(std::move(response->shipping_address)) : nullptr)

https://codereview.chromium.org/2038333002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentResponse.cpp:21: if (response
&& response->shipping_address)
remove this block

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

https://codereview.chromium.org/2038333002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp:50:
setSecurityOrigin("https://www.example.com");
You're calling this function only once. So it's simpler to copy the contents of
the function directly into here.

https://codereview.chromium.org/2038333002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp:84: explicit
PaymentResponseFunction(ScriptState* scriptState, ScriptValue* outValue)
No need for "explicit" keyword with more than one parameter.

https://codereview.chromium.org/2038333002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp:87: {
DCHECK(m_value);

https://codereview.chromium.org/2038333002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp:103:
ADD_FAILURE() << "Unexpected call to a null ScriptFunction.";
"Unexpected call"

https://codereview.chromium.org/2038333002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp:108:
UnreachableFunction(ScriptState* scriptState)
Need "explicit" keyword here to avoid implicit conversion from ScriptState to
UnreachableFunction.

https://codereview.chromium.org/2038333002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp:164:
TEST_F(PaymentResponseTest, ShippingAddressAttributeWithRequestShippingTrue)
Plus one more test: options.setRequestShipping(true) and the
mojom::blink::PaymentResponse does not have an address. This should reject the
promise.

https://codereview.chromium.org/2038333002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp:201:
request->show(getScriptState()).then(PaymentResponseFunction::create(getScriptState(),
&outValue), UnreachableFunction::create(getScriptState()));
My suggestion in PaymentRequest.cpp is to reject the show() promise if
options.setRequestShipping(false) and mojom::blink::PaymentResponse has a
shipping address. Therefore, let's ensure that this promise is rejected:

request->show(getScriptState()).then(MockFunction::expectNoCall(getScriptState()),
MockFunction::expectCall(getScriptState()));

Example:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen...

[commit-bot: I haz the power, 2016-06-06 01:56:06]

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

[commit-bot: I haz the power, 2016-06-06 01:56:07]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[zino, 2016-06-06 10:02:14]

Thank you for review.

I addressed your comments.

PTAL.

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

https://codereview.chromium.org/2038333002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:430: if
(response->shipping_address) {
On 2016/06/06 01:09:34, Rouslan (ツ) wrote:
> void PaymentRequest::OnPaymentResponse(mojom::blink::PaymentResponsePtr
> response)
> {
>     DCHECK(m_showResolver);
>     DCHECK(!m_completeResolver);
> 
>     if (m_options.requestShipping()) {
>         if (!response->shipping_address) {
>             m_showResolver->reject(DOMException::create(SyntaxError));
>             clearResolversAndCloseMojoConnection();
>             return;
>         }
> 
>         String errorMessage;
>         if
> (!PaymentsValidators::isValidShippingAddress(response->shipping_address,
> &errorMessage)) {
>             ...
> 
> The spec says "This should never occur", but I don't know how much I can trust
> the IPC message. (This data is coming from an IPC message.) Therefore, let's
> handle it and reject the promise.

Done.

https://codereview.chromium.org/2038333002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:438: if
(m_options.requestShipping())
On 2016/06/06 01:09:34, Rouslan (ツ) wrote:
> Replace this block with:
> 
> m_shippingAddress = new PaymentAddress(response->shipping_address->Clone());

Done.

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

https://codereview.chromium.org/2038333002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentResponse.cpp:17: ,
m_shippingAddress(nullptr)
On 2016/06/06 01:09:34, Rouslan (ツ) wrote:
> , m_shippingAddress(response->shipping_address ? new
> PaymentAddress(std::move(response->shipping_address)) : nullptr)

Done.

https://codereview.chromium.org/2038333002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentResponse.cpp:21: if (response
&& response->shipping_address)
On 2016/06/06 01:09:34, Rouslan (ツ) wrote:
> remove this block

Done.

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

https://codereview.chromium.org/2038333002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp:50:
setSecurityOrigin("https://www.example.com");
On 2016/06/06 01:09:35, Rouslan (ツ) wrote:
> You're calling this function only once. So it's simpler to copy the contents
of
> the function directly into here.

Done.

https://codereview.chromium.org/2038333002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp:84: explicit
PaymentResponseFunction(ScriptState* scriptState, ScriptValue* outValue)
On 2016/06/06 01:09:35, Rouslan (ツ) wrote:
> No need for "explicit" keyword with more than one parameter.

Done.

Sorry, this code was created based on MockFunction in PaymentRequest. (Copy &
paste, and then modify)
So.. I had to remove the keyword but I forgot it. :P

https://codereview.chromium.org/2038333002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp:87: {
On 2016/06/06 01:09:34, Rouslan (ツ) wrote:
> DCHECK(m_value);

Done.

https://codereview.chromium.org/2038333002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp:103:
ADD_FAILURE() << "Unexpected call to a null ScriptFunction.";
On 2016/06/06 01:09:35, Rouslan (ツ) wrote:
> "Unexpected call"

Done.

https://codereview.chromium.org/2038333002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp:108:
UnreachableFunction(ScriptState* scriptState)
On 2016/06/06 01:09:34, Rouslan (ツ) wrote:
> Need "explicit" keyword here to avoid implicit conversion from ScriptState to
> UnreachableFunction.

Done.

https://codereview.chromium.org/2038333002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp:164:
TEST_F(PaymentResponseTest, ShippingAddressAttributeWithRequestShippingTrue)
On 2016/06/06 01:09:35, Rouslan (ツ) wrote:
> Plus one more test: options.setRequestShipping(true) and the
> mojom::blink::PaymentResponse does not have an address. This should reject the
> promise.

Done.

https://codereview.chromium.org/2038333002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp:201:
request->show(getScriptState()).then(PaymentResponseFunction::create(getScriptState(),
&outValue), UnreachableFunction::create(getScriptState()));
On 2016/06/06 01:09:35, Rouslan (ツ) wrote:
> My suggestion in PaymentRequest.cpp is to reject the show() promise if
> options.setRequestShipping(false) and mojom::blink::PaymentResponse has a
> shipping address. Therefore, let's ensure that this promise is rejected:
> 
>
request->show(getScriptState()).then(MockFunction::expectNoCall(getScriptState()),
> MockFunction::expectCall(getScriptState()));
> 
> Example:
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen...

Done.

https://codereview.chromium.org/2038333002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp (right):

https://codereview.chromium.org/2038333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:235:
TEST_F(PaymentRequestTest,
RejectShowPromiseWithRequestShippingTrueAndEmptyShippingAddressInResponse)
option.requestShipping == true && shipping_address_from_ipc == null
-> reject

https://codereview.chromium.org/2038333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:249:
TEST_F(PaymentRequestTest,
RejectShowPromiseWithRequestShippingTrueAndInvalidShippingAddressInResponse)
option.requestShipping == true && shipping_address_from_ipc != null &&
!Valid(shipping_address_from_ipc)
-> reject

https://codereview.chromium.org/2038333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:264:
TEST_F(PaymentRequestTest,
RejectShowPromiseWithRequestShippingFalseAndShippingAddressExistsInResponse)
option.requestShipping == false && shipping_address_from_ipc != null
-> reject

https://codereview.chromium.org/2038333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:281:
TEST_F(PaymentRequestTest,
ResolveShowPromiseWithRequestShippingTrueAndValidShippingAddressInResponse)
option.requestShipping == true && shipping_address_from_ipc != null &&
Valid(shipping_address_from_ipc)
-> normal behavior

https://codereview.chromium.org/2038333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:305:
TEST_F(PaymentRequestTest,
ResolveShowPromiseWithRequestShippingFalseAndEmptyShippingAddressInResponse)
option.requestShipping == false && shipping_address_from_ipc == null
-> normal behavior but resolve shipping address as null.

[please use gerrit instead, 2016-06-06 16:03:33]

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

[commit-bot: I haz the power, 2016-06-06 16:03:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2038333002/20001

[please use gerrit instead, 2016-06-06 16:17:27]

lgtm

[commit-bot: I haz the power, 2016-06-06 17:41:06]

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

[commit-bot: I haz the power, 2016-06-06 17:41:07]

Dry run: This issue passed the CQ dry run.

[please use gerrit instead, 2016-06-06 17:43:57]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2016-06-06 17:44:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2038333002/20001

[commit-bot: I haz the power, 2016-06-06 17:50:05]

Description was changed from

==========
PaymentRequest: Provide shippingAddress in PaymentResponse.

The shippingAddress attribute was already added by this issue[1] but current
implementation isn't supporting the attribute yet. So, we should provide the
attribute in PaymentResponse.

[1] https://github.com/w3c/browser-payment-api/pull/139

BUG=617194
==========

to

==========
PaymentRequest: Provide shippingAddress in PaymentResponse.

The shippingAddress attribute was already added by this issue[1] but current
implementation isn't supporting the attribute yet. So, we should provide the
attribute in PaymentResponse.

[1] https://github.com/w3c/browser-payment-api/pull/139

BUG=617194
==========

[commit-bot: I haz the power, 2016-06-06 17:50:06]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-06 17:51:51]

Description was changed from

==========
PaymentRequest: Provide shippingAddress in PaymentResponse.

The shippingAddress attribute was already added by this issue[1] but current
implementation isn't supporting the attribute yet. So, we should provide the
attribute in PaymentResponse.

[1] https://github.com/w3c/browser-payment-api/pull/139

BUG=617194
==========

to

==========
PaymentRequest: Provide shippingAddress in PaymentResponse.

The shippingAddress attribute was already added by this issue[1] but current
implementation isn't supporting the attribute yet. So, we should provide the
attribute in PaymentResponse.

[1] https://github.com/w3c/browser-payment-api/pull/139

BUG=617194

Committed: https://crrev.com/11456cf3138b836726da2975e75f44005de2a266
Cr-Commit-Position: refs/heads/master@{#398058}
==========

[commit-bot: I haz the power, 2016-06-06 17:51:52]

Patchset 2 (id:??) landed as
https://crrev.com/11456cf3138b836726da2975e75f44005de2a266
Cr-Commit-Position: refs/heads/master@{#398058}