PaymentRequest: Introduce PaymentDetailsInit and PaymentDetailsUpdate.

PaymentRequest: Introduce PaymentDetailsInit and PaymentDetailsUpdate.

The current implementation uses PaymentDetails structure when creating
PaymentRequest object and calling updateWith(). But some parameters like error
are only allowed in the updateWith(). If developer tries to use error parameter
when creating PaymentRequest, then throws exception. So, to solve the problem
in the latest spec[1][2], common parts are combined and each different parts
are separated into PaymentDetailsInit and PaymentDetailsUpdate.

This change only affects input dictionaries and doesn't break existing behavior.
Therefore, we don't need a intent to implement and ship.

[1] https://w3c.github.io/browser-payment-api/#paymentdetailsinit-dictionary
[2] https://w3c.github.io/browser-payment-api/#paymentdetailsupdate-dictionary

BUG=704166
Review-Url: https://codereview.chromium.org/2748093003
Cr-Commit-Position: refs/heads/master@{#460053}
Committed: https://chromium.googlesource.com/chromium/src/+/aec033c55fbc09dade60bc543373deb86c517b1b

[zino, 2017-03-14 15:47:01]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[zino, 2017-03-14 15:47:09]

Description was changed from

==========
PD

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

to

==========
PD

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

[commit-bot: I haz the power, 2017-03-14 15:47:55]

Dry run: 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-14 15:50:19]

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

[commit-bot: I haz the power, 2017-03-14 15:50:20]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[zino, 2017-03-14 16:05:35]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 16:06:22]

Dry run: 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-14 16:09:21]

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

[commit-bot: I haz the power, 2017-03-14 16:09:22]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[zino, 2017-03-14 16:12:45]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 16:13:25]

Dry run: 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-14 16:17:10]

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

[commit-bot: I haz the power, 2017-03-14 16:17:12]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[zino, 2017-03-14 16:45:06]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 16:45:33]

Dry run: 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-14 16:47:30]

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

[commit-bot: I haz the power, 2017-03-14 16:47:31]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[zino, 2017-03-14 16:55:37]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 16:55:58]

Dry run: 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-14 17:01:59]

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

[commit-bot: I haz the power, 2017-03-14 17:02:00]

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

[zino, 2017-03-19 06:11:14]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-19 06:11:25]

Dry run: 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-19 06:13:16]

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

[commit-bot: I haz the power, 2017-03-19 06:13:16]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[zino, 2017-03-19 06:15:14]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-19 06:15:25]

Dry run: 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-19 07:37:37]

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

[commit-bot: I haz the power, 2017-03-19 07:37:38]

Dry run: This issue passed the CQ dry run.

[zino, 2017-03-19 08:19:21]

Description was changed from

==========
PD

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

to

==========
PaymentRequest: Introduce PaymentDetailsInit and PaymentDetailsUpdate.

The current implementation uses PaymentDetails structure when creating
PaymentRequest object and calling updateWith(). But some parameters like error
are only allowed in the updateWith(). If developer tries to use error parameter
when creating PaymentRequest, then throws exception. So, to solve the problem
in the latest spec[1][2], common parts are combined and each different parts
are separated into PaymentDetailsInit and PaymentDetailsUpdate.

This change only affects input dictionaries and doesn't break existing behavior.
Therefore, we don't need a intent to implement and ship.

[1] https://w3c.github.io/browser-payment-api/#paymentdetailsinit-dictionary
[2] https://w3c.github.io/browser-payment-api/#paymentdetailsupdate-dictionary

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

[zino, 2017-03-19 08:19:21]

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

[zino, 2017-03-19 08:19:45]

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

[zino, 2017-03-19 08:22:12]

PTAL.

Tom Sepez@ for mojom
haraken@ for modules_idl_files.gni
rouslan@ for payments

[Tom Sepez, 2017-03-20 17:19:42]

mojom LGTM

[zino, 2017-03-21 13:05:53]

Owners, PTAL

[please use gerrit instead, 2017-03-21 13:42:21]

Thank you for helping out! :-D

https://codereview.chromium.org/2748093003/diff/60001/components/payments/con...
File components/payments/content/payment_request.mojom (right):

