PaymentHandler: Merge PaymentAppRequest and PaymentRequestEvent.

PaymentHandler: Merge PaymentAppRequest and PaymentRequestEvent.

It was changed in spec side[1][2]. There is no reason to have both of those.
Also, this CL is adding new attributes. (e.g. topLevelOrigin, instrumentKey)

[1] https://github.com/w3c/payment-handler/issues/142
[2] https://w3c.github.io/payment-handler/#the-paymentrequestevent

BUG=661608, 720027
Review-Url: https://codereview.chromium.org/2887713002
Cr-Commit-Position: refs/heads/master@{#474791}
Committed: https://chromium.googlesource.com/chromium/src/+/35b8d35a15ee876c0a6a5ce1b69769fabf9bb82b

[zino, 2017-05-16 13:42:17]

Description was changed from

==========
PaymentHandler: Merge PaymentAppRequest and PaymentRequestEvent.

It was changed in spec side[1][2]. There is no reason to have both of those.

[1] https://github.com/w3c/payment-handler/issues/142
[2] https://w3c.github.io/payment-handler/#the-paymentrequestevent

BUG=661608,720027
==========

to

==========
PaymentHandler: Merge PaymentAppRequest and PaymentRequestEvent.

It was changed in spec side[1][2]. There is no reason to have both of those.

[1] https://github.com/w3c/payment-handler/issues/142
[2] https://w3c.github.io/payment-handler/#the-paymentrequestevent

BUG=661608,720027
==========

[zino, 2017-05-21 11:47:08]

Description was changed from

==========
PaymentHandler: Merge PaymentAppRequest and PaymentRequestEvent.

It was changed in spec side[1][2]. There is no reason to have both of those.

[1] https://github.com/w3c/payment-handler/issues/142
[2] https://w3c.github.io/payment-handler/#the-paymentrequestevent

BUG=661608,720027
==========

to

==========
PaymentHandler: Merge PaymentAppRequest and PaymentRequestEvent.

It was changed in spec side[1][2]. There is no reason to have both of those.
Also, this CL is adding new attributes. (e.g. topLevelOrigin, instrumentKey)

[1] https://github.com/w3c/payment-handler/issues/142
[2] https://w3c.github.io/payment-handler/#the-paymentrequestevent

BUG=661608,720027
==========

[zino, 2017-05-21 11:47:56]

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

[zino, 2017-05-21 11:48:19]

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

[commit-bot: I haz the power, 2017-05-21 11:48:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[zino, 2017-05-21 11:51:39]

jinho.bang@samsung.com changed reviewers:
+ gogerald@chromium.org
- haraken@chromium.org

[zino, 2017-05-21 11:51:58]

PTAL

rouslan@, gogerald1@ for payments
Tom Sepez@ for mojom
nhiroki@ for service worker

[commit-bot: I haz the power, 2017-05-21 12:56:33]

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

[commit-bot: I haz the power, 2017-05-21 12:56:34]

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

[nhiroki, 2017-05-21 23:41:40]

lgtm from SW POV.

https://codereview.chromium.org/2887713002/diff/40001/content/test/data/payme...
File content/test/data/payments/payment_app.js (right):

https://codereview.chromium.org/2887713002/diff/40001/content/test/data/payme...
content/test/data/payments/payment_app.js:22: sendResultToTest(e.instrumentKey);
sendResultToTest()s run async operations, so we may need to expand the lifetime
of this event (ie., e.waitUntil()) until they finish in order to prevent SW from
being terminated.

https://codereview.chromium.org/2887713002/diff/40001/content/test/data/payme...
content/test/data/payments/payment_app.js:55: })
ditto. This needs to extend the lifetime of the event.

https://codereview.chromium.org/2887713002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentAppRequest.idl (right):

https://codereview.chromium.org/2887713002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentAppRequest.idl:1: // Copyright
2016 The Chromium Authors. All rights reserved.
Why don't you remove this idl?

[please use gerrit instead, 2017-05-22 14:20:05]

