PaymentRequest API.

PaymentRequest API.

An API that allows browsers to act as an intermediary between the three
key parties in a financial transaction: the merchant (e.g. an online web
store), the buyer (e.g. the user buying from the online web store), and
the Payment Method (e.g. credit card). Information necessary to process
and confirm a transaction is passed between the Payment Method and the
merchant via the browser with the buyer confirming and authorizing as
necessary across the flow.

Intent to implement:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/gbSs15ZSWtA/JFU3H7fTDQAJ

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

[please use gerrit instead, 2016-02-16 23:27:55]

Patchset #1 (id:1) has been deleted

[please use gerrit instead, 2016-02-16 23:30:03]

Marjin, fyi I've started to put the proper pieces together for the API. I'm
doing this part by part as I learn about the right ways of doing things in
Blink. Let me know if you spot something horrible!

[Marijn Kruisselbrink, 2016-02-17 01:33:21]

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

[Marijn Kruisselbrink, 2016-02-17 01:33:21]

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

https://codereview.chromium.org/1702223002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.h:24: static
RawPtr<PaymentRequest> create(const Vector<String>&);
No need to use RawPtr<Foo>. Just Foo* should work fine (at least that's my
understanding. RawPtr<T> only exists for things like RefPtrWillBeRawPtr, and
should go away after all WillBe types have been removed. Which should happen
sometime in the next couple of weeks I believe).

[Marijn Kruisselbrink, 2016-02-17 01:36:54]

On 2016/02/16 at 23:30:03, rouslan wrote:
> Marjin, fyi I've started to put the proper pieces together for the API. I'm
doing this part by part as I learn about the right ways of doing things in
Blink. Let me know if you spot something horrible!

Oops, I had written more of a response here, but accidentally discarded that...
You probably want to guard this feature as a runtime enabled feature. You do
that by adding a line (with status=experimental) to
Source/platform/RuntimeEnabledFeatures.in and adding a [RuntimeEnabled=]
extended attribute to your IDL interface definition. That way the feature isn't
exposed unless the experimental web platform features flag is enabled. Which
would fix the test failure in
virtual/stable/webexposed/global-interface-listing.html.

[please use gerrit instead, 2016-02-17 01:42:20]

You're savior!

On Tue, Feb 16, 2016 at 5:36 PM <mek@chromium.org> wrote:

>
> On 2016/02/16 at 23:30:03, rouslan wrote:
> > Marjin, fyi I've started to put the proper pieces together for the API. I'm
> doing this part by part as I learn about the right ways of doing things in
> Blink. Let me know if you spot something horrible!
>
> Oops, I had written more of a response here, but accidentally discarded
that...
> You probably want to guard this feature as a runtime enabled feature. You do
> that by adding a line (with status=experimental) to
> Source/platform/RuntimeEnabledFeatures.in and adding a [RuntimeEnabled=]
> extended attribute to your IDL interface definition. That way the feature
isn't
> exposed unless the experimental web platform features flag is enabled. Which
> would fix the test failure in
> virtual/stable/webexposed/global-interface-listing.html.
> https://codereview.chromium.org/1702223002/
>
>

-- 
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-02-17 01:42:21]

You're savior!

On Tue, Feb 16, 2016 at 5:36 PM <mek@chromium.org> wrote:

>
> On 2016/02/16 at 23:30:03, rouslan wrote:
> > Marjin, fyi I've started to put the proper pieces together for the API. I'm
> doing this part by part as I learn about the right ways of doing things in
> Blink. Let me know if you spot something horrible!
>
> Oops, I had written more of a response here, but accidentally discarded
that...
> You probably want to guard this feature as a runtime enabled feature. You do
> that by adding a line (with status=experimental) to
> Source/platform/RuntimeEnabledFeatures.in and adding a [RuntimeEnabled=]
> extended attribute to your IDL interface definition. That way the feature
isn't
> exposed unless the experimental web platform features flag is enabled. Which
> would fix the test failure in
> virtual/stable/webexposed/global-interface-listing.html.
> https://codereview.chromium.org/1702223002/
>
>

