[Web Payments] Common Payments validation

[Web Payments] Common Payments validation

Provides Payment Validation JNI interface to Java(Android). The interface
is also available to Views, but Blink still uses existing validation
routines due to type conflicts (which we hope to resolve.)

This is a follow up to 2373103002, which solely moved the Mojo file from
Blink to components/

BUG=659644
Committed: https://crrev.com/be5499627776990fbf5ed4af9538f6aff414eaac
Cr-Commit-Position: refs/heads/master@{#429685}

[Kevin Bailey, 2016-10-07 16:11:57]

Description was changed from

==========
Experiment: Common Mojo Payments validation

Please ignore.

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

to

==========
Common Payments validation

Moves the payment_request.mojom file from Blink to components, and provides JNI
interface to Java(Android). The interface is also available to Views, but Blink
still uses existing validation routines due to type conflicts.

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

[Kevin Bailey, 2016-10-07 16:11:57]

krb@chromium.org changed reviewers:
+ jdonnelly@chromium.org, rouslan@chromium.org

[Kevin Bailey, 2016-10-07 16:22:56]

Here is the Payment validation factorization omnibus CL. It moves the mojom file
to a common location, makes it callable by Java and Views and fixes up Blink.
Unfortunately, Blink doesn't use it yet, due to type conflicts.

If the CL is too big, it could be split into "move mojom file" and separately
"add JNI".

[please use gerrit instead, 2016-10-10 15:20:38]

On 2016/10/07 16:22:56, Kevin Bailey wrote:
> If the CL is too big, it could be split into "move mojom file" and separately
> "add JNI".

I'm going to be slow to review this week, so please do split the CL, if possible
:-)

[Kevin Bailey, 2016-10-26 15:04:25]

Description was changed from

==========
Common Payments validation

Moves the payment_request.mojom file from Blink to components, and provides JNI
interface to Java(Android). The interface is also available to Views, but Blink
still uses existing validation routines due to type conflicts.

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

to

==========
Provides JNI interface to Java(Android). The interface is also available to
Views, but Blink still uses existing validation routines due to type conflicts.

This is a follow up to 2373103002, which solely moved the Mojo file.

BUG=659644
==========

[Kevin Bailey, 2016-10-26 19:38:08]

This CL now only has the validation code migration. Java reaches it through Mojo
serialization and JNI.

Note that the Blink side still uses the old code. We could make it use the new
code but:

1) It would either require manual conversion from Blink Mojo structures to
non-Blink structures, or the serialization trick that I used for the JNI stuff.
Unfortunately, the latter wouldn't work when I tried and the Mojo guys couldn't
tell me why.
2) I wanted to keep the CL small and just hooked up the Java side.

Any of this can be addressed in a later CL.

[please use gerrit instead, 2016-10-27 17:14:26]

Great work! Lots of complexity in here.

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

