PaymentRequest Mojo bindings.

PaymentRequest Mojo bindings.

The Mojo (new IPC replacement) bindings for PaymentRequest Blink API.
Blink uses the bindings to procure the functionality for the API. Mojo
eliminates multiple layers of abstraction, letting the embedder provide
the functionality by implementing the PaymentRequest interface in C++ or
Java for Android. The PaymentRequest interface lives in:

third_party/Webkit/public/platform/modules/payments/payment_request.mojom

The embedder does not provide any functionality in this patch.

PaymentRequest draft spec:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html

BUG=587995
TEST=PaymentRequestTest.cpp
TEST=PaymentResponseTest.cpp
TEST=PaymentsValidatorsTest.cpp
TEST=ShippingAddressTest.cpp
TEST=payment-request-interface.html

Committed: https://crrev.com/763f2a3acbf7ee75d8eb4f1c0a6d90399f75082f
Cr-Commit-Position: refs/heads/master@{#384439}

[please use gerrit instead, 2016-03-04 18:55:52]

Description was changed from

==========
PaymentRequest Mojo bindings.

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

to

==========
PaymentRequest Mojo bindings.

The code in modules/payments/ talks to Mojo through
platform/payments/PaymentRequestProxy.h, because Blink can use Mojo only
through platform/.

The platform/payments/ directory contains data structures that mirror
the dictionaries and interfaces in modules/payments/, because platform/
cannot include modules/.

The Mojo interface is defined in src/components/payments/, which is a
new dependency for platform/.

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

[please use gerrit instead, 2016-03-04 19:24:09]

Description was changed from

==========
PaymentRequest Mojo bindings.

The code in modules/payments/ talks to Mojo through
platform/payments/PaymentRequestProxy.h, because Blink can use Mojo only
through platform/.

The platform/payments/ directory contains data structures that mirror
the dictionaries and interfaces in modules/payments/, because platform/
cannot include modules/.

The Mojo interface is defined in src/components/payments/, which is a
new dependency for platform/.

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

to

==========
PaymentRequest Mojo bindings.

The code in modules/payments/PaymentRequest.cpp talks to Mojo through
platform/payments/PaymentRequestProxy.h, because Blink can use Mojo only
through platform/.

The platform/payments/ directory contains data structures that mirror
the dictionaries and interfaces in modules/payments/, because platform/
cannot include modules/.

The Mojo interface is defined in
components/payments/payment_request.mojom.h, which is a new dependency
for platform/payments/.

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

[please use gerrit instead, 2016-03-07 19:55:34]

Description was changed from

==========
PaymentRequest Mojo bindings.

The code in modules/payments/PaymentRequest.cpp talks to Mojo through
platform/payments/PaymentRequestProxy.h, because Blink can use Mojo only
through platform/.

The platform/payments/ directory contains data structures that mirror
the dictionaries and interfaces in modules/payments/, because platform/
cannot include modules/.

The Mojo interface is defined in
components/payments/payment_request.mojom.h, which is a new dependency
for platform/payments/.

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

to

==========
PaymentRequest Mojo bindings.

The code in modules/payments/PaymentRequest.cpp talks to Mojo through
platform/payments/PaymentRequestProxy.h, because Blink can use Mojo only
through platform/.

The platform/payments/ directory contains data structures that mirror
the dictionaries and interfaces in modules/payments/, because platform/
cannot include modules/. These files are named "PlatformFoo" to avoid
conflicts with files named "Foo" in modules/payments/.

The Mojo interface is defined in
components/payments/payment_request.mojom.h, which is a new dependency
for platform/payments/.

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

[please use gerrit instead, 2016-03-07 21:01:53]

rouslan@chromium.org changed reviewers:
+ esprehn@chromium.org

[please use gerrit instead, 2016-03-07 21:01:54]

Elliott, ptal platform/ in patch 9.

I'm used to a single reviewer per section. If anyone CC-ed would like to chip
in, go ahead.

[please use gerrit instead, 2016-03-07 21:03:08]

rouslan@chromium.org changed reviewers:
+ mek@chromium.org

[please use gerrit instead, 2016-03-07 21:03:08]

Marijn, ptal modules/payments/ in patch 9.

[please use gerrit instead, 2016-03-07 21:05:20]

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

[please use gerrit instead, 2016-03-07 21:05:21]

Chris, ptal components/payments/payment_request.mojom in patch 9.

[haraken, 2016-03-08 00:04:59]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org, yukishiino@chromium.org

[haraken, 2016-03-08 00:05:00]

Hmm. It's unfortunate that you have to add a lot of PlatformXXX classes due to
the restriction that modules/ cannot (yet) directly use mojo...

Elliott: Maybe it would be nicer to allow modules/ to talk with mojo (in
parallel with the efforts to make the Mojo-WTF type conversion faster)?

[palmer, 2016-03-08 22:49:36]

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

https://codereview.chromium.org/1753543002/diff/160001/components/payments/pa...
components/payments/payment_request.mojom:22: string stringified_details;
This is vague and weakly-typed. Can you document what goes in here, or split out
the details into separate fields, or use richer types?

https://codereview.chromium.org/1753543002/diff/160001/components/payments/pa...
components/payments/payment_request.mojom:34: string currency_code;
currency_code, region_code, language_code, et c. feel more like enums than
strings, to me. It's good to keep IPC messages as type-constrained as possible.
(https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc)

https://codereview.chromium.org/1753543002/diff/160001/components/payments/pa...
components/payments/payment_request.mojom:35: string value;
Hmm, a string for money feels odd. Why not use a tuple of ints?

struct CurrentAmount {
  string currency_code;
  int64 units;
  int16 thousandths;
};

[esprehn, 2016-03-08 22:57:43]

Can we whitelist one module at a time? How much std::string and std::vector does
this drag into the module?

[please use gerrit instead, 2016-03-08 23:13:42]

On 2016/03/08 22:57:43, esprehn wrote:
> Can we whitelist one module at a time?

Sure, can modules/payment/ be whitelisted for this CL?

> How much std::string and std::vector does
> this drag into the module?

Everything from these two files.

https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...

[please use gerrit instead, 2016-03-09 18:45:47]

Description was changed from

==========
PaymentRequest Mojo bindings.

The code in modules/payments/PaymentRequest.cpp talks to Mojo through
platform/payments/PaymentRequestProxy.h, because Blink can use Mojo only
through platform/.

The platform/payments/ directory contains data structures that mirror
the dictionaries and interfaces in modules/payments/, because platform/
cannot include modules/. These files are named "PlatformFoo" to avoid
conflicts with files named "Foo" in modules/payments/.

The Mojo interface is defined in
components/payments/payment_request.mojom.h, which is a new dependency
for platform/payments/.

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

to

==========
PaymentRequest Mojo bindings.

The code in modules/payments/PaymentRequest.cpp talks to Mojo through
platform/payments/PaymentRequestProxy.h, because Blink can use Mojo only
through platform/.

The platform/payments/ directory contains data structures that mirror
the dictionaries and interfaces in modules/payments/, because platform/
cannot include modules/. These files are named "PlatformFoo" to avoid
conflicts with files named "Foo" in modules/payments/.

The Mojo interface is defined in
components/payments/payment_request.mojom.h, which is a new dependency
for platform/payments/.

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

[please use gerrit instead, 2016-03-10 00:38:05]

Chris, ptal in patch 11:

components/payments/*.mojom

third_party/WebKit/Source/platform/payments/CountryCodeToString.*
third_party/WebKit/Source/platform/payments/LanguageCodeToString.*
third_party/WebKit/Source/platform/payments/ScriptCodeToString.*
third_party/WebKit/Source/platform/payments/StringToCurrencyCode.*

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

https://codereview.chromium.org/1753543002/diff/160001/components/payments/pa...
components/payments/payment_request.mojom:22: string stringified_details;
On 2016/03/08 22:49:36, palmer wrote:
> This is vague and weakly-typed. Can you document what goes in here, or split
out
> the details into separate fields, or use richer types?

I've documented stringified_details and stringified_data. How does it look to
you?

https://codereview.chromium.org/1753543002/diff/160001/components/payments/pa...
components/payments/payment_request.mojom:34: string currency_code;
On 2016/03/08 22:49:36, palmer wrote:
> currency_code, region_code, language_code, et c. feel more like enums than
> strings, to me. It's good to keep IPC messages as type-constrained as
possible.
>
(https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc)

Done.

https://codereview.chromium.org/1753543002/diff/160001/components/payments/pa...
components/payments/payment_request.mojom:35: string value;
On 2016/03/08 22:49:36, palmer wrote:
> Hmm, a string for money feels odd. Why not use a tuple of ints?
> 
> struct CurrentAmount {
>   string currency_code;
>   int64 units;
>   int16 thousandths;
> };

Done.

[haavardm, 2016-03-10 12:46:56]

haavardm@opera.com changed reviewers:
+ haavardm@opera.com

[haavardm, 2016-03-10 12:46:58]

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

https://codereview.chromium.org/1753543002/diff/160001/components/payments/pa...
components/payments/payment_request.mojom:34: string currency_code;
On 2016/03/08 22:49:36, palmer wrote:
> currency_code, region_code, language_code, et c. feel more like enums than
> strings, to me. It's good to keep IPC messages as type-constrained as
possible.
>
(https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc)

I'm skeptical about enuming any of the input from the JS api, as long as the
spec is not enuming them. What if new currency codes are created (think new
bitcoin like inventions).  I don't think chromium should add restrictions like
that and I think it's best if currency code is passed unchanged to the payment
app. I'm not sure I see the security problems for these strings as they are not
blogs of data, they are pure string values.

https://codereview.chromium.org/1753543002/diff/160001/components/payments/pa...
components/payments/payment_request.mojom:35: string value;
On 2016/03/10 00:38:05, Rouslan wrote:
> On 2016/03/08 22:49:36, palmer wrote:
> > Hmm, a string for money feels odd. Why not use a tuple of ints?
> > 
> > struct CurrentAmount {
> >   string currency_code;
> >   int64 units;
> >   int16 thousandths;
> > };
> 
> Done.
The payment request api defines it as string, so I'm not sure blink should
redefine that. It's up to the payment app to interpret the string:
https://github.com/w3c/webpayments/issues/40

[palmer, 2016-03-10 19:24:36]

> I'm skeptical about enuming any of the input from the JS api, as long as the
> spec is not enuming them. What if new currency codes are created (think new
> bitcoin like inventions).  I don't think chromium should add restrictions like
> that and I think it's best if currency code is passed unchanged to the payment
> app. I'm not sure I see the security problems for these strings as they are
not
> blogs of data, they are pure string values.

Strings are hostile blobs until proven innocent. :) Some code, somewhere in a
trustworthy place (like the browser process), is going to have to validate those
blobs. Everything is a hostile blob until proven innocent, really. The question
is, which validation code would you rather write, and run?

enum Currency {
  // ...
};

void HandleCurrency(Currency c) {
  switch (c) {
    case NorwayKrone:
      // ...
    case EUEuro:
      // ...
    default:
      explode("Monopoly money detected");
  }
}

Or:

std::set<std::string> kCurrencies;

void HandleCurrency(std::string c) {
  if (c.length() > kMaximumCurrencyStringLength) {
    explode("Monopoly money detected");
  }

  if (kCurrencies.empty()) {
    // Lazily initialize
  }

  if (kCurrencies.end() == kCurrencies.find(c)) {
    explode("Monopoly money detected");
  }

  // Handle each c
}

I suspect the web payments people defined the currency to be a string because it
was convenient, or because they didn't want to have to start a registry, or
refer to one. (https://en.wikipedia.org/wiki/ISO_4217) Code that handles money
has to keep up-to-date with that registry regardless.

Now, perhaps Chrome and other applications that use Blink and this component do
not need to validate that a given currency name string is real. They'd still
have to have a way to reject overly large and malformed strings.
https://tools.ietf.org/html/draft-thomson-postel-was-wrong-00

[esprehn, 2016-03-10 19:27:45]

The web uses strings for enum values. That's why the apis are shapes like
this. We can convert them internally however you want.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[esprehn, 2016-03-10 19:27:46]

The web uses strings for enum values. That's why the apis are shapes like
this. We can convert them internally however you want.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[please use gerrit instead, 2016-03-10 20:09:37]

If bitcoin is the only currency that we do not support, I can add it to the
code generator.

On Thu, Mar 10, 2016 at 11:33 AM Elliott Sprehn <esprehn@chromium.org>
wrote:

> The web uses strings for enum values. That's why the apis are shapes like
> this. We can convert them internally however you want.
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

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

If bitcoin is the only currency that we do not support, I can add it to the
code generator.

On Thu, Mar 10, 2016 at 11:33 AM Elliott Sprehn <esprehn@chromium.org>
wrote:

> The web uses strings for enum values. That's why the apis are shapes like
> this. We can convert them internally however you want.
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[please use gerrit instead, 2016-03-10 20:33:45]

As a matter of fact, I should probably add all unofficial currency codes
listed here:
https://en.wikipedia.org/wiki/ISO_4217#Unofficial_currency_codes
<https://www.google.com/url?q=https%3A%2F%2Fen.wikipedia.org%2Fwiki%2FISO_4217...>
 .

On Thu, Mar 10, 2016 at 12:09 PM Rouslan Solomakhin <rouslan@chromium.org>
wrote:

> If bitcoin is the only currency that we do not support, I can add it to
> the code generator.
>
> On Thu, Mar 10, 2016 at 11:33 AM Elliott Sprehn <esprehn@chromium.org>
> wrote:
>
>> The web uses strings for enum values. That's why the apis are shapes like
>> this. We can convert them internally however you want.
>>
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[please use gerrit instead, 2016-03-10 20:33:45]

As a matter of fact, I should probably add all unofficial currency codes
listed here:
https://en.wikipedia.org/wiki/ISO_4217#Unofficial_currency_codes
<https://www.google.com/url?q=https%3A%2F%2Fen.wikipedia.org%2Fwiki%2FISO_4217...>
 .

On Thu, Mar 10, 2016 at 12:09 PM Rouslan Solomakhin <rouslan@chromium.org>
wrote:

> If bitcoin is the only currency that we do not support, I can add it to
> the code generator.
>
> On Thu, Mar 10, 2016 at 11:33 AM Elliott Sprehn <esprehn@chromium.org>
> wrote:
>
>> The web uses strings for enum values. That's why the apis are shapes like
>> this. We can convert them internally however you want.
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[please use gerrit instead, 2016-03-10 22:44:43]

I've added the unofficial currencies in patch 13.

[esprehn, 2016-03-11 06:06:59]

This patch is over a thousand lines of string to enum conversions, that really
doesn't seem worth it. Why does Chrome need the enum instead of just passing the
string for the currency to the payment provider? I don't think adding this much
code to the binary for currency conversions is worth it.

[haavardm, 2016-03-11 13:07:26]

On 2016/03/10 19:24:36, palmer wrote:
> > I'm skeptical about enuming any of the input from the JS api, as long as the
> > spec is not enuming them. What if new currency codes are created (think new
> > bitcoin like inventions).  I don't think chromium should add restrictions
like
> > that and I think it's best if currency code is passed unchanged to the
payment
> > app. I'm not sure I see the security problems for these strings as they are
> not
> > blogs of data, they are pure string values.
> 
> Strings are hostile blobs until proven innocent. :) Some code, somewhere in a
> trustworthy place (like the browser process), is going to have to validate
those
> blobs. Everything is a hostile blob until proven innocent, really. The
question
> is, which validation code would you rather write, and run?
> 
> enum Currency {
>   // ...
> };
> 
> void HandleCurrency(Currency c) {
>   switch (c) {
>     case NorwayKrone:
>       // ...
>     case EUEuro:
>       // ...
>     default:
>       explode("Monopoly money detected");
>   }
> }
> 
> Or:
> 
> std::set<std::string> kCurrencies;
> 
> void HandleCurrency(std::string c) {
>   if (c.length() > kMaximumCurrencyStringLength) {
>     explode("Monopoly money detected");
>   }
> 
>   if (kCurrencies.empty()) {
>     // Lazily initialize
>   }
> 
>   if (kCurrencies.end() == kCurrencies.find(c)) {
>     explode("Monopoly money detected");
>   }
> 
>   // Handle each c
> }
> 
> I suspect the web payments people defined the currency to be a string because
it
> was convenient, or because they didn't want to have to start a registry, or
> refer to one. (https://en.wikipedia.org/wiki/ISO_4217) Code that handles money
> has to keep up-to-date with that registry regardless.
> 
> Now, perhaps Chrome and other applications that use Blink and this component
do
> not need to validate that a given currency name string is real. They'd still
> have to have a way to reject overly large and malformed strings.
> https://tools.ietf.org/html/draft-thomson-postel-was-wrong-00

The browser is normally not supposed handle these values [1]. They are plumbed
right through the browser and forwarded to a payment app. The payment app might
be another web app shown in some special browser UI, or it might a native app
(like an Android App). In case it's a web app, we need to convert it back to a
string that the payment app needs to parse anyway. Thus, I have a hard time
seeing the value of turning these into enums. In any case, the merchant site and
payment app needs to be compatible, and I don't think it's up to the browser to
restrict what currencies to use. Chromium should validate the formats as given
by the spec,  but I don't think the values should be restricted. The code size
for the enum conversions (as pointed out by esprehn below) is another argument
against this.

The same arguments holds for turning the amount string into numbers. It
certainly makes sense to check that the format is according to spec though.

[1] The browser might implement support for some payment instruments like visa
cards etc, but these are special cases.

[please use gerrit instead, 2016-03-11 17:20:37]

Chris: Håvard and Elliot bring up good points. Perhaps we can use the knowledge
of the formats of these strings for validation? For example:

1) Country code is two upper case letters.
2) Language code is two or three lower case letters.
3) Script code is four letters long. The first letter is capitalized.
4) Currency code is three upper case letters.
5) Monetary amount is an arbitrary number of digits, optionally followed by a
period, optionally followed by an arbitrary amounts of digits.

[please use gerrit instead, 2016-03-11 17:50:21]

On 2016/03/11 17:20:37, Rouslan wrote:
> 1) Country code is two upper case letters.
> 2) Language code is two or three lower case letters.
> 3) Script code is four letters long. The first letter is capitalized.
> 4) Currency code is three upper case letters.
> 5) Monetary amount is an arbitrary number of digits, optionally followed by a
> period, optionally followed by an arbitrary amounts of digits.

A letter here means [a-zA-Z].

[please use gerrit instead, 2016-03-11 19:15:18]

Chris, also note that translate IPCs use strings for language codes:

https://code.google.com/p/chromium/codesearch#chromium/src/components/transla...

[palmer, 2016-03-14 20:51:56]

On 2016/03/11 19:15:18, Rouslan wrote:
> Chris, also note that translate IPCs use strings for language codes:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/transla...

Yes, but in that case (if I'm reading it right) it's the browser (trusted)
sending the strings to the renderer (untrusted, low-privilege). So there's no
attack potential that we would recognize as a bug.

[palmer, 2016-03-14 20:52:29]

Anyway, after talking to Rouslan, using strings SGTM.

[please use gerrit instead, 2016-03-17 02:22:22]

Description was changed from

==========
PaymentRequest Mojo bindings.

The code in modules/payments/PaymentRequest.cpp talks to Mojo through
platform/payments/PaymentRequestProxy.h, because Blink can use Mojo only
through platform/.

The platform/payments/ directory contains data structures that mirror
the dictionaries and interfaces in modules/payments/, because platform/
cannot include modules/. These files are named "PlatformFoo" to avoid
conflicts with files named "Foo" in modules/payments/.

The Mojo interface is defined in
components/payments/payment_request.mojom.h, which is a new dependency
for platform/payments/.

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

to

==========
PaymentRequest Mojo bindings.

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

[please use gerrit instead, 2016-03-17 19:34:30]

rouslan@chromium.org changed reviewers:
+ yzshen@chromium.org

[please use gerrit instead, 2016-03-17 19:34:31]

+yzshen@

https://codereview.chromium.org/1753543002/diff/360001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/wtf_string_serialization.cc (right):

https://codereview.chromium.org/1753543002/diff/360001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_string_serialization.cc:90:
DCHECK_EQ(utf8_adaptors.front().original_input,
yzshen@: Why would this DCHECK?

For context, here's the JavaScript code that triggers it in my patch:

    var request = new PaymentRequest(
            ["foo"],
            {
                "items": [{"id": "total", "label": "Total", "amount":
{"currencyCode": "USD", "value": "10.00"}}],
                "shippingOptions": [{"id": "self", "label": "Pick up", "amount":
{"currencyCode": "USD", "value": "-5.00"}}]
            },
            {"requestShipping": true},
            {"foo": {"gateway": "bar"}});
    request.show();

The exception stack trace in debug mode:

[1:1:0317/111607:FATAL:wtf_string_serialization.cc(91)] Check failed:
utf8_adaptors.front().original_input == reinterpret_cast<uintptr_t>(&input)
(20812052480624 vs. 140734308029200) string: 'USD'
#0 0x7f6130d1200e base::debug::StackTrace::StackTrace()
#1 0x7f6130d6d43f logging::LogMessage::~LogMessage()
#2 0x7f611cd4296d WTF::Serialize_()
#3 0x7f611cd343e9 blink::mojom::Serialize_()
#4 0x7f611cd34ee1 blink::mojom::Serialize_()
#5 0x7f611cd3ed45 mojo::internal::ArraySerializer<>::SerializeCaller<>::Run()
#6 0x7f611cd3ea06 mojo::internal::ArraySerializer<>::SerializeElements()
#7 0x7f611cd3e86e mojo::internal::ArraySerializationStrategy<>::Serialize<>()
#8 0x7f611cd3e6bf mojo::internal::ArraySerializationImpl<>::Serialize<>()
#9 0x7f611cd381f6 mojo::SerializeArray_<>()
#10 0x7f611cd314d6 blink::mojom::Serialize_()
#11 0x7f611cd30e32 blink::mojom::PaymentRequestProxy::Show()
#12 0x7f611c795815 blink::PaymentRequest::show()
#13 0x7f611ca8f624 blink::PaymentRequestV8Internal::showMethod()
#14 0x7f611ca8f585 blink::PaymentRequestV8Internal::showMethodCallback()
#15 0x7f612457db7b v8::internal::FunctionCallbackArguments::Call()
#16 0x7f61245cb651 v8::internal::(anonymous namespace)::HandleApiCallHelper<>()
#17 0x7f6124600d27 v8::internal::Builtin_Impl_HandleApiCall()
#18 0x7f61245dc219 v8::internal::Builtin_HandleApiCall()

[yzshen1, 2016-03-17 21:41:17]

https://codereview.chromium.org/1753543002/diff/360001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/wtf_string_serialization.cc (right):

https://codereview.chromium.org/1753543002/diff/360001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_string_serialization.cc:90:
DCHECK_EQ(utf8_adaptors.front().original_input,
I will look into it. Would you please tell me the repo steps (what patch to
apply, what target I should build and run)?

[please use gerrit instead, 2016-03-17 21:48:26]

On 2016/03/17 21:41:17, yzshen1 wrote:
>
https://codereview.chromium.org/1753543002/diff/360001/mojo/public/cpp/bindin...
> File mojo/public/cpp/bindings/lib/wtf_string_serialization.cc (right):
> 
>
https://codereview.chromium.org/1753543002/diff/360001/mojo/public/cpp/bindin...
> mojo/public/cpp/bindings/lib/wtf_string_serialization.cc:90:
> DCHECK_EQ(utf8_adaptors.front().original_input,
> I will look into it. Would you please tell me the repo steps (what patch to
> apply, what target I should build and run)?

$ git new-branch payments
$ git cl patch 1753543002
$ build/gyp_chromium
$ ninja -j100 -Cout/Release  blink_tests &&
third_party/WebKit/Tools/Scripts/run-webkit-tests payment

[please use gerrit instead, 2016-03-17 21:52:57]

On 2016/03/17 21:48:26, Rouslan wrote:
> On 2016/03/17 21:41:17, yzshen1 wrote:
> >
>
https://codereview.chromium.org/1753543002/diff/360001/mojo/public/cpp/bindin...
> > File mojo/public/cpp/bindings/lib/wtf_string_serialization.cc (right):
> > 
> >
>
https://codereview.chromium.org/1753543002/diff/360001/mojo/public/cpp/bindin...
> > mojo/public/cpp/bindings/lib/wtf_string_serialization.cc:90:
> > DCHECK_EQ(utf8_adaptors.front().original_input,
> > I will look into it. Would you please tell me the repo steps (what patch to
> > apply, what target I should build and run)?
> 
> $ git new-branch payments
> $ git cl patch 1753543002
> $ build/gyp_chromium
> $ ninja -j100 -Cout/Release  blink_tests &&
> third_party/WebKit/Tools/Scripts/run-webkit-tests payment

Doh, need a debug build to trigger the dcheck:

$ ninja -j100 -Cout/Debug blink_tests
$ third_party/WebKit/Tools/Scripts/run-webkit-tests --debug payment

[please use gerrit instead, 2016-03-18 00:14:39]

Chris, ptal
third_party/WebKit/public/platform/modules/payments/payment_request.mojom in
patch 22.

Blink-side validation code for region, language, script, and currency is in:

third_party/WebKit/Source/modules/payments/PaymentRequest.cpp
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp

[please use gerrit instead, 2016-03-18 00:15:13]

Elliott, mojo integration architecture ptal.

[please use gerrit instead, 2016-03-18 00:15:27]

On 2016/03/18 00:15:13, Rouslan wrote:
> Elliott, mojo integration architecture ptal.

In patch 22.

[please use gerrit instead, 2016-03-18 00:16:01]

Marijn, owners ptal third_party/WebKit/Source/modules/payments/* in patch 22.

[esprehn, 2016-03-18 00:20:50]

lgtm, does this need unit tests? Can you make those Ptr objects to construct the
objects without having a mojo connection for writing tests?

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

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:28: bool
isValidCurrencyCode(const String& code, ExceptionState& exceptionState)
Is this all covered by tests? Seems like we should write unit tests.

[please use gerrit instead, 2016-03-18 00:28:36]

On 2016/03/18 00:20:50, esprehn wrote:
> unit tests

On it.

[please use gerrit instead, 2016-03-18 00:36:58]

On 2016/03/18 00:20:50, esprehn wrote:
> Can you make those Ptr objects to construct the
> objects without having a mojo connection for writing tests?

Yes.

[groby-ooo-7-16, 2016-03-18 00:47:24]

groby@chromium.org changed reviewers:
+ groby@chromium.org

[groby-ooo-7-16, 2016-03-18 00:47:26]

Validation nitpick driveby :)

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

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:65: bool isDigit =
amount[i] >= '0' && amount[i] <= '9';
isdigit? (Well, OK, isASCIIDigit - I forgot we're using AsciiType.h)

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:67:
exceptionState.throwTypeError("'" + amount + "' is not a valid ISO 200222
CurrencyAnd30Amount currency amount, should contain only '-', '.', and [0-9]");
CurrencyAnd30Amount? (Here and elsewhere)

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/ShippingAddress.cpp (right):

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:10: bool
isValidRegionCodeString(const String& code)
Please add comments on what _kind_ of region codes those are. This looks like
ISO 3166-1, except you're not checking for lowers.

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:15: for (size_t i
= 0; i < code.length(); ++i) {
I take it String doesn't have iterators, so no range loops?

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:16: if (code[i] <
'A' || code[i] > 'Z')
Probably isalpha() instead, unless there's a reason to stay with upper case, in
which case isupper

Which simplifies the whole thing to
  return (code.length()==2) && isupper(code[0]) && isupper(code[1]);

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:28: if
(code.length() != 2 && code.length() != 3)
ISO639, I take it? (Although, again, that would allow alpha, not only lower)

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:32: if (code[i] <
'a' || code[i] > 'z')
islower

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:39: bool
isValidScriptCodeString(const String& code)
ISO 15924 ?

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:47: if (code[0] <
'A' || code[0] > 'A')
Technically, that's again isalpha for all 4.

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:71: ,
m_validRegionCode(isValidRegionCodeString(address->region_code))
Assuming we're doing the validation for security reasons, shouldn't we never
copy those strings if they fail validation?

Also, would this be better off as a mojo message validator?

[Marijn Kruisselbrink, 2016-03-18 01:39:49]

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

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:45: bool
isValidAmount(const String& amount, ExceptionState& exceptionState)
This doesn't seem to exactly match the (informative) regex in the PaymentRequest
spec. The spec requires at least one digit before a period (and at least one
digit in general), and if a period is present it also requires at least one
digit after that period.
I guess it does match the normative description of amounts in the spec, so maybe
it is correct (but then I'd say the informative note describing the format in
the spec is somewhat misleading).

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:107: if
(item.amount().hasValue() && isValidAmount(item.amount().value(),
exceptionState))
Should this be !isValidAmount?
And some kind of tests to catch that indeed...

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:142: return
m_showResolver->promise().rejectWithDOMException(scriptState,
DOMException::create(InvalidStateError, "Already called show() once"));
This seems weird. rejectWithDOMException is a static method, so why are you
calling it on an instance?

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:147: return
m_showResolver->promise().rejectWithDOMException(scriptState,
DOMException::create(SyntaxError, "Not implemented on this platform"));
Same here, should just be ScriptPromise::rejectWithDOMException, especially
because m_showResolver is known to be null here.
And does SyntaxError make sense? Maybe just keep it as NotSupportedError?

Actually, this entire check doesn't make sense mojo::GetProxy will always set
m_paymentProvider to a non-null value. If you want to catch failure to connect
to the remote service you should install a connection error handler on the
InterfacePtr

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:179: return
m_completeResolver->promise().rejectWithDOMException(scriptState,
DOMException::create(InvalidStateError, "Already called complete() once"));
Same static method call thing.

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/ShippingAddress.cpp (right):

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:16: if (code[i] <
'A' || code[i] > 'Z')
On 2016/03/18 at 00:47:26, groby wrote:
> Probably isalpha() instead, unless there's a reason to stay with upper case,
in which case isupper
> 
> Which simplifies the whole thing to
>   return (code.length()==2) && isupper(code[0]) && isupper(code[1]);

isASCIIUpper(), rather than isupper(). std::isupper() takes the current locale
into account, which is definitely not something we want here.

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:32: if (code[i] <
'a' || code[i] > 'z')
On 2016/03/18 at 00:47:26, groby wrote:
> islower

isASCIILower()

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:47: if (code[0] <
'A' || code[0] > 'A')
I assume you didn't mean to write "if (code[0] != 'A')" ?

[haraken, 2016-03-18 09:19:45]

The architectural part LGTM.

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

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:31:
exceptionState.throwTypeError("'" + code + "' is not a valid ISO 4217 currency
code, should be 3 letters");

Just to confirm: Maybe should this be a DOMException? It depends on the spec.

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:48:
exceptionState.throwTypeError("'" + amount + "' is not a valid ISO 200222
CurrencyAnd30Amount currency amount, should be nonempty");

Ditto.

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:53:
exceptionState.throwTypeError("'" + amount + "' is not a valid ISO 200222
CurrencyAnd30Amount currency amount, should be at most 32 characters");

Ditto.

[rwlbuis, 2016-03-18 14:37:57]

rob.buis@samsung.com changed reviewers:
+ rob.buis@samsung.com

[rwlbuis, 2016-03-18 14:37:59]

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/ShippingAddress.cpp (right):

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:10: bool
isValidRegionCodeString(const String& code)
Why not make all three helpers static?

[please use gerrit instead, 2016-03-18 23:39:12]

Description was changed from

==========
PaymentRequest Mojo bindings.

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

to

==========
PaymentRequest Mojo bindings.

BUG=587995
TEST=PaymentsValidatorsTest.cpp
TEST=payment-request-interface.html
==========

[haraken, 2016-03-19 00:44:53]

BTW, where is the unit-tests/layout-tests for the Payment feature going to be
written?

[please use gerrit instead, 2016-03-21 16:02:41]

On 2016/03/19 00:44:53, haraken wrote:
> BTW, where is the unit-tests/layout-tests for the Payment feature going to be
> written?

Writing the unit tests now. Layout test is here:

https://codereview.chromium.org/1753543002/diff/520001/third_party/WebKit/Lay...

[groby-ooo-7-16, 2016-03-21 19:36:27]

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

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:45: bool
isValidAmount(const String& amount, ExceptionState& exceptionState)
On 2016/03/18 01:39:48, Marijn Kruisselbrink wrote:
> This doesn't seem to exactly match the (informative) regex in the
PaymentRequest
> spec. The spec requires at least one digit before a period (and at least one
> digit in general), and if a period is present it also requires at least one
> digit after that period.
> I guess it does match the normative description of amounts in the spec, so
maybe
> it is correct (but then I'd say the informative note describing the format in
> the spec is somewhat misleading).

Hm. If we have a regexp, why not use ScriptRegex and be done?

[please use gerrit instead, 2016-03-21 22:28:22]

Description was changed from

==========
PaymentRequest Mojo bindings.

BUG=587995
TEST=PaymentsValidatorsTest.cpp
TEST=payment-request-interface.html
==========

to

==========
PaymentRequest Mojo bindings.

BUG=587995
TEST=PaymentResponseTest.cpp
TEST=PaymentsValidatorsTest.cpp
TEST=ShippingAddressTest.cpp
TEST=payment-request-interface.html
==========

[please use gerrit instead, 2016-03-21 23:40:56]

Description was changed from

==========
PaymentRequest Mojo bindings.

BUG=587995
TEST=PaymentResponseTest.cpp
TEST=PaymentsValidatorsTest.cpp
TEST=ShippingAddressTest.cpp
TEST=payment-request-interface.html
==========

to

==========
PaymentRequest Mojo bindings.

BUG=587995
TEST=PaymentRequestTest.cpp
TEST=PaymentResponseTest.cpp
TEST=PaymentsValidatorsTest.cpp
TEST=ShippingAddressTest.cpp
TEST=payment-request-interface.html
==========

[please use gerrit instead, 2016-03-23 00:14:52]

groby@, mek@, rwlbuis@, haraken@, ptal patch 33.

I've written several tests:
 - PaymentRequestTest.cpp
 - PaymentResponseTest.cpp
 - PaymentsValidatorsTest.cpp
 - ShippingAddressTest.cpp
 - payment-request-interface.html

Using ScriptRegexp instead of manual validation.

I'd like to defer injecting custom validators into mojo until a later patch,
because this one has grown to ~1.5k lines now.

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

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:28: bool
isValidCurrencyCode(const String& code, ExceptionState& exceptionState)
On 2016/03/18 00:20:50, esprehn wrote:
> Is this all covered by tests? Seems like we should write unit tests.

Covered in PaymentRequestDetailsTest.

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:31:
exceptionState.throwTypeError("'" + code + "' is not a valid ISO 4217 currency
code, should be 3 letters");
On 2016/03/18 09:19:44, haraken wrote:
> 
> Just to confirm: Maybe should this be a DOMException? It depends on the spec.
> 

Spec does not touch on this yet. It's using a lot of TypeErrors, so I decided to
use that here as well, for now.

https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#paymentre...
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#currencya...

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:45: bool
isValidAmount(const String& amount, ExceptionState& exceptionState)
On 2016/03/21 19:36:26, groby wrote:
> On 2016/03/18 01:39:48, Marijn Kruisselbrink wrote:
> > This doesn't seem to exactly match the (informative) regex in the
> PaymentRequest
> > spec. The spec requires at least one digit before a period (and at least one
> > digit in general), and if a period is present it also requires at least one
> > digit after that period.
> > I guess it does match the normative description of amounts in the spec, so
> maybe
> > it is correct (but then I'd say the informative note describing the format
in
> > the spec is somewhat misleading).
> 
> Hm. If we have a regexp, why not use ScriptRegex and be done?

Using ScriptRegexp with the exact regular expression from the spec. Tested in
PaymentsValidatorsTest.

The spec does not limit the total number of digits as suggested in some of the
discussions inside of W3C. I will keep an eye on the spec and update this
regular expression if the situation changes. Should not be an issue for Chrome,
because we don't ever need to parse this (e.g., into int64).

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:48:
exceptionState.throwTypeError("'" + amount + "' is not a valid ISO 200222
CurrencyAnd30Amount currency amount, should be nonempty");
On 2016/03/18 09:19:44, haraken wrote:
> 
> Ditto.

Acknowledged.

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:53:
exceptionState.throwTypeError("'" + amount + "' is not a valid ISO 200222
CurrencyAnd30Amount currency amount, should be at most 32 characters");
On 2016/03/18 09:19:44, haraken wrote:
> 
> Ditto.

Acknowledged.

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:65: bool isDigit =
amount[i] >= '0' && amount[i] <= '9';
On 2016/03/18 00:47:25, groby wrote:
> isdigit? (Well, OK, isASCIIDigit - I forgot we're using AsciiType.h)

Using the regex from the spec instead.

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:67:
exceptionState.throwTypeError("'" + amount + "' is not a valid ISO 200222
CurrencyAnd30Amount currency amount, should contain only '-', '.', and [0-9]");
On 2016/03/18 00:47:25, groby wrote:
> CurrencyAnd30Amount? (Here and elsewhere)

That's a newer replacement for ISO 4217 that our W3C peers suggest we should be
using in the spec.

http://www.iso20022.org/standardsrepository/public/wqt/Description/mx/dico/da...

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:107: if
(item.amount().hasValue() && isValidAmount(item.amount().value(),
exceptionState))
On 2016/03/18 01:39:48, Marijn Kruisselbrink wrote:
> Should this be !isValidAmount?
> And some kind of tests to catch that indeed...

Good catch. Tested and fixed.

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:142: return
m_showResolver->promise().rejectWithDOMException(scriptState,
DOMException::create(InvalidStateError, "Already called show() once"));
On 2016/03/18 01:39:48, Marijn Kruisselbrink wrote:
> This seems weird. rejectWithDOMException is a static method, so why are you
> calling it on an instance?

Did not realize it's static. Calling it properly now.

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:147: return
m_showResolver->promise().rejectWithDOMException(scriptState,
DOMException::create(SyntaxError, "Not implemented on this platform"));
On 2016/03/18 01:39:48, Marijn Kruisselbrink wrote:
> Same here, should just be ScriptPromise::rejectWithDOMException, especially
> because m_showResolver is known to be null here.
> And does SyntaxError make sense? Maybe just keep it as NotSupportedError?
> 
> Actually, this entire check doesn't make sense mojo::GetProxy will always set
> m_paymentProvider to a non-null value. If you want to catch failure to connect
> to the remote service you should install a connection error handler on the
> InterfacePtr

Removed the check from this patch. Not writing connection error handler in this
patch in the interest of patch size.

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:179: return
m_completeResolver->promise().rejectWithDOMException(scriptState,
DOMException::create(InvalidStateError, "Already called complete() once"));
On 2016/03/18 01:39:48, Marijn Kruisselbrink wrote:
> Same static method call thing.

Done.

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/ShippingAddress.cpp (right):

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:10: bool
isValidRegionCodeString(const String& code)
On 2016/03/18 14:37:59, rwlbuis wrote:
> Why not make all three helpers static?

Done.

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:10: bool
isValidRegionCodeString(const String& code)
On 2016/03/18 00:47:26, groby wrote:
> Please add comments on what _kind_ of region codes those are. This looks like
> ISO 3166-1, except you're not checking for lowers.

This comes from region_data_constants.cc in Chrome (for now). These are all
upper case in Chrome.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libadd...

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:15: for (size_t i
= 0; i < code.length(); ++i) {
On 2016/03/18 00:47:25, groby wrote:
> I take it String doesn't have iterators, so no range loops?

That's right.

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:16: if (code[i] <
'A' || code[i] > 'Z')
On 2016/03/18 00:47:26, groby wrote:
> Probably isalpha() instead, unless there's a reason to stay with upper case,
in
> which case isupper
> 
> Which simplifies the whole thing to
>   return (code.length()==2) && isupper(code[0]) && isupper(code[1]);

Using ScriptRegexp instead.

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:16: if (code[i] <
'A' || code[i] > 'Z')
On 2016/03/18 01:39:48, Marijn Kruisselbrink wrote:
> On 2016/03/18 at 00:47:26, groby wrote:
> > Probably isalpha() instead, unless there's a reason to stay with upper case,
> in which case isupper
> > 
> > Which simplifies the whole thing to
> >   return (code.length()==2) && isupper(code[0]) && isupper(code[1]);
> 
> isASCIIUpper(), rather than isupper(). std::isupper() takes the current locale
> into account, which is definitely not something we want here.

Using ScriptRegexp instead.

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:28: if
(code.length() != 2 && code.length() != 3)
On 2016/03/18 00:47:26, groby wrote:
> ISO639, I take it? (Although, again, that would allow alpha, not only lower)

This comes from region_data_constants.cc in Chrome (for now), which has
https://tools.ietf.org/html/bcp47 language tags. The first part of the BCP 47
language tag is the shortest ISO 639 language code. These are all lower case in
region_data_constants.cc.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libadd...

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:32: if (code[i] <
'a' || code[i] > 'z')
On 2016/03/18 00:47:26, groby wrote:
> islower

Using ScriptRegexp instead.

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:32: if (code[i] <
'a' || code[i] > 'z')
On 2016/03/18 00:47:26, groby wrote:
> islower

Using ScriptRegexp instead.

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:39: bool
isValidScriptCodeString(const String& code)
On 2016/03/18 00:47:26, groby wrote:
> ISO 15924 ?

Added comments and error message strings that specify the ISO.

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:47: if (code[0] <
'A' || code[0] > 'A')
On 2016/03/18 00:47:25, groby wrote:
> Technically, that's again isalpha for all 4. 

Using ScriptRegexp instead.

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:47: if (code[0] <
'A' || code[0] > 'A')
On 2016/03/18 01:39:48, Marijn Kruisselbrink wrote:
> I assume you didn't mean to write "if (code[0] != 'A')" ?

Fixed, tested in PaymentsValidatorsTest.cpp, and switched to ScriptRegexp in the
end.

https://codereview.chromium.org/1753543002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:71: ,
m_validRegionCode(isValidRegionCodeString(address->region_code))
On 2016/03/18 00:47:26, groby wrote:
> Assuming we're doing the validation for security reasons, shouldn't we never
> copy those strings if they fail validation?

Moved validation to PaymentRequest::onShippingAddressChanged(), before copying
the strings.

> Also, would this be better off as a mojo message validator?

Going to consider mojo validators in follow up patches. This patch has grown to
~1.5k lines already.

[Marijn Kruisselbrink, 2016-03-25 17:13:31]

https://codereview.chromium.org/1753543002/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/MockPaymentRequest.h (right):

https://codereview.chromium.org/1753543002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/MockPaymentRequest.h:30: enum
ModificaitonType {
ModificationType

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

https://codereview.chromium.org/1753543002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:30: bool
areValidItems(HeapVector<T> items, ExceptionState& exceptionState)
Maybe add a comment explaining why this is a template function/what types this
is used for?

https://codereview.chromium.org/1753543002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:73: template
<typename Output, typename Input>
Same here, without already knowing the rest of the code it is unclear why this
is a template/what types this is used for.

https://codereview.chromium.org/1753543002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:83:
mojom::wtf::PaymentDetailsPtr toMojoPaymentDetails(const PaymentDetails& input)
All these "toMojo*" methods should probably be specializations of
mojo::TypeConverter, rather than being your own custom methods (at least that's
my understanding of what "typical" mojo usage looks like/does for conversions
to/from mojo types/structs).

https://codereview.chromium.org/1753543002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:138:
ASSERT(!m_paymentProvider.is_bound());
DCHECK?

https://codereview.chromium.org/1753543002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:139:
blink::Platform::current()->connectToRemoteService(mojo::GetProxy(&m_paymentProvider));
Maybe add a TODO to handle errors for the two mojo connections?

https://codereview.chromium.org/1753543002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:203:
exceptionState.throwTypeError("Must specify at least one payment methods
identifiers");
"one payment methods identifiers" doesn't seem like correct English. "one
payment method identifier" would perhaps be better?

https://codereview.chromium.org/1753543002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:310:
m_paymentProvider.reset();
I wonder if wherever the connection to the paymentProvider is closed you also
want to call Close() on the client binding? It probably doesn't matter, since
it's likely that closing the connection to the provider will cause that provider
to also close the connection to the client, but might still be cleaner to be
explicit.

https://codereview.chromium.org/1753543002/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentResponse.cpp (right):

https://codereview.chromium.org/1753543002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentResponse.cpp:21:
ASSERT(paymentRequest);
DCHECK? (probably applies to all ASSERTs)

[please use gerrit instead, 2016-03-25 19:26:37]

mek@, ptal patch 39.

https://codereview.chromium.org/1753543002/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/MockPaymentRequest.h (right):

https://codereview.chromium.org/1753543002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/MockPaymentRequest.h:30: enum
ModificaitonType {
On 2016/03/25 17:13:30, Marijn Kruisselbrink wrote:
> ModificationType

Done.

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

https://codereview.chromium.org/1753543002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:30: bool
areValidItems(HeapVector<T> items, ExceptionState& exceptionState)
On 2016/03/25 17:13:31, Marijn Kruisselbrink wrote:
> Maybe add a comment explaining why this is a template function/what types this
> is used for?

Done.

https://codereview.chromium.org/1753543002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:73: template
<typename Output, typename Input>
On 2016/03/25 17:13:30, Marijn Kruisselbrink wrote:
> Same here, without already knowing the rest of the code it is unclear why this
> is a template/what types this is used for.

Done.

https://codereview.chromium.org/1753543002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:83:
mojom::wtf::PaymentDetailsPtr toMojoPaymentDetails(const PaymentDetails& input)
On 2016/03/25 17:13:30, Marijn Kruisselbrink wrote:
> All these "toMojo*" methods should probably be specializations of
> mojo::TypeConverter, rather than being your own custom methods (at least
that's
> my understanding of what "typical" mojo usage looks like/does for conversions
> to/from mojo types/structs).

Done.

https://codereview.chromium.org/1753543002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:138:
ASSERT(!m_paymentProvider.is_bound());
On 2016/03/25 17:13:31, Marijn Kruisselbrink wrote:
> DCHECK?

Done.

https://codereview.chromium.org/1753543002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:139:
blink::Platform::current()->connectToRemoteService(mojo::GetProxy(&m_paymentProvider));
On 2016/03/25 17:13:30, Marijn Kruisselbrink wrote:
> Maybe add a TODO to handle errors for the two mojo connections?

Done.

https://codereview.chromium.org/1753543002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:203:
exceptionState.throwTypeError("Must specify at least one payment methods
identifiers");
On 2016/03/25 17:13:30, Marijn Kruisselbrink wrote:
> "one payment methods identifiers" doesn't seem like correct English. "one
> payment method identifier" would perhaps be better?

Done.

https://codereview.chromium.org/1753543002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:310:
m_paymentProvider.reset();
On 2016/03/25 17:13:30, Marijn Kruisselbrink wrote:
> I wonder if wherever the connection to the paymentProvider is closed you also
> want to call Close() on the client binding? It probably doesn't matter, since
> it's likely that closing the connection to the provider will cause that
provider
> to also close the connection to the client, but might still be cleaner to be
> explicit.

Done.

https://codereview.chromium.org/1753543002/diff/740001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentResponse.cpp (right):

https://codereview.chromium.org/1753543002/diff/740001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentResponse.cpp:21:
ASSERT(paymentRequest);
On 2016/03/25 17:13:31, Marijn Kruisselbrink wrote:
> DCHECK? (probably applies to all ASSERTs)

Done.

[please use gerrit instead, 2016-03-25 19:35:39]

groby@, rwlbuis@, haraken@, ping. Please let me know whether I have adequately
addressed your comments. (patch 39)

[rwlbuis, 2016-03-25 22:14:41]

@Rouslan my previous comment is addressed. Patch looks good to me, found one
possible improvement.

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

https://codereview.chromium.org/1753543002/diff/780001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:306: }
Looks like you can combine above three if blocks into one if with three checks.

[please use gerrit instead, 2016-03-25 22:41:31]

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

https://codereview.chromium.org/1753543002/diff/780001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:306: }
On 2016/03/25 22:14:41, rwlbuis wrote:
> Looks like you can combine above three if blocks into one if with three
checks.

Done.

[Marijn Kruisselbrink, 2016-03-25 22:46:25]

lgtm with nits

but could you update the description to be a bit more descriptive of what this
patch actually does and doesn't implement?

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

https://codereview.chromium.org/1753543002/diff/780001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:70:
output->items[i] = mojom::wtf::PaymentItem::From(input.items()[i]);
hmm, unfortunate that there don't seem to be default mojo::TypeConverter
implementations for mojo::WTFArray like there are for mojo::Array (to convert
from a std::vector<T> to a mojo::Array<U> you can simple call
mojo::Array<U>::From(vector), which will use mojo::TypeConverter<U, V> under the
hood).

Maybe that's just an oversight, and wtf_array.h should have similar
mojo::TypeConverter definition to what mojo::Array has (or maybe I'm missing
something and the ones for WTFArray are defined somewhere else).

https://codereview.chromium.org/1753543002/diff/780001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp (right):

https://codereview.chromium.org/1753543002/diff/780001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:101:
EXPECT_LE(2U, details.shippingOptions().size());
This is testing that MockPaymentRequest returns data that is suitable for this
test? I suppose that's fine, but maybe move the check to just after getting the
PaymentDetails creating, to make clear it isn't actually testing anything
related to creating the PaymentRequest.

[esprehn, 2016-03-25 22:50:37]

Please link to the spec or explainer in the patch description and explain what
this patch does. Any large patch needs a decent description.

[please use gerrit instead, 2016-03-25 23:35:31]

Description was changed from

==========
PaymentRequest Mojo bindings.

BUG=587995
TEST=PaymentRequestTest.cpp
TEST=PaymentResponseTest.cpp
TEST=PaymentsValidatorsTest.cpp
TEST=ShippingAddressTest.cpp
TEST=payment-request-interface.html
==========

to

==========
PaymentRequest Mojo bindings.

The Mojo (new IPC replacement) bindings for PaymentRequest Blink API.
Blink uses the bindings to procure the functionality for the API. Mojo
eliminates multiple layers of abstraction, letting the embedder provide
the functionality by implementing the PaymentRequest interface in C++ or
Java for Android. The PaymentRequest interface lives in:

third_party/Webkit/public/platform/modules/payments/payment_request.mojom

The embedder does not provide any functionality in this patch.

PaymentRequest draft spec:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html

BUG=587995
TEST=PaymentRequestTest.cpp
TEST=PaymentResponseTest.cpp
TEST=PaymentsValidatorsTest.cpp
TEST=ShippingAddressTest.cpp
TEST=payment-request-interface.html
==========

[please use gerrit instead, 2016-03-25 23:37:31]

Description was changed from

==========
PaymentRequest Mojo bindings.

The Mojo (new IPC replacement) bindings for PaymentRequest Blink API.
Blink uses the bindings to procure the functionality for the API. Mojo
eliminates multiple layers of abstraction, letting the embedder provide
the functionality by implementing the PaymentRequest interface in C++ or
Java for Android. The PaymentRequest interface lives in:

third_party/Webkit/public/platform/modules/payments/payment_request.mojom

The embedder does not provide any functionality in this patch.

PaymentRequest draft spec:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html

BUG=587995
TEST=PaymentRequestTest.cpp
TEST=PaymentResponseTest.cpp
TEST=PaymentsValidatorsTest.cpp
TEST=ShippingAddressTest.cpp
TEST=payment-request-interface.html
==========

to

==========
PaymentRequest Mojo bindings.

The Mojo (new IPC replacement) bindings for PaymentRequest Blink API.
Blink uses the bindings to procure the functionality for the API. Mojo
eliminates multiple layers of abstraction, letting the embedder provide
the functionality by implementing the PaymentRequest interface in C++ or
Java for Android. The PaymentRequest interface lives in:

third_party/Webkit/public/platform/modules/payments/payment_request.mojom

The embedder does not provide any functionality in this patch.

PaymentRequest draft spec:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html

BUG=587995
TEST=PaymentRequestTest.cpp
TEST=PaymentResponseTest.cpp
TEST=PaymentsValidatorsTest.cpp
TEST=ShippingAddressTest.cpp
TEST=payment-request-interface.html
==========

[esprehn, 2016-03-25 23:40:12]

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/MockPaymentRequest.h (right):

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/MockPaymentRequest.h:17: DetailNone,
these are all global, we'd usually pick more unique names or make them emum
classes. I guess maybe it's okay for tests...

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/MockPaymentRequest.h:37: static
PaymentDetails buildDetails(DetailToChange = DetailNone, DataToChange =
DataNone, ModificationType = OverwriteValue, const String& valueToUse =
String());
hmm, why does this static method belong here? Seems like it should just be a
free function called createPaymentDetailsForTest() ?

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

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:75:
output->shipping_options[i] =
mojom::wtf::ShippingOption::From(input.shippingOptions()[i]);
if we're going to be writing a lot of these copy ::From() loops we probably need
a templateized helper.

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:99: bool
areValidItems(HeapVector<T> items, ExceptionState& exceptionState)
hmm what two things have the exact same fields below that require using
templates?

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:139: return true;
you actually don't need the boolean, you can just check hadException(), then you
can join all the if statements in here to be:

if (...) {

} else if (...) {

} ...

and once you hit one you'll then fall out of the function and the caller checks
hadException(). 

Then you name this function something like validateFoo()

you could even just return exceptionState.hadException() on the last line if you
still want the boolean.

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:173: //
TODO(rouslan): Handle connection errors.
What's the correct way to handle this? Often todo's like this end up sticking
around for very long periods of time.

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:258:
RefPtr<JSONValue> value = toJSONValue(data.context(), data.v8Value());
this only returns null if maxDepth is reached btw

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:268:
exceptionState.throwTypeError("'" + paymentMethodSpecificKeyValue.key + "'
should match one of the payment method identifiers");
hmm, if we have real requirements on the |data| why isn't it a dictionary?

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:271: if
(!paymentMethodSpecificKeyValue.value ||
paymentMethodSpecificKeyValue.value->isNull() ||
paymentMethodSpecificKeyValue.value->getType() != JSONValue::TypeObject) {
ditto, all these null checks are kind of suspect. Should toJSONValue just return
JSONValue::null() instead?

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:277:
m_stringifiedData = jsonData->toJSONString();
why can't we reuse the data? This code is doing string -> json -> string

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:281: if
(details.hasShippingOptions() && details.shippingOptions().size() == 1)
what happens if you specified more than one? It seems like we just ignore it
entirely. Is that right?

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

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.h:31: class
MODULES_EXPORT PaymentRequest : public
RefCountedGarbageCollectedEventTargetWithInlineData<PaymentRequest> ,
WTF_NON_EXPORTED_BASE(public mojom::wtf::PaymentRequestClient) {
why not final? I'm not a big fan of un-finaling for tests, can we write the test
a different way? Maybe it's okay :/

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp (right):

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:131: class
DetailsTestCase {
lets put these in separate files, mega unit test files are hard to maintain

[esprehn, 2016-03-25 23:48:40]

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/MockPaymentRequest.cpp (right):

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/MockPaymentRequest.cpp:87:
ON_CALL(*this, complete(testing::_, testing::_))
Yeah I really think we want to either mock some pure virtual interface or expose
a test function. Removing the final and having this subclass doesn't feel right.

[Marijn Kruisselbrink, 2016-03-25 23:59:40]

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

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:75:
output->shipping_options[i] =
mojom::wtf::ShippingOption::From(input.shippingOptions()[i]);
On 2016/03/25 at 23:40:11, esprehn wrote:
> if we're going to be writing a lot of these copy ::From() loops we probably
need a templateized helper.

See also my earlier comment; mojo::Array has these templateized helpers already,
it seems to just be an oversight that mojo::WTFArray doesn't.

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:139: return true;
On 2016/03/25 at 23:40:11, esprehn wrote:
> you actually don't need the boolean, you can just check hadException(), then
you can join all the if statements in here to be:
> 
> if (...) {
> 
> } else if (...) {
> 
> } ...
> 
> and once you hit one you'll then fall out of the function and the caller
checks hadException(). 
> 
> Then you name this function something like validateFoo()
> 
> you could even just return exceptionState.hadException() on the last line if
you still want the boolean.

of course you'd still need to return as soon as you come across an individual
invalid item. Otherwise ExceptionState will end up holding the last thrown error
rather than the first.

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:268:
exceptionState.throwTypeError("'" + paymentMethodSpecificKeyValue.key + "'
should match one of the payment method identifiers");
On 2016/03/25 at 23:40:11, esprehn wrote:
> hmm, if we have real requirements on the |data| why isn't it a dictionary?

a WebIDL dictionary has a fixed, hardcoded set of keys. The |data| here doesn't
have a fixed set of allowed keys. It just has the requirement that its keys
match values in the supportedMethods parameter (not sure why the spec even has
that separate supportedMethods parameter though, it seems like it could just
reuse the details object to also get the supported methods).

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:277:
m_stringifiedData = jsonData->toJSONString();
On 2016/03/25 at 23:40:11, esprehn wrote:
> why can't we reuse the data? This code is doing string -> json -> string

Where is it doing that? It starts as ScriptValue, and is converted to JSONObject
to get a string out of it. I suppose arguably it could do the validation on the
ScriptValue directly rather than converting to a JSONObject first, but it isn't
doing any extra conversions.

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:281: if
(details.hasShippingOptions() && details.shippingOptions().size() == 1)
On 2016/03/25 at 23:40:11, esprehn wrote:
> what happens if you specified more than one? It seems like we just ignore it
entirely. Is that right?

I was confused here for a bit too, all the options are stored in m_details. This
just sets the currently selected option if only one option was passed.

[rwlbuis, 2016-03-29 17:44:22]

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/payments/payment-request-interface.html
(right):

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/payments/payment-request-interface.html:7:
function substiute(originalObject, substiuteKeyValuePair) {
Typo: substitute substiute with substitute :)

[rwlbuis, 2016-03-29 18:26:43]

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

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:258:
RefPtr<JSONValue> value = toJSONValue(data.context(), data.v8Value());
On 2016/03/25 23:40:11, esprehn wrote:
> this only returns null if maxDepth is reached btw

And I think there is a problem with the way it is not handled when it is null. I
added an infinite JSON object to payment-request-interface.html and it is not
throwing:

['Infinite JSON value for one of the payment method specific data pieces should
throw', null, function() {
    var data = { foo: false }
    data.foo = data;
    new PaymentRequest(['foo'], buildDetails(), {}, data)
}],

[rwlbuis, 2016-03-29 19:30:46]

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

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:271: if
(!paymentMethodSpecificKeyValue.value ||
paymentMethodSpecificKeyValue.value->isNull() ||
paymentMethodSpecificKeyValue.value->getType() != JSONValue::TypeObject) {
On 2016/03/25 23:40:11, esprehn wrote:
> ditto, all these null checks are kind of suspect. Should toJSONValue just
return
> JSONValue::null() instead?

Independent of this, note that getType() != TypeObject also is false when
JSONValue is of null type, so I think you can skip the isNull check with the
code as-is.

[groby-ooo-7-16, 2016-03-29 19:43:45]

If you meant 33 - yes, LGTM :)

[rwlbuis, 2016-03-29 20:42:44]

@Rouslan I notice you already have a lot of thumbs up for this patch, since I
only have minor gripes don't let me block you. I can probably make a patch to
correct the stuff I spotted.

[rwlbuis, 2016-03-29 21:10:06]

lgtm (once it builds).

[please use gerrit instead, 2016-03-29 22:15:45]

esprehn@ & mek@, ptal patch 43.

yzshen@: ptal mojo/public/cpp/bindings/wtf_array.h in patch 43.

palmer@, ptal
third_party/WebKit/public/platform/modules/payments/payment_request.mojom in
patch 43.

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

https://codereview.chromium.org/1753543002/diff/780001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:70:
output->items[i] = mojom::wtf::PaymentItem::From(input.items()[i]);
On 2016/03/25 22:46:25, Marijn Kruisselbrink wrote:
> hmm, unfortunate that there don't seem to be default mojo::TypeConverter
> implementations for mojo::WTFArray like there are for mojo::Array (to convert
> from a std::vector<T> to a mojo::Array<U> you can simple call
> mojo::Array<U>::From(vector), which will use mojo::TypeConverter<U, V> under
the
> hood).
> 
> Maybe that's just an oversight, and wtf_array.h should have similar
> mojo::TypeConverter definition to what mojo::Array has (or maybe I'm missing
> something and the ones for WTFArray are defined somewhere else).

Added a type converter.

https://codereview.chromium.org/1753543002/diff/780001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp (right):

https://codereview.chromium.org/1753543002/diff/780001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:101:
EXPECT_LE(2U, details.shippingOptions().size());
On 2016/03/25 22:46:25, Marijn Kruisselbrink wrote:
> This is testing that MockPaymentRequest returns data that is suitable for this
> test? I suppose that's fine, but maybe move the check to just after getting
the
> PaymentDetails creating, to make clear it isn't actually testing anything
> related to creating the PaymentRequest.

Done.

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/payments/payment-request-interface.html
(right):

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/payments/payment-request-interface.html:7:
function substiute(originalObject, substiuteKeyValuePair) {
On 2016/03/29 17:44:22, rwlbuis wrote:
> Typo: substitute substiute with substitute :)

Done.

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/MockPaymentRequest.cpp (right):

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/MockPaymentRequest.cpp:87:
ON_CALL(*this, complete(testing::_, testing::_))
On 2016/03/25 23:48:39, esprehn wrote:
> Yeah I really think we want to either mock some pure virtual interface or
expose
> a test function. Removing the final and having this subclass doesn't feel
right.

Done.

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/MockPaymentRequest.h (right):

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/MockPaymentRequest.h:17: DetailNone,
On 2016/03/25 23:40:11, esprehn wrote:
> these are all global, we'd usually pick more unique names or make them emum
> classes. I guess maybe it's okay for tests...

Prefixed with PaymentTest just in case.

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/MockPaymentRequest.h:37: static
PaymentDetails buildDetails(DetailToChange = DetailNone, DataToChange =
DataNone, ModificationType = OverwriteValue, const String& valueToUse =
String());
On 2016/03/25 23:40:11, esprehn wrote:
> hmm, why does this static method belong here? Seems like it should just be a
> free function called createPaymentDetailsForTest() ?

Moved to PaymentDetailsTestHelper.

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

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:75:
output->shipping_options[i] =
mojom::wtf::ShippingOption::From(input.shippingOptions()[i]);
On 2016/03/25 23:59:39, Marijn Kruisselbrink wrote:
> On 2016/03/25 at 23:40:11, esprehn wrote:
> > if we're going to be writing a lot of these copy ::From() loops we probably
> need a templateized helper.
> 
> See also my earlier comment; mojo::Array has these templateized helpers
already,
> it seems to just be an oversight that mojo::WTFArray doesn't.

Created a helper.

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:99: bool
areValidItems(HeapVector<T> items, ExceptionState& exceptionState)
On 2016/03/25 23:40:11, esprehn wrote:
> hmm what two things have the exact same fields below that require using
> templates?

See the comment for this function: "ShippingOption and PaymentItem dictionaries,
which happen to have identical fields."

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:139: return true;
On 2016/03/25 23:59:39, Marijn Kruisselbrink wrote:
> On 2016/03/25 at 23:40:11, esprehn wrote:
> > you actually don't need the boolean, you can just check hadException(), then
> you can join all the if statements in here to be:
> > 
> > if (...) {
> > 
> > } else if (...) {
> > 
> > } ...
> > 
> > and once you hit one you'll then fall out of the function and the caller
> checks hadException(). 
> > 
> > Then you name this function something like validateFoo()
> > 
> > you could even just return exceptionState.hadException() on the last line if
> you still want the boolean.
> 
> of course you'd still need to return as soon as you come across an individual
> invalid item. Otherwise ExceptionState will end up holding the last thrown
error
> rather than the first.

Done. Note that "else if" statements are not suitable, because I am checking
"item.hasAmount()" not "item.hasValidAmount()". Having an amount does not
preclude checking the label as well, for example.

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:173: //
TODO(rouslan): Handle connection errors.
On 2016/03/25 23:40:11, esprehn wrote:
> What's the correct way to handle this? Often todo's like this end up sticking
> around for very long periods of time.

Handling it now. It was not very well documented, but reading the source code
helped me out here.

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:258:
RefPtr<JSONValue> value = toJSONValue(data.context(), data.v8Value());
On 2016/03/29 18:26:43, rwlbuis wrote:
> On 2016/03/25 23:40:11, esprehn wrote:
> > this only returns null if maxDepth is reached btw
> 
> And I think there is a problem with the way it is not handled when it is null.
I
> added an infinite JSON object to payment-request-interface.html and it is not
> throwing:
> 
> ['Infinite JSON value for one of the payment method specific data pieces
should
> throw', null, function() {
>     var data = { foo: false }
>     data.foo = data;
>     new PaymentRequest(['foo'], buildDetails(), {}, data)
> }],

Added a layout test and throwing on infinite json, which is detected here as a
null |value|.

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:268:
exceptionState.throwTypeError("'" + paymentMethodSpecificKeyValue.key + "'
should match one of the payment method identifiers");
On 2016/03/25 23:59:40, Marijn Kruisselbrink wrote:
> On 2016/03/25 at 23:40:11, esprehn wrote:
> > hmm, if we have real requirements on the |data| why isn't it a dictionary?
> 
> a WebIDL dictionary has a fixed, hardcoded set of keys. The |data| here
doesn't
> have a fixed set of allowed keys. It just has the requirement that its keys
> match values in the supportedMethods parameter.

Correct.

> (not sure why the spec even has
> that separate supportedMethods parameter though, it seems like it could just
> reuse the details object to also get the supported methods).

I think the idea is that you can get credit cards by saying
PaymentRequest(["visa", "amex", mastercard"]) as opposed to
PaymentRequest({"visa": null, "amex": null, "mastercard": null}). That's when
you don't have any special data to pass into the API.

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:271: if
(!paymentMethodSpecificKeyValue.value ||
paymentMethodSpecificKeyValue.value->isNull() ||
paymentMethodSpecificKeyValue.value->getType() != JSONValue::TypeObject) {
On 2016/03/25 23:40:11, esprehn wrote:
> ditto, all these null checks are kind of suspect. Should toJSONValue just
return
> JSONValue::null() instead?

Removed the "!paymentMethodSpecificKeyValue.value" check, because toJSONValue()
checks this for us already.

Also removed the "paymentMethodSpecificKeyValue.value->isNull()" check, because
that's covered by the "paymentMethodSpecificKeyValue.value->getType() !=
JSONValue::TypeObject" check already.

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:271: if
(!paymentMethodSpecificKeyValue.value ||
paymentMethodSpecificKeyValue.value->isNull() ||
paymentMethodSpecificKeyValue.value->getType() != JSONValue::TypeObject) {
On 2016/03/29 19:30:46, rwlbuis wrote:
> On 2016/03/25 23:40:11, esprehn wrote:
> > ditto, all these null checks are kind of suspect. Should toJSONValue just
> return
> > JSONValue::null() instead?
> 
> Independent of this, note that getType() != TypeObject also is false when
> JSONValue is of null type, so I think you can skip the isNull check with the
> code as-is.

Done.

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:277:
m_stringifiedData = jsonData->toJSONString();
On 2016/03/25 23:59:40, Marijn Kruisselbrink wrote:
> On 2016/03/25 at 23:40:11, esprehn wrote:
> > why can't we reuse the data? This code is doing string -> json -> string
> 
> Where is it doing that? It starts as ScriptValue, and is converted to
JSONObject
> to get a string out of it. I suppose arguably it could do the validation on
the
> ScriptValue directly rather than converting to a JSONObject first, but it
isn't
> doing any extra conversions.

Acknowledged.

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:281: if
(details.hasShippingOptions() && details.shippingOptions().size() == 1)
On 2016/03/25 23:59:40, Marijn Kruisselbrink wrote:
> On 2016/03/25 at 23:40:11, esprehn wrote:
> > what happens if you specified more than one? It seems like we just ignore it
> entirely. Is that right?

If multiple options are available, the user has to pick one in browser UI. Then
onShippingOptionChange() will set the value of this member variable.

> I was confused here for a bit too, all the options are stored in m_details.
This
> just sets the currently selected option if only one option was passed.

Correct. Added a comment to that respect.

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

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.h:31: class
MODULES_EXPORT PaymentRequest : public
RefCountedGarbageCollectedEventTargetWithInlineData<PaymentRequest> ,
WTF_NON_EXPORTED_BASE(public mojom::wtf::PaymentRequestClient) {
On 2016/03/25 23:40:11, esprehn wrote:
> why not final? I'm not a big fan of un-finaling for tests, can we write the
test
> a different way? Maybe it's okay :/

Extracted complete() into a pure virtual interface, so the test can override
that instead.

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp (right):

https://codereview.chromium.org/1753543002/diff/800001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequestTest.cpp:131: class
DetailsTestCase {
On 2016/03/25 23:40:11, esprehn wrote:
> lets put these in separate files, mega unit test files are hard to maintain

Done.

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

https://codereview.chromium.org/1753543002/diff/840001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:70: output->items
= mojo::WTFArray<mojom::wtf::PaymentItemPtr>::From(static_cast<const
WTF::Vector<blink::PaymentItem, 0, blink::HeapAllocator>>(input.items()));
This is ugly, but //src/mojo/public/cpp/bindings/wtf_array.h arguably should not
contain type converters for blink::HeapVector, because it deals only with
WTF::Vector.

[yzshen1, 2016-03-29 22:49:31]

https://codereview.chromium.org/1753543002/diff/840001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/wtf_array.h (right):

https://codereview.chromium.org/1753543002/diff/840001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/wtf_array.h:242: struct TypeConverter<WTFArray<T>,
WTF::Vector<E, 0, blink::HeapAllocator>> {
Is it a common operation to use a non-default allocator? It is unfortunate
because it requires a deep copy. :/

Can this converter be generalized to have the allocator as a template parameter
as well? That way we don't have to have blink::HeapAllocator here which seems a
little weird?

An alternative is to consider putting it somewhere inside blink, because mojo
bindings itself doesn't need this converter, putting it inside blink reduces
what the bindings has to know about blink.

WDYT? Thanks!

[esprehn, 2016-03-29 22:57:07]

Hmm this goes back to the discussion we had about GCed objects in blink. This
should be a HeapVector I think? Originally we said there would be no GC'ed
objects in IPC messages. haraken@ ?

[please use gerrit instead, 2016-03-29 23:08:11]

https://codereview.chromium.org/1753543002/diff/840001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/wtf_array.h (right):

https://codereview.chromium.org/1753543002/diff/840001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/wtf_array.h:242: struct TypeConverter<WTFArray<T>,
WTF::Vector<E, 0, blink::HeapAllocator>> {
On 2016/03/29 22:49:31, yzshen1 wrote:
> Is it a common operation to use a non-default allocator?

The blink::HeapAllocator is used in blink::HeapVector<E>, which is a subclass of
WTF::Vector<E, 0, blink::HeapAllocator>. That's the type used by code generated
from WebIDL. These objects represent their JavaScript counterparts. See:

https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...

...for an example of a blink::HeapVector<E> generated from:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

> It is unfortunate
> because it requires a deep copy. :/

Why does this require a deep copy?

> Can this converter be generalized to have the allocator as a template
parameter
> as well? That way we don't have to have blink::HeapAllocator here which seems
a
> little weird?

Unfortunately, "struct TypeConverter" needs to be a template of exactly two
typenames. See:

https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...

> An alternative is to consider putting it somewhere inside blink, because mojo
> bindings itself doesn't need this converter, putting it inside blink reduces
> what the bindings has to know about blink.

I'm OK with that.

esprehn@: where would be a good place for such a common code?

[please use gerrit instead, 2016-03-29 23:10:05]

On 2016/03/29 22:57:07, esprehn wrote:
> Hmm this goes back to the discussion we had about GCed objects in blink. This
> should be a HeapVector I think?

Yes, WebIDL is generating HeapVector objects. I'm converting them to
mojo::WTFArray.

[yzshen1, 2016-03-29 23:20:16]

https://codereview.chromium.org/1753543002/diff/840001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/wtf_array.h (right):

https://codereview.chromium.org/1753543002/diff/840001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/wtf_array.h:242: struct TypeConverter<WTFArray<T>,
WTF::Vector<E, 0, blink::HeapAllocator>> {
On 2016/03/29 23:08:11, Rouslan wrote:
> On 2016/03/29 22:49:31, yzshen1 wrote:
> > Is it a common operation to use a non-default allocator?
> 
> The blink::HeapAllocator is used in blink::HeapVector<E>, which is a subclass
of
> WTF::Vector<E, 0, blink::HeapAllocator>. That's the type used by code
generated
> from WebIDL. These objects represent their JavaScript counterparts. See:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...
> 
> ...for an example of a blink::HeapVector<E> generated from:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> > It is unfortunate
> > because it requires a deep copy. :/
> 
> Why does this require a deep copy?

I mean, this TypeConverter is a deep copy.

> 
> > Can this converter be generalized to have the allocator as a template
> parameter
> > as well? That way we don't have to have blink::HeapAllocator here which
seems
> a
> > little weird?
> 
> Unfortunately, "struct TypeConverter" needs to be a template of exactly two
> typenames. See:

The TypeConverter takes exactly two template parameters, but I think you can do:

template <typename T, typename E, typename Allocator>
struct TypeConverter<WTFArray<T>, WTF::Vector<E, 0, Allocator>> {
};


> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...
> 
> > An alternative is to consider putting it somewhere inside blink, because
mojo
> > bindings itself doesn't need this converter, putting it inside blink reduces
> > what the bindings has to know about blink.
> 
> I'm OK with that.
> 
> esprehn@: where would be a good place for such a common code?

[please use gerrit instead, 2016-03-29 23:49:17]

yzshen@: ptal mojo/public/cpp/bindings/wtf_array.h in patch 44.

[haraken, 2016-03-30 01:23:20]

On 2016/03/29 23:08:11, Rouslan wrote:
>
https://codereview.chromium.org/1753543002/diff/840001/mojo/public/cpp/bindin...
> File mojo/public/cpp/bindings/wtf_array.h (right):
> 
>
https://codereview.chromium.org/1753543002/diff/840001/mojo/public/cpp/bindin...
> mojo/public/cpp/bindings/wtf_array.h:242: struct TypeConverter<WTFArray<T>,
> WTF::Vector<E, 0, blink::HeapAllocator>> {
> On 2016/03/29 22:49:31, yzshen1 wrote:
> > Is it a common operation to use a non-default allocator?
> 
> The blink::HeapAllocator is used in blink::HeapVector<E>, which is a subclass
of
> WTF::Vector<E, 0, blink::HeapAllocator>. That's the type used by code
generated
> from WebIDL. These objects represent their JavaScript counterparts. See:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...
> 
> ...for an example of a blink::HeapVector<E> generated from:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> > It is unfortunate
> > because it requires a deep copy. :/
> 
> Why does this require a deep copy?
> 
> > Can this converter be generalized to have the allocator as a template
> parameter
> > as well? That way we don't have to have blink::HeapAllocator here which
seems
> a
> > little weird?
> 
> Unfortunately, "struct TypeConverter" needs to be a template of exactly two
> typenames. See:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...
> 
> > An alternative is to consider putting it somewhere inside blink, because
mojo
> > bindings itself doesn't need this converter, putting it inside blink reduces
> > what the bindings has to know about blink.
> 
> I'm OK with that.
> 
> esprehn@: where would be a good place for such a common code?

I'd suggest creating platform/MojoHelper.h to put all helper functions for Mojo
bindings. sameThreadBindForMojo should be moved there.

[haraken, 2016-03-30 01:23:40]

https://codereview.chromium.org/1753543002/diff/860001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentsValidators.h (right):

https://codereview.chromium.org/1753543002/diff/860001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentsValidators.h:11: namespace
blink {

Maybe we want to put these methods in a static class or a dedicated namespace.

[please use gerrit instead, 2016-03-30 15:53:35]

haraken@, ptal patch 47.

On 2016/03/30 01:23:20, haraken wrote:
> I'd suggest creating platform/MojoHelper.h to put all helper functions for
Mojo
> bindings. sameThreadBindForMojo should be moved there.

Done.

https://codereview.chromium.org/1753543002/diff/860001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentsValidators.h (right):

https://codereview.chromium.org/1753543002/diff/860001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentsValidators.h:11: namespace
blink {
On 2016/03/30 01:23:40, haraken wrote:
> 
> Maybe we want to put these methods in a static class or a dedicated namespace.

Done. Placed these into PaymentsValidators class.

[palmer, 2016-03-30 20:48:18]

lgtm

[haraken, 2016-03-31 02:19:34]

Mostly looks good.

(I'm fine with landing this CL but please consider splitting a large CL into
pieces next time.)

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

https://codereview.chromium.org/1753543002/diff/920001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:288:
DCHECK(m_showResolver);

To make this assert meaningful, do we need to clear m_showResolver after
rejecting it below?

https://codereview.chromium.org/1753543002/diff/920001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:322: void
PaymentRequest::OnPaymentResponse(mojom::wtf::PaymentResponsePtr response)

How is it guaranteed that PaymentRequest::OnPaymentResponse is not called if we
rejected the m_showResolver in PaymentRequest::OnShippingAddressChange?

https://codereview.chromium.org/1753543002/diff/920001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:333: else if
(m_showResolver)

Nit: I'd use 'if' rather than 'else if' just in case.

https://codereview.chromium.org/1753543002/diff/920001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentsValidators.h (right):

https://codereview.chromium.org/1753543002/diff/920001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentsValidators.h:13: class
MODULES_EXPORT PaymentsValidators final {

Add STATIC_ONLY.

https://codereview.chromium.org/1753543002/diff/920001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentsValidators.h:32:
~PaymentsValidators() {}

Remove the constructor and destructor.

[esprehn, 2016-03-31 04:44:58]

lgtm once haraken@ is happy. Note that payments is literally the only user of
toJSONValue, and I really want to remove that API. Please plan a follow up CL
that uses JSON.stringify instead.

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

https://codereview.chromium.org/1753543002/diff/920001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:254:
RefPtr<JSONValue> value = toJSONValue(data.context(), data.v8Value());
you're the only consumer of toJSONValue. I want to remove it and all of the
JSONValue stuff in platform. I think you want to get v8 to expose
v8::JSON::stringify and invoke that here.

https://codereview.chromium.org/1753543002/diff/920001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentsValidators.h (right):

https://codereview.chromium.org/1753543002/diff/920001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentsValidators.h:13: class
MODULES_EXPORT PaymentsValidators final {
On 2016/03/31 at 02:19:34, haraken wrote:
> Add STATIC_ONLY.

I'd prefer we just used namespaces tbh, but we can do that too.

[haraken, 2016-03-31 08:26:31]

haraken@chromium.org changed reviewers:
+ yhirano@chromium.org

[haraken, 2016-03-31 08:26:34]

+yhirano for Promise part

LGTM (assuming the questions in my last review comment were answered).

[Marijn Kruisselbrink, 2016-03-31 18:21:15]

still lgtm

https://codereview.chromium.org/1753543002/diff/920001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentDetailsTestHelper.cpp
(right):

https://codereview.chromium.org/1753543002/diff/920001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentDetailsTestHelper.cpp:14:
PaymentDetails buildPaymentDetailsForTest(PaymentTestDetailToChange detail,
PaymentTestDataToChange data, PaymentTestModificationType modificationType,
const String& valueToUse)
What is the purpose of the modificationType parameter? This method only seems to
expect "PaymentTestOverwriteValue" as value for the parameter, so it seems there
is no need to pass it?

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

https://codereview.chromium.org/1753543002/diff/920001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:166:
m_paymentProvider->SetClient(m_clientBinding.CreateInterfacePtrAndBind());
Do you need a connection error handler for the client binding as well? (or the
client binding could possibly use an "associated" interface to reuse the same
mojo pipe for messages in both directions).

Not sure what "typical" mojo usage looks like in cases like this.

[please use gerrit instead, 2016-03-31 20:43:09]

yhirano@, ptal promises in 
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp in patch 48.

https://codereview.chromium.org/1753543002/diff/920001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentDetailsTestHelper.cpp
(right):

https://codereview.chromium.org/1753543002/diff/920001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentDetailsTestHelper.cpp:14:
PaymentDetails buildPaymentDetailsForTest(PaymentTestDetailToChange detail,
PaymentTestDataToChange data, PaymentTestModificationType modificationType,
const String& valueToUse)
On 2016/03/31 18:21:14, Marijn Kruisselbrink wrote:
> What is the purpose of the modificationType parameter? This method only seems
to
> expect "PaymentTestOverwriteValue" as value for the parameter, so it seems
there
> is no need to pass it?

I specify both PaymentTestOverwriteValue and PaymentTestRemoveKey in my test:

https://codereview.chromium.org/1753543002/diff/940001/third_party/WebKit/Sou...

If modificationType is PaymentTestRemoveKey, then a call like
item.setId("total") is never invoked. This results in absence of "id" field from
the dictionary. That is, item.hasId() will return false. The "id" field is
required. My tests verify that PaymentRequest will throw an exception if an "id"
is missing.

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

https://codereview.chromium.org/1753543002/diff/920001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:166:
m_paymentProvider->SetClient(m_clientBinding.CreateInterfacePtrAndBind());
On 2016/03/31 18:21:15, Marijn Kruisselbrink wrote:
> Do you need a connection error handler for the client binding as well? (or the
> client binding could possibly use an "associated" interface to reuse the same
> mojo pipe for messages in both directions).
> 
> Not sure what "typical" mojo usage looks like in cases like this.

chromium-mojo@ says the second connection handler is not necessary:

https://groups.google.com/a/chromium.org/d/msg/chromium-mojo/jBXvBIlROZ4/cVZp...

Nevertheless, I will continue investigating whether it possible to use an
associated interface to reuse the same pipe.

https://codereview.chromium.org/1753543002/diff/920001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:254:
RefPtr<JSONValue> value = toJSONValue(data.context(), data.v8Value());
On 2016/03/31 04:44:58, esprehn wrote:
> you're the only consumer of toJSONValue. I want to remove it and all of the
> JSONValue stuff in platform. I think you want to get v8 to expose
> v8::JSON::stringify and invoke that here.

Planning to do this in one of the following patches.

https://codereview.chromium.org/1753543002/diff/920001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:288:
DCHECK(m_showResolver);
On 2016/03/31 02:19:34, haraken wrote:
> 
> To make this assert meaningful, do we need to clear m_showResolver after
> rejecting it below?

Done.

https://codereview.chromium.org/1753543002/diff/920001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:322: void
PaymentRequest::OnPaymentResponse(mojom::wtf::PaymentResponsePtr response)
On 2016/03/31 02:19:34, haraken wrote:
> 
> How is it guaranteed that PaymentRequest::OnPaymentResponse is not called if
we
> rejected the m_showResolver in PaymentRequest::OnShippingAddressChange?

Closing the Mojo connection now.

https://codereview.chromium.org/1753543002/diff/920001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:333: else if
(m_showResolver)
On 2016/03/31 02:19:34, haraken wrote:
> 
> Nit: I'd use 'if' rather than 'else if' just in case.
> 

Done.

https://codereview.chromium.org/1753543002/diff/920001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentsValidators.h (right):

https://codereview.chromium.org/1753543002/diff/920001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentsValidators.h:13: class
MODULES_EXPORT PaymentsValidators final {
On 2016/03/31 02:19:34, haraken wrote:
> 
> Add STATIC_ONLY.

Done.

https://codereview.chromium.org/1753543002/diff/920001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentsValidators.h:13: class
MODULES_EXPORT PaymentsValidators final {
On 2016/03/31 04:44:58, esprehn wrote:
> On 2016/03/31 at 02:19:34, haraken wrote:
> > Add STATIC_ONLY.
> 
> I'd prefer we just used namespaces tbh, but we can do that too.

Acknowledged.

https://codereview.chromium.org/1753543002/diff/920001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentsValidators.h:32:
~PaymentsValidators() {}
On 2016/03/31 02:19:34, haraken wrote:
> 
> Remove the constructor and destructor.

Done.

[please use gerrit instead, 2016-03-31 20:43:20]

yhirano@, ptal promises in 
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp in patch 48.

[please use gerrit instead, 2016-03-31 23:25:47]

haraken@ says a review from yhirano@ is not necessary, because I've answered
haraken@'s questions. Sending to CQ.

[please use gerrit instead, 2016-03-31 23:26:04]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2016-03-31 23:26:05]

The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org,
rob.buis@samsung.com, esprehn@chromium.org, palmer@chromium.org,
haraken@chromium.org, mek@chromium.org
Link to the patchset: https://codereview.chromium.org/1753543002/#ps940001
(title: "haraken@'s + esprehn@'s comments")

[commit-bot: I haz the power, 2016-03-31 23:26:52]

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

[commit-bot: I haz the power, 2016-04-01 00:04:03]

Description was changed from

==========
PaymentRequest Mojo bindings.

The Mojo (new IPC replacement) bindings for PaymentRequest Blink API.
Blink uses the bindings to procure the functionality for the API. Mojo
eliminates multiple layers of abstraction, letting the embedder provide
the functionality by implementing the PaymentRequest interface in C++ or
Java for Android. The PaymentRequest interface lives in:

third_party/Webkit/public/platform/modules/payments/payment_request.mojom

The embedder does not provide any functionality in this patch.

PaymentRequest draft spec:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html

BUG=587995
TEST=PaymentRequestTest.cpp
TEST=PaymentResponseTest.cpp
TEST=PaymentsValidatorsTest.cpp
TEST=ShippingAddressTest.cpp
TEST=payment-request-interface.html
==========

to

==========
PaymentRequest Mojo bindings.

The Mojo (new IPC replacement) bindings for PaymentRequest Blink API.
Blink uses the bindings to procure the functionality for the API. Mojo
eliminates multiple layers of abstraction, letting the embedder provide
the functionality by implementing the PaymentRequest interface in C++ or
Java for Android. The PaymentRequest interface lives in:

third_party/Webkit/public/platform/modules/payments/payment_request.mojom

The embedder does not provide any functionality in this patch.

PaymentRequest draft spec:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html

BUG=587995
TEST=PaymentRequestTest.cpp
TEST=PaymentResponseTest.cpp
TEST=PaymentsValidatorsTest.cpp
TEST=ShippingAddressTest.cpp
TEST=payment-request-interface.html
==========

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

Committed patchset #48 (id:940001)

[commit-bot: I haz the power, 2016-04-01 00:06:02]

Description was changed from

==========
PaymentRequest Mojo bindings.

The Mojo (new IPC replacement) bindings for PaymentRequest Blink API.
Blink uses the bindings to procure the functionality for the API. Mojo
eliminates multiple layers of abstraction, letting the embedder provide
the functionality by implementing the PaymentRequest interface in C++ or
Java for Android. The PaymentRequest interface lives in:

third_party/Webkit/public/platform/modules/payments/payment_request.mojom

The embedder does not provide any functionality in this patch.

PaymentRequest draft spec:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html

BUG=587995
TEST=PaymentRequestTest.cpp
TEST=PaymentResponseTest.cpp
TEST=PaymentsValidatorsTest.cpp
TEST=ShippingAddressTest.cpp
TEST=payment-request-interface.html
==========

to

==========
PaymentRequest Mojo bindings.

The Mojo (new IPC replacement) bindings for PaymentRequest Blink API.
Blink uses the bindings to procure the functionality for the API. Mojo
eliminates multiple layers of abstraction, letting the embedder provide
the functionality by implementing the PaymentRequest interface in C++ or
Java for Android. The PaymentRequest interface lives in:

third_party/Webkit/public/platform/modules/payments/payment_request.mojom

The embedder does not provide any functionality in this patch.

PaymentRequest draft spec:
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html

BUG=587995
TEST=PaymentRequestTest.cpp
TEST=PaymentResponseTest.cpp
TEST=PaymentsValidatorsTest.cpp
TEST=ShippingAddressTest.cpp
TEST=payment-request-interface.html

Committed: https://crrev.com/763f2a3acbf7ee75d8eb4f1c0a6d90399f75082f
Cr-Commit-Position: refs/heads/master@{#384439}
==========

[commit-bot: I haz the power, 2016-04-01 00:06:05]

Patchset 48 (id:??) landed as
https://crrev.com/763f2a3acbf7ee75d8eb4f1c0a6d90399f75082f
Cr-Commit-Position: refs/heads/master@{#384439}

[krasin, 2016-04-05 20:46:01]

For the record, this change introduced https://crbug.com/600808, which is the
cause for 'CFI Linux' buildbot being red:
https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux

[kinuko, 2016-04-22 01:31:00]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2016-04-22 01:31:03]

https://codereview.chromium.org/1753543002/diff/940001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/DEPS (right):

https://codereview.chromium.org/1753543002/diff/940001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/DEPS:4: "+base/bind.h",
Sorry, I just noticed this... so now we allow base/bind.h in all
Source/platform, which I believe we've been trying to avoid.  Having
MojoHelper.h in platform/ makes sense, but having this line in platform/DEPS
feels a bit unfortunate.  Could MojoHelper.h be in Source/platform/mojo or
somewhere slightly more scoped until unification happens?

[Marijn Kruisselbrink, 2016-04-22 01:40:07]

On 2016/04/22 at 01:31:03, kinuko wrote:
>
https://codereview.chromium.org/1753543002/diff/940001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/DEPS (right):
> 
>
https://codereview.chromium.org/1753543002/diff/940001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/DEPS:4: "+base/bind.h",
> Sorry, I just noticed this... so now we allow base/bind.h in all
Source/platform, which I believe we've been trying to avoid.  Having
MojoHelper.h in platform/ makes sense, but having this line in platform/DEPS
feels a bit unfortunate.  Could MojoHelper.h be in Source/platform/mojo or
somewhere slightly more scoped until unification happens?

I think having a platform/mojo subdirectory makes a lot of sense, also for
example as a place to put mojo typemaps to convert things like URLs and origins
between mojo types and blink types.

[kinuko, 2016-04-22 02:02:15]

On 2016/04/22 01:40:07, Marijn Kruisselbrink wrote:
> On 2016/04/22 at 01:31:03, kinuko wrote:
> >
>
https://codereview.chromium.org/1753543002/diff/940001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/platform/DEPS (right):
> > 
> >
>
https://codereview.chromium.org/1753543002/diff/940001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/platform/DEPS:4: "+base/bind.h",
> > Sorry, I just noticed this... so now we allow base/bind.h in all
> Source/platform, which I believe we've been trying to avoid.  Having
> MojoHelper.h in platform/ makes sense, but having this line in platform/DEPS
> feels a bit unfortunate.  Could MojoHelper.h be in Source/platform/mojo or
> somewhere slightly more scoped until unification happens?
> 
> I think having a platform/mojo subdirectory makes a lot of sense, also for
> example as a place to put mojo typemaps to convert things like URLs and
origins
> between mojo types and blink types.

https://codereview.chromium.org/1910373002/ created a CL to see how others feel.