-- 
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-02-17 02:53:02]

Patchset #2 (id:40001) has been deleted

[please use gerrit instead, 2016-02-17 02:53:07]

Patchset #2 (id:60001) has been deleted

[please use gerrit instead, 2016-02-17 18:15:21]

Description was changed from

==========
PaymentRequest API.

BUG=587207
==========

to

==========
PaymentRequest API.

BUG=587207
==========

[Marijn Kruisselbrink, 2016-02-18 21:59:19]

https://codereview.chromium.org/1702223002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/CurrencyAmount.idl (right):

https://codereview.chromium.org/1702223002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/CurrencyAmount.idl:9:
GarbageCollected,
I don't think [GarbageCollected] is necessary (or meaningful) on a dictionary
(also for all the other dictionary types).

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

https://codereview.chromium.org/1702223002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:33: return
ScriptPromiseResolver::create(scriptState)->promise();
I think this will crash, probably better to just return
ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(NotSupportedError, "some message"));

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

https://codereview.chromium.org/1702223002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.h:33: static
PaymentRequest* create(ExecutionContext*, const Vector<String>&, const
PaymentDetails&, const PaymentOptions& = PaymentOptions(), const ScriptValue& =
ScriptValue());
you should include parameter names for the parameters where it isn't obvious
from the type what the parameter is about (so probably the Vector<String> and
the ScriptValue parameters).

https://codereview.chromium.org/1702223002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.h:35:
PaymentRequest(ExecutionContext*, const Vector<String>&, const PaymentDetails&,
const PaymentOptions&, const ScriptValue&);
Since you have a static create method, you probably want to make the constructor
private (and same comment about parameter names)

https://codereview.chromium.org/1702223002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.h:61: ScriptValue
m_data;
If I understand things correctly ScriptValue is hard to use correctly, and in
particular holding on to a reference to a ScriptValue like this is problematic
and causes memory leaks (something to do with cyclic references between objects
managed by oilpan and objects managed by the v8 garbage collector). Probably
want to discuss this with somebody in the bindings team like bashi@ to figure
out what the correct thing to do is here.

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

https://codereview.chromium.org/1702223002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentResponse.cpp:21: return
ScriptPromiseResolver::create(scriptState)->promise();
Don't create and not resolve a ScriptPromiseResolver, just return a rejected
promise (see my previous comment).

https://codereview.chromium.org/1702223002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentResponse.h (right):

https://codereview.chromium.org/1702223002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentResponse.h:30: ScriptPromise
complete(ScriptState*, bool);
Add a parameter name for this bool

https://codereview.chromium.org/1702223002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentResponse.h:36: ScriptValue
m_details;
Same issue around using ScriptValue where you probably need the input of
somebody on the bindings team.

[please use gerrit instead, 2016-02-18 22:01:46]

Description was changed from

==========
PaymentRequest API.

BUG=587207
==========

to

==========
PaymentRequest API.

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

[please use gerrit instead, 2016-02-18 22:41:33]

Marijn, I've addressed your comments in patch 12 and will be reaching out to
bashi@ for ScriptValue storage question. 

ScriptValue's param name in spec is "data". That's not informative. Can I change
the param name to clarify what it is?

https://codereview.chromium.org/1702223002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/CurrencyAmount.idl (right):

https://codereview.chromium.org/1702223002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/CurrencyAmount.idl:9:
GarbageCollected,
On 2016/02/18 21:59:18, Marijn Kruisselbrink wrote:
> I don't think [GarbageCollected] is necessary (or meaningful) on a dictionary
> (also for all the other dictionary types).

Done.

Also removed RuntimeEnabled for dictionaries. It seems to have no effect.

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