https://codereview.chromium.org/2748093003/diff/60001/components/payments/con...
components/payments/content/payment_request.mojom:186: PaymentItem total;
Should be nullable, because it's optional in PaymentDetailsUpdate.idl. Since
it's nullable, please update payment_request.cc and PaymentRequestImpl.java to
(1) require the total in init() method and (2) not require the total in
updateWIth() method.

https://cs.chromium.org/chromium/src/components/payments/content/payment_requ...
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

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

https://codereview.chromium.org/2748093003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:513:
exceptionState);
Check for error in the exception state, please. If there's an error, return.

https://codereview.chromium.org/2748093003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:520: if
(exceptionState.hadException())
No need for this exception state check, because you're at the end of the
function already.

https://codereview.chromium.org/2748093003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:532:
exceptionState);
Check for error in the exception state, please. If there's an error, return.

https://codereview.chromium.org/2748093003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:533: if
(!input.hasTotal()) {
Total is optional in PaymentDetailsUpdate. If it's present, validate it.
Otherwise, no need to throw an error.

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

https://codereview.chromium.org/2748093003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.h:16: #include
"modules/payments/PaymentDetailsInit.h"
Please forward declare this instead of including it in the header.

https://codereview.chromium.org/2748093003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.h:17: #include
"modules/payments/PaymentDetailsUpdate.h"
Please do not include this here. It's not used.

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

https://codereview.chromium.org/2748093003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp:58: void
buildPaymentDetailsBaseInternal(
No need to use the "Internal" suffix. The "Base" word in the name sufficiently
distinguishes this function from its peers and describes its purpose well.
Please remove the "Internal" suffix.

https://codereview.chromium.org/2748093003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp:59:
PaymentDetailsBase* details,
The output parameter should be the last one.

[please use gerrit instead, 2017-03-22 01:58:24]

Please file tracking bug for this.

[zino, 2017-03-22 16:35:27]

Description was changed from

==========
PaymentRequest: Introduce PaymentDetailsInit and PaymentDetailsUpdate.

The current implementation uses PaymentDetails structure when creating
PaymentRequest object and calling updateWith(). But some parameters like error
are only allowed in the updateWith(). If developer tries to use error parameter
when creating PaymentRequest, then throws exception. So, to solve the problem
in the latest spec[1][2], common parts are combined and each different parts
are separated into PaymentDetailsInit and PaymentDetailsUpdate.

This change only affects input dictionaries and doesn't break existing behavior.
Therefore, we don't need a intent to implement and ship.

[1] https://w3c.github.io/browser-payment-api/#paymentdetailsinit-dictionary
[2] https://w3c.github.io/browser-payment-api/#paymentdetailsupdate-dictionary

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

to

==========
PaymentRequest: Introduce PaymentDetailsInit and PaymentDetailsUpdate.

The current implementation uses PaymentDetails structure when creating
PaymentRequest object and calling updateWith(). But some parameters like error
are only allowed in the updateWith(). If developer tries to use error parameter
when creating PaymentRequest, then throws exception. So, to solve the problem
in the latest spec[1][2], common parts are combined and each different parts
are separated into PaymentDetailsInit and PaymentDetailsUpdate.

This change only affects input dictionaries and doesn't break existing behavior.
Therefore, we don't need a intent to implement and ship.

[1] https://w3c.github.io/browser-payment-api/#paymentdetailsinit-dictionary
[2] https://w3c.github.io/browser-payment-api/#paymentdetailsupdate-dictionary

BUG=704166
==========

[zino, 2017-03-22 16:43:45]

Thank you for review. :)

I filed a bug and updated commit description.

PTAL

https://codereview.chromium.org/2748093003/diff/60001/components/payments/con...
File components/payments/content/payment_request.mojom (right):

https://codereview.chromium.org/2748093003/diff/60001/components/payments/con...
components/payments/content/payment_request.mojom:186: PaymentItem total;
On 2017/03/21 13:42:21, ಠ_ಠ wrote:
> Should be nullable, because it's optional in PaymentDetailsUpdate.idl. Since
> it's nullable, please update payment_request.cc and PaymentRequestImpl.java to
> (1) require the total in init() method and (2) not require the total in
> updateWIth() method.
> 
>
https://cs.chromium.org/chromium/src/components/payments/content/payment_requ...
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

Yes, I agree with you but I think this is non-trivial.

In the current implementation, the updateWith() function doesn’t match with the
payment request spec.
The spec says that only if some attribute of details is present, then the value
corresponding to the attribute is updated.
However, the current implementation will update all values regardless of whether
the attribute is present.

For example, the following code should update only total value on payment sheet
UI.
  e.updateWith({ total: { …. } });

But the current implementation will update all values such as shippingOptions.
So, the shippingOptions is set as empty sequence and then the UI will indicate
that the merchant cannot ship to the current shipping address.

I’ll fix them in follow-up CL.
(and I already filed the bug here: http://crbug.com/704162)
We will have to investigate whether the change breaks existing usage.

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

https://codereview.chromium.org/2748093003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:513:
exceptionState);
On 2017/03/21 13:42:21, ಠ_ಠ wrote:
> Check for error in the exception state, please. If there's an error, return.

Done.

https://codereview.chromium.org/2748093003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:520: if
(exceptionState.hadException())
On 2017/03/21 13:42:21, ಠ_ಠ wrote:
> No need for this exception state check, because you're at the end of the
> function already.

Done.

https://codereview.chromium.org/2748093003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:532:
exceptionState);
On 2017/03/21 13:42:21, ಠ_ಠ wrote:
> Check for error in the exception state, please. If there's an error, return.