https://codereview.chromium.org/2887713002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentAppRequest.idl (right):

https://codereview.chromium.org/2887713002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentAppRequest.idl:1: // Copyright
2016 The Chromium Authors. All rights reserved.
On 2017/05/21 23:41:40, nhiroki (slow) wrote:
> Why don't you remove this idl?

Seems like all of the PaymentAppRequest* should be deleted or renamed into
PaymentRequestEvent*.

https://codereview.chromium.org/2887713002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.idl (right):

https://codereview.chromium.org/2887713002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestEvent.idl:5: //
https://w3c.github.io/webpayments-payment-apps-api/#idl-def-paymentrequestevent
Update the link please.

https://codereview.chromium.org/2887713002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestEvent.idl:19: //
TODO(gegerald): We should implement this function.
gogerald

https://codereview.chromium.org/2887713002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/payments/WebPaymentAppRequest.h
(right):

https://codereview.chromium.org/2887713002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/payments/WebPaymentAppRequest.h:6:
#define WebPaymentAppRequest_h
Delete or rename?

[Tom Sepez, 2017-05-22 16:14:32]

mojom LGTM

[zino, 2017-05-23 14:51:29]

Thank you for review.

PTAL a new patch set with my comments.

https://codereview.chromium.org/2887713002/diff/40001/content/test/data/payme...
File content/test/data/payments/payment_app.js (right):

https://codereview.chromium.org/2887713002/diff/40001/content/test/data/payme...
content/test/data/payments/payment_app.js:22: sendResultToTest(e.instrumentKey);
On 2017/05/21 23:41:40, nhiroki (slow) wrote:
> sendResultToTest()s run async operations, so we may need to expand the
lifetime
> of this event (ie., e.waitUntil()) until they finish in order to prevent SW
from
> being terminated.

Done.

https://codereview.chromium.org/2887713002/diff/40001/content/test/data/payme...
content/test/data/payments/payment_app.js:55: })
On 2017/05/21 23:41:39, nhiroki (slow) wrote:
> ditto. This needs to extend the lifetime of the event.

I think the lifetime will be extended by e.respondWith() until calling
maybeSendPaymentRequest().

https://codereview.chromium.org/2887713002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentAppRequest.idl (right):

https://codereview.chromium.org/2887713002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentAppRequest.idl:1: // Copyright
2016 The Chromium Authors. All rights reserved.
On 2017/05/22 14:20:04, ಠ_ಠ wrote:
> On 2017/05/21 23:41:40, nhiroki (slow) wrote:
> > Why don't you remove this idl?
> 
> Seems like all of the PaymentAppRequest* should be deleted or renamed into
> PaymentRequestEvent*.

I think the PaymentRequestEvent should add a constructor and init dictionary for
compatibility with DOM events.
So, I sent a pull request[1] to the spec and then left a comment here.
Once the issue is resolved in spec side, I'll update this dictionary and add a
new constructor.

[1] https://github.com/w3c/payment-handler/pull/162

https://codereview.chromium.org/2887713002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.idl (right):

https://codereview.chromium.org/2887713002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestEvent.idl:5: //
https://w3c.github.io/webpayments-payment-apps-api/#idl-def-paymentrequestevent
On 2017/05/22 14:20:05, ಠ_ಠ wrote:
> Update the link please.

Done.

https://codereview.chromium.org/2887713002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestEvent.idl:19: //
TODO(gegerald): We should implement this function.
On 2017/05/22 14:20:05, ಠ_ಠ wrote:
> gogerald

Done.

https://codereview.chromium.org/2887713002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/payments/WebPaymentAppRequest.h
(right):

https://codereview.chromium.org/2887713002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/payments/WebPaymentAppRequest.h:6:
#define WebPaymentAppRequest_h
On 2017/05/22 14:20:05, ಠ_ಠ wrote:
> Delete or rename?

This struct is needed to pass payment app request data from content side to the
blink side.
Currently, the service worker related feature is still in content side. So, we
can not use mojom struct yet in blink side.

