Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
259763010:
Forward secondary touch points even if the first had no consumer (Closed)
|
Created:
6 years, 7 months ago by jdduke (slow) Modified:
6 years, 7 months ago Reviewers:
Rick Byers CC:
chromium-reviews, jam, jdduke+watch_chromium.org, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionForward secondary touch points even if the first had no consumer
Previously, if the first touch point in a sequence had no consumer, no further
touches from that sequence would be forwarded to the renderer. Instead, only
prevent touches from being forwarded if *none* of the current touch points have
a consumer when scrolling begins.
Also fix a corner case where follow-up touches triggered acks from deferred
async touchmoves would never be forwarded.
BUG=363321
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266934
Patch Set 1 #
Total comments: 3
Patch Set 2 : Cleanup #Patch Set 3 : Clarify comment #
Messages
Total messages: 28 (0 generated)
rbyers@: PTAL, thanks. https://codereview.chromium.org/259763010/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/259763010/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/touch_event_queue.cc:526: for (TouchPointAckStates::const_iterator iter = touch_ack_states_.begin(), Rick, do you think this is a worthwhile optimization? The case I'm thinking of is when you start scrolling then pinching; if the secondary touchstart is async, we'll never know if it has a consumer, so we'd start forwarding the pinch touchmoves (even if the first touchstart had no consumer).
LGTM. Nice catch on the follow-up event corner case! https://codereview.chromium.org/259763010/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/259763010/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/touch_event_queue.cc:526: for (TouchPointAckStates::const_iterator iter = touch_ack_states_.begin(), On 2014/04/28 18:08:15, jdduke wrote: > Rick, do you think this is a worthwhile optimization? The case I'm thinking of > is when you start scrolling then pinching; if the secondary touchstart is async, > we'll never know if it has a consumer, so we'd start forwarding the pinch > touchmoves (even if the first touchstart had no consumer). Hmm - interesting scenario. My gut instinct is that we should really send the secondary touchstart synchronously. Eg. should we allow people to build UIs that allow you to, for example, scroll a list with one finger and grab something from that list with another? That doesn't seem completely insane to me and it may be possible in Android browser, Firefox and Safari div scrolling (we should test). It's an edge case though, so I'm OK continuing to leave this scenario as future follow-up. In that case, I'm OK with this optimization but I don't think it buys us that much really (a touchmove every 200ms should be nothing compared to the scroll events that are being sent/processed every frame). I think I'd prefer to err on the side of simplicity and avoid it for now - we can always revisit if we ever encounter a scenario where this shows up in a trace. Your call though...
https://codereview.chromium.org/259763010/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/259763010/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/touch_event_queue.cc:526: for (TouchPointAckStates::const_iterator iter = touch_ack_states_.begin(), On 2014/04/28 21:45:22, Rick Byers wrote: > On 2014/04/28 18:08:15, jdduke wrote: > > Rick, do you think this is a worthwhile optimization? The case I'm thinking of > > is when you start scrolling then pinching; if the secondary touchstart is > async, > > we'll never know if it has a consumer, so we'd start forwarding the pinch > > touchmoves (even if the first touchstart had no consumer). > > Hmm - interesting scenario. My gut instinct is that we should really send the > secondary touchstart synchronously. Eg. should we allow people to build UIs > that allow you to, for example, scroll a list with one finger and grab something > from that list with another? That doesn't seem completely insane to me and it > may be possible in Android browser, Firefox and Safari div scrolling (we should > test). It's an edge case though, so I'm OK continuing to leave this scenario as > future follow-up. Interesting, that sounds like a reasonable use-case, but I'm curious how that would interact with the gesture detector. I guess their handling of the secondary touchstart would suppress the GesturePinchBegin and all subsequent pinch events? That would probably require some tweaking of the current model, perhaps we revert to synchronous touchmoves in that case? In that case it would probably work. > > In that case, I'm OK with this optimization but I don't think it buys us that > much really (a touchmove every 200ms should be nothing compared to the scroll > events that are being sent/processed every frame). I think I'd prefer to err on > the side of simplicity and avoid it for now - we can always revisit if we ever > encounter a scenario where this shows up in a trace. Your call though... Yeah. I'm less worried about the 200ms touchmoves than I am the web app seeing its first touchstart (as it never got the one with no consumer) but not being able to preventDefault it. Let's come up with a solution that handles the case you mentioned (might be as simple as making touchstart synchronous), and we can remove this (I'll update the comment, as it's less an optimization and more a band-aid on the async touchstart case).
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/259763010/60001
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/259763010/60001
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/259763010/60001
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/259763010/60001
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/259763010/60001
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/259763010/60001
On 2014/04/28 22:04:59, jdduke wrote: > https://codereview.chromium.org/259763010/diff/1/content/browser/renderer_hos... > File content/browser/renderer_host/input/touch_event_queue.cc (right): > > https://codereview.chromium.org/259763010/diff/1/content/browser/renderer_hos... > content/browser/renderer_host/input/touch_event_queue.cc:526: for > (TouchPointAckStates::const_iterator iter = touch_ack_states_.begin(), > On 2014/04/28 21:45:22, Rick Byers wrote: > > On 2014/04/28 18:08:15, jdduke wrote: > > > Rick, do you think this is a worthwhile optimization? The case I'm thinking > of > > > is when you start scrolling then pinching; if the secondary touchstart is > > async, > > > we'll never know if it has a consumer, so we'd start forwarding the pinch > > > touchmoves (even if the first touchstart had no consumer). > > > > Hmm - interesting scenario. My gut instinct is that we should really send the > > secondary touchstart synchronously. Eg. should we allow people to build UIs > > that allow you to, for example, scroll a list with one finger and grab > something > > from that list with another? That doesn't seem completely insane to me and it > > may be possible in Android browser, Firefox and Safari div scrolling (we > should > > test). It's an edge case though, so I'm OK continuing to leave this scenario > as > > future follow-up. > > Interesting, that sounds like a reasonable use-case, but I'm curious how that > would interact with the gesture detector. I guess their handling of the > secondary touchstart would suppress the GesturePinchBegin and all subsequent > pinch events? That would probably require some tweaking of the current model, > perhaps we revert to synchronous touchmoves in that case? In that case it would > probably work. Right. I guess our rule could be that we send touchmove async if all active fingers are being consumed by a gesture. That may give us some other tricky cases (eg. consuming some moves and not others) but they're specified by our rules here, and I _think_ the implications are reasonable: https://docs.google.com/a/chromium.org/document/d/176xYUC3WbiSl7qd08SBW4NiI4_... > > > > In that case, I'm OK with this optimization but I don't think it buys us that > > much really (a touchmove every 200ms should be nothing compared to the scroll > > events that are being sent/processed every frame). I think I'd prefer to err > on > > the side of simplicity and avoid it for now - we can always revisit if we ever > > encounter a scenario where this shows up in a trace. Your call though... > > Yeah. I'm less worried about the 200ms touchmoves than I am the web app seeing > its first touchstart (as it never got the one with no consumer) but not being > able to preventDefault it. Let's come up with a solution that handles the case > you mentioned (might be as simple as making touchstart synchronous), and we can > remove this (I'll update the comment, as it's less an optimization and more a > band-aid on the async touchstart case).
On 2014/04/28 22:04:59, jdduke wrote: > https://codereview.chromium.org/259763010/diff/1/content/browser/renderer_hos... > File content/browser/renderer_host/input/touch_event_queue.cc (right): > > https://codereview.chromium.org/259763010/diff/1/content/browser/renderer_hos... > content/browser/renderer_host/input/touch_event_queue.cc:526: for > (TouchPointAckStates::const_iterator iter = touch_ack_states_.begin(), > On 2014/04/28 21:45:22, Rick Byers wrote: > > On 2014/04/28 18:08:15, jdduke wrote: > > > Rick, do you think this is a worthwhile optimization? The case I'm thinking > of > > > is when you start scrolling then pinching; if the secondary touchstart is > > async, > > > we'll never know if it has a consumer, so we'd start forwarding the pinch > > > touchmoves (even if the first touchstart had no consumer). > > > > Hmm - interesting scenario. My gut instinct is that we should really send the > > secondary touchstart synchronously. Eg. should we allow people to build UIs > > that allow you to, for example, scroll a list with one finger and grab > something > > from that list with another? That doesn't seem completely insane to me and it > > may be possible in Android browser, Firefox and Safari div scrolling (we > should > > test). It's an edge case though, so I'm OK continuing to leave this scenario > as > > future follow-up. > > Interesting, that sounds like a reasonable use-case, but I'm curious how that > would interact with the gesture detector. I guess their handling of the > secondary touchstart would suppress the GesturePinchBegin and all subsequent > pinch events? That would probably require some tweaking of the current model, > perhaps we revert to synchronous touchmoves in that case? In that case it would > probably work. > > > > > In that case, I'm OK with this optimization but I don't think it buys us that > > much really (a touchmove every 200ms should be nothing compared to the scroll > > events that are being sent/processed every frame). I think I'd prefer to err > on > > the side of simplicity and avoid it for now - we can always revisit if we ever > > encounter a scenario where this shows up in a trace. Your call though... > > Yeah. I'm less worried about the 200ms touchmoves than I am the web app seeing > its first touchstart (as it never got the one with no consumer) but not being > able to preventDefault it. Let's come up with a solution that handles the case > you mentioned (might be as simple as making touchstart synchronous), and we can > remove this (I'll update the comment, as it's less an optimization and more a > band-aid on the async touchstart case). Comment looks good by the way, thanks.
Message was sent while issue was closed.
Change committed as 266934 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
