Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
156783006:
Consuming a touch move prevents only the next scroll update. (Closed)
|
Created:
6 years, 10 months ago by tdresser Modified:
6 years, 10 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionConsuming a touch move prevents only the next scroll update.
This implements (In Clank) the touch disposition handling method outlined here:
https://docs.google.com/a/chromium.org/document/d/176xYUC3WbiSl7qd08SBW4NiI4_gO0N4PN6tPAq49-68/edit?pli=1#bookmark=id.wu82d53abe2w
We also modify the way in which Aura handles touch move events to line up with the new way Android handles them. That is, consuming a touch move event prevents the next scroll update event, but does not prevent future scroll update events.
This change will have little impact until https://codereview.chromium.org/166923002/ has landed.
BUG=328503
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251533
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address jdduke's comments, and add tests. #Patch Set 3 : Fix minor nit. #Patch Set 4 : Clean up error handling and tests. #
Total comments: 18
Patch Set 5 : Address jdduke's comments. #
Total comments: 17
Patch Set 6 : Address jdduke comments. #Patch Set 7 : Remove touch scroll modes flags. #Patch Set 8 : Reuploading to try to avoid chunk mismatch. #
Total comments: 22
Patch Set 9 : Address rbyers' comments. #
Total comments: 15
Patch Set 10 : Address jdduke's comments. #Patch Set 11 : Undo accidental whitespace change. #Messages
Total messages: 32 (0 generated)
In general, I think this is a nice addition and should make it a good deal easier to experiment in the future. https://codereview.chromium.org/156783006/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/input/gesture_event_packet.h (right): https://codereview.chromium.org/156783006/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/gesture_event_packet.h:22: TOUCH_END, // A touch point lifted. Now that we're adding a few types, do you think it's worth having a TOUCH_SEQUENCE_END that would pop a sequence as soon as it's ack'ed? Rather than just waiting until we get the ack of a TOUCH_SEQUENCE_BEGIN? Also, are we taking into consideration that a TouchCancel cannot be preventDefault'ed? Looking at the code, I think this gets handled properly (e.g., GestureScrollEnd/GestureTapCancel on a TouchCancel), but we should probably add some test for this. https://codereview.chromium.org/156783006/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/gesture_event_packet.h:47: bool is_independent_; Do we still need this flag? https://codereview.chromium.org/156783006/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/input/touch_disposition_gesture_filter.cc (right): https://codereview.chromium.org/156783006/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:103: AddDispositionHandlingInfo(WebInputEvent::GestureTapDown, RT_START); These are such lightweight structs, why not simply have a function local to this file that instead does something like: GetDispositionHandlingInfo(type) { switch (type) { case FOO: return Info(a, b, c); } I'd prefer that over duplicating the map for each TouchDispositionGestureFilter instance. The alternative would be building either a static map or lookup table (re-indexed from the WebInputEvent::Type to something 0-based). I think either of these approaches would make the code a bit easier to follow as well. https://codereview.chromium.org/156783006/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:307: (last_event_of_type_dropped_.count( Nit: Indent "current ==" by 4
https://codereview.chromium.org/156783006/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/input/gesture_event_packet.h (right): https://codereview.chromium.org/156783006/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/gesture_event_packet.h:22: TOUCH_END, // A touch point lifted. On 2014/02/11 16:02:25, jdduke wrote: > Now that we're adding a few types, do you think it's worth having a > TOUCH_SEQUENCE_END that would pop a sequence as soon as it's ack'ed? Rather > than just waiting until we get the ack of a TOUCH_SEQUENCE_BEGIN? > > Also, are we taking into consideration that a TouchCancel cannot be > preventDefault'ed? Looking at the code, I think this gets handled properly > (e.g., GestureScrollEnd/GestureTapCancel on a TouchCancel), but we should > probably add some test for this. Done. https://codereview.chromium.org/156783006/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/gesture_event_packet.h:47: bool is_independent_; On 2014/02/11 16:02:25, jdduke wrote: > Do we still need this flag? Nope, removed. https://codereview.chromium.org/156783006/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/input/touch_disposition_gesture_filter.cc (right): https://codereview.chromium.org/156783006/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:103: AddDispositionHandlingInfo(WebInputEvent::GestureTapDown, RT_START); On 2014/02/11 16:02:25, jdduke wrote: > These are such lightweight structs, why not simply have a function local to this > file that instead does something like: > > GetDispositionHandlingInfo(type) { > switch (type) { > case FOO: return Info(a, b, c); > } > > I'd prefer that over duplicating the map for each TouchDispositionGestureFilter > instance. > > The alternative would be building either a static map or lookup table > (re-indexed from the WebInputEvent::Type to something 0-based). I think either > of these approaches would make the code a bit easier to follow as well. Done. https://codereview.chromium.org/156783006/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:307: (last_event_of_type_dropped_.count( On 2014/02/11 16:02:25, jdduke wrote: > Nit: Indent "current ==" by 4 http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Boolea... The example of a boolean expression in the style guide doesn't indent by 4 in this case, nor does clang-format.
https://codereview.chromium.org/156783006/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/input/touch_disposition_gesture_filter.cc (right): https://codereview.chromium.org/156783006/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:307: (last_event_of_type_dropped_.count( On 2014/02/11 21:04:33, tdresser wrote: > On 2014/02/11 16:02:25, jdduke wrote: > > Nit: Indent "current ==" by 4 > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Boolea... > > The example of a boolean expression in the style guide doesn't indent by 4 in > this case, nor does clang-format. Of course, sorry, you're completely right... I just finished a Java code review =/ Hmm, but I was also thinking about readability, what if we had something like: bool current_consumed = current == INPUT_EVENT_ACK_STATE_CONSUMED ... if ((required_touches & RT_MOVE && state.move_consumed) || (required_touches & RT_START && state.start_consumed) || (required_touches & RT_CURRENT && current_consumed) || ... It's pretty nitty, but then you get a nice lineup of the checks and it's a little easier to follow. Not a big deal.
https://codereview.chromium.org/156783006/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/input/touch_disposition_gesture_filter.cc (right): https://codereview.chromium.org/156783006/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:307: (last_event_of_type_dropped_.count( On 2014/02/11 21:18:16, jdduke wrote: > On 2014/02/11 21:04:33, tdresser wrote: > > On 2014/02/11 16:02:25, jdduke wrote: > > > Nit: Indent "current ==" by 4 > > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Boolea... > > > > The example of a boolean expression in the style guide doesn't indent by 4 in > > this case, nor does clang-format. > > Of course, sorry, you're completely right... I just finished a Java code review > =/ Hmm, but I was also thinking about readability, what if we had something > like: > > bool current_consumed = current == INPUT_EVENT_ACK_STATE_CONSUMED > ... > if ((required_touches & RT_MOVE && state.move_consumed) || > (required_touches & RT_START && state.start_consumed) || > (required_touches & RT_CURRENT && current_consumed) || > ... > > It's pretty nitty, but then you get a nice lineup of the checks and it's a > little easier to follow. Not a big deal. Definitely a little nicer. Done.
https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... File content/browser/renderer_host/input/touch_disposition_gesture_filter.cc (right): https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:99: CancelTapIfNecessary(); I'm not sure we can cancel the tap here... With the double-tap delay, we may still be waiting on GestureTapConfirmed. https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:136: DCHECK(!needs_tap_ending_event_); As noted above, we may need to switch this to |CancelTapIfNecessary()|. https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:233: bool TouchDispositionGestureFilter::IsGesturePrevented( This is another mostly static function that we should move into namespace {}. Just pass in |last_event_of_type_dropped_|. https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... File content/browser/renderer_host/input/touch_disposition_gesture_filter.h (right): https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.h:65: struct DispositionHandlingInfo { It looks to me like all of the DispositionHandlingInfo can be moved into namespace {} within the .cc file. https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.h:71: DispositionHandlingInfo Info(int required_touches) const { Let's use constructors for this instead of factory functions. https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.h:93: GestureHandlingState() { Nit: Move constructor impl into .cc. https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.h:111: void UpdateState(InputEventAckState ack_state); Do we still need this method? https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.h:54: TOUCH_SCROLLING_MODE_ABSORB_TOUCHMOVE Let's add a TOUCH_SCROLLING_MODE_DEFAULT = TOUCH_SCROLLING_MODE_TOUCHCANCEL that we use in InputRouterImpl to determine the default mode. https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.h:59: TouchScrollingMode mode = TOUCH_SCROLLING_MODE_TOUCHCANCEL); Nit: No default args, and remove explicit.
https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... File content/browser/renderer_host/input/touch_disposition_gesture_filter.cc (right): https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:99: CancelTapIfNecessary(); On 2014/02/13 16:53:06, jdduke wrote: > I'm not sure we can cancel the tap here... With the double-tap delay, we may > still be waiting on GestureTapConfirmed. Done. https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:136: DCHECK(!needs_tap_ending_event_); On 2014/02/13 16:53:06, jdduke wrote: > As noted above, we may need to switch this to |CancelTapIfNecessary()|. This will have been handled above in the Head().IsEmpty() check. https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:233: bool TouchDispositionGestureFilter::IsGesturePrevented( On 2014/02/13 16:53:06, jdduke wrote: > This is another mostly static function that we should move into namespace {}. > Just pass in |last_event_of_type_dropped_|. This would require making GestureHandlingState public somehow, but that's exposing implementation details. I've left it as it is for now. Let me know if you think it's superior to make GestureHandlingState public, or if you have a different approach in mind. https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... File content/browser/renderer_host/input/touch_disposition_gesture_filter.h (right): https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.h:65: struct DispositionHandlingInfo { On 2014/02/13 16:53:06, jdduke wrote: > It looks to me like all of the DispositionHandlingInfo can be moved into > namespace {} within the .cc file. Done. https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.h:71: DispositionHandlingInfo Info(int required_touches) const { On 2014/02/13 16:53:06, jdduke wrote: > > Let's use constructors for this instead of factory functions. Done. https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.h:93: GestureHandlingState() { On 2014/02/13 16:53:06, jdduke wrote: > Nit: Move constructor impl into .cc. Done. https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.h:111: void UpdateState(InputEventAckState ack_state); On 2014/02/13 16:53:06, jdduke wrote: > Do we still need this method? Yikes, nope. Good catch. https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.h:54: TOUCH_SCROLLING_MODE_ABSORB_TOUCHMOVE On 2014/02/13 16:53:06, jdduke wrote: > Let's add a TOUCH_SCROLLING_MODE_DEFAULT = TOUCH_SCROLLING_MODE_TOUCHCANCEL that > we use in InputRouterImpl to determine the default mode. Done. https://codereview.chromium.org/156783006/diff/270001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.h:59: TouchScrollingMode mode = TOUCH_SCROLLING_MODE_TOUCHCANCEL); On 2014/02/13 16:53:06, jdduke wrote: > Nit: No default args, and remove explicit. Done. https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... File content/browser/renderer_host/input/gesture_event_packet.h (right): https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... content/browser/renderer_host/input/gesture_event_packet.h:21: TOUCH_SEQUENCE_END, // The end of gesture sequence. This isn't currently used, but will likely be in the future. Should I strip it out for now? https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... File content/browser/renderer_host/input/touch_disposition_gesture_filter.cc (right): https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:41: DispositionHandlingInfo GetDispositionHandlingInfo( I think this was a lot more readable using the |Info| factory instead of the constructor (just due to line length). I don't really want to use a type name much less explicit than DispositionHandlingInfo though. Do you think this is reasonable?
This is looking pretty good, maybe rbyers@ can take another look and we can see about landing before branch? On 2014/02/13 18:14:08, tdresser wrote: > This would require making GestureHandlingState public somehow, but that's > exposing implementation details. I've left it as it is for now. > > Let me know if you think it's superior to make GestureHandlingState public, or > if you have a different approach in mind. Ah, you're right, I think it's fine as-is. https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... File content/browser/renderer_host/input/gesture_event_packet.cc (right): https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... content/browser/renderer_host/input/gesture_event_packet.cc:22: return GestureEventPacket::OUT_OF_ORDER; I'm not sure about the OUT_OF_ORDER source... the packet isn't really determining that, it's simply determining if we have a valid touch... What if we instead rename INVALID to UNDEFINED, and rename OUT_OF_ORDER to INVALID? Also, I guess you might as well move the "if (!event.touchesLength)" check to the beginning of the function. https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... File content/browser/renderer_host/input/gesture_event_packet.h (right): https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... content/browser/renderer_host/input/gesture_event_packet.h:21: TOUCH_SEQUENCE_END, // The end of gesture sequence. On 2014/02/13 18:14:09, tdresser wrote: > This isn't currently used, but will likely be in the future. Should I strip it > out for now? Hmm, I suppose it doesn't hurt if for no other reason than consistency. https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... File content/browser/renderer_host/input/touch_disposition_gesture_filter.cc (right): https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:41: DispositionHandlingInfo GetDispositionHandlingInfo( On 2014/02/13 18:14:09, tdresser wrote: > I think this was a lot more readable using the |Info| factory instead of the > constructor (just due to line length). > > I don't really want to use a type name much less explicit than > DispositionHandlingInfo though. > > Do you think this is reasonable? I see, you're right, any reason you can't use a one-line factory function(s) that simply delegate to the constructor? https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:156: sequence.UpdateState(packet.gesture_source(), ack_state); Since we know we only have valid packets (we can always DCHECK) at this point, we might shorten the conditional by instead checking the exceptional case, e.g. if (packet.gesture_source() != GestureEventPacket::TOUCH_TIMEOUT) { } https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:171: const GestureSequence& sequence, Hmm, I guess we might as well pass the sequence state here? https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... File content/browser/renderer_host/input/touch_disposition_gesture_filter_unittest.cc (right): https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter_unittest.cc:252: SendTouchEventACK(INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS); Why this change? https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter_unittest.cc:300: PushGesture(WebInputEvent::GestureScrollUpdate); Nice. https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter_unittest.cc:310: } For completeness, could we add a final TouchEnd and make sure a GestureScrollEnd gets through (when the TouchEnd is consumed)? I think we handle this below but it's nice to see the full sequence handled properly. https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter_unittest.cc:743: TEST_F(TouchDispositionGestureFilterTest, TimeoutEventAfterRelease) { Awesome, thanks!
sadrul@, can you take a look at ui/aura? joi@, can you take a look at content/public/common/content_switches.* rbyers@, how is this looking to you? https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... File content/browser/renderer_host/input/gesture_event_packet.cc (right): https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... content/browser/renderer_host/input/gesture_event_packet.cc:22: return GestureEventPacket::OUT_OF_ORDER; On 2014/02/13 19:40:00, jdduke wrote: > > I'm not sure about the OUT_OF_ORDER source... the packet isn't really > determining that, it's simply determining if we have a valid touch... What if we > instead rename INVALID to UNDEFINED, and rename OUT_OF_ORDER to INVALID? > > Also, I guess you might as well move the "if (!event.touchesLength)" check to > the beginning of the function. Done. https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... File content/browser/renderer_host/input/touch_disposition_gesture_filter.cc (right): https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:41: DispositionHandlingInfo GetDispositionHandlingInfo( On 2014/02/13 19:40:00, jdduke wrote: > On 2014/02/13 18:14:09, tdresser wrote: > > I think this was a lot more readable using the |Info| factory instead of the > > constructor (just due to line length). > > > > I don't really want to use a type name much less explicit than > > DispositionHandlingInfo though. > > > > Do you think this is reasonable? > > I see, you're right, any reason you can't use a one-line factory function(s) > that simply delegate to the constructor? Done. Why is this superior to just using the factory functions? https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:156: sequence.UpdateState(packet.gesture_source(), ack_state); On 2014/02/13 19:40:00, jdduke wrote: > Since we know we only have valid packets (we can always DCHECK) at this point, > we might shorten the conditional by instead checking the exceptional case, e.g. > > if (packet.gesture_source() != GestureEventPacket::TOUCH_TIMEOUT) { > > } Done. https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:171: const GestureSequence& sequence, On 2014/02/13 19:40:00, jdduke wrote: > Hmm, I guess we might as well pass the sequence state here? Done. https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... File content/browser/renderer_host/input/touch_disposition_gesture_filter_unittest.cc (right): https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter_unittest.cc:252: SendTouchEventACK(INPUT_EVENT_ACK_STATE_NO_CONSUMER_EXISTS); On 2014/02/13 19:40:00, jdduke wrote: > Why this change? It must have made sense to me at one point, but it sure doesn't anymore... Removed. https://codereview.chromium.org/156783006/diff/340002/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter_unittest.cc:310: } On 2014/02/13 19:40:00, jdduke wrote: > For completeness, could we add a final TouchEnd and make sure a GestureScrollEnd > gets through (when the TouchEnd is consumed)? I think we handle this below but > it's nice to see the full sequence handled properly. Done.
Is the CL description up-to-date?
On 2014/02/13 20:39:22, sadrul wrote: > Is the CL description up-to-date? Hmmm, yeah, this really ought to be split up into two patches. I'll split it up now...
On 2014/02/13 20:47:52, tdresser wrote: > On 2014/02/13 20:39:22, sadrul wrote: > > Is the CL description up-to-date? > > Hmmm, yeah, this really ought to be split up into two patches. > I'll split it up now... The second patch, which depends on this one, will come later. How's this, sadrul?
This looks pretty good to me, thanks! Overall I think it does give us the flexibility we want without complicating the code much (replaces some somewhat ad-hoc logic with a somewhat ad-hoc table). https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... File content/browser/renderer_host/input/touch_disposition_gesture_filter.cc (left): https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:54: // Handle the timeout packet immediately if the packet preceding the Did you remove this comment intentionally? https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... File content/browser/renderer_host/input/touch_disposition_gesture_filter.cc (right): https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:50: DispositionHandlingInfo GetDispositionHandlingInfo( Add link to chromium design doc that discusses this https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:178: if (!dropped) re-order if clause to avoid double negative also it's probably simpler to combine with the below if statement (and even eliminate the 'dropped' local entirely if you want). https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:293: } else if (gesture_source == GestureEventPacket::TOUCH_MOVE) { Should be able to get rid of this too... https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:316: (last_event_of_type_dropped_.count( I find the implicit conversion from int to bool a little confusing here. Perhaps use '> 0' or 'set.find() != set.end()' to be clearer that your testing containment here? https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... File content/browser/renderer_host/input/touch_disposition_gesture_filter.h (right): https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.h:8: #include <map> doesn't look like you're actually using map here... https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.h:22: RT_MOVE = 1 << 1, As discussed now in person and in the doc, I think we can eliminate the MOVE state (treat it just like CURRENT). https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.h:73: bool seen_ack; Since the precise definition of these bits are critical to the behavior, I think it's worth adding comments for each that precisely define their semantics. start_consumed/move_consumed are pretty obvious, but the exact semantics of seen_ack and no_consumer aren't obvious to me before looking at the code. https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.h:117: last_event_of_type_dropped_; please add a comment describing the precise semantics. In particular, should we expect this set to be cleared at the start of every touch sequence? https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... File content/browser/renderer_host/input/touch_disposition_gesture_filter_unittest.cc (right): https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter_unittest.cc:471: // Independent packets are never sent when the global state is to drop. The notion of 'independent packets' is gone... https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.h:40: TouchEventQueue(TouchEventQueueClient* client); why did you need to make this non-explicit? I assume we'd never want an implicit conversion from TouchEventQueueClient* to TouchEventQueue...
https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... File content/browser/renderer_host/input/touch_disposition_gesture_filter.cc (left): https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:54: // Handle the timeout packet immediately if the packet preceding the On 2014/02/13 21:59:25, Rick Byers wrote: > Did you remove this comment intentionally? Nope, thanks for catching that. https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... File content/browser/renderer_host/input/touch_disposition_gesture_filter.cc (right): https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:50: DispositionHandlingInfo GetDispositionHandlingInfo( On 2014/02/13 21:59:25, Rick Byers wrote: > Add link to chromium design doc that discusses this Done. https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:178: if (!dropped) On 2014/02/13 21:59:25, Rick Byers wrote: > re-order if clause to avoid double negative > also it's probably simpler to combine with the below if statement (and even > eliminate the 'dropped' local entirely if you want). Oy, that was a mess, thanks. Done. https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:293: } else if (gesture_source == GestureEventPacket::TOUCH_MOVE) { On 2014/02/13 21:59:25, Rick Byers wrote: > Should be able to get rid of this too... Done. https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:316: (last_event_of_type_dropped_.count( On 2014/02/13 21:59:25, Rick Byers wrote: > I find the implicit conversion from int to bool a little confusing here. > Perhaps use '> 0' or 'set.find() != set.end()' to be clearer that your testing > containment here? Done. https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... File content/browser/renderer_host/input/touch_disposition_gesture_filter.h (right): https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.h:8: #include <map> On 2014/02/13 21:59:25, Rick Byers wrote: > doesn't look like you're actually using map here... Done. https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.h:22: RT_MOVE = 1 << 1, On 2014/02/13 21:59:25, Rick Byers wrote: > As discussed now in person and in the doc, I think we can eliminate the MOVE > state (treat it just like CURRENT). Done. https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.h:73: bool seen_ack; On 2014/02/13 21:59:25, Rick Byers wrote: > Since the precise definition of these bits are critical to the behavior, I think > it's worth adding comments for each that precisely define their semantics. > start_consumed/move_consumed are pretty obvious, but the exact semantics of > seen_ack and no_consumer aren't obvious to me before looking at the code. Done. https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.h:117: last_event_of_type_dropped_; On 2014/02/13 21:59:25, Rick Byers wrote: > please add a comment describing the precise semantics. In particular, should we > expect this set to be cleared at the start of every touch sequence? Done. I previously wasn't clearing this at the start of a new touch sequence, and I don't think it's strictly necessary, but it does make things easier to reason about, and there's definitely room for potential bugs if it isn't cleared. https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... File content/browser/renderer_host/input/touch_disposition_gesture_filter_unittest.cc (right): https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter_unittest.cc:471: // Independent packets are never sent when the global state is to drop. On 2014/02/13 21:59:25, Rick Byers wrote: > The notion of 'independent packets' is gone... Done. https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/156783006/diff/690001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.h:40: TouchEventQueue(TouchEventQueueClient* client); On 2014/02/13 21:59:25, Rick Byers wrote: > why did you need to make this non-explicit? I assume we'd never want an > implicit conversion from TouchEventQueueClient* to TouchEventQueue... Done. An artifact of my quick splitting up the patch into two parts - the next patch adds an additional parameter, so the explicit will be removed.
content/browser/renderer_host/input lgtm with a couple more nits. https://codereview.chromium.org/156783006/diff/770001/content/browser/rendere... File content/browser/renderer_host/input/touch_disposition_gesture_filter.cc (left): https://codereview.chromium.org/156783006/diff/770001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:181: // TouchDispositionGestureFilter::GestureSequence Hmm, did we still want this comment? https://codereview.chromium.org/156783006/diff/770001/content/browser/rendere... File content/browser/renderer_host/input/touch_disposition_gesture_filter.cc (right): https://codereview.chromium.org/156783006/diff/770001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:117: if (packet.gesture_source() == GestureEventPacket::INVALID || IsEmpty()) Please move this |GestureEventPacket::INVALID| check into the same check as |UNDEFINED|. https://codereview.chromium.org/156783006/diff/770001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:142: last_event_of_type_dropped_.clear(); Does std::set typically deallocate memory hear (unlike, say, std::vector)? If so, this is a shame, as set/map are notoriously awful at memory allocation. Perhaps this is premature optimization, but I may revisit this down the road with a bitset or SmallMap (sorry, this past week I've discovered that underpowered Android devices really get hammered by things like this). https://codereview.chromium.org/156783006/diff/770001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:153: DCHECK(packet.gesture_source() != GestureEventPacket::UNDEFINED); DCHECK_NE https://codereview.chromium.org/156783006/diff/770001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:157: // We should handle at most one touch-based packet corresponding to a Hmm, I realize this was my comment, but maybe change it to say "at most one non-timeout based packet". https://codereview.chromium.org/156783006/diff/770001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:278: GestureEventPacket::GestureSource gesture_source, Can't tell if this is aligned. https://codereview.chromium.org/156783006/diff/770001/content/browser/rendere... File content/browser/renderer_host/input/touch_disposition_gesture_filter.h (right): https://codereview.chromium.org/156783006/diff/770001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.h:18: enum RequiredTouches { Any reason we can't stick this in the .cc file? https://codereview.chromium.org/156783006/diff/770001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/156783006/diff/770001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:51: SendTouchEventAck(*sync_ack_result_.Pass()); I always read the uppercase version as "ACK" being yelled. This is much better :)
sadrul@, can you take a look at this? Thanks! https://codereview.chromium.org/156783006/diff/770001/content/browser/rendere... File content/browser/renderer_host/input/touch_disposition_gesture_filter.cc (left): https://codereview.chromium.org/156783006/diff/770001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:181: // TouchDispositionGestureFilter::GestureSequence On 2014/02/14 16:59:52, jdduke wrote: > Hmm, did we still want this comment? Readded. https://codereview.chromium.org/156783006/diff/770001/content/browser/rendere... File content/browser/renderer_host/input/touch_disposition_gesture_filter.cc (right): https://codereview.chromium.org/156783006/diff/770001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:117: if (packet.gesture_source() == GestureEventPacket::INVALID || IsEmpty()) On 2014/02/14 16:59:52, jdduke wrote: > Please move this |GestureEventPacket::INVALID| check into the same check as > |UNDEFINED|. Done. https://codereview.chromium.org/156783006/diff/770001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:142: last_event_of_type_dropped_.clear(); On 2014/02/14 16:59:52, jdduke wrote: > Does std::set typically deallocate memory hear (unlike, say, std::vector)? If > so, this is a shame, as set/map are notoriously awful at memory allocation. > Perhaps this is premature optimization, but I may revisit this down the road > with a bitset or SmallMap (sorry, this past week I've discovered that > underpowered Android devices really get hammered by things like this). Yeah, this does deallocate memory. I was initially thinking we could just never clear it, but this does feel safer. I'd be in favour of cleaning this up later. https://codereview.chromium.org/156783006/diff/770001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:153: DCHECK(packet.gesture_source() != GestureEventPacket::UNDEFINED); On 2014/02/14 16:59:52, jdduke wrote: > DCHECK_NE Done. https://codereview.chromium.org/156783006/diff/770001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:157: // We should handle at most one touch-based packet corresponding to a On 2014/02/14 16:59:52, jdduke wrote: > Hmm, I realize this was my comment, but maybe change it to say "at most one > non-timeout based packet". Done. https://codereview.chromium.org/156783006/diff/770001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.cc:278: GestureEventPacket::GestureSource gesture_source, On 2014/02/14 16:59:52, jdduke wrote: > Can't tell if this is aligned. It is. https://codereview.chromium.org/156783006/diff/770001/content/browser/rendere... File content/browser/renderer_host/input/touch_disposition_gesture_filter.h (right): https://codereview.chromium.org/156783006/diff/770001/content/browser/rendere... content/browser/renderer_host/input/touch_disposition_gesture_filter.h:18: enum RequiredTouches { On 2014/02/14 16:59:52, jdduke wrote: > Any reason we can't stick this in the .cc file? Done.
gestures lgtm
The CQ bit was checked by tdresser@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tdresser@chromium.org/156783006/970001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by tdresser@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tdresser@chromium.org/156783006/970001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by tdresser@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tdresser@chromium.org/156783006/970001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by tdresser@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tdresser@chromium.org/156783006/970001
Message was sent while issue was closed.
Change committed as 251533 |