Please see this comments in my previous patch:
https://codereview.chromium.org/2528683002/#msg10

[please use gerrit instead, 2017-05-23 15:03:00]

https://codereview.chromium.org/2887713002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/payments/WebPaymentAppRequest.h
(right):

https://codereview.chromium.org/2887713002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/payments/WebPaymentAppRequest.h:6:
#define WebPaymentAppRequest_h
On 2017/05/23 14:51:29, zino wrote:
> On 2017/05/22 14:20:05, ಠ_ಠ wrote:
> > Delete or rename?
> 
> This struct is needed to pass payment app request data from content side to
the
> blink side.
> Currently, the service worker related feature is still in content side. So, we
> can not use mojom struct yet in blink side.
> 
> Please see this comments in my previous patch:
> https://codereview.chromium.org/2528683002/#msg10

Aren't you removing everything named *PaymentAppRequest* and replacing it with
*PaymentRequestEvent* instead?

[please use gerrit instead, 2017-05-23 15:04:32]

https://codereview.chromium.org/2887713002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentAppRequest.idl (right):

https://codereview.chromium.org/2887713002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentAppRequest.idl:8: // Please
see https://github.com/w3c/payment-handler/pull/162
FYI, that issue is for PaymentRequestEvent, whereas this is PaymentAppRequest.

[zino, 2017-05-23 15:15:18]

https://codereview.chromium.org/2887713002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/payments/WebPaymentAppRequest.h
(right):

https://codereview.chromium.org/2887713002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/payments/WebPaymentAppRequest.h:6:
#define WebPaymentAppRequest_h
On 2017/05/23 15:02:59, ಠ_ಠ wrote:
> On 2017/05/23 14:51:29, zino wrote:
> > On 2017/05/22 14:20:05, ಠ_ಠ wrote:
> > > Delete or rename?
> > 
> > This struct is needed to pass payment app request data from content side to
> the
> > blink side.
> > Currently, the service worker related feature is still in content side. So,
we
> > can not use mojom struct yet in blink side.
> > 
> > Please see this comments in my previous patch:
> > https://codereview.chromium.org/2528683002/#msg10
> 
> Aren't you removing everything named *PaymentAppRequest* and replacing it with
> *PaymentRequestEvent* instead?

The request data will be exposed as attributes of PaymentRequestEvent but this
struct is not actual event.
The event object is created in ServiceWorkerGlobalScope[1]. This public struct
is just container to pass the request data such as origin and request id from
content side to blink side.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/ServiceWor...

If we should rename this, I think PaymentRequestEventData is better. WDYT?

[please use gerrit instead, 2017-05-23 15:17:30]

On 2017/05/23 15:15:18, zino wrote:
> If we should rename this, I think PaymentRequestEventData is better. WDYT?

Do you think WebPaymentRequestEvent.h would match the old pattern better?

[zino, 2017-05-23 15:21:36]

On 2017/05/23 15:04:32, ಠ_ಠ wrote:
>
https://codereview.chromium.org/2887713002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/payments/PaymentAppRequest.idl (right):
> 
>
https://codereview.chromium.org/2887713002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/payments/PaymentAppRequest.idl:8: // Please
> see https://github.com/w3c/payment-handler/pull/162
> FYI, that issue is for PaymentRequestEvent, whereas this is PaymentAppRequest.

Yeah, once the issue is resolved, I think we can rename this with
PaymentRequestEventInit and then implement a constructor taking the dictionary
in PaymentRequestEvent.
If you want to rename this with PaymentRequestEventInit, it's also possible. But
we still should have PaymentAppRequest(or PaymentRequestEventData).

[please use gerrit instead, 2017-05-23 15:24:23]

On 2017/05/23 15:21:36, zino wrote:
> we still should have PaymentAppRequest(or PaymentRequestEventData).

The spec has neither of these names. I thought our IDL files must come directly
from the spec, no?

[please use gerrit instead, 2017-05-23 15:36:56]