Done.

https://codereview.chromium.org/2748093003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:533: if
(!input.hasTotal()) {
On 2017/03/21 13:42:21, ಠ_ಠ wrote:
> Total is optional in PaymentDetailsUpdate. If it's present, validate it.
> Otherwise, no need to throw an error.

Done.

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

https://codereview.chromium.org/2748093003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.h:16: #include
"modules/payments/PaymentDetailsInit.h"
On 2017/03/21 13:42:21, ಠ_ಠ wrote:
> Please forward declare this instead of including it in the header.

Done.

https://codereview.chromium.org/2748093003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.h:17: #include
"modules/payments/PaymentDetailsUpdate.h"
On 2017/03/21 13:42:21, ಠ_ಠ wrote:
> Please do not include this here. It's not used.

Done.

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

https://codereview.chromium.org/2748093003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp:58: void
buildPaymentDetailsBaseInternal(
On 2017/03/21 13:42:21, ಠ_ಠ wrote:
> No need to use the "Internal" suffix. The "Base" word in the name sufficiently
> distinguishes this function from its peers and describes its purpose well.
> Please remove the "Internal" suffix.

Done.

https://codereview.chromium.org/2748093003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp:59:
PaymentDetailsBase* details,
On 2017/03/21 13:42:21, ಠ_ಠ wrote:
> The output parameter should be the last one.

Done.

[please use gerrit instead, 2017-03-22 17:14:01]

https://codereview.chromium.org/2748093003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right):

https://codereview.chromium.org/2748093003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:543: }
Since "total" is optional in updateWith(), but cannot be null in
payment_request.mojom, a merchant website can call updateWith() without the
"total" field and cause an exception in mojo. This will close the mojo pipe,
close the PaymentRequest UI, and reject the PaymentRequest.show() promise---not
something the merchant or the spec intended. Therefore, I insist that the
"total" field should be nullable in payment_request.mojom, please. If you get
stuck, let me know, so I can help out.

https://codereview.chromium.org/2748093003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentRequest.h (right):

https://codereview.chromium.org/2748093003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.h:16: #include
"modules/payments/PaymentDetailsInit.h"
No need to include it, because it's forward declared now.

[zino, 2017-03-26 19:31:24]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-26 19:31:33]

Dry run: 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-26 20:31:39]

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

[commit-bot: I haz the power, 2017-03-26 20:31:40]

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

[zino, 2017-03-27 13:14:15]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 13:14:28]

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

[zino, 2017-03-27 13:15:13]

