Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2808573002:
PaymentRequest: Change the timer task type. (Closed)
|
Created:
3 years, 8 months ago by zino Modified:
3 years, 8 months ago CC:
chromium-reviews, rouslan+payments_chromium.org, gogerald+paymentswatch_chromium.org, blink-reviews, mahmadi+paymentswatch_chromium.org, sebsg+paymentswatch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPaymentRequest: Change the timer task type.
Moved the timer task from per-renderer to per-frame in the following CL.
https://codereview.chromium.org/2640293002/
At that time, the spec didn't mention it properly but it has become clear
recently. The spec says that we should use UserInteraction task source.
Related spec change:
https://github.com/w3c/browser-payment-api/pull/405
BUG=none
Review-Url: https://codereview.chromium.org/2808573002
Cr-Commit-Position: refs/heads/master@{#463353}
Committed: https://chromium.googlesource.com/chromium/src/+/85be9792615f94cd01c32c7232314e969ff03880
Patch Set 1 #
Total comments: 3
Patch Set 2 : rebase #Messages
Total messages: 17 (7 generated)
Description was changed from ========== PaymentRequest: Change the timer task type. Moved the timer task from per-renderer to per-frame in the following CLs. https://codereview.chromium.org/2640293002/ https://codereview.chromium.org/2640233003/ At that time, the spec didn't mention it properly but it has become clear recently. The spec says that we should use UserInteraction task source. Related spec change: https://github.com/w3c/browser-payment-api/pull/405 BUG=none ========== to ========== PaymentRequest: Change the timer task type. Moved the timer task from per-renderer to per-frame in the following CLs. https://codereview.chromium.org/2640293002/ https://codereview.chromium.org/2640233003/ At that time, the spec didn't mention it properly but it has become clear recently. The spec says that we should use UserInteraction task source. Related spec change: https://github.com/w3c/browser-payment-api/pull/405 BUG=none ==========
jinho.bang@samsung.com changed reviewers: + haraken@chromium.org, rouslan@chromium.org
PTAL
LGTM
https://codereview.chromium.org/2808573002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2808573002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:857: m_completeTimer(TaskRunnerHelper::get(TaskType::UserInteraction, frame()), Please don't change this line. According to the spec, user interaction timers should be used only for 'shippingaddresschange' and 'shippingoptionchange' methods. These two timers are in PaymentRequestUpdateEvent.abort_timer_. So you're good there. There's some language about queueing a task in user interaction task source for .abort() method, but there's no delay specified in there. Because Chromium's implementation of .abort() is asynchronous, we don't need a timer.
https://codereview.chromium.org/2808573002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2808573002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:857: m_completeTimer(TaskRunnerHelper::get(TaskType::UserInteraction, frame()), On 2017/04/10 15:35:06, ಠ_ಠ wrote: > Please don't change this line. > > According to the spec, user interaction timers should be used only for > 'shippingaddresschange' and 'shippingoptionchange' methods. These two timers are > in PaymentRequestUpdateEvent.abort_timer_. So you're good there. > > There's some language about queueing a task in user interaction task source for > .abort() method, but there's no delay specified in there. Because Chromium's > implementation of .abort() is asynchronous, we don't need a timer. I was confused here: User accepts the payment request algorithm https://w3c.github.io/browser-payment-api/#user-accepts-the-payment-request-a... https://w3c.github.io/browser-payment-api/#complete-method But my thought was incorrect according to your explaination. After show() has been resolved, if we do not call complete(), then user interface is blocked. So, it's used for causing timeout if the page never calls complete(). Then, do we have to change it to kTimer vs kUnspecedTimer? WDYT?
https://codereview.chromium.org/2808573002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2808573002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:857: m_completeTimer(TaskRunnerHelper::get(TaskType::UserInteraction, frame()), On 2017/04/10 16:11:12, zino wrote: > On 2017/04/10 15:35:06, ಠ_ಠ wrote: > > Please don't change this line. > > > > According to the spec, user interaction timers should be used only for > > 'shippingaddresschange' and 'shippingoptionchange' methods. These two timers > are > > in PaymentRequestUpdateEvent.abort_timer_. So you're good there. > > > > There's some language about queueing a task in user interaction task source > for > > .abort() method, but there's no delay specified in there. Because Chromium's > > implementation of .abort() is asynchronous, we don't need a timer. > > I was confused here: User accepts the payment request algorithm > https://w3c.github.io/browser-payment-api/#user-accepts-the-payment-request-a... > https://w3c.github.io/browser-payment-api/#complete-method > > But my thought was incorrect according to your explaination. > After show() has been resolved, if we do not call complete(), then user > interface is blocked. > So, it's used for causing timeout if the page never calls complete(). > > Then, do we have to change it to kTimer vs kUnspecedTimer? WDYT? We cannot keep MiscPlatformAPI task type?
I've filed https://github.com/w3c/browser-payment-api/issues/508 to define the algorithm for `.complete()` timeout in the spec. That should clarify the task type. You're OK to land the change for PaymentRequestUpdateEvent in the meantime.
On 2017/04/10 16:18:02, ಠ_ಠ wrote: > I've filed https://github.com/w3c/browser-payment-api/issues/508 to define the > algorithm for `.complete()` timeout in the spec. That should clarify the task > type. You're OK to land the change for PaymentRequestUpdateEvent in the > meantime. Okay, I've just uploaded a new patch set. Thanks.
Description was changed from ========== PaymentRequest: Change the timer task type. Moved the timer task from per-renderer to per-frame in the following CLs. https://codereview.chromium.org/2640293002/ https://codereview.chromium.org/2640233003/ At that time, the spec didn't mention it properly but it has become clear recently. The spec says that we should use UserInteraction task source. Related spec change: https://github.com/w3c/browser-payment-api/pull/405 BUG=none ========== to ========== PaymentRequest: Change the timer task type. Moved the timer task from per-renderer to per-frame in the following CL. https://codereview.chromium.org/2640293002/ At that time, the spec didn't mention it properly but it has become clear recently. The spec says that we should use UserInteraction task source. Related spec change: https://github.com/w3c/browser-payment-api/pull/405 BUG=none ==========
lgtm
The CQ bit was checked by jinho.bang@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2808573002/#ps20001 (title: "rebase")
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": 20001, "attempt_start_ts": 1491845236650410,
"parent_rev": "dde55ab75d6af6c19e99372ef7678c38915be7f6", "commit_rev":
"85be9792615f94cd01c32c7232314e969ff03880"}
Message was sent while issue was closed.
Description was changed from ========== PaymentRequest: Change the timer task type. Moved the timer task from per-renderer to per-frame in the following CL. https://codereview.chromium.org/2640293002/ At that time, the spec didn't mention it properly but it has become clear recently. The spec says that we should use UserInteraction task source. Related spec change: https://github.com/w3c/browser-payment-api/pull/405 BUG=none ========== to ========== PaymentRequest: Change the timer task type. Moved the timer task from per-renderer to per-frame in the following CL. https://codereview.chromium.org/2640293002/ At that time, the spec didn't mention it properly but it has become clear recently. The spec says that we should use UserInteraction task source. Related spec change: https://github.com/w3c/browser-payment-api/pull/405 BUG=none Review-Url: https://codereview.chromium.org/2808573002 Cr-Commit-Position: refs/heads/master@{#463353} Committed: https://chromium.googlesource.com/chromium/src/+/85be9792615f94cd01c32c723231... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/85be9792615f94cd01c32c723231... |