https://codereview.chromium.org/2373103002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:535:
private boolean parseAndValidateDetailsOrDisconnectFromClient(PaymentDetails
details) {
All of the code in here can be changed to:

if (!PaymentValidator.validatePaymentDetails(details)) {
    disconnectFromClientWithDebugMessage("Invalid details");
    recordAbortReasonHistogram(
            PaymentRequestMetrics.ABORT_REASON_INVALID_DATA_FROM_RENDERER);
    return false;
}

// Initialize totalCurrency, formatter, uiTotal, uiLineItems, mUiShoppingCart,
// mRawTotals, mRawLineItems, and mUiShippingOptions without further validation.

return true;

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

https://codereview.chromium.org/2373103002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentValidator.java:21:
private static native long nativeInit();
nativeInit() does not seem to be useful for this use case.

https://codereview.chromium.org/2373103002/diff/140001/chrome/browser/payment...
File chrome/browser/payments/android/component_jni_registrar.cc (right):

https://codereview.chromium.org/2373103002/diff/140001/chrome/browser/payment...
chrome/browser/payments/android/component_jni_registrar.cc:9: namespace payments
{
Anonymous namespace should provide better encapsulation.

https://codereview.chromium.org/2373103002/diff/140001/chrome/browser/payment...
chrome/browser/payments/android/component_jni_registrar.cc:33: return
payments::RegisterNativesImpl(env);
Where is this payments::RegisterNativesImpl() defined?

https://codereview.chromium.org/2373103002/diff/140001/chrome/browser/payment...
File chrome/browser/payments/android/component_jni_registrar.h (right):

https://codereview.chromium.org/2373103002/diff/140001/chrome/browser/payment...
chrome/browser/payments/android/component_jni_registrar.h:1: // Copyright 2016
The Chromium Authors. All rights reserved.
I think it's more typical to see RegisterPaymentValidator() in a
payments-specific file name, like payment_validator_android.h/cc or
payments_jni_registrar.h. See
https://cs.chromium.org/chromium/src/chrome/browser/autofill/android/personal...
and
https://cs.chromium.org/chromium/src/components/sync/android/sync_jni_registr...
patterns for reference.

In addition, it would be great to keep this file in components/payments/android/
similar to sync_jni_registrar.h for example. Do you think that's feasible?

https://codereview.chromium.org/2373103002/diff/140001/components/payments/pa...
File components/payments/payment_details_validation.cc (right):

https://codereview.chromium.org/2373103002/diff/140001/components/payments/pa...
components/payments/payment_details_validation.cc:132: 
The functions above can be in anonymous namespace for better encapsulation.

https://codereview.chromium.org/2373103002/diff/140001/components/payments/pa...
File components/payments/payment_details_validation.h (right):

https://codereview.chromium.org/2373103002/diff/140001/components/payments/pa...
components/payments/payment_details_validation.h:28: bool
validatePaymentDetails(const blink::mojom::PaymentDetailsPtr& details);
Instead of recreating the ExceptionState concept from Blink, let's use a pointer
to an optional std::string error message and return a boolean that indicates
validity.

bool validatePaymentDetails(
    const blink::mojom::PaymentDetailsPtr& details,
    std::string* optional_error_message);

https://codereview.chromium.org/2373103002/diff/140001/components/payments/pa...
File components/payments/payments_validators.h (right):

https://codereview.chromium.org/2373103002/diff/140001/components/payments/pa...
components/payments/payments_validators.h:27: 
The methods below are not relevant for PaymentDetails validator. Let's not add
them here, or at least not just yet.

https://codereview.chromium.org/2373103002/diff/140001/components/payments/pa...
File components/payments/payments_validators_test.cc (right):

https://codereview.chromium.org/2373103002/diff/140001/components/payments/pa...
components/payments/payments_validators_test.cc:5: #include
"payments_validators.h"
Should be the full path by Chromium conventions.

#include "components/payments/payments_validators.h"

[Kevin Bailey, 2016-10-27 22:17:53]

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

https://codereview.chromium.org/2373103002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:535:
private boolean parseAndValidateDetailsOrDisconnectFromClient(PaymentDetails
details) {
On 2016/10/27 17:14:25, rouslan wrote:
> All of the code in here can be changed to:
> 
> if (!PaymentValidator.validatePaymentDetails(details)) {
>     disconnectFromClientWithDebugMessage("Invalid details");
>     recordAbortReasonHistogram(
>             PaymentRequestMetrics.ABORT_REASON_INVALID_DATA_FROM_RENDERER);
>     return false;
> }
> 
> // Initialize totalCurrency, formatter, uiTotal, uiLineItems, mUiShoppingCart,
> // mRawTotals, mRawLineItems, and mUiShippingOptions without further
validation.
> 
> return true;

Done, but there's a lot of repetition here. What do you think about throwing
(maybe the error message) from each error and have a catch deal with it? Looks
like it would save 24 identical lines.

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

https://codereview.chromium.org/2373103002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentValidator.java:21:
private static native long nativeInit();
On 2016/10/27 17:14:25, rouslan wrote:
> nativeInit() does not seem to be useful for this use case.

Done.

https://codereview.chromium.org/2373103002/diff/140001/chrome/browser/payment...
File chrome/browser/payments/android/component_jni_registrar.cc (right):

https://codereview.chromium.org/2373103002/diff/140001/chrome/browser/payment...
chrome/browser/payments/android/component_jni_registrar.cc:9: namespace payments
{
On 2016/10/27 17:14:25, rouslan wrote:
> Anonymous namespace should provide better encapsulation.

JNI picked this namespace. The functions need to be reachable from
'RegisterNativesImpl()'.

https://codereview.chromium.org/2373103002/diff/140001/chrome/browser/payment...
chrome/browser/payments/android/component_jni_registrar.cc:33: return
payments::RegisterNativesImpl(env);
On 2016/10/27 17:14:25, rouslan wrote:
> Where is this payments::RegisterNativesImpl() defined?

With the latest changes, it's here:

out/*/gen/components/payments/android/jni_headers/payments/jni/PaymentValidator_jni.h

https://codereview.chromium.org/2373103002/diff/140001/chrome/browser/payment...
File chrome/browser/payments/android/component_jni_registrar.h (right):

https://codereview.chromium.org/2373103002/diff/140001/chrome/browser/payment...
chrome/browser/payments/android/component_jni_registrar.h:1: // Copyright 2016
The Chromium Authors. All rights reserved.
On 2016/10/27 17:14:25, rouslan wrote:
> I think it's more typical to see RegisterPaymentValidator() in a
> payments-specific file name, like payment_validator_android.h/cc or
> payments_jni_registrar.h. See
>
https://cs.chromium.org/chromium/src/chrome/browser/autofill/android/personal...
> and
>
https://cs.chromium.org/chromium/src/components/sync/android/sync_jni_registr...
> patterns for reference.
> 
> In addition, it would be great to keep this file in
components/payments/android/
> similar to sync_jni_registrar.h for example. Do you think that's feasible?

Done.

https://codereview.chromium.org/2373103002/diff/140001/components/payments/pa...
File components/payments/payment_details_validation.cc (right):

https://codereview.chromium.org/2373103002/diff/140001/components/payments/pa...
components/payments/payment_details_validation.cc:132: 
On 2016/10/27 17:14:25, rouslan wrote:
> The functions above can be in anonymous namespace for better encapsulation.

Done.

https://codereview.chromium.org/2373103002/diff/140001/components/payments/pa...
File components/payments/payment_details_validation.h (right):

https://codereview.chromium.org/2373103002/diff/140001/components/payments/pa...
components/payments/payment_details_validation.h:28: bool
validatePaymentDetails(const blink::mojom::PaymentDetailsPtr& details);
On 2016/10/27 17:14:25, rouslan wrote:
> Instead of recreating the ExceptionState concept from Blink, let's use a
pointer
> to an optional std::string error message and return a boolean that indicates
> validity.
> 
> bool validatePaymentDetails(
>     const blink::mojom::PaymentDetailsPtr& details,
>     std::string* optional_error_message);

Done.

https://codereview.chromium.org/2373103002/diff/140001/components/payments/pa...
File components/payments/payments_validators.h (right):

https://codereview.chromium.org/2373103002/diff/140001/components/payments/pa...
components/payments/payments_validators.h:27: 
On 2016/10/27 17:14:25, rouslan wrote:
> The methods below are not relevant for PaymentDetails validator. Let's not add
> them here, or at least not just yet.

Done.

https://codereview.chromium.org/2373103002/diff/140001/components/payments/pa...
File components/payments/payments_validators_test.cc (right):

https://codereview.chromium.org/2373103002/diff/140001/components/payments/pa...
components/payments/payments_validators_test.cc:5: #include
"payments_validators.h"
On 2016/10/27 17:14:25, rouslan wrote:
> Should be the full path by Chromium conventions.
> 
> #include "components/payments/payments_validators.h"

Done.

[please use gerrit instead, 2016-10-28 18:38:58]

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

https://codereview.chromium.org/2373103002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:535:
private boolean parseAndValidateDetailsOrDisconnectFromClient(PaymentDetails
details) {
On 2016/10/27 22:17:52, Kevin Bailey wrote:
> On 2016/10/27 17:14:25, rouslan wrote:
> > All of the code in here can be changed to:
> > 
> > if (!PaymentValidator.validatePaymentDetails(details)) {
> >     disconnectFromClientWithDebugMessage("Invalid details");
> >     recordAbortReasonHistogram(
> >             PaymentRequestMetrics.ABORT_REASON_INVALID_DATA_FROM_RENDERER);
> >     return false;
> > }
> > 
> > // Initialize totalCurrency, formatter, uiTotal, uiLineItems,
mUiShoppingCart,
> > // mRawTotals, mRawLineItems, and mUiShippingOptions without further
> validation.
> > 
> > return true;
> 
> Done, but there's a lot of repetition here. What do you think about throwing
> (maybe the error message) from each error and have a catch deal with it? Looks
> like it would save 24 identical lines.

Since all validation happens inside of
PaymentValidator.validatePaymentDetails(), there's no need for any more
validation or throwing. The rest of the function should be operating under the
assumption that the data is now valid.

The purpose of the rest of the function is to massage the data from IPC messages
into display model, where necessary.

By the way, the null-check on lines 539-544 can be incorporated into
PaymentValidator.validatePaymentDetails().

https://codereview.chromium.org/2373103002/diff/240001/components/payments/pa...
File components/payments/payments_validators.cc (right):

https://codereview.chromium.org/2373103002/diff/240001/components/payments/pa...
components/payments/payments_validators.cc:29: //    if
(ScriptRegexp("^-?[0-9]+(\\.[0-9]+)?$",
Can you use https://cs.chromium.org/chromium/src/third_party/re2/ to revive this
validator?

https://codereview.chromium.org/2373103002/diff/240001/components/payments/pa...
File components/payments/payments_validators.h (right):

https://codereview.chromium.org/2373103002/diff/240001/components/payments/pa...
components/payments/payments_validators.h:20: static bool
isValidCurrencyCodeFormat(const std::string& code,
This validator now has "system" parameter.

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

Sorry!

[Kevin Bailey, 2016-10-28 20:53:05]

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

https://codereview.chromium.org/2373103002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:535:
private boolean parseAndValidateDetailsOrDisconnectFromClient(PaymentDetails
details) {
On 2016/10/28 18:38:58, rouslan wrote:
> On 2016/10/27 22:17:52, Kevin Bailey wrote:
> > On 2016/10/27 17:14:25, rouslan wrote:
> > > All of the code in here can be changed to:
> > > 
> > > if (!PaymentValidator.validatePaymentDetails(details)) {
> > >     disconnectFromClientWithDebugMessage("Invalid details");
> > >     recordAbortReasonHistogram(
> > >            
PaymentRequestMetrics.ABORT_REASON_INVALID_DATA_FROM_RENDERER);
> > >     return false;
> > > }
> > > 
> > > // Initialize totalCurrency, formatter, uiTotal, uiLineItems,
> mUiShoppingCart,
> > > // mRawTotals, mRawLineItems, and mUiShippingOptions without further
> > validation.
> > > 
> > > return true;
> > 
> > Done, but there's a lot of repetition here. What do you think about throwing
> > (maybe the error message) from each error and have a catch deal with it?
Looks
> > like it would save 24 identical lines.
> 
> Since all validation happens inside of
> PaymentValidator.validatePaymentDetails(), there's no need for any more
> validation or throwing. The rest of the function should be operating under the
> assumption that the data is now valid.
> 
> The purpose of the rest of the function is to massage the data from IPC
messages
> into display model, where necessary.
> 
> By the way, the null-check on lines 539-544 can be incorporated into
> PaymentValidator.validatePaymentDetails().

I'm worried about lines like:

if (uiLineItems == null) {

If there's a single difference between the validators, a user would only see,
"Chromium stopped." Are you ok without them?

https://codereview.chromium.org/2373103002/diff/240001/components/payments/pa...
File components/payments/payments_validators.cc (right):

https://codereview.chromium.org/2373103002/diff/240001/components/payments/pa...
components/payments/payments_validators.cc:29: //    if
(ScriptRegexp("^-?[0-9]+(\\.[0-9]+)?$",
On 2016/10/28 18:38:58, rouslan wrote:
> Can you use https://cs.chromium.org/chromium/src/third_party/re2/ to revive
this
> validator?

Done. (Sorry if I nuked it.) But there was a problem here. In spite of adding
the dependency to BUILD.gn, and something to DEPS, the presubmit still
complained that I was evil for trying to use re2. Could you glance at the new
changes and see if you see anything missing? thx

https://codereview.chromium.org/2373103002/diff/240001/components/payments/pa...
File components/payments/payments_validators.h (right):

https://codereview.chromium.org/2373103002/diff/240001/components/payments/pa...
components/payments/payments_validators.h:20: static bool
isValidCurrencyCodeFormat(const std::string& code,
On 2016/10/28 18:38:58, rouslan wrote:
> This validator now has "system" parameter.
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen...
> 
> Sorry!

I've added the parameter, but no one added it to the Mojo file after they added
it to the WebIDL. Shall I do that, and the converter too?

[please use gerrit instead, 2016-10-31 13:17:33]

Thank you!

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

https://codereview.chromium.org/2373103002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:535:
private boolean parseAndValidateDetailsOrDisconnectFromClient(PaymentDetails
details) {
On 2016/10/28 20:53:05, Kevin Bailey wrote:
> On 2016/10/28 18:38:58, rouslan wrote:
> > On 2016/10/27 22:17:52, Kevin Bailey wrote:
> > > On 2016/10/27 17:14:25, rouslan wrote:
> > > > All of the code in here can be changed to:
> > > > 
> > > > if (!PaymentValidator.validatePaymentDetails(details)) {
> > > >     disconnectFromClientWithDebugMessage("Invalid details");
> > > >     recordAbortReasonHistogram(
> > > >            
> PaymentRequestMetrics.ABORT_REASON_INVALID_DATA_FROM_RENDERER);
> > > >     return false;
> > > > }
> > > > 
> > > > // Initialize totalCurrency, formatter, uiTotal, uiLineItems,
> > mUiShoppingCart,
> > > > // mRawTotals, mRawLineItems, and mUiShippingOptions without further
> > > validation.
> > > > 
> > > > return true;
> > > 
> > > Done, but there's a lot of repetition here. What do you think about
throwing
> > > (maybe the error message) from each error and have a catch deal with it?
> Looks
> > > like it would save 24 identical lines.
> > 
> > Since all validation happens inside of
> > PaymentValidator.validatePaymentDetails(), there's no need for any more
> > validation or throwing. The rest of the function should be operating under
the
> > assumption that the data is now valid.
> > 
> > The purpose of the rest of the function is to massage the data from IPC
> messages
> > into display model, where necessary.
> > 
> > By the way, the null-check on lines 539-544 can be incorporated into
> > PaymentValidator.validatePaymentDetails().
> 
> I'm worried about lines like:
> 
> if (uiLineItems == null) {
> 
> If there's a single difference between the validators, a user would only see,
> "Chromium stopped." Are you ok without them?

This should not happen, which can be verified with "assert stuff != null" and
some tests.

https://codereview.chromium.org/2373103002/diff/240001/components/payments/pa...
File components/payments/payments_validators.cc (right):

https://codereview.chromium.org/2373103002/diff/240001/components/payments/pa...
components/payments/payments_validators.cc:29: //    if
(ScriptRegexp("^-?[0-9]+(\\.[0-9]+)?$",
On 2016/10/28 20:53:05, Kevin Bailey wrote:
> On 2016/10/28 18:38:58, rouslan wrote:
> > Can you use https://cs.chromium.org/chromium/src/third_party/re2/ to revive
> this
> > validator?
> 
> Done. (Sorry if I nuked it.) But there was a problem here. In spite of adding
> the dependency to BUILD.gn, and something to DEPS, the presubmit still
> complained that I was evil for trying to use re2. Could you glance at the new
> changes and see if you see anything missing? thx

Seems that "re2" dependency was added to components/payments/android/DEPS, but
should be in components/payments/DEPS. if that still does not work, try changing
the dependency from "re2" to "third_party/re2" (i.e., full path).

https://codereview.chromium.org/2373103002/diff/240001/components/payments/pa...
File components/payments/payments_validators.h (right):

https://codereview.chromium.org/2373103002/diff/240001/components/payments/pa...
components/payments/payments_validators.h:20: static bool
isValidCurrencyCodeFormat(const std::string& code,
On 2016/10/28 20:53:05, Kevin Bailey wrote:
> On 2016/10/28 18:38:58, rouslan wrote:
> > This validator now has "system" parameter.
> > 
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen...
> > 
> > Sorry!
> 
> I've added the parameter, but no one added it to the Mojo file after they
added
> it to the WebIDL. Shall I do that, and the converter too?

SGTM. It's good that we're finding these discrepancies now and unifying the
renderer and browser implementations. This will help us to move faster in the
future.

[Kevin Bailey, 2016-10-31 14:10:03]

https://codereview.chromium.org/2373103002/diff/240001/components/payments/pa...
File components/payments/payments_validators.cc (right):

https://codereview.chromium.org/2373103002/diff/240001/components/payments/pa...
components/payments/payments_validators.cc:29: //    if
(ScriptRegexp("^-?[0-9]+(\\.[0-9]+)?$",
On 2016/10/31 13:17:33, rouslan wrote:
> On 2016/10/28 20:53:05, Kevin Bailey wrote:
> > On 2016/10/28 18:38:58, rouslan wrote:
> > > Can you use https://cs.chromium.org/chromium/src/third_party/re2/ to
revive
> > this
> > > validator?
> > 
> > Done. (Sorry if I nuked it.) But there was a problem here. In spite of
adding
> > the dependency to BUILD.gn, and something to DEPS, the presubmit still
> > complained that I was evil for trying to use re2. Could you glance at the
new
> > changes and see if you see anything missing? thx
> 
> Seems that "re2" dependency was added to components/payments/android/DEPS, but
> should be in components/payments/DEPS. if that still does not work, try
changing
> the dependency from "re2" to "third_party/re2" (i.e., full path).

Ya, it required the full path.

https://codereview.chromium.org/2373103002/diff/240001/components/payments/pa...
File components/payments/payments_validators.h (right):

https://codereview.chromium.org/2373103002/diff/240001/components/payments/pa...
components/payments/payments_validators.h:20: static bool
isValidCurrencyCodeFormat(const std::string& code,
On 2016/10/31 13:17:33, rouslan wrote:
> On 2016/10/28 20:53:05, Kevin Bailey wrote:
> > On 2016/10/28 18:38:58, rouslan wrote:
> > > This validator now has "system" parameter.
> > > 
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen...
> > > 
> > > Sorry!
> > 
> > I've added the parameter, but no one added it to the Mojo file after they
> added
> > it to the WebIDL. Shall I do that, and the converter too?
> 
> SGTM. It's good that we're finding these discrepancies now and unifying the
> renderer and browser implementations. This will help us to move faster in the
> future.

Done.

[please use gerrit instead, 2016-10-31 14:31:15]

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

https://codereview.chromium.org/2373103002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:587:
* Validates a list of payment items and returns their parsed representation or
null if invalid.
s/Validates/Converts/

Please remove "or null if invalid".

https://codereview.chromium.org/2373103002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:592:
* @return A list of valid line items or null if invalid.
Please remove "or null if invalid" from this comment.

https://codereview.chromium.org/2373103002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:594:
private static List<LineItem> getValidatedLineItems(
Please remove the "validated" keyword from the function. This function should no
longer do any validation. That's being done inside of
PaymentValidator.validatePaymentDetails() now.

https://codereview.chromium.org/2373103002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:603:
if (!hasAllPaymentItemFields(item)) return null;
You can either assert that this is true or remove this statement all together.
This function should no longer return null.

https://codereview.chromium.org/2373103002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:606:
if (!item.amount.currency.equals(totalCurrency)) return null;
Ditto

https://codereview.chromium.org/2373103002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:609:
if (!formatter.isValidAmountValue(item.amount.value)) return null;
Ditto

https://codereview.chromium.org/2373103002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:618:
/**
Same validation comments for this function.

https://codereview.chromium.org/2373103002/diff/280001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentValidator.java
(right):

https://codereview.chromium.org/2373103002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentValidator.java:18:
if (details == null) {
Formatting nit: Chrome's Android code usually inlines short statements like
this.

if (details == null) return false;

https://codereview.chromium.org/2373103002/diff/280001/components/payments/pa...
File components/payments/payment_details_validation.cc (right):

https://codereview.chromium.org/2373103002/diff/280001/components/payments/pa...
components/payments/payment_details_validation.cc:41: item->amount->currency,
"", error_message)) {
Ideally "" would be replaced with item->amount->currencySystem. currencySystem
validation is that it should never be null.

https://codereview.chromium.org/2373103002/diff/280001/components/payments/pa...
File components/payments/payment_request.mojom (right):

https://codereview.chromium.org/2373103002/diff/280001/components/payments/pa...
components/payments/payment_request.mojom:46: 
Please add a comment from
https://w3c.github.io/browser-payment-api/#paymentcurrencyamount

// currencySystem is a URL that indicates the currency system that the
// currency identifier belongs to. By default, the value is
// urn:iso:std:iso:4217 indicating that currency is defined by [ISO4217]
// (for example, USD for US Dollars).

https://codereview.chromium.org/2373103002/diff/280001/components/payments/pa...
File components/payments/payments_validators_test.cc (right):

https://codereview.chromium.org/2373103002/diff/280001/components/payments/pa...
components/payments/payments_validators_test.cc:63: testing::Values(
These tests have been updated to include the currencySystem. See:

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

[Kevin Bailey, 2016-10-31 20:32:00]

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

https://codereview.chromium.org/2373103002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:587:
* Validates a list of payment items and returns their parsed representation or
null if invalid.
On 2016/10/31 14:31:15, rouslan wrote:
> s/Validates/Converts/
> 
> Please remove "or null if invalid".

Done.

https://codereview.chromium.org/2373103002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:592:
* @return A list of valid line items or null if invalid.
On 2016/10/31 14:31:15, rouslan wrote:
> Please remove "or null if invalid" from this comment.

Done.

https://codereview.chromium.org/2373103002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:594:
private static List<LineItem> getValidatedLineItems(
On 2016/10/31 14:31:15, rouslan wrote:
> Please remove the "validated" keyword from the function. This function should
no
> longer do any validation. That's being done inside of
> PaymentValidator.validatePaymentDetails() now.

Done.

https://codereview.chromium.org/2373103002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:603:
if (!hasAllPaymentItemFields(item)) return null;
On 2016/10/31 14:31:14, rouslan wrote:
> You can either assert that this is true or remove this statement all together.
> This function should no longer return null.

Done.

https://codereview.chromium.org/2373103002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:606:
if (!item.amount.currency.equals(totalCurrency)) return null;
On 2016/10/31 14:31:14, rouslan wrote:
> Ditto

Done.

https://codereview.chromium.org/2373103002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:609:
if (!formatter.isValidAmountValue(item.amount.value)) return null;
On 2016/10/31 14:31:15, rouslan wrote:
> Ditto

Done.

https://codereview.chromium.org/2373103002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:618:
/**
On 2016/10/31 14:31:15, rouslan wrote:
> Same validation comments for this function.

Done.

https://codereview.chromium.org/2373103002/diff/280001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentValidator.java
(right):

https://codereview.chromium.org/2373103002/diff/280001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentValidator.java:18:
if (details == null) {
On 2016/10/31 14:31:15, rouslan wrote:
> Formatting nit: Chrome's Android code usually inlines short statements like
> this.
> 
> if (details == null) return false;

Interesting. How would you set a breakpoint on the return statement if you
needed to?

https://codereview.chromium.org/2373103002/diff/280001/components/payments/pa...
File components/payments/payment_details_validation.cc (right):

https://codereview.chromium.org/2373103002/diff/280001/components/payments/pa...
components/payments/payment_details_validation.cc:41: item->amount->currency,
"", error_message)) {
On 2016/10/31 14:31:15, rouslan wrote:
> Ideally "" would be replaced with item->amount->currencySystem. currencySystem
> validation is that it should never be null.

Sorry, missed this one. Also, really named it 'currencySystem' this time.

The field can be absent, right? From websites that haven't updated yet?

https://codereview.chromium.org/2373103002/diff/280001/components/payments/pa...
File components/payments/payment_request.mojom (right):

https://codereview.chromium.org/2373103002/diff/280001/components/payments/pa...
components/payments/payment_request.mojom:46: 
On 2016/10/31 14:31:15, rouslan wrote:
> Please add a comment from
> https://w3c.github.io/browser-payment-api/#paymentcurrencyamount
> 
> // currencySystem is a URL that indicates the currency system that the
> // currency identifier belongs to. By default, the value is
> // urn:iso:std:iso:4217 indicating that currency is defined by [ISO4217]
> // (for example, USD for US Dollars).

Done.

https://codereview.chromium.org/2373103002/diff/280001/components/payments/pa...
File components/payments/payments_validators_test.cc (right):

https://codereview.chromium.org/2373103002/diff/280001/components/payments/pa...
components/payments/payments_validators_test.cc:63: testing::Values(
On 2016/10/31 14:31:15, rouslan wrote:
> These tests have been updated to include the currencySystem. See:
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen...

Looks like a few things diverged. I sync'd them.

[please use gerrit instead, 2016-11-01 14:20:03]

We should remove the "blink" namespace from "components/payments/" either in
this CL or a follow up.

https://codereview.chromium.org/2373103002/diff/340001/components/payments/pa...
File components/payments/payment_details_validation.cc (right):

https://codereview.chromium.org/2373103002/diff/340001/components/payments/pa...
components/payments/payment_details_validation.cc:183: }
Paymentdetails also has error_message. Please run that through
isValidErrorMsgFormat, which you have in the payments_validators.h.

https://codereview.chromium.org/2373103002/diff/340001/components/payments/pa...
File components/payments/payments_validators.cc (left):

https://codereview.chromium.org/2373103002/diff/340001/components/payments/pa...
components/payments/payments_validators.cc:37: }
Let's not delete validating of "system". It should be a valid URL. You should be
able to use GURL::is_valid() for this.

https://cs.chromium.org/chromium/src/url/gurl.h?rcl=0&l=86

https://codereview.chromium.org/2373103002/diff/340001/components/payments/pa...
File components/payments/payments_validators.h (right):

https://codereview.chromium.org/2373103002/diff/340001/components/payments/pa...
components/payments/payments_validators.h:50: std::string*
optionalErrorMessage);
No need for isValidCountryCodeFormat, isValidLanguageCodeFormat,
isValidScriptCodeFormat, and isValidShippingAddress yet.

[Kevin Bailey, 2016-11-01 15:32:13]

I would definitely like to correct the namespace ASAP, before
anyone writes code to it, but it is just too wide a change
for this CL. Let me know if "payments" is too overused, or
what you'd prefer.

https://codereview.chromium.org/2373103002/diff/340001/components/payments/pa...
File components/payments/payment_details_validation.cc (right):

https://codereview.chromium.org/2373103002/diff/340001/components/payments/pa...
components/payments/payment_details_validation.cc:183: }
On 2016/11/01 14:20:02, rouslan wrote:
> Paymentdetails also has error_message. Please run that through
> isValidErrorMsgFormat, which you have in the payments_validators.h.

Done.

https://codereview.chromium.org/2373103002/diff/340001/components/payments/pa...
File components/payments/payments_validators.cc (left):

https://codereview.chromium.org/2373103002/diff/340001/components/payments/pa...
components/payments/payments_validators.cc:37: }
On 2016/11/01 14:20:03, rouslan wrote:
> Let's not delete validating of "system". It should be a valid URL. You should
be
> able to use GURL::is_valid() for this.
> 
> https://cs.chromium.org/chromium/src/url/gurl.h?rcl=0&l=86

Thank you for spotting that. I thought I had kept it.

https://codereview.chromium.org/2373103002/diff/340001/components/payments/pa...
File components/payments/payments_validators.h (right):

https://codereview.chromium.org/2373103002/diff/340001/components/payments/pa...
components/payments/payments_validators.h:50: std::string*
optionalErrorMessage);
On 2016/11/01 14:20:03, rouslan wrote:
> No need for isValidCountryCodeFormat, isValidLanguageCodeFormat,
> isValidScriptCodeFormat, and isValidShippingAddress yet.

Your tests wanted them, and the new guys may not know about them, so
I didn't see a reason to remove them unless you will never use them.

[please use gerrit instead, 2016-11-01 15:33:54]

"payments" namespace sounds good.

This patch lgtm after the namespace change.

[please use gerrit instead, 2016-11-01 15:40:50]

Follow up CL for namespace change is also good.

[Kevin Bailey, 2016-11-01 16:05:56]

krb@chromium.org changed reviewers:
+ jochen@chromium.org, mkwst@chromium.org
- jdonnelly@chromium.org

[Kevin Bailey, 2016-11-01 16:09:33]

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

[commit-bot: I haz the power, 2016-11-01 16:09:57]

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, 2016-11-01 16:12:18]

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

[commit-bot: I haz the power, 2016-11-01 16:12:19]

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_...)

[Kevin Bailey, 2016-11-01 16:37:19]

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

[commit-bot: I haz the power, 2016-11-01 16:37:37]

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, 2016-11-01 16:41:27]

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

[commit-bot: I haz the power, 2016-11-01 16:41:28]

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

[Kevin Bailey, 2016-11-01 17:49:47]

On 2016/11/01 15:40:50, rouslan wrote:
> Follow up CL for namespace change is also good.

This CL moves(copies) some RPC validation from Blink to a more common
location, but required an IDL change and some JNI registration, and we
hooked up some unit tests.

jochen@, could you approve

  chrome/browser/android/chrome_jni_registrar.cc
  components/BUILD.gn

?

mkwst@, could you approve

  components/payments/payment_request.mojom

?

thanks!

[jochen (gone - plz use gerrit), 2016-11-02 10:56:32]

please format the CL description according to
https://www.chromium.org/developers/contributing-code#Writing_change_list_des...
(break long lines at 80c)

then this lgtm

[Kevin Bailey, 2016-11-02 14:28:32]

Description was changed from

==========
Provides JNI interface to Java(Android). The interface is also available to
Views, but Blink still uses existing validation routines due to type conflicts.

This is a follow up to 2373103002, which solely moved the Mojo file.

BUG=659644
==========

to

==========
Provides Payment Validation JNI interface to Java(Android). The interface
is also available to Views, but Blink still uses existing validation
routines due to type conflicts (which we hope to resolve.)

This is a follow up to 2373103002, which solely moved the Mojo file from
Blink to components/

BUG=659644
==========

[please use gerrit instead, 2016-11-02 15:54:17]

Description was changed from

==========
Provides Payment Validation JNI interface to Java(Android). The interface
is also available to Views, but Blink still uses existing validation
routines due to type conflicts (which we hope to resolve.)

This is a follow up to 2373103002, which solely moved the Mojo file from
Blink to components/

BUG=659644
==========

to

==========
[Web Payments] Common Payments validation

Provides Payment Validation JNI interface to Java(Android). The interface
is also available to Views, but Blink still uses existing validation
routines due to type conflicts (which we hope to resolve.)

This is a follow up to 2373103002, which solely moved the Mojo file from
Blink to components/

BUG=659644
==========

[Mike West, 2016-11-03 12:58:11]

mojom LGTM

[Kevin Bailey, 2016-11-03 13:26:00]

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

[commit-bot: I haz the power, 2016-11-03 13:26:12]

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, 2016-11-03 14:40:42]

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

[commit-bot: I haz the power, 2016-11-03 14:40:44]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Kevin Bailey, 2016-11-03 16:15:26]

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

[commit-bot: I haz the power, 2016-11-03 16:16:10]

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, 2016-11-03 16:53:12]

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

[commit-bot: I haz the power, 2016-11-03 16:53:13]

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

[Kevin Bailey, 2016-11-03 17:16:29]

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

[commit-bot: I haz the power, 2016-11-03 17:16:50]

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, 2016-11-03 19:54:51]

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

[commit-bot: I haz the power, 2016-11-03 19:54:52]

Dry run: This issue passed the CQ dry run.

[Kevin Bailey, 2016-11-03 20:08:35]

The CQ bit was checked by krb@chromium.org

[Kevin Bailey, 2016-11-03 20:08:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, mkwst@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2373103002/#ps440001
(title: "Fixed validator 2")

[commit-bot: I haz the power, 2016-11-03 20:09:17]

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-11-03 20:17:26]

Description was changed from

==========
[Web Payments] Common Payments validation

Provides Payment Validation JNI interface to Java(Android). The interface
is also available to Views, but Blink still uses existing validation
routines due to type conflicts (which we hope to resolve.)

This is a follow up to 2373103002, which solely moved the Mojo file from
Blink to components/

BUG=659644
==========

to

==========
[Web Payments] Common Payments validation

Provides Payment Validation JNI interface to Java(Android). The interface
is also available to Views, but Blink still uses existing validation
routines due to type conflicts (which we hope to resolve.)

This is a follow up to 2373103002, which solely moved the Mojo file from
Blink to components/

BUG=659644
==========

[commit-bot: I haz the power, 2016-11-03 20:17:28]

Committed patchset #23 (id:440001)

[commit-bot: I haz the power, 2016-11-03 20:19:30]

Description was changed from

==========
[Web Payments] Common Payments validation

Provides Payment Validation JNI interface to Java(Android). The interface
is also available to Views, but Blink still uses existing validation
routines due to type conflicts (which we hope to resolve.)

This is a follow up to 2373103002, which solely moved the Mojo file from
Blink to components/

BUG=659644
==========

to

==========
[Web Payments] Common Payments validation

Provides Payment Validation JNI interface to Java(Android). The interface
is also available to Views, but Blink still uses existing validation
routines due to type conflicts (which we hope to resolve.)

This is a follow up to 2373103002, which solely moved the Mojo file from
Blink to components/

BUG=659644

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

[commit-bot: I haz the power, 2016-11-03 20:19:31]

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