rouslan@, You're right. I addressed comments.

PTAL :)

https://codereview.chromium.org/2748093003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right):

https://codereview.chromium.org/2748093003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:543: }
On 2017/03/22 17:14:01, ಠ_ಠ slow - w3c wrote:
> Since "total" is optional in updateWith(), but cannot be null in
> payment_request.mojom, a merchant website can call updateWith() without the
> "total" field and cause an exception in mojo. This will close the mojo pipe,
> close the PaymentRequest UI, and reject the PaymentRequest.show()
promise---not
> something the merchant or the spec intended. Therefore, I insist that the
> "total" field should be nullable in payment_request.mojom, please. If you get
> stuck, let me know, so I can help out.

Done.

https://codereview.chromium.org/2748093003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentRequest.h (right):

https://codereview.chromium.org/2748093003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.h:16: #include
"modules/payments/PaymentDetailsInit.h"
On 2017/03/22 17:14:01, ಠ_ಠ slow - w3c wrote:
> No need to include it, because it's forward declared now.

Done.

[please use gerrit instead, 2017-03-27 14:00:06]

Thank you!

https://codereview.chromium.org/2748093003/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2748093003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:421:
if (!parseAndValidateDetailsOrDisconnectFromClient(details)) return;
Please add this block of code here:

if (mRawTotal == null) {
    disconnectFromClientWithDebugMessage("Missing total");
    recordAbortReasonHistogram(
            PaymentRequestMetrics.ABORT_REASON_INVALID_DATA_FROM_RENDERER);
    return;
}

A similar block of code should be added after line 48 in payment_request.cc:

if (!details->total) {
  LOG(ERROR) << "Missing total";
  OnConnectionTerminated();
  return;
}

https://cs.chromium.org/chromium/src/components/payments/content/payment_requ...

https://codereview.chromium.org/2748093003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:742:
if (mRawTotal != null) {
Please remove the lines 740-742. That should make the code simpler without
changing the logic.

[zino, 2017-03-27 14:47:47]

Thank you for review!

PTAL a new patch set. I've just modified according to your input :)

https://codereview.chromium.org/2748093003/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2748093003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:421:
if (!parseAndValidateDetailsOrDisconnectFromClient(details)) return;
On 2017/03/27 14:00:05, ಠ_ಠ slow - w3c wrote:
> Please add this block of code here:
> 
> if (mRawTotal == null) {
>     disconnectFromClientWithDebugMessage("Missing total");
>     recordAbortReasonHistogram(
>             PaymentRequestMetrics.ABORT_REASON_INVALID_DATA_FROM_RENDERER);
>     return;
> }
> 
> A similar block of code should be added after line 48 in payment_request.cc:
> 
> if (!details->total) {
>   LOG(ERROR) << "Missing total";
>   OnConnectionTerminated();
>   return;
> }
> 
>
https://cs.chromium.org/chromium/src/components/payments/content/payment_requ...

Done.

https://codereview.chromium.org/2748093003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:742:
if (mRawTotal != null) {
On 2017/03/27 14:00:05, ಠ_ಠ slow - w3c wrote:
> Please remove the lines 740-742. That should make the code simpler without
> changing the logic.

Done.

[zino, 2017-03-27 14:47:56]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 14:48:10]

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

[please use gerrit instead, 2017-03-27 14:59:39]

LGTM % comment.

https://codereview.chromium.org/2748093003/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2748093003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:742:
if (mRawTotal != null) {
On 2017/03/27 14:47:47, zino wrote:
> On 2017/03/27 14:00:05, ಠ_ಠ slow - w3c wrote:
> > Please remove the lines 740-742. That should make the code simpler without
> > changing the logic.
> 
> Done.

I'm so sorry. I think you original code was actually correctly handling the case
of updateWith() called without "total", but with "displayItems". My suggestion
was wrong. Please bring back the lines 740-742 :-(...

[zino, 2017-03-27 15:04:40]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 15:04:55]

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

[zino, 2017-03-27 15:05:55]

haraken@, PTAL (for modules_idl_files.gni)

Thanks.

https://codereview.chromium.org/2748093003/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2748093003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:742:
if (mRawTotal != null) {
On 2017/03/27 14:59:39, ಠ_ಠ slow - w3c wrote:
> On 2017/03/27 14:47:47, zino wrote:
> > On 2017/03/27 14:00:05, ಠ_ಠ slow - w3c wrote:
> > > Please remove the lines 740-742. That should make the code simpler without
> > > changing the logic.
> > 
> > Done.
> 
> I'm so sorry. I think you original code was actually correctly handling the
case
> of updateWith() called without "total", but with "displayItems". My suggestion
> was wrong. Please bring back the lines 740-742 :-(...

Oh, right :) Thanks!

[haraken, 2017-03-27 16:04:08]

LGTM

(But I think you'll need an approval from an API owner since PaymentRequest is
enabled on Android.)

[zino, 2017-03-27 16:16:17]

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

[zino, 2017-03-27 16:17:13]

On 2017/03/27 16:04:08, haraken wrote:
> LGTM
> 
> (But I think you'll need an approval from an API owner since PaymentRequest is
> enabled on Android.)

Thank you for review!

I think it looks trivial platform changes[1].
This CL adds many new IDL but it's split the existing dictionary into sub
dictionaries.
So, this CL doesn't break existing web and there are no any new APIs (new
interface/methods/attributes).

[1] https://www.chromium.org/blink#trivial-changes

[zino, 2017-03-27 16:20:51]

foolip@,

I think this is a trivial change but the web platform change guide[1] says that
"ask an API OWNER for advice when in doubt". :)
If you are okay, could you please review this CL?

Thanks.

[commit-bot: I haz the power, 2017-03-27 17:01:25]

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

[commit-bot: I haz the power, 2017-03-27 17:01:26]

Dry run: This issue passed the CQ dry run.

[zino, 2017-03-28 02:25:37]

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

[zino, 2017-03-28 02:29:11]

tkent@, PTAL (foolip@ is OOO)

I think this is a trivial platform change but the web platform change guide[1]
says that
"ask an API OWNER for advice when in doubt". :)

