Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2632463003:
Add a timeout if the page doesn't call complete() in a timely fashion. (Closed)
|
Created:
3 years, 11 months ago by Justin Donnelly Modified:
3 years, 11 months ago CC:
chromium-reviews, pkl (ping after 24h if needed), gogerald+paymentswatch_chromium.org, rouslan+payments_chromium.org, noyau+watch_chromium.org, marq+watch_chromium.org, sebsg+paymentswatch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a timeout if the page doesn't call complete() in a timely fashion.
Also:
- Periodically execute a JS noop to work around an issue where the JS event queue is blocked when presenting the Payment Request UI.
- Remove handling for rejecting PaymentResponse, since the spec doesn't actually call for ever doing this. (See https://www.w3.org/TR/payment-request/#complete-method.)
BUG=679399, 677337
Review-Url: https://codereview.chromium.org/2632463003
Cr-Commit-Position: refs/heads/master@{#444480}
Committed: https://chromium.googlesource.com/chromium/src/+/c2b04f241c4d84d52c9c01c8044007c92cbb7ae4
Patch Set 1 #
Total comments: 12
Patch Set 2 : Merge #Patch Set 3 : Fix comment. #
Messages
Total messages: 19 (11 generated)
The CQ bit was checked by jdonnelly@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 ========== Timeout BUG= ========== to ========== Timeout BUG=679399,677337 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Timeout BUG=679399,677337 ========== to ========== Add a timeout if the page doesn't call complete() in a timely fashion. Also: - Periodically execute a JS noop to work around an issue where the JS event queue is blocked when presenting the Payment Request UI. - Remove handling for rejecting PaymentResponse, since the spec doesn't actually call for ever doing this. (See https://www.w3.org/TR/payment-request/#complete-method.) BUG=679399,677337 ==========
jdonnelly@chromium.org changed reviewers: + lpromero@chromium.org, mahmadi@chromium.org
Rubberstamp lgtm. You might want another reviewer on the js changes. https://codereview.chromium.org/2632463003/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/js_payment_request_manager.h (right): https://codereview.chromium.org/2632463003/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/js_payment_request_manager.h:18: // Execute a JS noop function. This is used to work around an issue where the JS Executes
Thanks for the fixes Justin! I posted a few comments. https://codereview.chromium.org/2632463003/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/payment_request_manager.mm (right): https://codereview.chromium.org/2632463003/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_manager.mm:351: repeats:YES] retain]); is executeNoop what fixes the landscape issue? Why is it only necessary after the payment is completed? https://codereview.chromium.org/2632463003/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/resources/payment_request_manager.js (left): https://codereview.chromium.org/2632463003/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/resources/payment_request_manager.js:343: if (__gCrWeb['paymentRequestManager'].pendingResponse) { Should we have a check that __gCrWeb['paymentRequestManager'].pendingResponse isn't null in case the page makes subsequent calls to complete()? https://codereview.chromium.org/2632463003/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/resources/payment_request_manager.js (right): https://codereview.chromium.org/2632463003/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/resources/payment_request_manager.js:347: // always resolve the promise. Does that mean we should resolve event if reject is called? or do nothing? Also, the comment is a bit confusing. The reject function isn't really provided, it is called on the promise. https://codereview.chromium.org/2632463003/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/resources/payment_request_manager.js:355: // If the page has not yet provided a resolve function, do nothing. This can I also find this comment confusing. The page doesn't really provide a resolve function, it resolves the promise returned by the complete() function. Could you change this to something similar to: "We can reach here if the UI times out while waiting for the page to call complete(). In that case this.resolve_ is null. If so, do nothing and return."
https://codereview.chromium.org/2632463003/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/payment_request_manager.mm (right): https://codereview.chromium.org/2632463003/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_manager.mm:351: repeats:YES] retain]); On 2017/01/16 21:52:58, moe wrote: > is executeNoop what fixes the landscape issue? Why is it only necessary after > the payment is completed? Correct, this fixes the landscape issue. Switching orientation is one thing that causes the event queue to wake up, but it turns out that any messaging to the web view also solves the issue. It's necessary when the user hits the Pay button, which is what this method is handling. At that point, the PR.show() promise is resolved and that's when the page may do some asynchronous work (xhr or setTimeout simulating same). https://codereview.chromium.org/2632463003/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/resources/payment_request_manager.js (left): https://codereview.chromium.org/2632463003/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/resources/payment_request_manager.js:343: if (__gCrWeb['paymentRequestManager'].pendingResponse) { On 2017/01/16 21:52:58, moe wrote: > Should we have a check that __gCrWeb['paymentRequestManager'].pendingResponse > isn't null in case the page makes subsequent calls to complete()? I don't think that's possible, given that the only way to get a PaymentResponse is by calling show() and having the promise be resolved which sets pendingResponse. I do think that it would be good to have a check here to make sure the page isn't calling complete more than once. Since you're blocked on me landing this CL, maybe you could add a variable that's set and checked here in your CL? https://codereview.chromium.org/2632463003/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/resources/payment_request_manager.js (right): https://codereview.chromium.org/2632463003/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/resources/payment_request_manager.js:347: // always resolve the promise. On 2017/01/16 21:52:58, moe wrote: > Does that mean we should resolve event if reject is called? or do nothing? > Also, the comment is a bit confusing. The reject function isn't really provided, > it is called on the promise. Only the user agent can call reject and we never do that. I generally find Promises to be a great tool, but confusing. I always have to walk through what's happening to make sure I'm getting it straight. But a reject function *is* provided. Here's an example of how the promise API is used in a page: request.show() .then(function(paymentResponse) { instrumentResponse.complete('success'); }) .catch(function(err) { alert(err); }); The page calls show() which returns a Promise. The page calls then() on the Promise and provides a function as an argument. That function is the resolver. Likewise, what my comment refers to as the "reject function" is the anonymous function provided by the page as an argument to catch(). We can ignore that function because we (the user agent) are never directed by the spec to reject the promise (i.e. call the reject function). What always confuses me about this is that there's plumbing that seems slightly magic that takes the arguments to then() and catch() and supplies them to me, the creator of the Promise, via the anonymous function that I give to the Promise constructor. https://codereview.chromium.org/2632463003/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/resources/payment_request_manager.js:355: // If the page has not yet provided a resolve function, do nothing. This can On 2017/01/16 21:52:58, moe wrote: > I also find this comment confusing. The page doesn't really provide a resolve > function, it resolves the promise returned by the complete() function. Could you > change this to something similar to: > "We can reach here if the UI times out while waiting for the page to call > complete(). In that case this.resolve_ is null. If so, do nothing and return." Similar comments as above. The page can call complete() but that doesn't necessarily mean we'd have a value for resolve_. That value is provided by the call to then() on the Promise returned by complete().
lgtm https://codereview.chromium.org/2632463003/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/resources/payment_request_manager.js (left): https://codereview.chromium.org/2632463003/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/resources/payment_request_manager.js:343: if (__gCrWeb['paymentRequestManager'].pendingResponse) { On 2017/01/18 19:36:21, Justin Donnelly wrote: > On 2017/01/16 21:52:58, moe wrote: > > Should we have a check that __gCrWeb['paymentRequestManager'].pendingResponse > > isn't null in case the page makes subsequent calls to complete()? > > I don't think that's possible, given that the only way to get a PaymentResponse > is by calling show() and having the promise be resolved which sets > pendingResponse. I do think that it would be good to have a check here to make > sure the page isn't calling complete more than once. Since you're blocked on me > landing this CL, maybe you could add a variable that's set and checked here in > your CL? Makes sense. https://codereview.chromium.org/2632463003/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/resources/payment_request_manager.js (right): https://codereview.chromium.org/2632463003/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/resources/payment_request_manager.js:347: // always resolve the promise. On 2017/01/18 19:36:21, Justin Donnelly wrote: > On 2017/01/16 21:52:58, moe wrote: > > Does that mean we should resolve event if reject is called? or do nothing? > > Also, the comment is a bit confusing. The reject function isn't really > provided, > > it is called on the promise. > > Only the user agent can call reject and we never do that. I generally find > Promises to be a great tool, but confusing. I always have to walk through what's > happening to make sure I'm getting it straight. But a reject function *is* > provided. Here's an example of how the promise API is used in a page: > > request.show() > .then(function(paymentResponse) { > instrumentResponse.complete('success'); > }) > .catch(function(err) { > alert(err); > }); > > The page calls show() which returns a Promise. The page calls then() on the > Promise and provides a function as an argument. That function is the resolver. > Likewise, what my comment refers to as the "reject function" is the anonymous > function provided by the page as an argument to catch(). We can ignore that > function because we (the user agent) are never directed by the spec to reject > the promise (i.e. call the reject function). > > What always confuses me about this is that there's plumbing that seems slightly > magic that takes the arguments to then() and catch() and supplies them to me, > the creator of the Promise, via the anonymous function that I give to the > Promise constructor. Thanks for the explanation. I was confused about who is providing and therefore resolving/rejecting the promise.
https://codereview.chromium.org/2632463003/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/js_payment_request_manager.h (right): https://codereview.chromium.org/2632463003/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/js_payment_request_manager.h:18: // Execute a JS noop function. This is used to work around an issue where the JS On 2017/01/13 10:24:29, lpromero wrote: > Executes Done.
The CQ bit was checked by jdonnelly@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lpromero@chromium.org, mahmadi@chromium.org Link to the patchset: https://codereview.chromium.org/2632463003/#ps40001 (title: "Fix comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484773297677430,
"parent_rev": "0e3b718d3342338cb1ba21f4e3ba8c508ca807ae", "commit_rev":
"c2b04f241c4d84d52c9c01c8044007c92cbb7ae4"}
Message was sent while issue was closed.
Description was changed from ========== Add a timeout if the page doesn't call complete() in a timely fashion. Also: - Periodically execute a JS noop to work around an issue where the JS event queue is blocked when presenting the Payment Request UI. - Remove handling for rejecting PaymentResponse, since the spec doesn't actually call for ever doing this. (See https://www.w3.org/TR/payment-request/#complete-method.) BUG=679399,677337 ========== to ========== Add a timeout if the page doesn't call complete() in a timely fashion. Also: - Periodically execute a JS noop to work around an issue where the JS event queue is blocked when presenting the Payment Request UI. - Remove handling for rejecting PaymentResponse, since the spec doesn't actually call for ever doing this. (See https://www.w3.org/TR/payment-request/#complete-method.) BUG=679399,677337 Review-Url: https://codereview.chromium.org/2632463003 Cr-Commit-Position: refs/heads/master@{#444480} Committed: https://chromium.googlesource.com/chromium/src/+/c2b04f241c4d84d52c9c01c80440... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/c2b04f241c4d84d52c9c01c80440... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
