Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1120293003:
Make sure send one WebTouchEvent ack per ui::TouchEvent (Closed)
DescriptionMake sure send one WebTouchEvent ack per ui::TouchEvent
In our current code (RenderWidgetHostViewAura), we may send more than one ack
for one WebTouchEvent in the multi-touch scenario, but we should only send
one WebTouchEvent ack for each ui::TouchEvent. For touchmove and
touchcancel, we will only change the state for the actual touch point which
causes this touchevent for multi-touch.
BUG=484276
Committed: https://crrev.com/901538b70297c3984afc8c045381ebff5bfdaa9e
Cr-Commit-Position: refs/heads/master@{#329161}
Patch Set 1 : #
Total comments: 3
Patch Set 2 : put in RenderWidgetHostViewAura #
Total comments: 8
Patch Set 3 : Use touch_id and add unittest #
Total comments: 15
Patch Set 4 : #
Total comments: 3
Patch Set 5 : Change the unittest #
Total comments: 1
Patch Set 6 : #
Messages
Total messages: 32 (9 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:40001) has been deleted
lanwei@chromium.org changed reviewers: + jdduke@chromium.org, tdresser@chromium.org
Patchset #1 (id:60001) has been deleted
https://codereview.chromium.org/1120293003/diff/80001/ui/events/blink/blink_e... File ui/events/blink/blink_event_util.cc (right): https://codereview.chromium.org/1120293003/diff/80001/ui/events/blink/blink_e... ui/events/blink/blink_event_util.cc:56: return static_cast<int>(pointer_index) == event.GetActionIndex() GetActionIndex is only valid if |GetAction()| returns ACTION_POINTER_UP or ACTION_POINTER_DOWN.
https://codereview.chromium.org/1120293003/diff/80001/ui/events/gestures/moti... File ui/events/gestures/motion_event_aura.cc (right): https://codereview.chromium.org/1120293003/diff/80001/ui/events/gestures/moti... ui/events/gestures/motion_event_aura.cc:185: set_action_index(GetIndexFromId(touch.touch_id())); This causes us to deviate from the Android implementation (http://developer.android.com/reference/android/view/MotionEvent.html#getActio...), which isn't ideal. I think this change won't work correctly on Android.
Hmm, I don't see where this actually prevents us from getting multiple acks per ack'ed WebTouchEvent? Is there more code here? Or do we use GetActionIndex() in some way that we're not supposed to? https://codereview.chromium.org/1120293003/diff/80001/ui/events/gestures/moti... File ui/events/gestures/motion_event_aura.cc (right): https://codereview.chromium.org/1120293003/diff/80001/ui/events/gestures/moti... ui/events/gestures/motion_event_aura.cc:185: set_action_index(GetIndexFromId(touch.touch_id())); On 2015/05/04 20:41:10, tdresser wrote: > This causes us to deviate from the Android implementation > (http://developer.android.com/reference/android/view/MotionEvent.html#getActio...), > which isn't ideal. > > I think this change won't work correctly on Android. Yeah, I'd prefer that we not overload the meaning of this property. In particular, on Android, we get batched movement, so for multitouch all pointers can move for a single event.
See if you can add a test. https://codereview.chromium.org/1120293003/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1120293003/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:811: || event->type == blink::WebInputEvent::TouchCancel) { Why TouchMove and TouchCancel? Can't this occur with TouchStart/TouchEnd? https://codereview.chromium.org/1120293003/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:812: size_t index = static_cast<int>(pointer_state_.GetIndexFromId(touch_id)); I'm a bit uneasy about relying on the fact that we use the same index in MotionEventAura and WebInputEvent. Can't we just check the id for each pointer?
https://codereview.chromium.org/1120293003/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1120293003/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:811: || event->type == blink::WebInputEvent::TouchCancel) { On 2015/05/06 11:58:18, tdresser wrote: > Why TouchMove and TouchCancel? > Can't this occur with TouchStart/TouchEnd? I don't think we will ever generate multiple press/release pointers per MotionEvent, but it wouldn't hurt validate that. https://codereview.chromium.org/1120293003/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:812: size_t index = static_cast<int>(pointer_state_.GetIndexFromId(touch_id)); On 2015/05/06 11:58:18, tdresser wrote: > I'm a bit uneasy about relying on the fact that we use the same index in > MotionEventAura and WebInputEvent. > > Can't we just check the id for each pointer? +1
https://codereview.chromium.org/1120293003/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1120293003/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:811: || event->type == blink::WebInputEvent::TouchCancel) { On 2015/05/06 15:23:45, jdduke wrote: > On 2015/05/06 11:58:18, tdresser wrote: > > Why TouchMove and TouchCancel? > > Can't this occur with TouchStart/TouchEnd? > > I don't think we will ever generate multiple press/release pointers per > MotionEvent, but it wouldn't hurt validate that. Right, thanks. Do we generate multiple cancel pointers for a single MotionEvent on Aura?
On 2015/05/06 15:28:34, tdresser wrote: > https://codereview.chromium.org/1120293003/diff/100001/content/browser/render... > File content/browser/renderer_host/render_widget_host_view_aura.cc (right): > > https://codereview.chromium.org/1120293003/diff/100001/content/browser/render... > content/browser/renderer_host/render_widget_host_view_aura.cc:811: || > event->type == blink::WebInputEvent::TouchCancel) { > On 2015/05/06 15:23:45, jdduke wrote: > > On 2015/05/06 11:58:18, tdresser wrote: > > > Why TouchMove and TouchCancel? > > > Can't this occur with TouchStart/TouchEnd? > > > > I don't think we will ever generate multiple press/release pointers per > > MotionEvent, but it wouldn't hurt validate that. > > Right, thanks. > > Do we generate multiple cancel pointers for a single MotionEvent on Aura? It looks like we set all WebTouchEvent pointers to cancel regardless of the platform when converting from MotionEvent (same with move events). So I guess the question is whether the per-pointer cancellation events would get filtered before arriving here. I *think* we allow them to pass through the FilteredGestureProvider, so probably makes sense to do the cleanup for them here as well?
On 2015/05/06 15:33:29, jdduke wrote: > On 2015/05/06 15:28:34, tdresser wrote: > > > https://codereview.chromium.org/1120293003/diff/100001/content/browser/render... > > File content/browser/renderer_host/render_widget_host_view_aura.cc (right): > > > > > https://codereview.chromium.org/1120293003/diff/100001/content/browser/render... > > content/browser/renderer_host/render_widget_host_view_aura.cc:811: || > > event->type == blink::WebInputEvent::TouchCancel) { > > On 2015/05/06 15:23:45, jdduke wrote: > > > On 2015/05/06 11:58:18, tdresser wrote: > > > > Why TouchMove and TouchCancel? > > > > Can't this occur with TouchStart/TouchEnd? > > > > > > I don't think we will ever generate multiple press/release pointers per > > > MotionEvent, but it wouldn't hurt validate that. > > > > Right, thanks. > > > > Do we generate multiple cancel pointers for a single MotionEvent on Aura? > > It looks like we set all WebTouchEvent pointers to cancel regardless of the > platform when converting from MotionEvent (same with move events). So I guess > the question is whether the per-pointer cancellation events would get filtered > before arriving here. I *think* we allow them to pass through the > FilteredGestureProvider, so probably makes sense to do the cleanup for them here > as well? SGTM.
Patchset #3 (id:120001) has been deleted
https://codereview.chromium.org/1120293003/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1120293003/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:811: || event->type == blink::WebInputEvent::TouchCancel) { On 2015/05/06 15:28:34, tdresser wrote: > On 2015/05/06 15:23:45, jdduke wrote: > > On 2015/05/06 11:58:18, tdresser wrote: > > > Why TouchMove and TouchCancel? > > > Can't this occur with TouchStart/TouchEnd? > > > > I don't think we will ever generate multiple press/release pointers per > > MotionEvent, but it wouldn't hurt validate that. > > Right, thanks. > > Do we generate multiple cancel pointers for a single MotionEvent on Aura? For touchstart and touchend, because motion_event has type of ACTION_DOWN and ACTION_POINTER_DOWN, when converting from motion_event to webtouchevent, the state is correctly setup. https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/blink/bl... I tested for touchcancel, it has the same problem as touchmove, all the touch points will be set to cancel ad the states, so we need to cleanup both touchmove and touchcancel. https://codereview.chromium.org/1120293003/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:812: size_t index = static_cast<int>(pointer_state_.GetIndexFromId(touch_id)); On 2015/05/06 11:58:18, tdresser wrote: > I'm a bit uneasy about relying on the fact that we use the same index in > MotionEventAura and WebInputEvent. > > Can't we just check the id for each pointer? You are right, index is not safe, I am using touch_id instead. Done.
Thanks for the test! LGTM. https://codereview.chromium.org/1120293003/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1120293003/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:811: || event->type == blink::WebInputEvent::TouchCancel) { On 2015/05/07 02:37:56, lanwei wrote: > On 2015/05/06 15:28:34, tdresser wrote: > > On 2015/05/06 15:23:45, jdduke wrote: > > > On 2015/05/06 11:58:18, tdresser wrote: > > > > Why TouchMove and TouchCancel? > > > > Can't this occur with TouchStart/TouchEnd? > > > > > > I don't think we will ever generate multiple press/release pointers per > > > MotionEvent, but it wouldn't hurt validate that. > > > > Right, thanks. > > > > Do we generate multiple cancel pointers for a single MotionEvent on Aura? > > For touchstart and touchend, because motion_event has type of ACTION_DOWN and > ACTION_POINTER_DOWN, when converting from motion_event to webtouchevent, the > state is correctly setup. > https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/blink/bl... > > I tested for touchcancel, it has the same problem as touchmove, all the touch > points will be set to cancel ad the states, so we need to cleanup both touchmove > and touchcancel. Acknowledged. https://codereview.chromium.org/1120293003/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/1120293003/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1155: ui::TouchEvent press1(ui::ET_TOUCH_PRESSED, gfx::Point(10, 10), 1, nit: Can we use press1/press2 (or press0/press1) instead of press/press1? https://codereview.chromium.org/1120293003/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1178: Can we add an additional move here, to test the case where both pointers move, one after the other?
https://codereview.chromium.org/1120293003/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1120293003/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:808: void RenderWidgetHostViewAura::SetTouchPointStateStationary( Hmm, ideally this would just be a free, nonmember function, see my comments in the test. Maybe call this |MarkUnchangedTouchPointsAsStationary()|? https://codereview.chromium.org/1120293003/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:810: int touch_id) { Maybe |changed_touch_id|? https://codereview.chromium.org/1120293003/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:1298: for (size_t i = 0; i < touch.event.touchesLength; ++i) { Should we be validating here that we only process one touch point per ack? https://codereview.chromium.org/1120293003/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/1120293003/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:303: touch_event_.reset( Hmm, why do have separate bookkeeping here for touches? This increases the maintenance burden on the tests and makes it more likely for tested behavior to deviate from actual behavior.
https://codereview.chromium.org/1120293003/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1120293003/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:808: void RenderWidgetHostViewAura::SetTouchPointStateStationary( On 2015/05/07 15:20:28, jdduke wrote: > Hmm, ideally this would just be a free, nonmember function, see my comments in > the test. Maybe call this |MarkUnchangedTouchPointsAsStationary()|? Because I need to use this function in the unittest, so have to make it as a member function. https://codereview.chromium.org/1120293003/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:810: int touch_id) { On 2015/05/07 15:20:28, jdduke wrote: > Maybe |changed_touch_id|? Done. https://codereview.chromium.org/1120293003/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:1298: for (size_t i = 0; i < touch.event.touchesLength; ++i) { On 2015/05/07 15:20:28, jdduke wrote: > Should we be validating here that we only process one touch point per ack? Done. https://codereview.chromium.org/1120293003/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/1120293003/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:303: touch_event_.reset( On 2015/05/07 15:20:28, jdduke wrote: > Hmm, why do have separate bookkeeping here for touches? This increases the > maintenance burden on the tests and makes it more likely for tested behavior to > deviate from actual behavior. Tim, I saw that you added this touch_event_, can you please explain why you added this and reset it here every time? Thanks. https://codereview.chromium.org/1120293003/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1155: ui::TouchEvent press1(ui::ET_TOUCH_PRESSED, gfx::Point(10, 10), 1, On 2015/05/07 12:13:06, tdresser wrote: > nit: Can we use press1/press2 (or press0/press1) instead of press/press1? Done. https://codereview.chromium.org/1120293003/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1178: On 2015/05/07 12:13:06, tdresser wrote: > Can we add an additional move here, to test the case where both pointers move, > one after the other? Done.
https://codereview.chromium.org/1120293003/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1120293003/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:808: void RenderWidgetHostViewAura::SetTouchPointStateStationary( On 2015/05/08 02:12:32, lanwei wrote: > On 2015/05/07 15:20:28, jdduke wrote: > > Hmm, ideally this would just be a free, nonmember function, see my comments in > > the test. Maybe call this |MarkUnchangedTouchPointsAsStationary()|? > > Because I need to use this function in the unittest, so have to make it as a > member function. Yeah, noticed that later, let's see what Tim says about needing to do the touch bookkeeping. https://codereview.chromium.org/1120293003/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.h (right): https://codereview.chromium.org/1120293003/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.h:369: void SetTouchPointStateStationary(blink::WebTouchEvent* event, int touch_id); If we need this let's go ahead and make it static.
Jared, thanks for your attention to detail here. https://codereview.chromium.org/1120293003/diff/140001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/1120293003/diff/140001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:303: touch_event_.reset( On 2015/05/08 02:12:32, lanwei wrote: > On 2015/05/07 15:20:28, jdduke wrote: > > Hmm, why do have separate bookkeeping here for touches? This increases the > > maintenance burden on the tests and makes it more likely for tested behavior > to > > deviate from actual behavior. > > Tim, I saw that you added this touch_event_, can you please explain why you > added this and reset it here every time? Thanks. This was put here to avoid code churn (or out of laziness, depending on your perspective). We used to keep track of state in RWHVAura using a WebTouchEvent, so all the tests were written with expectations on a WebTouchEvent. When I switched us over to storing the state using a MotionEventAura, I added this WebTouchEvent so we could avoid modifying all the tests. I think it did make sense to avoid conflating a bunch of test churn with a functional change, but it's true that in the long term this would be simpler without this additional logic. I've filed http://crbug.com/485968 to address this cleanup. Lan, I've put it on my plate for now, but if you want to take it go ahead.
https://codereview.chromium.org/1120293003/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/1120293003/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1131: TEST_F(RenderWidgetHostViewAuraTest, MultiTouchPointsStates) { I guess I'm wondering why we can't validate the result of |FakeWindowEventDispatcher::ProcessedTouchEvent()| and just verify that we only get one call per event?
https://codereview.chromium.org/1120293003/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/1120293003/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1131: TEST_F(RenderWidgetHostViewAuraTest, MultiTouchPointsStates) { On 2015/05/08 15:28:56, jdduke wrote: > I guess I'm wondering why we can't validate the result of > |FakeWindowEventDispatcher::ProcessedTouchEvent()| and just verify that we only > get one call per event? That would be an improvement.
https://codereview.chromium.org/1120293003/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/1120293003/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1131: TEST_F(RenderWidgetHostViewAuraTest, MultiTouchPointsStates) { On 2015/05/08 15:34:26, tdresser wrote: > On 2015/05/08 15:28:56, jdduke wrote: > > I guess I'm wondering why we can't validate the result of > > |FakeWindowEventDispatcher::ProcessedTouchEvent()| and just verify that we > only > > get one call per event? > > That would be an improvement. Thanks for the good suggestion, it is better to use in this test.
lgtm https://codereview.chromium.org/1120293003/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1120293003/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:280: // Reset unchange touch point to StateStationary for touchmove and Nit: unchanged.
The CQ bit was checked by lanwei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, jdduke@chromium.org Link to the patchset: https://codereview.chromium.org/1120293003/#ps200001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1120293003/200001
Message was sent while issue was closed.
Committed patchset #6 (id:200001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/901538b70297c3984afc8c045381ebff5bfdaa9e Cr-Commit-Position: refs/heads/master@{#329161} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