This CL adds some new IDLs but it's just spliting the existing dictionary into
sub dictionaries.
So, this CL doesn't break existing web and there are no any new APIs (new
interface/methods/attributes).

[1] https://www.chromium.org/blink#trivial-changes

If you are okay, could you please review this CL?

[tkent, 2017-03-28 03:17:39]

https://codereview.chromium.org/2748093003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentDetailsBase.idl (left):

https://codereview.chromium.org/2748093003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentDetailsBase.idl:12:
[RuntimeEnabled=PaymentDetailsError] DOMString error;
This removes |error| IDL attribute.  Is it safe?

[zino, 2017-03-28 05:35:48]

On 2017/03/28 03:17:39, tkent wrote:
>
https://codereview.chromium.org/2748093003/diff/180001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/payments/PaymentDetailsBase.idl (left):
> 
>
https://codereview.chromium.org/2748093003/diff/180001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/payments/PaymentDetailsBase.idl:12:
> [RuntimeEnabled=PaymentDetailsError] DOMString error;
> This removes |error| IDL attribute.  Is it safe?

Yeah, it's safe because the error attribute could not use in PaymentRequest
constructor according to old spec[1].
[1]
https://github.com/w3c/browser-payment-api/commit/3c3a10057b3fd97ab21cb4f76b0...

[zino, 2017-03-28 05:52:23]

On 2017/03/28 05:35:48, zino wrote:
> On 2017/03/28 03:17:39, tkent wrote:
> >
>
https://codereview.chromium.org/2748093003/diff/180001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/modules/payments/PaymentDetailsBase.idl
(left):
> > 
> >
>
https://codereview.chromium.org/2748093003/diff/180001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/modules/payments/PaymentDetailsBase.idl:12:
> > [RuntimeEnabled=PaymentDetailsError] DOMString error;
> > This removes |error| IDL attribute.  Is it safe?
> 
> Yeah, it's safe because the error attribute could not use in PaymentRequest
> constructor according to old spec[1].
> [1]
>
https://github.com/w3c/browser-payment-api/commit/3c3a10057b3fd97ab21cb4f76b0...

