Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(17)

Issue 2718013004: PaymentApp: Implement respondWith() in PaymentRequestEvent. (content side) (Closed)

Created:
3 years, 9 months ago by zino
Modified:
3 years, 8 months ago
CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, kinuko+watch, jsbell+serviceworker_chromium.org, tzik, jam, abarth-chromium, darin-cc_chromium.org, gogerald+paymentswatch_chromium.org, blink-worker-reviews_chromium.org, mlamouri+watch-content_chromium.org, nhiroki, rouslan+payments_chromium.org, michaeln, shimazu+serviceworker_chromium.org, serviceworker-reviews, Aaron Boodman, kinuko+serviceworker, mahmadi+paymentswatch_chromium.org, horo+watch_chromium.org, darin (slow to review), sebsg+paymentswatch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

PaymentApp: Implement respondWith() in PaymentRequestEvent. (content side) The respondWith() method is used by the payment app to provide a PaymentAppResponse when the payment successfully completes. This CL includes a content-side implementation and browser test. Especially, add a callback to InvokePaymentApp() in PaymentAppProvider. The callback is used to pass a response data from payment app using respondWith(). Related Spec Link: https://w3c.github.io/webpayments-payment-apps-api/#idl-def-paymentrequestevent See the other CLs in this series: [1/3] https://codereview.chromium.org/2715663002/ (RespondWithObserver) [2/3] https://codereview.chromium.org/2705293010/ (blink side) [3/3] This patch BUG=661608 TEST=payment_app_browsertest.cc Review-Url: https://codereview.chromium.org/2718013004 Cr-Commit-Position: refs/heads/master@{#460634} Committed: https://chromium.googlesource.com/chromium/src/+/865ce0550e0ba864c2a830bde1f7d74004edca9b

Patch Set 1 #

Patch Set 2 : Invoke #

Patch Set 3 : invoke #

Patch Set 4 : PaymentApp: Implement respondWith() in PaymentRequestEvent. (content side) #

Total comments: 20

Patch Set 5 : PaymentApp: Implement respondWith() in PaymentRequestEvent. (content side) #

Total comments: 26

Patch Set 6 : PaymentApp: Implement respondWith() in PaymentRequestEvent. (content side) #

Patch Set 7 : PaymentApp: Implement respondWith() in PaymentRequestEvent. (content side) #

