PaymentRequest: Remove id attribute from PaymentItem.

PaymentRequest: Remove id attribute from PaymentItem.

The id attribute was removed from the spec[1].

[1] https://github.com/w3c/browser-payment-api/commit/c536e57530c324d27c71ae0d564395c364e2cc24

BUG=none

Committed: https://crrev.com/60054bceeb9300e1f9cfdfccb95ffb04e59d28cd
Cr-Commit-Position: refs/heads/master@{#395510}

[zino, 2016-05-19 16:52:27]

Description was changed from

==========
PaymentRequest: Remove id attribute from PaymentItem.

The id attribute was removed from the spec[1].

[1]
https://github.com/w3c/browser-payment-api/commit/c536e57530c324d27c71ae0d564...

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

to

==========
PaymentRequest: Remove id attribute from PaymentItem.

The id attribute was removed from the spec[1].

[1]
https://github.com/w3c/browser-payment-api/commit/c536e57530c324d27c71ae0d564...

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

[zino, 2016-05-19 16:52:27]

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

[zino, 2016-05-19 16:52:54]

PTAL.

[please use gerrit instead, 2016-05-19 17:48:21]

https://codereview.chromium.org/1994913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentDetailsTestHelper.cpp
(left):

https://codereview.chromium.org/1994913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentDetailsTestHelper.cpp:13: //
PaymentItem and ShippingOption have identical structure.
... except for the "id" field, which is present only in ShippingOption.

https://codereview.chromium.org/1994913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentDetailsTestHelper.cpp:34: if
(data == PaymentTestDataId) {
Move this clause into buildShippingOptionForTest(), because ShippingOption still
uses this field.

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

https://codereview.chromium.org/1994913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:107: // Validates
ShippingOption and PaymentItem dictionaries, which happen to have identical
fields.
..except for "id", which is present only in ShippingOption.

https://codereview.chromium.org/1994913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:113: if
(!item.hasId() || item.id().isEmpty()) {
Move this clause into validateShippingOptionIds().

https://codereview.chromium.org/1994913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:167:
validateShippingOptionsOrPaymentItems(details.shippingOptions(),
exceptionState);
Also call validateShippingOptionIds() here.

[please use gerrit instead, 2016-05-19 17:49:50]

https://codereview.chromium.org/1994913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentDetailsTestHelper.cpp
(left):

https://codereview.chromium.org/1994913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentDetailsTestHelper.cpp:50:
PaymentItem buildPaymentItemForTest(PaymentTestDataToChange data,
PaymentTestModificationType modificationType, const String& valueToUse)
DCHECK_NE(data, PaymentTestDataId);

[please use gerrit instead, 2016-05-19 17:49:55]

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

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

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

[commit-bot: I haz the power, 2016-05-19 18:38:25]

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

[commit-bot: I haz the power, 2016-05-19 18:38:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zino, 2016-05-19 19:08:41]

Patchset #4 (id:60001) has been deleted

[zino, 2016-05-19 19:11:41]

Thank you for detailed review.


PTAL.

https://codereview.chromium.org/1994913002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentDetailsTestHelper.cpp
(left):

https://codereview.chromium.org/1994913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentDetailsTestHelper.cpp:13: //
PaymentItem and ShippingOption have identical structure.
On 2016/05/19 17:48:20, Rouslan wrote:
> ... except for the "id" field, which is present only in ShippingOption.

Done.

https://codereview.chromium.org/1994913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentDetailsTestHelper.cpp:34: if
(data == PaymentTestDataId) {
On 2016/05/19 17:48:20, Rouslan wrote:
> Move this clause into buildShippingOptionForTest(), because ShippingOption
still
> uses this field.

Done.

https://codereview.chromium.org/1994913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentDetailsTestHelper.cpp:50:
PaymentItem buildPaymentItemForTest(PaymentTestDataToChange data,
PaymentTestModificationType modificationType, const String& valueToUse)
On 2016/05/19 17:49:50, Rouslan wrote:
> DCHECK_NE(data, PaymentTestDataId);

Done.

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

https://codereview.chromium.org/1994913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:107: // Validates
ShippingOption and PaymentItem dictionaries, which happen to have identical
fields.
On 2016/05/19 17:48:21, Rouslan wrote:
> ..except for "id", which is present only in ShippingOption.

Done.

https://codereview.chromium.org/1994913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:113: if
(!item.hasId() || item.id().isEmpty()) {
On 2016/05/19 17:48:20, Rouslan wrote:
> Move this clause into validateShippingOptionIds().

Done.

https://codereview.chromium.org/1994913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:167:
validateShippingOptionsOrPaymentItems(details.shippingOptions(),
exceptionState);
On 2016/05/19 17:48:21, Rouslan wrote:
> Also call validateShippingOptionIds() here.

Done.

[please use gerrit instead, 2016-05-19 19:19:57]

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

[commit-bot: I haz the power, 2016-05-19 19:21:02]

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

[commit-bot: I haz the power, 2016-05-19 20:29:01]

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

[commit-bot: I haz the power, 2016-05-19 20:29:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

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

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

https://codereview.chromium.org/1994913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:148: if
(!option.hasId() | option.id().isEmpty()) {
| should be ||

[zino, 2016-05-23 01:42:30]

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

https://codereview.chromium.org/1994913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:148: if
(!option.hasId() | option.id().isEmpty()) {
On 2016/05/22 22:55:29, Rouslan wrote:
> | should be ||

OMG! it's terrible mistake..

Done.

[please use gerrit instead, 2016-05-23 02:36:33]

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

[please use gerrit instead, 2016-05-23 02:36:34]

lgtm

dfalcantara@, ptal PaymentRequestImpl.java

palmer@, ptal payment_request.mojom

[gone, 2016-05-23 19:51:02]

PaymentRequestImpl lgtm

[palmer, 2016-05-23 21:23:36]

lgtm

[zino, 2016-05-23 23:57:11]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2016-05-23 23:57:37]

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

[commit-bot: I haz the power, 2016-05-24 01:40:35]

Description was changed from

==========
PaymentRequest: Remove id attribute from PaymentItem.

The id attribute was removed from the spec[1].

[1]
https://github.com/w3c/browser-payment-api/commit/c536e57530c324d27c71ae0d564...

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

to

==========
PaymentRequest: Remove id attribute from PaymentItem.

The id attribute was removed from the spec[1].

[1]
https://github.com/w3c/browser-payment-api/commit/c536e57530c324d27c71ae0d564...

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

[commit-bot: I haz the power, 2016-05-24 01:40:36]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-05-24 01:41:58]

Description was changed from

==========
PaymentRequest: Remove id attribute from PaymentItem.

The id attribute was removed from the spec[1].

[1]
https://github.com/w3c/browser-payment-api/commit/c536e57530c324d27c71ae0d564...

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

to

==========
PaymentRequest: Remove id attribute from PaymentItem.

The id attribute was removed from the spec[1].

[1]
https://github.com/w3c/browser-payment-api/commit/c536e57530c324d27c71ae0d564...

BUG=none

Committed: https://crrev.com/60054bceeb9300e1f9cfdfccb95ffb04e59d28cd
Cr-Commit-Position: refs/heads/master@{#395510}
==========

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

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