Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
538653002:
Changing the bounding box for show press and tap gesture (Closed)
Descriptionchanging bounding box for show press and tap gesture
The bounding boxes of show press and tap gesture are the same right now,
which are based on the touch start location and the largest radius seen
before the show press gesture is sent.
BUG=403747
Committed: https://crrev.com/647665c49d055d6a4645f7c842b35f2d0c20a025
Cr-Commit-Position: refs/heads/master@{#293807}
Patch Set 1 : Delete some unused lines #
Total comments: 16
Patch Set 2 : Keeping track of max diameter in GestureListenerImpl #
Total comments: 11
Patch Set 3 : Made changes based on comments #
Total comments: 4
Patch Set 4 : Correct the comment #
Messages
Total messages: 23 (8 generated)
lanwei@chromium.org changed reviewers: + tdresser@chromium.org
Please look at the second patch, thanks.
tdresser@chromium.org changed reviewers: + jdduke@chromium.org
Jared, feel free to delay doing a full review until I've taken a closer look at things. Do you have any thoughts on https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... ? https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... File ui/events/gesture_detection/gesture_detector.h (right): https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/gesture_detector.h:206: // sent. Perhaps: // The maximum touch diameter before the show press gesture is sent. https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/gesture_detector.h:207: float max_touch_diameter_; Perhaps |max_diameter_before_show_press_|. It's a little long, but I think it's better to be explicit here. https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... File ui/events/gesture_detection/gesture_provider.cc (right): https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/gesture_provider.cc:126: GestureEventData CreateGesture(const GestureEventDetails& details, Hmmm, I don't really like adding even more |CreateGesture| methods. We need to figure out a way to clean these up. One possibility is to have the GestureProvider tell the GestureListenerImpl about the current max radius and current_down_event_. Then if we moved CreateGesture and GetBoundingBox into the GestureListenerImpl, and gave GetBoundingBox a parameter indicating the gesture type, then GetBoundingBox could calculate the bounding box correctly for all gesture types. That feels a little ugly though. Any other ideas? Jared, any thoughts? https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/gesture_provider.cc:551: ET_GESTURE_TAP_UNCONFIRMED, e, x, y, max_diameter)); Add a space to line up with "Create" https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... File ui/events/gesture_detection/gesture_provider_unittest.cc (right): https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/gesture_provider_unittest.cc:2410: EXPECT_EQ(0, GetMostRecentGestureEvent().details.bounding_box_f().height()); Why did this change? (We probably need to include the ACTION_DOWN radius in our calculation of the maximum touch radius.) https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/gesture_provider_unittest.cc:2438: // Test the bounding box for show press and tap gestures. Nice test! https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/gesture_provider_unittest.cc:2464: RunTasksAndWait(showpress_timeout * 2 + kOneMicrosecond); Probably don't need that kOneMicrosecond, as we're already doubling the wait time. https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... File ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc (right): https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc:212: GestureEventData CreateGesture(EventType type) { Rewrite this in terms of the |CreateGesture| method you added. https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc:1102: PushGesture(ET_GESTURE_TAP_DOWN, x, y, diameter); You're trying to test that we don't just use the diameter from the tap down gesture, so we shouldn't use the same diameter in the tap down as the tap.
Haven't looked at this too closely, but one initial thought. https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... File ui/events/gesture_detection/gesture_detector.h (right): https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/gesture_detector.h:76: virtual bool OnSingleTapUp(const MotionEvent& e, These callbacks variations are rather unfortunate, and I'm not sure we need to couple the GestureDetector to our specific interpretation of how a press/tap should be positioned. What if we instead do the necessary bookkeeping in the GestureListenerImpl?
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #2 (id:100001) has been deleted
Keeping track of max diameter in GestureListenerImpl https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... File ui/events/gesture_detection/gesture_detector.h (right): https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/gesture_detector.h:206: // sent. On 2014/09/04 13:54:00, tdresser wrote: > Perhaps: > // The maximum touch diameter before the show press gesture is sent. > Done. https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/gesture_detector.h:207: float max_touch_diameter_; On 2014/09/04 13:54:00, tdresser wrote: > Perhaps |max_diameter_before_show_press_|. > > It's a little long, but I think it's better to be explicit here. Done. https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... File ui/events/gesture_detection/gesture_provider_unittest.cc (right): https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/gesture_provider_unittest.cc:2410: EXPECT_EQ(0, GetMostRecentGestureEvent().details.bounding_box_f().height()); On 2014/09/04 13:54:01, tdresser wrote: > Why did this change? (We probably need to include the ACTION_DOWN radius in our > calculation of the maximum touch radius.) Now we do not care about the radius of ACTION_UP anymore, we consider about the largest radius from ACTION_DOWN and all the ACTION_MOVE before the show press is fired for bounding box, so it is 0 now for the bounding box's width and height. https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/gesture_provider_unittest.cc:2464: RunTasksAndWait(showpress_timeout * 2 + kOneMicrosecond); On 2014/09/04 13:54:01, tdresser wrote: > Probably don't need that kOneMicrosecond, as we're already doubling the wait > time. Done. https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... File ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc (right): https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc:212: GestureEventData CreateGesture(EventType type) { On 2014/09/04 13:54:01, tdresser wrote: > Rewrite this in terms of the |CreateGesture| method you added. Done. https://codereview.chromium.org/538653002/diff/20001/ui/events/gesture_detect... ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc:1102: PushGesture(ET_GESTURE_TAP_DOWN, x, y, diameter); On 2014/09/04 13:54:01, tdresser wrote: > You're trying to test that we don't just use the diameter from the tap down > gesture, so we shouldn't use the same diameter in the tap down as the tap. > Done.
Thanks! This looks good, I'll let tdresser@ do final review. https://codereview.chromium.org/538653002/diff/120001/ui/events/gesture_detec... File ui/events/gesture_detection/gesture_provider.cc (right): https://codereview.chromium.org/538653002/diff/120001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider.cc:14: #include "ui/gfx/point.h" Hmm, I looked for the gfx::Point usage, but I only see PointF? https://codereview.chromium.org/538653002/diff/120001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider.cc:124: Nit: Might as well make this an else if (action ==... https://codereview.chromium.org/538653002/diff/120001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider.cc:126: if (!show_press_event_sent_) Maybe |if(!show_press_event_sent_ && !scroll_event_sent_)| ? https://codereview.chromium.org/538653002/diff/120001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider.cc:563: type == ET_GESTURE_TAP_UNCONFIRMED) { Let's add a |DCHECK_EQ(0U, i)| here to be sure (I don't see that validated anywhere else, maybe it's unnecessary?).
Maybe update the patch description so that the body comes before BUG=? I've also found it useful in the past to limit the description text width to 72 characters (80 is git+terminal friendly, and 72 means a reverted patch still fits within 80 characters :), but that point is open to debate (i.e., I don't think it's a formal restriction, or presubmit would complain). Thanks.
LGTM (once you've addressed Jared's comments) https://codereview.chromium.org/538653002/diff/120001/ui/events/gesture_detec... File ui/events/gesture_detection/gesture_provider.cc (right): https://codereview.chromium.org/538653002/diff/120001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider.cc:562: if (type == ET_GESTURE_SHOW_PRESS || type == ET_GESTURE_TAP || Lets add a comment explaining why we special case these gesture types. https://codereview.chromium.org/538653002/diff/120001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider.cc:652: // Tracks whether ET_GESTURE_SHOW_PRESS event has been sent. Lets be a bit more specific here. Something like: Tracks whether an ET_GESTURE_SHOW_PRESS event has been sent for this touch sequence.
On 2014/09/05 21:05:14, jdduke wrote: > Maybe update the patch description so that the body comes before BUG=? I've also > found it useful in the past to limit the description text width to 72 characters > (80 is git+terminal friendly, and 72 means a reverted patch still fits within 80 > characters :), but that point is open to debate (i.e., I don't think it's a > formal restriction, or presubmit would complain). Thanks. Thank you very much for your advice, it is much nicer and easier to read after I changed the format :)
https://codereview.chromium.org/538653002/diff/120001/ui/events/gesture_detec... File ui/events/gesture_detection/gesture_provider.cc (right): https://codereview.chromium.org/538653002/diff/120001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider.cc:14: #include "ui/gfx/point.h" On 2014/09/05 20:55:56, jdduke wrote: > Hmm, I looked for the gfx::Point usage, but I only see PointF? I changed to #include "ui/gfx/geometry/point_f.h" Done. https://codereview.chromium.org/538653002/diff/120001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider.cc:124: On 2014/09/05 20:55:56, jdduke wrote: > Nit: Might as well make this an else if (action ==... Done. https://codereview.chromium.org/538653002/diff/120001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider.cc:126: if (!show_press_event_sent_) On 2014/09/05 20:55:56, jdduke wrote: > Maybe |if(!show_press_event_sent_ && !scroll_event_sent_)| ? Done. https://codereview.chromium.org/538653002/diff/120001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider.cc:562: if (type == ET_GESTURE_SHOW_PRESS || type == ET_GESTURE_TAP || On 2014/09/08 13:24:36, tdresser wrote: > Lets add a comment explaining why we special case these gesture types. Done. https://codereview.chromium.org/538653002/diff/120001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider.cc:563: type == ET_GESTURE_TAP_UNCONFIRMED) { On 2014/09/05 20:55:56, jdduke wrote: > Let's add a |DCHECK_EQ(0U, i)| here to be sure (I don't see that validated > anywhere else, maybe it's unnecessary?). It is a good idea that we check if it is single fingle. Done.
lgtm, thanks.
https://codereview.chromium.org/538653002/diff/140001/ui/events/gesture_detec... File ui/events/gesture_detection/gesture_provider.cc (right): https://codereview.chromium.org/538653002/diff/140001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider.cc:124: if (!show_press_event_sent_ && !scroll_event_sent_) I prefer adding {} when the body of a conditional is more than 1 line long. https://codereview.chromium.org/538653002/diff/140001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider.cc:650: // sent and there must be a tap event happening afterwards. It isn't that a tap event must happen afterwards, its that a tap must still be possible for this touch sequence.
Thanks for your comments. They are very helpful. https://codereview.chromium.org/538653002/diff/140001/ui/events/gesture_detec... File ui/events/gesture_detection/gesture_provider.cc (right): https://codereview.chromium.org/538653002/diff/140001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider.cc:124: if (!show_press_event_sent_ && !scroll_event_sent_) On 2014/09/08 15:52:47, tdresser wrote: > I prefer adding {} when the body of a conditional is more than 1 line long. Done. https://codereview.chromium.org/538653002/diff/140001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_provider.cc:650: // sent and there must be a tap event happening afterwards. On 2014/09/08 15:52:47, tdresser wrote: > It isn't that a tap event must happen afterwards, its that a tap must still be > possible for this touch sequence. Thanks. Done.
The CQ bit was checked by lanwei@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lanwei@chromium.org/538653002/160001
Message was sent while issue was closed.
Committed patchset #4 (id:160001) as e28d4e3b62c5913bf00459aeef3cf1fc82cc2ab0
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/647665c49d055d6a4645f7c842b35f2d0c20a025 Cr-Commit-Position: refs/heads/master@{#293807} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