To clarify,
The PaymentDetails could be used in PaymentRequest constructor and
PaymentResponse.updateWith() function.
PaymentDetails.error is only allowed in PaymentResponse.updateWith().

Before this patch, if the PaymentRequest constructor takes |error| attribute,
the PaymentRequest will be failed.
It means that we can not use the |error| attribute in PaymentRequest
constructor.

After this patch, the PaymentRequest takes PaymentDetailsInit instead of
PaymentDetails.
The PaymentDetailsInit doesn't have |error| attribute. So, there is no behavior
changes.

[tkent, 2017-03-28 06:19:03]

Thank you for the explanation.  The web-exposed change LGTM.

[zino, 2017-03-28 09:30:59]

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

[zino, 2017-03-28 09:31:00]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/2748093003/#ps180001
(title: "PaymentRequest: Introduce PaymentDetailsInit and
PaymentDetailsUpdate.")

[commit-bot: I haz the power, 2017-03-28 09:31:20]

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-28 09:36:11]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1490693459658080,
"parent_rev": "08dded86021b3231e8db6ea77163b88c036bba76", "commit_rev":
"aec033c55fbc09dade60bc543373deb86c517b1b"}

[commit-bot: I haz the power, 2017-03-28 09:36:59]

Description was changed from

==========
PaymentRequest: Introduce PaymentDetailsInit and PaymentDetailsUpdate.

The current implementation uses PaymentDetails structure when creating
PaymentRequest object and calling updateWith(). But some parameters like error
are only allowed in the updateWith(). If developer tries to use error parameter
when creating PaymentRequest, then throws exception. So, to solve the problem
in the latest spec[1][2], common parts are combined and each different parts
are separated into PaymentDetailsInit and PaymentDetailsUpdate.

This change only affects input dictionaries and doesn't break existing behavior.
Therefore, we don't need a intent to implement and ship.

[1] https://w3c.github.io/browser-payment-api/#paymentdetailsinit-dictionary
[2] https://w3c.github.io/browser-payment-api/#paymentdetailsupdate-dictionary

BUG=704166
==========

to

==========
PaymentRequest: Introduce PaymentDetailsInit and PaymentDetailsUpdate.

The current implementation uses PaymentDetails structure when creating
PaymentRequest object and calling updateWith(). But some parameters like error
are only allowed in the updateWith(). If developer tries to use error parameter
when creating PaymentRequest, then throws exception. So, to solve the problem
in the latest spec[1][2], common parts are combined and each different parts
are separated into PaymentDetailsInit and PaymentDetailsUpdate.

This change only affects input dictionaries and doesn't break existing behavior.
Therefore, we don't need a intent to implement and ship.

[1] https://w3c.github.io/browser-payment-api/#paymentdetailsinit-dictionary
[2] https://w3c.github.io/browser-payment-api/#paymentdetailsupdate-dictionary

BUG=704166

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

[commit-bot: I haz the power, 2017-03-28 09:37:01]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/aec033c55fbc09dade60bc543373...

[Eric Seckler, 2017-03-28 10:18:16]

On 2017/03/28 09:37:01, commit-bot: I haz the power wrote:
> Committed patchset #10 (id:180001) as
>
https://chromium.googlesource.com/chromium/src/+/aec033c55fbc09dade60bc543373...

This seems to be breaking some builds, e.g.:
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium%2FMac%2F25397%2...
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium%2FAndroid%2F703...

compile error:

[41042/50158] CXX
obj/third_party/WebKit/Source/modules/unit_tests/PaymentRequestDetailsTest.o
FAILED:
obj/third_party/WebKit/Source/modules/unit_tests/PaymentRequestDetailsTest.o
../../third_party/WebKit/Source/modules/payments/PaymentRequestDetailsTest.cpp:13:10:
fatal error: 'modules/payments/PaymentDetails.h' file not found
#include "modules/payments/PaymentDetails.h"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

[kolos1, 2017-03-28 10:33:30]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2779013002/ by kolos@chromium.org.

The reason for reverting is: This CL broke compilation on Android and Mac..