PaymentRequest: Ignore shipping options if there are duplicated IDs.

PaymentRequest: Ignore shipping options if there are duplicated IDs.

Related spec change:
  https://github.com/w3c/browser-payment-api/pull/249

BUG=587995
Committed: https://crrev.com/d9cf4f407c7bc31ade2d59490b9db23781191ec9
Cr-Commit-Position: refs/heads/master@{#425116}

[zino, 2016-10-08 18:27:45]

Description was changed from

==========
Hello

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

to

==========
Hello

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

[zino, 2016-10-09 13:40:43]

Description was changed from

==========
Hello

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

to

==========
PaymentRequest: Ignore shipping options if there are duplicated IDs.

Related spec change:
  https://github.com/w3c/browser-payment-api/pull/249

BUG=none
==========

[zino, 2016-10-10 17:50:59]

Description was changed from

==========
PaymentRequest: Ignore shipping options if there are duplicated IDs.

Related spec change:
  https://github.com/w3c/browser-payment-api/pull/249

BUG=none
==========

to

==========
PaymentRequest: Ignore shipping options if there are duplicated IDs.

Related spec change:
  https://github.com/w3c/browser-payment-api/pull/249

BUG=587995
==========

[zino, 2016-10-10 17:50:59]

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

[zino, 2016-10-10 17:53:44]

Please review when you have spare time.

[please use gerrit instead, 2016-10-12 01:28:02]

Thank you for the patch!

https://codereview.chromium.org/2406713002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/payments/payment-request-interface.html
(left):

https://codereview.chromium.org/2406713002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/payments/payment-request-interface.html:270: new
PaymentRequest([{'supportedMethods': ['foo']}], {'total': buildItem(),
'displayItems': [buildItem()], 'shippingOptions': shippingOptions},
{'requestShipping': true})
Please add a test 'No shipping option selected for duplicate shipping option
identifiers'. Provide duplicate shipping options IDs, one of which is
'selected'. Then check that request.shippingOption is null.

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

https://codereview.chromium.org/2406713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:226: void
validateAndFixupShippingOptions(PaymentDetails& details,
nit: Pass in only "HeapVector<PaymentShippingOption>& options", so it's clear
that this function acts only on the shipping options. You can reset it via
"options = HeapVector<PaymentShippingOption>();"

https://codereview.chromium.org/2406713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:596: if
(details.hasError() && !details.error().isEmpty()) {
To ensure correctness, stop using "details" immediately after you make a
writable copy. So please use "fixedDetails" on this line.

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

https://codereview.chromium.org/2406713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:631:
"\"value\": \"5.00\"}, \"selected\": true}]}";
nit: newline

https://codereview.chromium.org/2406713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:635:
scope.getExceptionState())));
nit: newline

[zino, 2016-10-12 18:05:24]

Thank you for review!

https://codereview.chromium.org/2406713002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/payments/payment-request-interface.html
(left):

https://codereview.chromium.org/2406713002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/payments/payment-request-interface.html:270: new
PaymentRequest([{'supportedMethods': ['foo']}], {'total': buildItem(),
'displayItems': [buildItem()], 'shippingOptions': shippingOptions},
{'requestShipping': true})
On 2016/10/12 01:28:02, rouslan (slow 10.10-10.17) wrote:
> Please add a test 'No shipping option selected for duplicate shipping option
> identifiers'. Provide duplicate shipping options IDs, one of which is
> 'selected'. Then check that request.shippingOption is null.

Done.

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

https://codereview.chromium.org/2406713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:226: void
validateAndFixupShippingOptions(PaymentDetails& details,
On 2016/10/12 01:28:02, rouslan (slow 10.10-10.17) wrote:
> nit: Pass in only "HeapVector<PaymentShippingOption>& options", so it's clear
> that this function acts only on the shipping options. You can reset it via
> "options = HeapVector<PaymentShippingOption>();"

Done.

https://codereview.chromium.org/2406713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:596: if
(details.hasError() && !details.error().isEmpty()) {
On 2016/10/12 01:28:02, rouslan (slow 10.10-10.17) wrote:
> To ensure correctness, stop using "details" immediately after you make a
> writable copy. So please use "fixedDetails" on this line.

Done.

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

https://codereview.chromium.org/2406713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:631:
"\"value\": \"5.00\"}, \"selected\": true}]}";
On 2016/10/12 01:28:02, rouslan (slow 10.10-10.17) wrote:
> nit: newline

Done.

https://codereview.chromium.org/2406713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:635:
scope.getExceptionState())));
On 2016/10/12 01:28:02, rouslan (slow 10.10-10.17) wrote:
> nit: newline

Done.

[please use gerrit instead, 2016-10-12 23:19:11]

LGTM after the comment is addressed.

https://codereview.chromium.org/2406713002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right):

https://codereview.chromium.org/2406713002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:316:
HeapVector<PaymentShippingOption> fixedShippingOptions =
Don't break this line. There's no character limit in Blink, technically. Also,
this line break adds no value.

[zino, 2016-10-13 01:46:25]

Thank you for review.

https://codereview.chromium.org/2406713002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right):

https://codereview.chromium.org/2406713002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:316:
HeapVector<PaymentShippingOption> fixedShippingOptions =
On 2016/10/12 23:19:10, rouslan (slow 10.10-10.17) wrote:
> Don't break this line. There's no character limit in Blink, technically. Also,
> this line break adds no value.

Good point.

But there *was* no character limit in Blink.
Recently, introduced a new presubmit rule to restrict character limit like
Chromium. 
I don't like the new style but it can cause presubmit warning if there is no
line-break.

[please use gerrit instead, 2016-10-13 17:38:44]

On 2016/10/13 01:46:25, zino wrote:
> But there *was* no character limit in Blink.
> Recently, introduced a new presubmit rule to restrict character limit like
> Chromium. 
> I don't like the new style but it can cause presubmit warning if there is no
> line-break.

Ah, thank you for letting me know. I missed that e-mail when I was on vacation
:-D. Sorry!

[please use gerrit instead, 2016-10-13 17:39:21]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2016-10-13 17:39:56]

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, 2016-10-13 19:05:00]

Description was changed from

==========
PaymentRequest: Ignore shipping options if there are duplicated IDs.

Related spec change:
  https://github.com/w3c/browser-payment-api/pull/249

BUG=587995
==========

to

==========
PaymentRequest: Ignore shipping options if there are duplicated IDs.

Related spec change:
  https://github.com/w3c/browser-payment-api/pull/249

BUG=587995
==========

[commit-bot: I haz the power, 2016-10-13 19:05:01]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-13 19:08:58]

Description was changed from

==========
PaymentRequest: Ignore shipping options if there are duplicated IDs.

Related spec change:
  https://github.com/w3c/browser-payment-api/pull/249

BUG=587995
==========

to

==========
PaymentRequest: Ignore shipping options if there are duplicated IDs.

Related spec change:
  https://github.com/w3c/browser-payment-api/pull/249

BUG=587995

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

[commit-bot: I haz the power, 2016-10-13 19:08:59]

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