Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2250793003:
Separate PaymentRequest initialization and display. (Closed)
|
Created:
4 years, 4 months ago by please use gerrit instead Modified:
4 years, 4 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin (slow to review), dglazkov+blink, haraken, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSeparate PaymentRequest initialization and display.
In order to better accommodate delays in querying payment apps, separate
initialization of PaymentRequest from displaying it. Thus a merchant
website can create a JavaScript PaymentRequest object on page load, but
call PaymentRequest.show() only when user clicks the [BUY] button on the
page. With this patch, Chrome begins to query the installed payment apps
before the user clicks the [BUY] button. This enables faster startup of
the PaymentRequest UI.
BUG=638454
Committed: https://crrev.com/2525f9688bbb6fd26ff12d38a7c4ee5f31404122
Cr-Commit-Position: refs/heads/master@{#413893}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Set client in init(). Guard against multiple calls to show(). #Patch Set 3 : Update payment-request-mock.js #Messages
Total messages: 59 (47 generated)
The CQ bit was checked by rouslan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Separate PaymentRequest initialization and display. In order to better accommodate delays in querying payment apps, separate initialization of PaymentRequest from displaying it. Thus a merchant website can create a JavaScript PaymentRequest object on page load, but call PaymentRequest.show() only when user clicks the [BUY] button on the page. With this patch, Chrome begins to query the installed payment apps before the user clicks the [BUY] button. This enables faster startup of the PaymentRequest UI. BUG=NONE ========== to ========== Separate PaymentRequest initialization and display. In order to better accommodate delays in querying payment apps, separate initialization of PaymentRequest from displaying it. Thus a merchant website can create a JavaScript PaymentRequest object on page load, but call PaymentRequest.show() only when user clicks the [BUY] button on the page. With this patch, Chrome begins to query the installed payment apps before the user clicks the [BUY] button. This enables faster startup of the PaymentRequest UI. BUG=638454 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by rouslan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by rouslan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by rouslan@chromium.org to run a CQ dry run
Patchset #1 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by rouslan@chromium.org to run a CQ dry run
Patchset #1 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by rouslan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:70008) has been deleted
The CQ bit was checked by rouslan@chromium.org to run a CQ dry run
Patchset #1 (id:90001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
rouslan@chromium.org changed reviewers: + dfalcantara@chromium.org, mek@chromium.org, palmer@chromium.org
dfalcantara, ptal Java. mek, ptal Blink. palmer, ptal Mojom.
rouslan@chromium.org changed reviewers: + tsepez@chromium.org - palmer@chromium.org
tsepez, ptal Mojom.
lgtm, but one question around the mojom interface https://codereview.chromium.org/2250793003/diff/110001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/payments/payment_request.mojom (right): https://codereview.chromium.org/2250793003/diff/110001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/payments/payment_request.mojom:134: Init(array<PaymentMethodData> methodData, Would it make sense to pass the PaymentRequestClient also as part of the Init call? Not sure what the reasons for keeping it separate/merging them would be.
https://codereview.chromium.org/2250793003/diff/110001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/payments/payment_request.mojom (right): https://codereview.chromium.org/2250793003/diff/110001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/payments/payment_request.mojom:134: Init(array<PaymentMethodData> methodData, On 2016/08/22 16:46:15, Marijn Kruisselbrink wrote: > Would it make sense to pass the PaymentRequestClient also as part of the Init > call? Not sure what the reasons for keeping it separate/merging them would be. Good idea. I'm on it.
Java bits lgtm https://chromiumcodereview.appspot.com/2250793003/diff/110001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://chromiumcodereview.appspot.com/2250793003/diff/110001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:351: public void show() { Should we be worried about show being called twice?
The CQ bit was checked by rouslan@chromium.org to run a CQ dry run
https://codereview.chromium.org/2250793003/diff/110001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2250793003/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:351: public void show() { On 2016/08/22 17:25:24, dfalcantara wrote: > Should we be worried about show being called twice? Good point. A corrupted renderer can call show() twice. I've changed the Java code to gracefully handle that.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rouslan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
mojom lgtm
The CQ bit was checked by rouslan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mek@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2250793003/#ps150001 (title: "Update payment-request-mock.js")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by rouslan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Separate PaymentRequest initialization and display. In order to better accommodate delays in querying payment apps, separate initialization of PaymentRequest from displaying it. Thus a merchant website can create a JavaScript PaymentRequest object on page load, but call PaymentRequest.show() only when user clicks the [BUY] button on the page. With this patch, Chrome begins to query the installed payment apps before the user clicks the [BUY] button. This enables faster startup of the PaymentRequest UI. BUG=638454 ========== to ========== Separate PaymentRequest initialization and display. In order to better accommodate delays in querying payment apps, separate initialization of PaymentRequest from displaying it. Thus a merchant website can create a JavaScript PaymentRequest object on page load, but call PaymentRequest.show() only when user clicks the [BUY] button on the page. With this patch, Chrome begins to query the installed payment apps before the user clicks the [BUY] button. This enables faster startup of the PaymentRequest UI. BUG=638454 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:150001)
Message was sent while issue was closed.
Description was changed from ========== Separate PaymentRequest initialization and display. In order to better accommodate delays in querying payment apps, separate initialization of PaymentRequest from displaying it. Thus a merchant website can create a JavaScript PaymentRequest object on page load, but call PaymentRequest.show() only when user clicks the [BUY] button on the page. With this patch, Chrome begins to query the installed payment apps before the user clicks the [BUY] button. This enables faster startup of the PaymentRequest UI. BUG=638454 ========== to ========== Separate PaymentRequest initialization and display. In order to better accommodate delays in querying payment apps, separate initialization of PaymentRequest from displaying it. Thus a merchant website can create a JavaScript PaymentRequest object on page load, but call PaymentRequest.show() only when user clicks the [BUY] button on the page. With this patch, Chrome begins to query the installed payment apps before the user clicks the [BUY] button. This enables faster startup of the PaymentRequest UI. BUG=638454 Committed: https://crrev.com/2525f9688bbb6fd26ff12d38a7c4ee5f31404122 Cr-Commit-Position: refs/heads/master@{#413893} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2525f9688bbb6fd26ff12d38a7c4ee5f31404122 Cr-Commit-Position: refs/heads/master@{#413893} |
