Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
469523003:
Add the actual location coordinates to the touch cancel event (Closed)
DescriptionAdd the actual location coordinates to the touch cancel event
BUG=398882
Committed: https://crrev.com/9ed09a4d5b4416c2edc7ec0e54ebf675caea9b86
Cr-Commit-Position: refs/heads/master@{#292523}
Patch Set 1 #Patch Set 2 : Fixed the unit tests #
Total comments: 27
Patch Set 3 : Made changes based on comments #Patch Set 4 : canceling the touch points from GestureConsumer #
Total comments: 9
Patch Set 5 : change after removing the old gesture #
Total comments: 2
Patch Set 6 : fix unit tests #Patch Set 7 : clone the state_point #
Total comments: 1
Patch Set 8 : change the name to cancelled_touch_points #
Messages
Total messages: 32 (1 generated)
Add the actual location coordinates to the touch cancel event
On 2014/08/13 16:56:12, lanwei wrote: > Add the actual location coordinates to the touch cancel event You've got some compilation errors on the bots for the unit tests. Might as well fix those before I review. "BUG = 366603" can't have spaces (which is a bit silly). Modify the description to say "BUG=366603".
Fixed the unit tests
https://codereview.chromium.org/469523003/diff/20001/ui/aura/gestures/gesture... File ui/aura/gestures/gesture_recognizer_unittest.cc (left): https://codereview.chromium.org/469523003/diff/20001/ui/aura/gestures/gesture... ui/aura/gestures/gesture_recognizer_unittest.cc:3781: scoped_ptr<RemoveOnTouchCancelHandler> We should be able to delete the RemoveOnTouchCancelHandler, shouldn't we? https://codereview.chromium.org/469523003/diff/20001/ui/aura/gestures/gesture... File ui/aura/gestures/gesture_recognizer_unittest.cc (right): https://codereview.chromium.org/469523003/diff/20001/ui/aura/gestures/gesture... ui/aura/gestures/gesture_recognizer_unittest.cc:608: touch_points_() { You don't need to initialize the std::vector. https://codereview.chromium.org/469523003/diff/20001/ui/aura/gestures/gesture... ui/aura/gestures/gesture_recognizer_unittest.cc:642: int touch_cancelled_count() const { return (int) touch_points_.size(); } Use static_cast<int>(touch_points_.size()); We don't use C style casts in chromium. https://codereview.chromium.org/469523003/diff/20001/ui/aura/gestures/gesture... ui/aura/gestures/gesture_recognizer_unittest.cc:643: std::vector<gfx::PointF> touch_points() const { return touch_points_; } Return a const std::vector<gfx::PointF>&, to avoid copying the vector. https://codereview.chromium.org/469523003/diff/20001/ui/aura/gestures/gesture... ui/aura/gestures/gesture_recognizer_unittest.cc:3773: // Disabled for not unified GR This is a little hard to read (double negative!) Maybe try something like: // Only enabled for unified GR, as the old Aura GR doesn't assign // positions to touch cancel events correctly. https://codereview.chromium.org/469523003/diff/20001/ui/aura/gestures/gesture... ui/aura/gestures/gesture_recognizer_unittest.cc:3786: scoped_ptr<TestEventHandler> Would this fit on one line? https://codereview.chromium.org/469523003/diff/20001/ui/aura/gestures/gesture... ui/aura/gestures/gesture_recognizer_unittest.cc:3797: ui::ET_GESTURE_BEGIN, These aren't indented correctly (silly eclipse). See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... for details. Check out "clang format" or git cl format, they can help with getting the indentation right. https://codereview.chromium.org/469523003/diff/20001/ui/aura/gestures/gesture... ui/aura/gestures/gesture_recognizer_unittest.cc:3800: ui::TouchEvent p2(ui::ET_TOUCH_PRESSED, gfx::Point(50, 50), 1, tes.Now()); Store the touch id in another const int, similar to what we're doing with |kTouchId|. https://codereview.chromium.org/469523003/diff/20001/ui/aura/gestures/gesture... ui/aura/gestures/gesture_recognizer_unittest.cc:3828: EXPECT_EQ(2, (int) points.size()); To avoid the cast: EXPECT_EQ(2U, points.size()); https://codereview.chromium.org/469523003/diff/20001/ui/aura/window_event_dis... File ui/aura/window_event_dispatcher.cc (right): https://codereview.chromium.org/469523003/diff/20001/ui/aura/window_event_dis... ui/aura/window_event_dispatcher.cc:25: #include "ui/events/event_constants.h" Is this necessary? https://codereview.chromium.org/469523003/diff/20001/ui/aura/window_event_dis... ui/aura/window_event_dispatcher.cc:533: event->UpdateForRootTransform(host_->GetRootTransform()); Add a comment indicating why this is here. https://codereview.chromium.org/469523003/diff/20001/ui/events/gestures/gestu... File ui/events/gestures/gesture_recognizer_impl.cc (right): https://codereview.chromium.org/469523003/diff/20001/ui/events/gestures/gestu... ui/events/gestures/gesture_recognizer_impl.cc:268: std::vector<std::pair<int, GestureConsumer*> >* touches) { Can't we just look up the GestureProvider here, and get the touch position? GestureProviderAura* gesture_provider = consumer_gesture_provider_[consumer]; const MotionEventAura& pointer_state = gesture_provider->pointer_state(); and then get the index of the touch and look up it's position here? I'm not clear on why we need the map etc. https://codereview.chromium.org/469523003/diff/20001/ui/events/gestures/gestu... File ui/events/gestures/gesture_recognizer_impl.h (right): https://codereview.chromium.org/469523003/diff/20001/ui/events/gestures/gestu... ui/events/gestures/gesture_recognizer_impl.h:98: // for all the GestureConsumers in the touches Capitalize comments, and end them with a period. I'm not sure you need these though (see comments in gesture_recognizer_impl.cc)
https://codereview.chromium.org/469523003/diff/20001/ui/aura/gestures/gesture... File ui/aura/gestures/gesture_recognizer_unittest.cc (left): https://codereview.chromium.org/469523003/diff/20001/ui/aura/gestures/gesture... ui/aura/gestures/gesture_recognizer_unittest.cc:3781: scoped_ptr<RemoveOnTouchCancelHandler> On 2014/08/13 19:57:58, tdresser wrote: > We should be able to delete the RemoveOnTouchCancelHandler, shouldn't we? Done. https://codereview.chromium.org/469523003/diff/20001/ui/aura/gestures/gesture... File ui/aura/gestures/gesture_recognizer_unittest.cc (right): https://codereview.chromium.org/469523003/diff/20001/ui/aura/gestures/gesture... ui/aura/gestures/gesture_recognizer_unittest.cc:608: touch_points_() { On 2014/08/13 19:57:57, tdresser wrote: > You don't need to initialize the std::vector. Done. https://codereview.chromium.org/469523003/diff/20001/ui/aura/gestures/gesture... ui/aura/gestures/gesture_recognizer_unittest.cc:642: int touch_cancelled_count() const { return (int) touch_points_.size(); } On 2014/08/13 19:57:58, tdresser wrote: > Use static_cast<int>(touch_points_.size()); > We don't use C style casts in chromium. Done. https://codereview.chromium.org/469523003/diff/20001/ui/aura/gestures/gesture... ui/aura/gestures/gesture_recognizer_unittest.cc:643: std::vector<gfx::PointF> touch_points() const { return touch_points_; } On 2014/08/13 19:57:57, tdresser wrote: > Return a const std::vector<gfx::PointF>&, to avoid copying the vector. Done. https://codereview.chromium.org/469523003/diff/20001/ui/aura/gestures/gesture... ui/aura/gestures/gesture_recognizer_unittest.cc:3773: // Disabled for not unified GR On 2014/08/13 19:57:57, tdresser wrote: > This is a little hard to read (double negative!) > Maybe try something like: > > // Only enabled for unified GR, as the old Aura GR doesn't assign > // positions to touch cancel events correctly. Done. https://codereview.chromium.org/469523003/diff/20001/ui/aura/gestures/gesture... ui/aura/gestures/gesture_recognizer_unittest.cc:3786: scoped_ptr<TestEventHandler> On 2014/08/13 19:57:57, tdresser wrote: > Would this fit on one line? Done. https://codereview.chromium.org/469523003/diff/20001/ui/aura/gestures/gesture... ui/aura/gestures/gesture_recognizer_unittest.cc:3797: ui::ET_GESTURE_BEGIN, On 2014/08/13 19:57:58, tdresser wrote: > These aren't indented correctly (silly eclipse). > See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... > for details. > > Check out "clang format" or git cl format, they can help with getting the > indentation right. Done. https://codereview.chromium.org/469523003/diff/20001/ui/aura/gestures/gesture... ui/aura/gestures/gesture_recognizer_unittest.cc:3800: ui::TouchEvent p2(ui::ET_TOUCH_PRESSED, gfx::Point(50, 50), 1, tes.Now()); On 2014/08/13 19:57:57, tdresser wrote: > Store the touch id in another const int, similar to what we're doing with > |kTouchId|. Done. https://codereview.chromium.org/469523003/diff/20001/ui/aura/gestures/gesture... ui/aura/gestures/gesture_recognizer_unittest.cc:3828: EXPECT_EQ(2, (int) points.size()); On 2014/08/13 19:57:57, tdresser wrote: > To avoid the cast: > EXPECT_EQ(2U, points.size()); Done. https://codereview.chromium.org/469523003/diff/20001/ui/aura/window_event_dis... File ui/aura/window_event_dispatcher.cc (right): https://codereview.chromium.org/469523003/diff/20001/ui/aura/window_event_dis... ui/aura/window_event_dispatcher.cc:25: #include "ui/events/event_constants.h" On 2014/08/13 19:57:58, tdresser wrote: > Is this necessary? No, forgot to delete it. Done. https://codereview.chromium.org/469523003/diff/20001/ui/aura/window_event_dis... ui/aura/window_event_dispatcher.cc:533: event->UpdateForRootTransform(host_->GetRootTransform()); On 2014/08/13 19:57:58, tdresser wrote: > Add a comment indicating why this is here. Done. https://codereview.chromium.org/469523003/diff/20001/ui/events/gestures/gestu... File ui/events/gestures/gesture_recognizer_impl.cc (right): https://codereview.chromium.org/469523003/diff/20001/ui/events/gestures/gestu... ui/events/gestures/gesture_recognizer_impl.cc:268: std::vector<std::pair<int, GestureConsumer*> >* touches) { On 2014/08/13 19:57:58, tdresser wrote: > Can't we just look up the GestureProvider here, and get the touch position? > > GestureProviderAura* gesture_provider = > consumer_gesture_provider_[consumer]; > const MotionEventAura& pointer_state = gesture_provider->pointer_state(); > > and then get the index of the touch and look up it's position here? > > I'm not clear on why we need the map etc. Yes, we can, but I feel if we do this for each touch_id, it is kind of slow, because finding the index of the touch point will go through all the points in the pointer_state. Thus I decide to go through the pointer_state once and get the map and then find the corresponding touch location for the touch id will be fast. https://codereview.chromium.org/469523003/diff/20001/ui/events/gestures/gestu... File ui/events/gestures/gesture_recognizer_impl.h (right): https://codereview.chromium.org/469523003/diff/20001/ui/events/gestures/gestu... ui/events/gestures/gesture_recognizer_impl.h:98: // for all the GestureConsumers in the touches On 2014/08/13 19:57:58, tdresser wrote: > Capitalize comments, and end them with a period. > I'm not sure you need these though (see comments in gesture_recognizer_impl.cc) Done.
https://codereview.chromium.org/469523003/diff/20001/ui/events/gestures/gestu... File ui/events/gestures/gesture_recognizer_impl.cc (right): https://codereview.chromium.org/469523003/diff/20001/ui/events/gestures/gestu... ui/events/gestures/gesture_recognizer_impl.cc:268: std::vector<std::pair<int, GestureConsumer*> >* touches) { On 2014/08/14 00:38:46, lanwei wrote: > On 2014/08/13 19:57:58, tdresser wrote: > > Can't we just look up the GestureProvider here, and get the touch position? > > > > GestureProviderAura* gesture_provider = > > consumer_gesture_provider_[consumer]; > > const MotionEventAura& pointer_state = gesture_provider->pointer_state(); > > > > and then get the index of the touch and look up it's position here? > > > > I'm not clear on why we need the map etc. > > Yes, we can, but I feel if we do this for each touch_id, it is kind of slow, > because finding the index of the touch point will go through all the points in > the pointer_state. Thus I decide to go through the pointer_state once and get > the map and then find the corresponding touch location for the touch id will be > fast. An n^2 algorithm when n nearly always 1 or 2 isn't too bad, but it's a good thought. You could use a map local to this function, which I think would be a bit clearer than what you currently have. On average though, I expect the n^2 approach will be faster. Based on our small_map implementation (https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/sm...), we think it's faster to use an n^2 search when n<=4. This will be true in almost all cases, so I don't think it's worth the overhead.
Jared - just making sure you agree with me that finding touch indices via an n^2 walk of the pointers is preferable to creating a temporary map. We could also use a temporary small_map, which would likely perform better than the n^2 walk in cases where we did have many fingers down when the touch cancels were triggered. This is only called occasionally, so I don't think even the small_map is worth it.
canceling the touch points from GestureConsumer instead of going through the touch_id_target_ list
LGTM, with a few last comments. We'll hold off to land until the GR cleanup has landed though. https://codereview.chromium.org/469523003/diff/60001/ui/aura/window_event_dis... File ui/aura/window_event_dispatcher.cc (right): https://codereview.chromium.org/469523003/diff/60001/ui/aura/window_event_dis... ui/aura/window_event_dispatcher.cc:535: // function. Thus, we revert the location back in the below function. I think this could be a bit clearer. The key thing to mention is what units we have now, and what units we need to convert to. Perhaps: The touchcancel event's location is based on the last known location of the pointer, in dips. OnEventFromSource expects events with co-ordinates in raw pixels, so we convert back to raw pixels here. https://codereview.chromium.org/469523003/diff/60001/ui/events/gestures/gestu... File ui/events/gestures/gesture_recognizer_impl.cc (right): https://codereview.chromium.org/469523003/diff/60001/ui/events/gestures/gestu... ui/events/gestures/gesture_recognizer_impl.cc:168: std::set<GestureConsumer*> consumers; Just use a vector, set has a lot more overhead. https://codereview.chromium.org/469523003/diff/60001/ui/events/gestures/gestu... File ui/events/gestures/gesture_recognizer_impl.h (right): https://codereview.chromium.org/469523003/diff/60001/ui/events/gestures/gestu... ui/events/gestures/gesture_recognizer_impl.h:9: #include <set> Remove unnecessary include.
https://codereview.chromium.org/469523003/diff/60001/ui/aura/window_event_dis... File ui/aura/window_event_dispatcher.cc (right): https://codereview.chromium.org/469523003/diff/60001/ui/aura/window_event_dis... ui/aura/window_event_dispatcher.cc:535: // function. Thus, we revert the location back in the below function. On 2014/08/15 14:22:04, tdresser wrote: > I think this could be a bit clearer. The key thing to mention is what units we > have now, and what units we need to convert to. > > Perhaps: > The touchcancel event's location is based on the last known location of the > pointer, in dips. OnEventFromSource expects events with co-ordinates in raw > pixels, so we convert back to raw pixels here. What if you we don't transform the event in WindowEventDispatcher::PrepareEventForDispatch() if the event was not created from a native event (i.e. check event->HasNativeEvent() before doing the transform)? Would that work?
https://codereview.chromium.org/469523003/diff/60001/ui/aura/window_event_dis... File ui/aura/window_event_dispatcher.cc (right): https://codereview.chromium.org/469523003/diff/60001/ui/aura/window_event_dis... ui/aura/window_event_dispatcher.cc:535: // function. Thus, we revert the location back in the below function. On 2014/08/15 16:10:43, sadrul wrote: > On 2014/08/15 14:22:04, tdresser wrote: > > I think this could be a bit clearer. The key thing to mention is what units we > > have now, and what units we need to convert to. > > > > Perhaps: > > The touchcancel event's location is based on the last known location of the > > pointer, in dips. OnEventFromSource expects events with co-ordinates in raw > > pixels, so we convert back to raw pixels here. > > What if you we don't transform the event in > WindowEventDispatcher::PrepareEventForDispatch() if the event was not created > from a native event (i.e. check event->HasNativeEvent() before doing the > transform)? Would that work? Yeah, that sounds reasonable.
https://codereview.chromium.org/469523003/diff/60001/ui/aura/window_event_dis... File ui/aura/window_event_dispatcher.cc (right): https://codereview.chromium.org/469523003/diff/60001/ui/aura/window_event_dis... ui/aura/window_event_dispatcher.cc:535: // function. Thus, we revert the location back in the below function. On 2014/08/15 16:10:43, sadrul wrote: > On 2014/08/15 14:22:04, tdresser wrote: > > I think this could be a bit clearer. The key thing to mention is what units we > > have now, and what units we need to convert to. > > > > Perhaps: > > The touchcancel event's location is based on the last known location of the > > pointer, in dips. OnEventFromSource expects events with co-ordinates in raw > > pixels, so we convert back to raw pixels here. > > What if you we don't transform the event in > WindowEventDispatcher::PrepareEventForDispatch() if the event was not created > from a native event (i.e. check event->HasNativeEvent() before doing the > transform)? Would that work? This is a very good suggestion, I will give it a try.
changes after removing the old gesture
jdduke@chromium.org changed reviewers: + jdduke@chromium.org
https://codereview.chromium.org/469523003/diff/80001/ui/events/gestures/gestu... File ui/events/gestures/gesture_recognizer_impl.cc (right): https://codereview.chromium.org/469523003/diff/80001/ui/events/gestures/gestu... ui/events/gestures/gesture_recognizer_impl.cc:177: const MotionEventAura& pointer_state = Could the referenced MotionEventAura be mutated by the |helper->DispatchCancelTouchEvent(&touch_event);| call below? Also, I see that the cancel dispatch is taking a pointer to an object on the stack, does the cancel dispatch ever "consume" the pointer? Or does it simply mutate it? (sorry for my ignorance here!)
fix the unit tests https://codereview.chromium.org/469523003/diff/60001/ui/aura/window_event_dis... File ui/aura/window_event_dispatcher.cc (right): https://codereview.chromium.org/469523003/diff/60001/ui/aura/window_event_dis... ui/aura/window_event_dispatcher.cc:535: // function. Thus, we revert the location back in the below function. On 2014/08/15 14:22:04, tdresser wrote: > I think this could be a bit clearer. The key thing to mention is what units we > have now, and what units we need to convert to. > > Perhaps: > The touchcancel event's location is based on the last known location of the > pointer, in dips. OnEventFromSource expects events with co-ordinates in raw > pixels, so we convert back to raw pixels here. Done. https://codereview.chromium.org/469523003/diff/60001/ui/events/gestures/gestu... File ui/events/gestures/gesture_recognizer_impl.cc (right): https://codereview.chromium.org/469523003/diff/60001/ui/events/gestures/gestu... ui/events/gestures/gesture_recognizer_impl.cc:168: std::set<GestureConsumer*> consumers; On 2014/08/15 14:22:05, tdresser wrote: > Just use a vector, set has a lot more overhead. Done. https://codereview.chromium.org/469523003/diff/60001/ui/events/gestures/gestu... File ui/events/gestures/gesture_recognizer_impl.h (right): https://codereview.chromium.org/469523003/diff/60001/ui/events/gestures/gestu... ui/events/gestures/gesture_recognizer_impl.h:9: #include <set> On 2014/08/15 14:22:05, tdresser wrote: > Remove unnecessary include. Done.
https://codereview.chromium.org/469523003/diff/80001/ui/events/gestures/gestu... File ui/events/gestures/gesture_recognizer_impl.cc (right): https://codereview.chromium.org/469523003/diff/80001/ui/events/gestures/gestu... ui/events/gestures/gesture_recognizer_impl.cc:177: const MotionEventAura& pointer_state = On 2014/08/26 23:22:24, jdduke wrote: > Could the referenced MotionEventAura be mutated by the > |helper->DispatchCancelTouchEvent(&touch_event);| call below? > > Also, I see that the cancel dispatch is taking a pointer to an object on the > stack, does the cancel dispatch ever "consume" the pointer? Or does it simply > mutate it? (sorry for my ignorance here!) I think there is a risk of the MotionEventAura being mutated. We should either clone it, or generate the full list of touch events before we start dispatching them. Hmmm, I don't think anyone will ever hold on to the touch event, but I'm not 100% positive. We've counted on this behavior for a while now, (see the removed |CancelTouches| method). Sadrul, can you confirm that this should be safe?
I clone the pointer_state so that it is safe to iterator through.
https://codereview.chromium.org/469523003/diff/120001/ui/aura/gestures/gestur... File ui/aura/gestures/gesture_recognizer_unittest.cc (right): https://codereview.chromium.org/469523003/diff/120001/ui/aura/gestures/gestur... ui/aura/gestures/gesture_recognizer_unittest.cc:579: std::vector<gfx::PointF> touch_points_; Perhaps we should name this |cancelled_touch_points_|.
aura and events lgtm
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/469523003/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was unchecked by commit-bot@chromium.org
Failed to commit the patch.
The CQ bit was checked by lanwei@chromium.org
The CQ bit was unchecked by lanwei@chromium.org
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/469523003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as 462854023e95e486e950323a2f51910a92da21d6
Message was sent while issue was closed.
Patchset #9 (id:160001) has been deleted
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/9ed09a4d5b4416c2edc7ec0e54ebf675caea9b86 Cr-Commit-Position: refs/heads/master@{#292523} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