https://codereview.chromium.org/1702223002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:33: return
ScriptPromiseResolver::create(scriptState)->promise();
On 2016/02/18 21:59:18, Marijn Kruisselbrink wrote:
> I think this will crash, probably better to just return
> ScriptPromise::rejectWithDOMException(scriptState,
> DOMException::create(NotSupportedError, "some message"));

Done.

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

https://codereview.chromium.org/1702223002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.h:33: static
PaymentRequest* create(ExecutionContext*, const Vector<String>&, const
PaymentDetails&, const PaymentOptions& = PaymentOptions(), const ScriptValue& =
ScriptValue());
On 2016/02/18 21:59:18, Marijn Kruisselbrink wrote:
> you should include parameter names for the parameters where it isn't obvious
> from the type what the parameter is about (so probably the Vector<String> and
> the ScriptValue parameters).

Done.

ScriptValue's param name is "data" in the spec. Can I change the param name to
clarify what it is?

https://codereview.chromium.org/1702223002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.h:35:
PaymentRequest(ExecutionContext*, const Vector<String>&, const PaymentDetails&,
const PaymentOptions&, const ScriptValue&);
On 2016/02/18 21:59:18, Marijn Kruisselbrink wrote:
> Since you have a static create method, you probably want to make the
constructor
> private (and same comment about parameter names)

Done.

https://codereview.chromium.org/1702223002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.h:61: ScriptValue
m_data;
On 2016/02/18 21:59:18, Marijn Kruisselbrink wrote:
> If I understand things correctly ScriptValue is hard to use correctly, and in
> particular holding on to a reference to a ScriptValue like this is problematic
> and causes memory leaks (something to do with cyclic references between
objects
> managed by oilpan and objects managed by the v8 garbage collector). Probably
> want to discuss this with somebody in the bindings team like bashi@ to figure
> out what the correct thing to do is here.

Acknowledged.

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

https://codereview.chromium.org/1702223002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentResponse.cpp:21: return
ScriptPromiseResolver::create(scriptState)->promise();
On 2016/02/18 21:59:18, Marijn Kruisselbrink wrote:
> Don't create and not resolve a ScriptPromiseResolver, just return a rejected
> promise (see my previous comment).

Done.

https://codereview.chromium.org/1702223002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentResponse.h (right):

https://codereview.chromium.org/1702223002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentResponse.h:30: ScriptPromise
complete(ScriptState*, bool);
On 2016/02/18 21:59:19, Marijn Kruisselbrink wrote:
> Add a parameter name for this bool

Done.

https://codereview.chromium.org/1702223002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentResponse.h:36: ScriptValue
m_details;
On 2016/02/18 21:59:19, Marijn Kruisselbrink wrote:
> Same issue around using ScriptValue where you probably need the input of
> somebody on the bindings team.

Acknowledged.

[please use gerrit instead, 2016-02-19 20:36:13]

Marijn, ptal patch 17.

I've serialized the JSON object into a string for storage. That will sent to the
browser, which will deserialize it using its own facilities. (This was a
proposal from haavardm@.)

[Marijn Kruisselbrink, 2016-02-19 22:39:44]

Serializing the data sounds good indeed. That is similar to how Notification
deals with storing its data (although there I don't think the browser ever does
anything with the data).

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

https://codereview.chromium.org/1702223002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:59:
m_serializedData =
SerializedScriptValueFactory::instance().createAndSwallowExceptions(v8::Isolate::GetCurrent(),
data.v8Value())->toWireString();
Ultimately createAndSwallowExceptions here is wrong I imagine. Presumably you
want to actually do something when serializing data fails (although the spec
seems pretty weird in this area... but that's a separate issue).

Something like the code in
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
would probably be what you'd want to end up doing (except with the peculiarity
that the payments spec seems to require swallowing exceptions from parsing and
failing with a different exception instead).

