Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2170783002:
Add a timeout to the update event in case page doesn't resolve promise from updateWith (Closed)
DescriptionAdd a timeout to the update event in case page doesn't resolve promise from updateWith
Specification:
https://www.w3.org/TR/payment-request/#updatewith
BUG=629462
Committed: https://crrev.com/92c5d3d1a1c2659c20c9ac7eb2e5de90c2199f0b
Cr-Commit-Position: refs/heads/master@{#407082}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Fixed review comments #
Total comments: 2
Patch Set 3 : Added a comment #Patch Set 4 : Comment #Patch Set 5 : Missed the DCHECK #
Messages
Total messages: 29 (15 generated)
sanjoy.pal@samsung.com changed reviewers: + rouslan@chromium.org
PTAL.
Description was changed from ========== Add a timeout to the update event in case page doesn't resolve promise from updateWith BUG=629462 ========== to ========== Add a timeout to the update event in case page doesn't resolve promise from updateWith Specification: https://www.w3.org/TR/payment-request/#updatewith BUG=629462 ==========
rouslan@chromium.org changed reviewers: + mek@chromium.org
mek@: ptal. Timers+events+GC are tricky to get right. Can you sanity check this patch? https://codereview.chromium.org/2170783002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEvent.cpp (right): https://codereview.chromium.org/2170783002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEvent.cpp:96: m_abortTimer.startOneShot(s_abortTimeout, BLINK_FROM_HERE); #include "public/platform/WebTraceLocation.h" This is for BLINK_FROM_HERE, because of the "include what you use" guideline. https://codereview.chromium.org/2170783002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEvent.h (right): https://codereview.chromium.org/2170783002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEvent.h:37: static const int s_abortTimeout = 5; s_waitForUpdateSeconds = 60; 60 seconds seems more reasonable to me. We need to accommodate slow internet connections as well. Move this variable into the anonymous namespace in PaymentRequestUpdateEvent.cc, because it's used only inside of PaymentRequestUpdateEvent.cc. mek@: In Chromium these variables are formatted kLikeThis. Does Blink follow different style here? https://codereview.chromium.org/2170783002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEvent.h:42: void onTimerFired(Timer<PaymentRequestUpdateEvent>*); Make this public and call it from your unit test. This makes your unit test faster. https://codereview.chromium.org/2170783002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEventTest.cpp (right): https://codereview.chromium.org/2170783002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEventTest.cpp:70: testing::enterRunLoop(); This test will run 5 seconds. If I want to change the timeout to 60 seconds, then this test will run for 1 minute. That's too slow. Instead of running the loop, you should manually call onTimerFired() from this test. That also eliminates the need for MockPaymentUpdaterForTimeout.
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2170783002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEvent.cpp (right): https://codereview.chromium.org/2170783002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEvent.cpp:96: m_abortTimer.startOneShot(s_abortTimeout, BLINK_FROM_HERE); What happens if you hit setPaymentDetailsUpdater before the existing m_abortTimer is fired? Or can't it happen?
https://codereview.chromium.org/2170783002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEvent.cpp (right): https://codereview.chromium.org/2170783002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEvent.cpp:96: m_abortTimer.startOneShot(s_abortTimeout, BLINK_FROM_HERE); On 2016/07/21 16:37:27, haraken wrote: > > What happens if you hit setPaymentDetailsUpdater before the existing > m_abortTimer is fired? Or can't it happen? Should not happen. setPaymentDetailsUpdater is invoked right after the contructor: PaymentRequestUpdateEvent* event = PaymentRequestUpdateEvent::create(EventTypeNames::shippingaddresschange); event->setTarget(this); event->setPaymentDetailsUpdater(this); bool success = getExecutionContext()->getEventQueue()->enqueueEvent(event); https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen... We can make it more clear but firing the event inside of the constructor, but we have two constructors. So we would have some code repetition. Is this a good idea?
On 2016/07/21 16:46:42, Rouslan (ツ) wrote: > https://codereview.chromium.org/2170783002/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEvent.cpp > (right): > > https://codereview.chromium.org/2170783002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEvent.cpp:96: > m_abortTimer.startOneShot(s_abortTimeout, BLINK_FROM_HERE); > On 2016/07/21 16:37:27, haraken wrote: > > > > What happens if you hit setPaymentDetailsUpdater before the existing > > m_abortTimer is fired? Or can't it happen? > > Should not happen. setPaymentDetailsUpdater is invoked right after the > contructor: > > PaymentRequestUpdateEvent* event = > PaymentRequestUpdateEvent::create(EventTypeNames::shippingaddresschange); > event->setTarget(this); > event->setPaymentDetailsUpdater(this); > bool success = getExecutionContext()->getEventQueue()->enqueueEvent(event); > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen... > > We can make it more clear but firing the event inside of the constructor, but we > have two constructors. So we would have some code repetition. Is this a good > idea? Makes sense. You can just add DCHECK(!m_abortTimer.isActive()) before calling startOneShot(). That way you can check if the timer is not yet activated. From the GC + Timer perspective, LGTM.
sanjoy.pal@. please address all comments before submitting.
looks good, with the suggested changes to the test to not actually wait for the timeout
Thanks for the review. I have made the changes as suggested. PTAL. https://codereview.chromium.org/2170783002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEvent.cpp (right): https://codereview.chromium.org/2170783002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEvent.cpp:96: m_abortTimer.startOneShot(s_abortTimeout, BLINK_FROM_HERE); On 2016/07/21 16:35:24, rouslan wrote: > #include "public/platform/WebTraceLocation.h" > > This is for BLINK_FROM_HERE, because of the "include what you use" guideline. Done. https://codereview.chromium.org/2170783002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEvent.h (right): https://codereview.chromium.org/2170783002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEvent.h:37: static const int s_abortTimeout = 5; On 2016/07/21 16:35:24, rouslan wrote: > s_waitForUpdateSeconds = 60; > > 60 seconds seems more reasonable to me. We need to accommodate slow internet > connections as well. > > Move this variable into the anonymous namespace in PaymentRequestUpdateEvent.cc, > because it's used only inside of PaymentRequestUpdateEvent.cc. > > mek@: In Chromium these variables are formatted kLikeThis. Does Blink follow > different style here? Done. https://codereview.chromium.org/2170783002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEvent.h:42: void onTimerFired(Timer<PaymentRequestUpdateEvent>*); On 2016/07/21 16:35:24, rouslan wrote: > Make this public and call it from your unit test. This makes your unit test > faster. Done. https://codereview.chromium.org/2170783002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEventTest.cpp (right): https://codereview.chromium.org/2170783002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEventTest.cpp:70: testing::enterRunLoop(); On 2016/07/21 16:35:24, rouslan wrote: > This test will run 5 seconds. If I want to change the timeout to 60 seconds, > then this test will run for 1 minute. That's too slow. Instead of running the > loop, you should manually call onTimerFired() from this test. That also > eliminates the need for MockPaymentUpdaterForTimeout. Done.
Patchset #2 (id:20001) has been deleted
lgtm
lgtm https://codereview.chromium.org/2170783002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEvent.cpp (right): https://codereview.chromium.org/2170783002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEvent.cpp:17: static const int abortTimeout = 60; Maybe add a comment to mention what unit the 60 is in
https://codereview.chromium.org/2170783002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEvent.cpp (right): https://codereview.chromium.org/2170783002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequestUpdateEvent.cpp:17: static const int abortTimeout = 60; On 2016/07/22 04:18:46, Marijn Kruisselbrink wrote: > Maybe add a comment to mention what unit the 60 is in Done.
The CQ bit was checked by sanjoy.pal@samsung.com 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 checked by sanjoy.pal@samsung.com 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: This issue passed the CQ dry run.
The CQ bit was checked by sanjoy.pal@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, rouslan@chromium.org, mek@chromium.org Link to the patchset: https://codereview.chromium.org/2170783002/#ps100001 (title: "Missed the DCHECK")
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 ========== Add a timeout to the update event in case page doesn't resolve promise from updateWith Specification: https://www.w3.org/TR/payment-request/#updatewith BUG=629462 ========== to ========== Add a timeout to the update event in case page doesn't resolve promise from updateWith Specification: https://www.w3.org/TR/payment-request/#updatewith BUG=629462 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add a timeout to the update event in case page doesn't resolve promise from updateWith Specification: https://www.w3.org/TR/payment-request/#updatewith BUG=629462 ========== to ========== Add a timeout to the update event in case page doesn't resolve promise from updateWith Specification: https://www.w3.org/TR/payment-request/#updatewith BUG=629462 Committed: https://crrev.com/92c5d3d1a1c2659c20c9ac7eb2e5de90c2199f0b Cr-Commit-Position: refs/heads/master@{#407082} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/92c5d3d1a1c2659c20c9ac7eb2e5de90c2199f0b Cr-Commit-Position: refs/heads/master@{#407082} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
