Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
245833002:
Implement async touchmove dispatch during scroll (Closed)
|
Created:
6 years, 8 months ago by jdduke (slow) Modified:
6 years, 8 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionImplement async touchmove dispatch during scroll
This implements a touch dispatch model in which touchmove sending is throttled
while a scroll sequence is active and being consumed. Such touchmove's are
marked with |cancelable = false|, indicating to any potential consumers that the
event cannot be prevented from triggering a platform gesture. Throttling limits
the touchmove sending rate during scrolling to 1 event per 200ms.
BUG=346693
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266470
Patch Set 1 #Patch Set 2 : Cleanup #
Total comments: 26
Patch Set 3 : Fixed tests, will add more #Patch Set 4 : More testing #
Total comments: 24
Patch Set 5 : Code review #Patch Set 6 : Sanity check for async touch + ack timeout #Patch Set 7 : Rebase #Patch Set 8 : Fix tests #Patch Set 9 : Fix win #Messages
Total messages: 59 (0 generated)
This needs some more work (still need to figure out touchstart transitions, as well as determining if we need to special case the larger slop region), but it more or less works as we discussed. We'll definitely want to keep an eye on telemetry as we move forward with this. I was able to notice occasional jank every 200ms on certain heavy sites, though that was with a GN on a debug build.
https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:266: DCHECK(!ignore_ack_); This is a bit ugly. Instead of returning these iterators, can CoalescedWebTouchEvent have higher level methods like ApplyLatencyInfo that apply on the whole set (and do nothing if !ignore_ack_)? https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:296: bool ignore_ack_; Would async_ be a better name? https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:434: // past since sending the last touch event, flush any pending touch moves. nit: "past" -> "passed" https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:435: // TODO(jdduke): Should the touchmove be dropped if there are other pending I don't think it should be dropped. I think it's a good property that the app eventually gets the full touchmove motion. https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:442: // If there's a pending touchmove, coalescewith the new event. Otherwise nit: missing space after "coalesce" https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:532: // TODO(jdduke): Should this be flushed? I think it should. https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.h:59: TOUCH_SCROLLING_MODE_ABSORB_TOUCHMOVE, Can we delete this mode as part of this patch? There's no reason to ever turn it on again even for debugging. https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.h:218: bool async_touch_moves_; Instead of these bools, can we keep the enum value? https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.h:220: double last_sent_touch_timestamp_; Please specify "ms" or "sec" in the variable name.
Thanks Jared. Some initial thoughts. https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:397: FilterBeforeForwarding(touch_queue_.front()->coalesced_event().event); Thanks for the cleanup here - new names/signatures make things a little clearer IMHO. https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:435: // TODO(jdduke): Should the touchmove be dropped if there are other pending On 2014/04/22 01:56:39, aelias wrote: > I don't think it should be dropped. I think it's a good property that the app > eventually gets the full touchmove motion. Agreed. Coalescing according to our rules is the way we deal with event overload - I don't think we want to drop beyond that. The way this is written, throttling follows the exact same rules as coalescing, right? I.e. whenever you see a touchstart or touchend you know you've received the latest touchmove state. I'm thinking of scenarios like dragging one finger then adding a second. I think ideally sites should be able to differentiate that from placing one finger and mostly holding steady then placing another (even when there's some jank on the thread). https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:445: DCHECK(pending_async_touch_move_->CanCoalesceWith(touch)); Is this really guaranteed to be true? Eg. what if the modifiers change during a touch stream? https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.h:59: TOUCH_SCROLLING_MODE_ABSORB_TOUCHMOVE, On 2014/04/22 01:56:39, aelias wrote: > Can we delete this mode as part of this patch? There's no reason to ever turn > it on again even for debugging. Replacing ABSORB with ASYNC is OK with me (async just adds some things on top of absorb). https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.h:218: bool async_touch_moves_; On 2014/04/22 01:56:39, aelias wrote: > Instead of these bools, can we keep the enum value? We can remove this bool completely when we collapse the two modes into one. The above bool needs to stay because it's what toggles back and forth depending on GSU consumption. https://codereview.chromium.org/245833002/diff/20001/content/common/input/web... File content/common/input/web_input_event_traits.cc (right): https://codereview.chromium.org/245833002/diff/20001/content/common/input/web... content/common/input/web_input_event_traits.cc:366: return static_cast<const WebTouchEvent&>(event).cancelable; You're missing a !. Cancelable events must wait for their disposition. I've got an initial (as-yet untested) version of the blink-side patch that adds this field here: https://codereview.chromium.org/247483002/ https://codereview.chromium.org/245833002/diff/20001/content/common/input/web... File content/common/input/web_input_event_traits.h (right): https://codereview.chromium.org/245833002/diff/20001/content/common/input/web... content/common/input/web_input_event_traits.h:25: static bool IgnoresAckDisposition(blink::WebInputEvent::Type type); Can we get rid of this type-only version (to avoid mistakes)? If that's too painful, let's have it CHECK that the provided event isn't one of the event types that may or may not ignore ACK.
https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:266: DCHECK(!ignore_ack_); On 2014/04/22 01:56:39, aelias wrote: > This is a bit ugly. Instead of returning these iterators, can > CoalescedWebTouchEvent have higher level methods like ApplyLatencyInfo that > apply on the whole set (and do nothing if !ignore_ack_)? Done. https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:296: bool ignore_ack_; On 2014/04/22 01:56:39, aelias wrote: > Would async_ be a better name? Probably, yes. https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:434: // past since sending the last touch event, flush any pending touch moves. On 2014/04/22 01:56:39, aelias wrote: > nit: "past" -> "passed" Done. https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:442: // If there's a pending touchmove, coalescewith the new event. Otherwise On 2014/04/22 01:56:39, aelias wrote: > nit: missing space after "coalesce" Done. https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:445: DCHECK(pending_async_touch_move_->CanCoalesceWith(touch)); On 2014/04/22 14:52:59, Rick Byers wrote: > Is this really guaranteed to be true? Eg. what if the modifiers change during a > touch stream? Do our touch events use any modifiers? Android doesn't, not sure about Aura, but yeah, definitely need to make this robust. https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.h:59: TOUCH_SCROLLING_MODE_ABSORB_TOUCHMOVE, On 2014/04/22 14:52:59, Rick Byers wrote: > On 2014/04/22 01:56:39, aelias wrote: > > Can we delete this mode as part of this patch? There's no reason to ever turn > > it on again even for debugging. > > Replacing ABSORB with ASYNC is OK with me (async just adds some things on top of > absorb). > Done. https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.h:218: bool async_touch_moves_; On 2014/04/22 14:52:59, Rick Byers wrote: > On 2014/04/22 01:56:39, aelias wrote: > > Instead of these bools, can we keep the enum value? > > We can remove this bool completely when we collapse the two modes into one. The > above bool needs to stay because it's what toggles back and forth depending on > GSU consumption. Done. https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.h:220: double last_sent_touch_timestamp_; On 2014/04/22 01:56:39, aelias wrote: > Please specify "ms" or "sec" in the variable name. Done. https://codereview.chromium.org/245833002/diff/20001/content/common/input/web... File content/common/input/web_input_event_traits.cc (right): https://codereview.chromium.org/245833002/diff/20001/content/common/input/web... content/common/input/web_input_event_traits.cc:366: return static_cast<const WebTouchEvent&>(event).cancelable; On 2014/04/22 14:52:59, Rick Byers wrote: > You're missing a !. Cancelable events must wait for their disposition. > > I've got an initial (as-yet untested) version of the blink-side patch that adds > this field here: https://codereview.chromium.org/247483002/ Of course, yes! https://codereview.chromium.org/245833002/diff/20001/content/common/input/web... File content/common/input/web_input_event_traits.h (right): https://codereview.chromium.org/245833002/diff/20001/content/common/input/web... content/common/input/web_input_event_traits.h:25: static bool IgnoresAckDisposition(blink::WebInputEvent::Type type); On 2014/04/22 14:52:59, Rick Byers wrote: > Can we get rid of this type-only version (to avoid mistakes)? If that's too > painful, let's have it CHECK that the provided event isn't one of the event > types that may or may not ignore ACK. I think we can. There's one place that still uses it, but it's only as a sanity check.
On 2014/04/23 19:57:41, jdduke wrote: > https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... > File content/browser/renderer_host/input/touch_event_queue.cc (right): > > https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... > content/browser/renderer_host/input/touch_event_queue.cc:266: > DCHECK(!ignore_ack_); > On 2014/04/22 01:56:39, aelias wrote: > > This is a bit ugly. Instead of returning these iterators, can > > CoalescedWebTouchEvent have higher level methods like ApplyLatencyInfo that > > apply on the whole set (and do nothing if !ignore_ack_)? > > Done. > > https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... > content/browser/renderer_host/input/touch_event_queue.cc:296: bool ignore_ack_; > On 2014/04/22 01:56:39, aelias wrote: > > Would async_ be a better name? > > Probably, yes. > > https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... > content/browser/renderer_host/input/touch_event_queue.cc:434: // past since > sending the last touch event, flush any pending touch moves. > On 2014/04/22 01:56:39, aelias wrote: > > nit: "past" -> "passed" > > Done. > > https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... > content/browser/renderer_host/input/touch_event_queue.cc:442: // If there's a > pending touchmove, coalescewith the new event. Otherwise > On 2014/04/22 01:56:39, aelias wrote: > > nit: missing space after "coalesce" > > Done. > > https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... > content/browser/renderer_host/input/touch_event_queue.cc:445: > DCHECK(pending_async_touch_move_->CanCoalesceWith(touch)); > On 2014/04/22 14:52:59, Rick Byers wrote: > > Is this really guaranteed to be true? Eg. what if the modifiers change during > a > > touch stream? > > Do our touch events use any modifiers? Android doesn't, not sure about Aura, but > yeah, definitely need to make this robust. Yes, Aura does. I know we get people complaining when we break modifiers on gesture events (eg. Ctrl-Tapping a link should open it in a new tab, as ctrl-click does for mouse). I'm not sure if any sites really take advantage of modifiers on touch events, but we should allow them to if they want to (eg. maybe in selection scenarios ctrl-drag is a power-user feature to extend the selection like for mouse). > > https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... > File content/browser/renderer_host/input/touch_event_queue.h (right): > > https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... > content/browser/renderer_host/input/touch_event_queue.h:59: > TOUCH_SCROLLING_MODE_ABSORB_TOUCHMOVE, > On 2014/04/22 14:52:59, Rick Byers wrote: > > On 2014/04/22 01:56:39, aelias wrote: > > > Can we delete this mode as part of this patch? There's no reason to ever > turn > > > it on again even for debugging. > > > > Replacing ABSORB with ASYNC is OK with me (async just adds some things on top > of > > absorb). > > > > Done. > > https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... > content/browser/renderer_host/input/touch_event_queue.h:218: bool > async_touch_moves_; > On 2014/04/22 14:52:59, Rick Byers wrote: > > On 2014/04/22 01:56:39, aelias wrote: > > > Instead of these bools, can we keep the enum value? > > > > We can remove this bool completely when we collapse the two modes into one. > The > > above bool needs to stay because it's what toggles back and forth depending on > > GSU consumption. > > Done. > > https://codereview.chromium.org/245833002/diff/20001/content/browser/renderer... > content/browser/renderer_host/input/touch_event_queue.h:220: double > last_sent_touch_timestamp_; > On 2014/04/22 01:56:39, aelias wrote: > > Please specify "ms" or "sec" in the variable name. > > Done. > > https://codereview.chromium.org/245833002/diff/20001/content/common/input/web... > File content/common/input/web_input_event_traits.cc (right): > > https://codereview.chromium.org/245833002/diff/20001/content/common/input/web... > content/common/input/web_input_event_traits.cc:366: return static_cast<const > WebTouchEvent&>(event).cancelable; > On 2014/04/22 14:52:59, Rick Byers wrote: > > You're missing a !. Cancelable events must wait for their disposition. > > > > I've got an initial (as-yet untested) version of the blink-side patch that > adds > > this field here: https://codereview.chromium.org/247483002/ > > Of course, yes! > > https://codereview.chromium.org/245833002/diff/20001/content/common/input/web... > File content/common/input/web_input_event_traits.h (right): > > https://codereview.chromium.org/245833002/diff/20001/content/common/input/web... > content/common/input/web_input_event_traits.h:25: static bool > IgnoresAckDisposition(blink::WebInputEvent::Type type); > On 2014/04/22 14:52:59, Rick Byers wrote: > > Can we get rid of this type-only version (to avoid mistakes)? If that's too > > painful, let's have it CHECK that the provided event isn't one of the event > > types that may or may not ignore ACK. > > I think we can. There's one place that still uses it, but it's only as a sanity > check.
I'd like to add a few more tests, but this should be good for further review.
https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:312: WebTouchEventWithLatencyList events_; Could you make this a scoped_ptr<> and only create the object when async is false? Then, you won't need to hold the separate async_ bool but just check this for existence. https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:457: // new touchmove will simply replace the pending touchmove. This looks like it destroys information. Should we flush the old touchmove instead, even if it hasn't been 200ms (after all that's just a heuristic)?
This is looking great to me! I only have a couple minor comments now. https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl_unittest.cc (right): https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl_unittest.cc:947: // Now ack each event. Ack-ignoring events should not be dispatched until all update comment - only acking each ack-respecting event. https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:31: const double kOuterSlopRegionLengthDipsSqared = 15. * 15.; How about we call this 'application slop region' to make it clear it's not really some special region for chromium behavior (having two of our own is kinda nuts), but attempting to track typical app behavior? https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:277: void UpdateLatencyInfo(const ui::LatencyInfo& renderer_latency_info) { Maybe 'updateLatencyInfoForAck'? Otherwise it's not clear why we should skip async_ events. https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:447: // or sufficient time has passed since sending the last touch event. update comment to mention application slop region. Or better: the code is pretty self-explanatory as to the 'what', the comment should focus on why - i.e. throttling to minimize cpu usage to minimize risk of scroll jank but also trigger app tap-detection logic. https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:490: touch.event.cancelable = false; So you're intentionally making new touchstart events async too, right? Probably need a comment saying we may want to revisit this (but we definitely want to send touchend async). Perhaps async_touch_moves_ should be renamed something like send_touch_events_async? https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.h:213: // a sufficient time period has elapsed since the last sent touch event. This is complex enough that it's probably worth adding a link to the design doc, wdyt? https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:1564: // Consuming a scroll event prevents the next touch moves from being this comment isn't quite true anymore, maybe "enables throttling"? https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:1631: // Start a new touch sequence and verify that absorption has been replace reference to "absorption" I guess... https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:1648: All the additional tests below look great - cover all the cases I was waiting to see if you'd tested yet ;-) https://codereview.chromium.org/245833002/diff/100001/content/common/input/we... File content/common/input/web_input_event_traits.cc (right): https://codereview.chromium.org/245833002/diff/100001/content/common/input/we... content/common/input/web_input_event_traits.cc:358: return !static_cast<const WebTouchEvent&>(event).cancelable; Since it's simpler, perhaps we should land my change first (https://codereview.chromium.org/247433003/), then update this to treat TouchCancel just like all the other touch event types?
https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:457: // new touchmove will simply replace the pending touchmove. On 2014/04/23 21:42:22, aelias wrote: > This looks like it destroys information. Should we flush the old touchmove > instead, even if it hasn't been 200ms (after all that's just a heuristic)? I like that idea. Eg. if a modifier goes down and then quickly up during a touch event sequence, the app should be able to see that (without watching the key events themselves).
Thanks for review! Rick, were there any sites in particular (other than the several from the original absorption bug) you'd like me to try this on? https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl_unittest.cc (right): https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl_unittest.cc:947: // Now ack each event. Ack-ignoring events should not be dispatched until all On 2014/04/23 21:56:17, Rick Byers wrote: > update comment - only acking each ack-respecting event. Done. https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:31: const double kOuterSlopRegionLengthDipsSqared = 15. * 15.; On 2014/04/23 21:56:17, Rick Byers wrote: > How about we call this 'application slop region' to make it clear it's not > really some special region for chromium behavior (having two of our own is kinda > nuts), but attempting to track typical app behavior? Done. https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:277: void UpdateLatencyInfo(const ui::LatencyInfo& renderer_latency_info) { On 2014/04/23 21:56:17, Rick Byers wrote: > Maybe 'updateLatencyInfoForAck'? Otherwise it's not clear why we should skip > async_ events. Done. https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:312: WebTouchEventWithLatencyList events_; On 2014/04/23 21:42:22, aelias wrote: > Could you make this a scoped_ptr<> and only create the object when async is > false? Then, you won't need to hold the separate async_ bool but just check > this for existence. Eh, I guess. But then we're double heap allocating whenever the event is not async. I guess we could use an empty vector check instead, then we don't pay for heap allocation or the bool... ? I'll use a proper function name so it's clear what's going on. https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:447: // or sufficient time has passed since sending the last touch event. On 2014/04/23 21:56:17, Rick Byers wrote: > update comment to mention application slop region. Or better: the code is > pretty self-explanatory as to the 'what', the comment should focus on why - i.e. > throttling to minimize cpu usage to minimize risk of scroll jank but also > trigger app tap-detection logic. Done. https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:457: // new touchmove will simply replace the pending touchmove. On 2014/04/23 21:58:13, Rick Byers wrote: > On 2014/04/23 21:42:22, aelias wrote: > > This looks like it destroys information. Should we flush the old touchmove > > instead, even if it hasn't been 200ms (after all that's just a heuristic)? > > I like that idea. Eg. if a modifier goes down and then quickly up during a > touch event sequence, the app should be able to see that (without watching the > key events themselves). If the app relies on async touches for modifier updates, they've already lost =/. However, I have no problem forcing a flush here. https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:490: touch.event.cancelable = false; On 2014/04/23 21:56:17, Rick Byers wrote: > So you're intentionally making new touchstart events async too, right? Probably > need a comment saying we may want to revisit this (but we definitely want to > send touchend async). > > Perhaps async_touch_moves_ should be renamed something like > send_touch_events_async? Yeah, I think that makes the most sense at this point. https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.h:213: // a sufficient time period has elapsed since the last sent touch event. On 2014/04/23 21:56:17, Rick Byers wrote: > This is complex enough that it's probably worth adding a link to the design doc, > wdyt? Done. https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:1564: // Consuming a scroll event prevents the next touch moves from being On 2014/04/23 21:56:17, Rick Byers wrote: > this comment isn't quite true anymore, maybe "enables throttling"? Done. https://codereview.chromium.org/245833002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:1648: On 2014/04/23 21:56:17, Rick Byers wrote: > All the additional tests below look great - cover all the cases I was waiting to > see if you'd tested yet ;-) :) Yeah, I actually had a few subtle bugs caught by this test. https://codereview.chromium.org/245833002/diff/100001/content/common/input/we... File content/common/input/web_input_event_traits.cc (right): https://codereview.chromium.org/245833002/diff/100001/content/common/input/we... content/common/input/web_input_event_traits.cc:358: return !static_cast<const WebTouchEvent&>(event).cancelable; On 2014/04/23 21:56:17, Rick Byers wrote: > Since it's simpler, perhaps we should land my change first > (https://codereview.chromium.org/247433003/), then update this to treat > TouchCancel just like all the other touch event types? Sounds good.
This looks great to me. Did you say you're still adding some tests (i.e. I shouldn't stamp it yet)?
On 2014/04/23 23:22:52, Rick Byers wrote: > This looks great to me. Did you say you're still adding some tests (i.e. I > shouldn't stamp it yet)? Can you think of any additional cases we should cover? I guess I could write an additional test that covers the touch timeout after the transition from async to sync touch dispatch (i.e., async touch -> unconsumed scroll -> sync touch -> touch timeout -> ???).
On 2014/04/23 23:34:22, jdduke wrote: > On 2014/04/23 23:22:52, Rick Byers wrote: > > This looks great to me. Did you say you're still adding some tests (i.e. I > > shouldn't stamp it yet)? > > Can you think of any additional cases we should cover? I guess I could write an > additional test that covers the touch timeout after the transition from async to > sync touch dispatch (i.e., async touch -> unconsumed scroll -> sync touch -> > touch timeout -> ???). You've covered all the main cases I was concerned with. I know there's some edge cases here but the risk seems low. I'm happy with it as-is if you are.
On 2014/04/23 23:53:10, Rick Byers wrote: > On 2014/04/23 23:34:22, jdduke wrote: > > On 2014/04/23 23:22:52, Rick Byers wrote: > > > This looks great to me. Did you say you're still adding some tests (i.e. I > > > shouldn't stamp it yet)? > > > > Can you think of any additional cases we should cover? I guess I could write > an > > additional test that covers the touch timeout after the transition from async > to > > sync touch dispatch (i.e., async touch -> unconsumed scroll -> sync touch -> > > touch timeout -> ???). > > You've covered all the main cases I was concerned with. I know there's some > edge cases here but the risk seems low. I'm happy with it as-is if you are. I added a quick sanity check to make sure the ack timeout will resume after touches go sync following an unconsumed scroll. At some point, we'll want to flesh out content_browsertests with some end to end scenarios, but they're in such a sorry state on Android (sigh, over which there's building furor) it's hard to justify.
On 2014/04/24 00:11:53, jdduke wrote: > On 2014/04/23 23:53:10, Rick Byers wrote: > > On 2014/04/23 23:34:22, jdduke wrote: > > > On 2014/04/23 23:22:52, Rick Byers wrote: > > > > This looks great to me. Did you say you're still adding some tests (i.e. > I > > > > shouldn't stamp it yet)? > > > > > > Can you think of any additional cases we should cover? I guess I could write > > an > > > additional test that covers the touch timeout after the transition from > async > > to > > > sync touch dispatch (i.e., async touch -> unconsumed scroll -> sync touch -> > > > touch timeout -> ???). > > > > You've covered all the main cases I was concerned with. I know there's some > > edge cases here but the risk seems low. I'm happy with it as-is if you are. > > I added a quick sanity check to make sure the ack timeout will resume after > touches go sync following an unconsumed scroll. Yep this look good. Thanks. I just landed the related cancelable chromium change. Merge with that (with the tweaks we discussed) and this CL should be ready to go. > At some point, we'll want to flesh out content_browsertests with some end to end > scenarios, but they're in such a sorry state on Android (sigh, over which > there's building furor) it's hard to justify.
OK, rebased, should be good to go. We should probably be thinking about the case where the main thread is so far behind that even 200ms throttled touchmove's are too frequent. jochen@: OWNER review for content/public/common and content/renderer/render_widget.cc.
On 2014/04/24 21:53:03, jdduke wrote: > OK, rebased, should be good to go. LGTM > We should probably be thinking about the > case where the main thread is so far behind that even 200ms throttled > touchmove's are too frequent. Yeah, that's the problem with async events - don't coalesce easily. Let's chat about some options... > jochen@: OWNER review for content/public/common and > content/renderer/render_widget.cc.
lgtm
The CQ bit was checked by jdduke@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jdduke@chromium.org/245833002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
On 2014/04/25 00:52:36, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > tryserver.chromium on chromium_presubmit Of course... got a little bit ahead of myself there =/
lgtm
The CQ bit was checked by rbyers@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jdduke@chromium.org/245833002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by jdduke@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jdduke@chromium.org/245833002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by jdduke@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jdduke@chromium.org/245833002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by jdduke@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jdduke@chromium.org/245833002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_compile_dbg
The CQ bit was checked by jdduke@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jdduke@chromium.org/245833002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
The CQ bit was checked by jdduke@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jdduke@chromium.org/245833002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
The CQ bit was checked by jdduke@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jdduke@chromium.org/245833002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
The CQ bit was checked by jdduke@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jdduke@chromium.org/245833002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
The CQ bit was checked by jdduke@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jdduke@chromium.org/245833002/200001
Message was sent while issue was closed.
Change committed as 266470 |