But since you're not actually accessing m_serializedData anywhere yet (and not
validating any of the other arguments either) it might just be enough to not
parse at all?

https://codereview.chromium.org/1702223002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentResponse.h (right):

https://codereview.chromium.org/1702223002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentResponse.h:28: const
ScriptValue& details() const { return m_details; }
You should probably get rid of m_details and return either a default constructed
ScriptValue or ScriptValue::createNull directly (and change the method to return
by value).

Or go all the way and change m_details to Vector<char> and return
ScriptValue(scriptState,
SerializedScriptValueFactory::createFromWireBytes()->deserialize(scriptState->isolate());

(similar to Notification::data(), with the exception that the caching that is
done in that getter is wrong and has the same issues around ScriptValue)

https://codereview.chromium.org/1702223002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentResponse.idl (right):

https://codereview.chromium.org/1702223002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentResponse.idl:9:
NoInterfaceObject,
The [NoInterfaceObject] on this and other interfaces is not something that seems
to currently be part of the spec?

[Marijn Kruisselbrink, 2016-02-19 23:31:34]

Actually thinking about this more, SerializedScriptValue is probably not what
you want to use in any of these places. If I understand things correctly in this
particular case you need to be able to interpret the serialized data in the
browser process. That is not possible with SerializedScriptValue (also
SerializedScriptValue allows serializing of many things that can't be
represented by JSON). So you probably want to "just" directly serialize things
as JSON instead?

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

On 2016/02/19 23:31:34, Marijn Kruisselbrink wrote:
> So you probably want to "just" directly serialize things
> as JSON instead?

That was my intention. If that is not happening, I should fix it. Does Blink
have a function to directly serialize the object or do I need to write one
myself?

[Marijn Kruisselbrink, 2016-02-20 00:15:23]

On 2016/02/19 at 23:36:05, rouslan wrote:
> On 2016/02/19 23:31:34, Marijn Kruisselbrink wrote:
> > So you probably want to "just" directly serialize things
> > as JSON instead?
> 
> That was my intention. If that is not happening, I should fix it. Does Blink
have a function to directly serialize the object or do I need to write one
myself?

SerializedScriptValue isn't JSON. It's a custom serialization format that
supports object references (including circular references) and a bunch of custom
types and things like that. Blink seems to have Source/platform/JSONParser.h to
parse a JSON string, although that doesn't then have a way to convert it to a
v8/scriptvalue as far as I can tell. JSONValuesForV8 seems like it could be
doing the reverse (v8 value to JSON String).
V8 implements the JSON.parse/stringify methods, which seems like what you'd want
here, but those are implemented in javascript, so calling them from (native)
blink code is tricky.

Actually, refreshing my memory around this a bit more, it seems you should be
able to use v8::JSON::Parse()
(https://code.google.com/p/chromium/codesearch#chromium/src/v8/include/v8.h&sq...)
to parse a json string into a v8 object. And the before mentioned
JSONValuesForV8 seems to have the method to do the reverse. So it should be
possible to use a combination of the two to do the entire conversion. 

Another option would be to do something similar to what MessagePortChannelImpl
does and use content::V8ValueConverter (at the content level) to convert from a
v8 value to a base::Value, avoiding the string serialization all together (but
that still has problems around actually storing these things at the blink level,
especially when you want to convert back from a base::Value to a v8 value. This
is something I worked on for a bit for message ports; I never really found a
satisfactory solution to the problem of passing arbitrary data from blink to the
browser in a format the browser can read. Maybe these days it could be possible
to add a base::Value dependency to blink, move V8ValueConverter to somewhere
blink can use it, and use that to convert between v8::Value and base::Value at
the right level.
https://docs.google.com/document/d/1IDZCxwhoYCorp9ylKXgrBFVOZCwRuLeL141wIQc1_...
describes some of the problems I ran into)

[please use gerrit instead, 2016-02-23 16:56:15]

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

https://codereview.chromium.org/1702223002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:72:
m_serializedData =
SerializedScriptValueFactory::instance().create(v8::Isolate::GetCurrent(),
data.v8Value(), nullptr, nullptr, nullptr, exceptionState)->toWireString();
Note from Haavard:

WebString detailsString =
NativeValueTraits<JSONValuePtr>::nativeValue(scriptState->isolate(),
details.v8Value(), exceptionState)->toJSONString();

[please use gerrit instead, 2016-02-26 21:06:28]

Marijn, ptal patch 18. I'm storing JSON objects in strings now.

I don't think we should be swallowing exceptions, but that's a discussion to be
had with spec editors. Their upcoming test-harness.js tests will make sure that
the types of the exceptions are correct.

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

https://codereview.chromium.org/1702223002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:59:
m_serializedData =
SerializedScriptValueFactory::instance().createAndSwallowExceptions(v8::Isolate::GetCurrent(),
data.v8Value())->toWireString();
On 2016/02/19 22:39:44, Marijn Kruisselbrink wrote:
> Ultimately createAndSwallowExceptions here is wrong I imagine. Presumably you
> want to actually do something when serializing data fails (although the spec
> seems pretty weird in this area... but that's a separate issue).
> 
> Something like the code in
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> would probably be what you'd want to end up doing (except with the peculiarity
> that the payments spec seems to require swallowing exceptions from parsing and
> failing with a different exception instead).
> 
> But since you're not actually accessing m_serializedData anywhere yet (and not
> validating any of the other arguments either) it might just be enough to not
> parse at all?

Done.

https://codereview.chromium.org/1702223002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentResponse.h (right):

https://codereview.chromium.org/1702223002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentResponse.h:28: const
ScriptValue& details() const { return m_details; }
On 2016/02/19 22:39:44, Marijn Kruisselbrink wrote:
> You should probably get rid of m_details and return either a default
constructed
> ScriptValue or ScriptValue::createNull directly (and change the method to
return
> by value).
> 
> Or go all the way and change m_details to Vector<char> and return
> ScriptValue(scriptState,
>
SerializedScriptValueFactory::createFromWireBytes()->deserialize(scriptState->isolate());
> 
> (similar to Notification::data(), with the exception that the caching that is
> done in that getter is wrong and has the same issues around ScriptValue)

Storing JSON as a string and parsing it into a ScriptValue.

https://codereview.chromium.org/1702223002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentResponse.idl (right):

https://codereview.chromium.org/1702223002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentResponse.idl:9:
NoInterfaceObject,
On 2016/02/19 22:39:44, Marijn Kruisselbrink wrote:
> The [NoInterfaceObject] on this and other interfaces is not something that
seems
> to currently be part of the spec?

Removed.

[Marijn Kruisselbrink, 2016-02-26 22:02:54]

two small comments, but generally lgtm

You might also want to add an OWNERS file to
third_part/WebKit/Source/modules/payments? Although that can always be done
later as well of course.

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

https://codereview.chromium.org/1702223002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:74: if
(value->type() == JSONValue::TypeObject)
You should check if value is nullptr (which seems to be possible depending on
data).

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

https://codereview.chromium.org/1702223002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.idl:6: //
http://crbug.com/587207
That bug got closed and merged into 587995. Probably want to update or remove
the reference to the bug here.

[please use gerrit instead, 2016-02-27 00:43:44]

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

[please use gerrit instead, 2016-02-27 00:43:46]

jbroman@, owners ptal EventTypeNames.in and RuntimeEnabledFeatures.in in patch
20.

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

https://codereview.chromium.org/1702223002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:74: if
(value->type() == JSONValue::TypeObject)
On 2016/02/26 22:02:53, Marijn Kruisselbrink wrote:
> You should check if value is nullptr (which seems to be possible depending on
> data).

Done.

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

https://codereview.chromium.org/1702223002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.idl:6: //
http://crbug.com/587207
On 2016/02/26 22:02:53, Marijn Kruisselbrink wrote:
> That bug got closed and merged into 587995. Probably want to update or remove
> the reference to the bug here.

Done.

[please use gerrit instead, 2016-02-27 00:47:23]

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

[please use gerrit instead, 2016-02-27 00:47:24]

haraken@, owners ptal EventTargetModulesFactory.in and modules/payments/ in
patch 20.

[jbroman, 2016-02-27 05:11:34]

EventTypeNames.in and RuntimeEnabledFeatures.in lgtm

[haraken, 2016-02-27 15:38:55]

Would you add more explanations to the CL description? (A link to
Intent-to-implement etc.)

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

https://codereview.chromium.org/1702223002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.h:27: class
PaymentRequest final : public
RefCountedGarbageCollectedEventTargetWithInlineData<PaymentRequest>, public
ActiveDOMObject {

Why do you need to make this class an ActiveDOMObject?

https://codereview.chromium.org/1702223002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.h:62:
Member<ShippingAddress> m_shippingAddress;

Where is m_shippingAddress set? I couldn't find any code that updates the
variable.

https://codereview.chromium.org/1702223002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentRequest.idl (right):

https://codereview.chromium.org/1702223002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.idl:13:
DependentLifetime,

Why do you need [DependentLifetime]?

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

https://codereview.chromium.org/1702223002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentResponse.cpp:26:
ScriptState::Scope scope(scriptState);

This shouldn't be needed. It's guaranteed that PaymentResponse::details is
called by JS and thus it's already in a correct script state.

https://codereview.chromium.org/1702223002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentResponse.cpp:31: if
(!v8Call(v8::JSON::Parse(isolate, dataString), parsed, ignoreErrors))

Don't you need to throw an exception when the JSON::Parse fails?

https://codereview.chromium.org/1702223002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/ShippingOption.idl (right):

https://codereview.chromium.org/1702223002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingOption.idl:7: dictionary
ShippingOption {

Can we add [RuntimeEnabled=PaymentRequest] to these dictionaries?

[please use gerrit instead, 2016-02-29 17:52:09]

Description was changed from

==========
PaymentRequest API.

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

to

==========
PaymentRequest API.

Intent to implement:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/gbSs15ZSWtA/JFU3H7fT...

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

[please use gerrit instead, 2016-02-29 17:56:37]

Description was changed from

==========
PaymentRequest API.

Intent to implement:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/gbSs15ZSWtA/JFU3H7fT...

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

to

==========
PaymentRequest API.

An API that allows browsers to act as an intermediary between the three
key parties in a financial transaction: the merchant (e.g. an online web
store), the buyer (e.g. the user buying from the online web store), and
the Payment Method (e.g. credit card). Information necessary to process
and confirm a transaction is passed between the Payment Method and the
merchant via the browser with the buyer confirming and authorizing as
necessary across the flow.

Intent to implement:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/gbSs15ZSWtA/JFU3H7fT...

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

[please use gerrit instead, 2016-02-29 19:20:19]

haraken@, ptal patch 21.

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

https://codereview.chromium.org/1702223002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.h:27: class
PaymentRequest final : public
RefCountedGarbageCollectedEventTargetWithInlineData<PaymentRequest>, public
ActiveDOMObject {
On 2016/02/27 15:38:54, haraken wrote:
> 
> Why do you need to make this class an ActiveDOMObject?

There was no need. Replaced with ContextLifecycleObserver.

https://codereview.chromium.org/1702223002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.h:62:
Member<ShippingAddress> m_shippingAddress;
On 2016/02/27 15:38:54, haraken wrote:
> 
> Where is m_shippingAddress set? I couldn't find any code that updates the
> variable.

m_shippingAddress and m_shippingOption will be set by the embedder when any one
of these things happen:
1) shippingaddresschange event fires.
2) shippingoptionchange event fires.
3) show() promise resolves.

https://codereview.chromium.org/1702223002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/PaymentRequest.idl (right):

https://codereview.chromium.org/1702223002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.idl:13:
DependentLifetime,
On 2016/02/27 15:38:55, haraken wrote:
> 
> Why do you need [DependentLifetime]?

Not needed. Removed.

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

https://codereview.chromium.org/1702223002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentResponse.cpp:26:
ScriptState::Scope scope(scriptState);
On 2016/02/27 15:38:55, haraken wrote:
> 
> This shouldn't be needed. It's guaranteed that PaymentResponse::details is
> called by JS and thus it's already in a correct script state.

Done.

https://codereview.chromium.org/1702223002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentResponse.cpp:31: if
(!v8Call(v8::JSON::Parse(isolate, dataString), parsed, ignoreErrors))
On 2016/02/27 15:38:55, haraken wrote:
> 
> Don't you need to throw an exception when the JSON::Parse fails?

Done.

https://codereview.chromium.org/1702223002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/payments/ShippingOption.idl (right):

https://codereview.chromium.org/1702223002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingOption.idl:7: dictionary
ShippingOption {
On 2016/02/27 15:38:55, haraken wrote:
> 
> Can we add [RuntimeEnabled=PaymentRequest] to these dictionaries?

Done.

[haraken, 2016-03-01 02:33:47]

Mostly looks good.

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

https://codereview.chromium.org/1702223002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.h:27: class
PaymentRequest final : public
RefCountedGarbageCollectedEventTargetWithInlineData<PaymentRequest>, public
ContextLifecycleObserver {

I don't think PaymentRequest needs to be a ContextLifecycleObserver. Alternately
you can:

- Add [ConstructorCallWith=ScriptState].
- Make PaymentRequest hold RefPtr<ScriptState>.
- Extract the execution context with m_scriptState->executionContext().

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

https://codereview.chromium.org/1702223002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentResponse.cpp:30: if
(!v8Call(v8::JSON::Parse(isolate, v8String(isolate, m_stringifiedDetails)),
parsed, tryCatch)) {

Could you introduce a helper method like fromJSONValue to JSONValuesForV8.h and
encapsulate the deserialization logic in the helper method?

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

https://codereview.chromium.org/1702223002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:15: }

You can inline these contructor & destructor.

[please use gerrit instead, 2016-03-02 00:32:53]

haraken@, ptal patch 22.

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

https://codereview.chromium.org/1702223002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequest.h:27: class
PaymentRequest final : public
RefCountedGarbageCollectedEventTargetWithInlineData<PaymentRequest>, public
ContextLifecycleObserver {
On 2016/03/01 02:33:47, haraken wrote:
> 
> I don't think PaymentRequest needs to be a ContextLifecycleObserver.
Alternately
> you can:
> 
> - Add [ConstructorCallWith=ScriptState].
> - Make PaymentRequest hold RefPtr<ScriptState>.
> - Extract the execution context with m_scriptState->executionContext().

Done.

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

https://codereview.chromium.org/1702223002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentResponse.cpp:30: if
(!v8Call(v8::JSON::Parse(isolate, v8String(isolate, m_stringifiedDetails)),
parsed, tryCatch)) {
On 2016/03/01 02:33:47, haraken wrote:
> 
> Could you introduce a helper method like fromJSONValue to JSONValuesForV8.h
and
> encapsulate the deserialization logic in the helper method?

Done.

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

https://codereview.chromium.org/1702223002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/ShippingAddress.cpp:15: }
On 2016/03/01 02:33:47, haraken wrote:
> 
> You can inline these contructor & destructor.

Done.

[haraken, 2016-03-02 00:46:16]

LGTM

https://codereview.chromium.org/1702223002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/JSONValuesForV8.cpp (right):

https://codereview.chromium.org/1702223002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/JSONValuesForV8.cpp:89:
v8::Local<v8::Value> fromJSONString(v8::Local<v8::Context> context, const
String& stringifiedJSON, ExceptionState& exceptionState)

Nit: You can pass in ScriptState rather than v8::Context.

https://codereview.chromium.org/1702223002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/JSONValuesForV8.cpp:93: v8::TryCatch
tryCatch(isolate);

Use scriptState->isolate().

https://codereview.chromium.org/1702223002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/JSONValuesForV8.cpp:96:
exceptionState.rethrowV8Exception(tryCatch.Exception());

Have you confirmed that this throws an exception aligned with the spec?

[please use gerrit instead, 2016-03-02 01:24:34]

Sending to CQ.

https://codereview.chromium.org/1702223002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/JSONValuesForV8.cpp (right):

https://codereview.chromium.org/1702223002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/JSONValuesForV8.cpp:89:
v8::Local<v8::Value> fromJSONString(v8::Local<v8::Context> context, const
String& stringifiedJSON, ExceptionState& exceptionState)
On 2016/03/02 00:46:16, haraken wrote:
> 
> Nit: You can pass in ScriptState rather than v8::Context.

Done.

https://codereview.chromium.org/1702223002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/JSONValuesForV8.cpp:93: v8::TryCatch
tryCatch(isolate);
On 2016/03/02 00:46:16, haraken wrote:
> 
> Use scriptState->isolate().

Done.

https://codereview.chromium.org/1702223002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/JSONValuesForV8.cpp:96:
exceptionState.rethrowV8Exception(tryCatch.Exception());
On 2016/03/02 00:46:16, haraken wrote:
> 
> Have you confirmed that this throws an exception aligned with the spec?
> 

For PaymentResponse, the stringified JSON comes from the browser. The browser
should make sure that it's valid JSON. Therefore, this exception should not
fire. The spec does not mention anything on this topic, but it's not 100%
finalized yet.

[please use gerrit instead, 2016-03-02 01:25:39]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2016-03-02 01:25:40]

The patchset sent to the CQ was uploaded after l-g-t-m from mek@chromium.org,
jbroman@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1702223002/#ps500001
(title: "Rebase and address comments.")

[commit-bot: I haz the power, 2016-03-02 01:26:10]

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

[commit-bot: I haz the power, 2016-03-02 02:40:23]

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

[commit-bot: I haz the power, 2016-03-02 02:40:25]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

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

The CQ bit was checked by rouslan@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, mek@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1702223002/#ps520001
(title: "Fix Windows linking.")

[commit-bot: I haz the power, 2016-03-02 22:57:09]

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

[commit-bot: I haz the power, 2016-03-03 01:46:39]

Committed patchset #24 (id:520001)

[commit-bot: I haz the power, 2016-03-03 01:48:20]

Description was changed from

==========
PaymentRequest API.

An API that allows browsers to act as an intermediary between the three
key parties in a financial transaction: the merchant (e.g. an online web
store), the buyer (e.g. the user buying from the online web store), and
the Payment Method (e.g. credit card). Information necessary to process
and confirm a transaction is passed between the Payment Method and the
merchant via the browser with the buyer confirming and authorizing as
necessary across the flow.

Intent to implement:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/gbSs15ZSWtA/JFU3H7fT...

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

to

==========
PaymentRequest API.

An API that allows browsers to act as an intermediary between the three
key parties in a financial transaction: the merchant (e.g. an online web
store), the buyer (e.g. the user buying from the online web store), and
the Payment Method (e.g. credit card). Information necessary to process
and confirm a transaction is passed between the Payment Method and the
merchant via the browser with the buyer confirming and authorizing as
necessary across the flow.

Intent to implement:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/gbSs15ZSWtA/JFU3H7fT...

BUG=587995

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

[commit-bot: I haz the power, 2016-03-03 01:48:21]

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