Patch Set 8 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+208 lines, -92 lines) Patch
M chrome/browser/android/payments/service_worker_payment_app_bridge.cc View 1 2 3 4 5 6 1 chunk +2 lines, -1 line 0 comments Download
M components/payments/content/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M components/payments/content/payment_app.mojom View 1 2 3 4 5 2 chunks +13 lines, -0 lines 0 comments Download
M content/browser/payments/payment_app_browsertest.cc View 1 2 3 chunks +21 lines, -6 lines 0 comments Download
M content/browser/payments/payment_app_content_unittest_base.h View 1 2 3 4 5 6 1 chunk +0 lines, -1 line 0 comments Download
M content/browser/payments/payment_app_content_unittest_base.cc View 1 2 3 4 5 6 7 3 chunks +3 lines, -14 lines 0 comments Download
M content/browser/payments/payment_app_provider_impl.h View 1 2 3 4 5 2 chunks +4 lines, -9 lines 0 comments Download
M content/browser/payments/payment_app_provider_impl.cc View 1 2 3 4 5 7 chunks +62 lines, -19 lines 0 comments Download
M content/browser/payments/payment_app_provider_impl_unittest.cc View 1 2 3 4 5 6 3 chunks +12 lines, -5 lines 0 comments Download
M content/browser/service_worker/embedded_worker_test_helper.h View 1 2 3 4 5 6 7 2 chunks +2 lines, -0 lines 0 comments Download
M content/browser/service_worker/embedded_worker_test_helper.cc View 1 2 3 4 5 6 7 3 chunks +10 lines, -2 lines 0 comments Download
M content/common/service_worker/service_worker_event_dispatcher.mojom View 1 2 3 4 5 6 7 1 chunk +3 lines, -1 line 0 comments Download
M content/public/browser/DEPS View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M content/public/browser/payment_app_provider.h View 1 2 3 4 5 3 chunks +9 lines, -1 line 0 comments Download
M content/renderer/service_worker/service_worker_context_client.h View 1 2 3 4 5 6 7 3 chunks +7 lines, -4 lines 0 comments Download
M content/renderer/service_worker/service_worker_context_client.cc View 1 2 3 4 5 6 7 5 chunks +39 lines, -13 lines 0 comments Download
M content/test/data/payments/payment_app.js View 1 2 1 chunk +18 lines, -15 lines 0 comments Download
M content/test/data/payments/payment_app_window.html View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 46 (21 generated)
zino
PTAL
3 years, 9 months ago (2017-03-07 15:51:13 UTC) #4
Tom Sepez
https://codereview.chromium.org/2718013004/diff/60001/components/payments/content/payment_app.mojom File components/payments/content/payment_app.mojom (right): https://codereview.chromium.org/2718013004/diff/60001/components/payments/content/payment_app.mojom#newcode47 components/payments/content/payment_app.mojom:47: string method_name; What are the restrictions on the values ...
3 years, 9 months ago (2017-03-07 17:22:39 UTC) #5
shimazu
drive-by comments: https://codereview.chromium.org/2718013004/diff/60001/content/browser/payments/payment_app_provider_impl.cc File content/browser/payments/payment_app_provider_impl.cc (right): https://codereview.chromium.org/2718013004/diff/60001/content/browser/payments/payment_app_provider_impl.cc#newcode97 content/browser/payments/payment_app_provider_impl.cc:97: int event_id = active_version->StartRequest( |event_id| vs |event_finish_id| ...
3 years, 9 months ago (2017-03-08 01:39:40 UTC) #7
zino
Thank you for review. PTAL https://codereview.chromium.org/2718013004/diff/60001/content/browser/payments/payment_app_provider_impl.cc File content/browser/payments/payment_app_provider_impl.cc (right): https://codereview.chromium.org/2718013004/diff/60001/content/browser/payments/payment_app_provider_impl.cc#newcode97 content/browser/payments/payment_app_provider_impl.cc:97: int event_id = active_version->StartRequest( ...
3 years, 9 months ago (2017-03-19 04:47:23 UTC) #8
zino
Owners, PTAL
3 years, 9 months ago (2017-03-21 13:05:36 UTC) #9
please use gerrit instead
Great stuff! https://codereview.chromium.org/2718013004/diff/80001/components/payments/content/payment_app.mojom File components/payments/content/payment_app.mojom (right): https://codereview.chromium.org/2718013004/diff/80001/components/payments/content/payment_app.mojom#newcode47 components/payments/content/payment_app.mojom:47: struct PaymentAppResponse { If mojom language supports ...
3 years, 9 months ago (2017-03-21 16:09:42 UTC) #10
nhiroki
service worker lgtm https://codereview.chromium.org/2718013004/diff/80001/content/renderer/service_worker/service_worker_context_client.cc File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/2718013004/diff/80001/content/renderer/service_worker/service_worker_context_client.cc#newcode917 content/renderer/service_worker/service_worker_context_client.cc:917: std::make_pair(payment_request_id, std::move(callback))); It looks strange that ...
3 years, 9 months ago (2017-03-22 02:19:16 UTC) #11
nhiroki
https://codereview.chromium.org/2718013004/diff/80001/content/browser/service_worker/embedded_worker_test_helper.cc File content/browser/service_worker/embedded_worker_test_helper.cc (right): https://codereview.chromium.org/2718013004/diff/80001/content/browser/service_worker/embedded_worker_test_helper.cc#newcode209 content/browser/service_worker/embedded_worker_test_helper.cc:209: payments::mojom::PaymentAppResponseCallbackPtr response_callback, |response_callback| is not used. Is this OK? ...
3 years, 9 months ago (2017-03-22 02:24:48 UTC) #12
haraken
(I'll defer this review to nhiroki@.)
3 years, 9 months ago (2017-03-22 04:29:30 UTC) #13
zino
Thank you for review. rouslan@, I addressed your comments except base::OnceCallback. I'll work on it ...
3 years, 9 months ago (2017-03-22 18:04:45 UTC) #14
please use gerrit instead
On 2017/03/22 18:04:45, zino wrote: > rouslan@, I addressed your comments Looks like the fixes ...
3 years, 9 months ago (2017-03-22 18:19:09 UTC) #15
zino
On 2017/03/22 18:19:09, ಠ_ಠ wrote: > On 2017/03/22 18:04:45, zino wrote: > > rouslan@, I ...
3 years, 9 months ago (2017-03-22 18:30:09 UTC) #17
zino
+Avi@ for non-payment part in content/
3 years, 9 months ago (2017-03-22 18:31:14 UTC) #18
please use gerrit instead
lgtm
3 years, 9 months ago (2017-03-24 17:01:30 UTC) #28
Avi (use Gerrit)
lgtm
3 years, 9 months ago (2017-03-24 17:13:41 UTC) #29
zino
Tom Sepez@ PTAL I left a comment. If we need to add length limits, please ...
3 years, 9 months ago (2017-03-27 15:27:05 UTC) #30
zino
Tom Sepez@, gentle ping
3 years, 8 months ago (2017-03-28 13:51:12 UTC) #31
zino
On 2017/03/28 13:51:12, zino wrote: > Tom Sepez@, gentle ping Tom Sepez@, Could you please ...
3 years, 8 months ago (2017-03-29 15:59:26 UTC) #32
Tom Sepez
lgtm
3 years, 8 months ago (2017-03-29 20:33:16 UTC) #33
zino
+Ted C@ for chrome directory. PTAL (Other reviwers, I'm sorry for spam..)
3 years, 8 months ago (2017-03-30 00:00:04 UTC) #35
Ted C
On 2017/03/30 00:00:04, zino wrote: > +Ted C@ for chrome directory. > > PTAL > ...
3 years, 8 months ago (2017-03-30 00:17:47 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2718013004/160001
3 years, 8 months ago (2017-03-30 00:20:39 UTC) #39
commit-bot: I haz the power
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder ...
3 years, 8 months ago (2017-03-30 02:23:00 UTC) #41
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2718013004/160001
3 years, 8 months ago (2017-03-30 02:58:26 UTC) #43
commit-bot: I haz the power
3 years, 8 months ago (2017-03-30 03:06:46 UTC) #46
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/865ce0550e0ba864c2a830bde1f7...

Powered by Google App Engine
This is Rietveld 408576698