Ganggui explained what's going on exactly. You're doing it correctly. Sorry for
the confusion.

[please use gerrit instead, 2017-05-23 16:07:46]

So, to be clear, I believe that WebPaymentApPrequest.h is fine to stay, but
PaymentAppRequest.idl seems necessary to remove, because it has been removed
from the spec.

[zino, 2017-05-23 17:06:27]

On 2017/05/23 15:36:56, ಠ_ಠ wrote:
> Ganggui explained what's going on exactly. You're doing it correctly. Sorry
for
> the confusion.

No, I'm sorry to confuse you. (Also, thank you Ganggui@!)

[zino, 2017-05-23 17:33:02]

On 2017/05/23 16:07:46, ಠ_ಠ wrote:
> So, to be clear, I believe that WebPaymentApPrequest.h is fine to stay, but
> PaymentAppRequest.idl seems necessary to remove, because it has been removed
> from the spec.

Thank you for review!

I thought this CL just remove the appRequest attribute in PaymentRequestEvent
and should expose the attributes that PaymentAppRequest had.
I deliberately remained the PaymentAppRequest and thought it should be changed
as PaymentRequestEventInit once the spec issue[1] is resolved.
(FYI, the dictionary IDL is not exposed anyway after this CL. Because the
appRequest attribute was removed..)

[1] https://github.com/w3c/payment-handler/pull/162

I could remove them but it might inefficiency because we need to put them back
here again once the spec issue is resolved. So, I chose to leave TODO comments
instead of removing them.

I quickly made some additional CLs to help you understand.
[2] https://codereview.chromium.org/2898223002/
[3] https://codereview.chromium.org/2903643002

I thought the rename CL[2] is unnecessary but your opinion was reasonable. So, I
tried it.
The constructor CL[3] is eventually fixing the spec issue I mentioned.

[please use gerrit instead, 2017-05-25 14:05:14]

lgtm

[zino, 2017-05-25 16:33:27]

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

[zino, 2017-05-25 16:33:28]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/2887713002/#ps60001
(title: "PaymentHandler: Merge PaymentAppRequest and PaymentRequestEvent.")

[commit-bot: I haz the power, 2017-05-25 16:34:05]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 16:37:05]

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

[commit-bot: I haz the power, 2017-05-25 16:37:06]

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

[zino, 2017-05-25 18:07:58]

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

[zino, 2017-05-25 18:07:59]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
rouslan@chromium.org, nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/2887713002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-25 18:08:37]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 20:43:49]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495735678818430,
"parent_rev": "e4e28d32f77ff5e9bcb20c8f7514bd296bcb2b6d", "commit_rev":
"35b8d35a15ee876c0a6a5ce1b69769fabf9bb82b"}

[commit-bot: I haz the power, 2017-05-25 20:44:05]

Description was changed from

==========
PaymentHandler: Merge PaymentAppRequest and PaymentRequestEvent.

It was changed in spec side[1][2]. There is no reason to have both of those.
Also, this CL is adding new attributes. (e.g. topLevelOrigin, instrumentKey)

[1] https://github.com/w3c/payment-handler/issues/142
[2] https://w3c.github.io/payment-handler/#the-paymentrequestevent

BUG=661608,720027
==========

to

==========
PaymentHandler: Merge PaymentAppRequest and PaymentRequestEvent.

It was changed in spec side[1][2]. There is no reason to have both of those.
Also, this CL is adding new attributes. (e.g. topLevelOrigin, instrumentKey)

[1] https://github.com/w3c/payment-handler/issues/142
[2] https://w3c.github.io/payment-handler/#the-paymentrequestevent

BUG=661608,720027

Review-Url: https://codereview.chromium.org/2887713002
Cr-Commit-Position: refs/heads/master@{#474791}
Committed:
https://chromium.googlesource.com/chromium/src/+/35b8d35a15ee876c0a6a5ce1b697...
==========

[commit-bot: I haz the power, 2017-05-25 20:44:07]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/35b8d35a15ee876c0a6a5ce1b697...