Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1847823005:
Fix Event Latency Info on generate gesture events. (Closed)
|
Created:
4 years, 8 months ago by dtapuska Modified:
4 years, 8 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, dtapuska+chromiumwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix Event Latency Info on generate gesture events.
Generated gesture events weren't marking the begin component
correctly which caused the tag not to be set correctly.
In order to achieve this proper marking the events need
to propagate up from the InputRouterImpl into the
RenderWidgetHostImpl which adds the latency information.
BUG=596823
Committed: https://crrev.com/581656043e63a1e26fb6b038823b2d0bb2a28710
Cr-Commit-Position: refs/heads/master@{#384951}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Don't fully qualify blink::WebGestureEvent in mouse wheel event queue #Patch Set 3 : Fix build #Patch Set 4 : Fix DCHECKS failing #
Total comments: 2
Patch Set 5 : Remove debugging #
Total comments: 2
Messages
Total messages: 34 (12 generated)
Description was changed from ========== Fix Event Latency Info on generate gesture events. Generated gesture events weren't marking the begin component correctly which caused the tag not to be set correctly. In order to achieve this proper marking the events need to propagate up from the InputRouterImpl into the RenderWidgetHostImpl which adds the latency information. BUG=596823 ========== to ========== Fix Event Latency Info on generate gesture events. Generated gesture events weren't marking the begin component correctly which caused the tag not to be set correctly. In order to achieve this proper marking the events need to propagate up from the InputRouterImpl into the RenderWidgetHostImpl which adds the latency information. BUG=596823 ==========
dtapuska@chromium.org changed reviewers: + tdresser@chromium.org
Try bot was run here to verify fix: https://build.chromium.org/p/tryserver.chromium.perf/builders/mac_10_11_perf_...
LGTM. https://codereview.chromium.org/1847823005/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/1847823005/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:13: using ui::LatencyInfo; Feel free to add WebGestureEvent here if you want.
https://codereview.chromium.org/1847823005/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/1847823005/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:13: using ui::LatencyInfo; On 2016/04/01 19:28:56, tdresser wrote: > Feel free to add WebGestureEvent here if you want. Done.
The CQ bit was checked by dtapuska@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/1847823005/#ps20001 (title: "Don't fully qualify blink::WebGestureEvent in mouse wheel event queue")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1847823005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1847823005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
The CQ bit was checked by dtapuska@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/1847823005/#ps40001 (title: "Fix build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1847823005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1847823005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
dtapuska@chromium.org changed reviewers: + wjmaclean@chromium.org
On 2016/04/01 21:32:57, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) wjmaclean: Can you please take a look at the diff between patch set 3 and 4. I needed to fix some of the code that you wrote. It is possible to have a touchscreen gesture running at the same time a touchpad gesture is in flight.
https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... content/browser/renderer_host/input/mouse_wheel_event_queue.cc:219: scroll_end_timer_.Reset(); Although this typically wouldn't get hit. The one unit test was generating a GFS while a GSE was in the to be run state. That would generate a GSE after the GFS would have been processed. https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3946: SimulateGestureEvent(WebInputEvent::GestureScrollEnd, This test was just wrong it would generate two back to back GestureScrollBegin with sources as touchscreen. It needs to terminate the GestureScrollEnd of the one before sending the second.
On 2016/04/04 14:53:15, dtapuska wrote: > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > content/browser/renderer_host/input/mouse_wheel_event_queue.cc:219: > scroll_end_timer_.Reset(); > Although this typically wouldn't get hit. The one unit test was generating a GFS > while a GSE was in the to be run state. That would generate a GSE after the GFS > would have been processed. > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc > (right): > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3946: > SimulateGestureEvent(WebInputEvent::GestureScrollEnd, > This test was just wrong it would generate two back to back GestureScrollBegin > with sources as touchscreen. It needs to terminate the GestureScrollEnd of the > one before sending the second. LGTM. You're right, the original code never considered the possibility that there might be multiple gestures with different sources in flight. tdresser@ - I wonder if that might have bearing on the other Gesture issue we're seeing, namely GestureTapDown without a touch-event-related target?
On 2016/04/04 15:34:37, wjmaclean wrote: > On 2016/04/04 14:53:15, dtapuska wrote: > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > content/browser/renderer_host/input/mouse_wheel_event_queue.cc:219: > > scroll_end_timer_.Reset(); > > Although this typically wouldn't get hit. The one unit test was generating a > GFS > > while a GSE was in the to be run state. That would generate a GSE after the > GFS > > would have been processed. > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc > > (right): > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3946: > > SimulateGestureEvent(WebInputEvent::GestureScrollEnd, > > This test was just wrong it would generate two back to back GestureScrollBegin > > with sources as touchscreen. It needs to terminate the GestureScrollEnd of the > > one before sending the second. > > LGTM. You're right, the original code never considered the possibility that > there might be multiple gestures with different sources in flight. > > tdresser@ - I wonder if that might have bearing on the other Gesture issue we're > seeing, namely GestureTapDown without a touch-event-related target? I don't think so - we only send GestureTapDown for touch gestures.
Still LGTM % nit. https://codereview.chromium.org/1847823005/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1847823005/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:1035: : &is_in_touchscreen_gesture_scroll_; I think it would be simpler to just have a bool "is_in_gesture_scroll". Then on line 151, we'd assign is_in_touchpad_gesture_scroll_ or is_in_touchscreen_gesture_scroll_ based on the sourceDevice.
On 2016/04/04 15:37:55, tdresser wrote: > On 2016/04/04 15:34:37, wjmaclean wrote: > > On 2016/04/04 14:53:15, dtapuska wrote: > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > File content/browser/renderer_host/input/mouse_wheel_event_queue.cc (right): > > > > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > content/browser/renderer_host/input/mouse_wheel_event_queue.cc:219: > > > scroll_end_timer_.Reset(); > > > Although this typically wouldn't get hit. The one unit test was generating a > > GFS > > > while a GSE was in the to be run state. That would generate a GSE after the > > GFS > > > would have been processed. > > > > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3946: > > > SimulateGestureEvent(WebInputEvent::GestureScrollEnd, > > > This test was just wrong it would generate two back to back > GestureScrollBegin > > > with sources as touchscreen. It needs to terminate the GestureScrollEnd of > the > > > one before sending the second. > > > > LGTM. You're right, the original code never considered the possibility that > > there might be multiple gestures with different sources in flight. > > > > tdresser@ - I wonder if that might have bearing on the other Gesture issue > we're > > seeing, namely GestureTapDown without a touch-event-related target? > > I don't think so - we only send GestureTapDown for touch gestures. Ahh, ok. I had thought the structure of the gesture sequences would be the same. But it still seems to me we should be treating the non-touch sequence differently, no? For example, it should need to have the ability to route to a different target, and we should be checking the source of the events to determine which sequence we're dealing with when we have multiple sequences in flight. Or am I misunderstanding.
On 2016/04/04 15:48:01, wjmaclean wrote: > On 2016/04/04 15:37:55, tdresser wrote: > > On 2016/04/04 15:34:37, wjmaclean wrote: > > > On 2016/04/04 14:53:15, dtapuska wrote: > > > > > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > > File content/browser/renderer_host/input/mouse_wheel_event_queue.cc > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > > content/browser/renderer_host/input/mouse_wheel_event_queue.cc:219: > > > > scroll_end_timer_.Reset(); > > > > Although this typically wouldn't get hit. The one unit test was generating > a > > > GFS > > > > while a GSE was in the to be run state. That would generate a GSE after > the > > > GFS > > > > would have been processed. > > > > > > > > > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > > File > content/browser/renderer_host/render_widget_host_view_aura_unittest.cc > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > > > content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3946: > > > > SimulateGestureEvent(WebInputEvent::GestureScrollEnd, > > > > This test was just wrong it would generate two back to back > > GestureScrollBegin > > > > with sources as touchscreen. It needs to terminate the GestureScrollEnd of > > the > > > > one before sending the second. > > > > > > LGTM. You're right, the original code never considered the possibility that > > > there might be multiple gestures with different sources in flight. > > > > > > tdresser@ - I wonder if that might have bearing on the other Gesture issue > > we're > > > seeing, namely GestureTapDown without a touch-event-related target? > > > > I don't think so - we only send GestureTapDown for touch gestures. > > Ahh, ok. I had thought the structure of the gesture sequences would be the same. > But it still seems to me we should be treating the non-touch sequence > differently, no? For example, it should need to have the ability to route to a > different target, and we should be checking the source of the events to > determine which sequence we're dealing with when we have multiple sequences in > flight. Or am I misunderstanding. Hmmm, that does sound about right. What will the current behavior be? It would be worth making sure this is sane.
https://codereview.chromium.org/1847823005/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1847823005/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:1035: : &is_in_touchscreen_gesture_scroll_; On 2016/04/04 15:47:48, tdresser wrote: > I think it would be simpler to just have a bool "is_in_gesture_scroll". > > Then on line 151, we'd assign is_in_touchpad_gesture_scroll_ or > is_in_touchscreen_gesture_scroll_ based on the sourceDevice. Dave pointed out this is simpler, cause we don't need to check the sourceDevice twice this way.
On 2016/04/04 15:57:10, tdresser wrote: > On 2016/04/04 15:48:01, wjmaclean wrote: > > On 2016/04/04 15:37:55, tdresser wrote: > > > On 2016/04/04 15:34:37, wjmaclean wrote: > > > > On 2016/04/04 14:53:15, dtapuska wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > > > File content/browser/renderer_host/input/mouse_wheel_event_queue.cc > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > > > content/browser/renderer_host/input/mouse_wheel_event_queue.cc:219: > > > > > scroll_end_timer_.Reset(); > > > > > Although this typically wouldn't get hit. The one unit test was > generating > > a > > > > GFS > > > > > while a GSE was in the to be run state. That would generate a GSE after > > the > > > > GFS > > > > > would have been processed. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > > > File > > content/browser/renderer_host/render_widget_host_view_aura_unittest.cc > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > > > > > content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3946: > > > > > SimulateGestureEvent(WebInputEvent::GestureScrollEnd, > > > > > This test was just wrong it would generate two back to back > > > GestureScrollBegin > > > > > with sources as touchscreen. It needs to terminate the GestureScrollEnd > of > > > the > > > > > one before sending the second. > > > > > > > > LGTM. You're right, the original code never considered the possibility > that > > > > there might be multiple gestures with different sources in flight. > > > > > > > > tdresser@ - I wonder if that might have bearing on the other Gesture issue > > > we're > > > > seeing, namely GestureTapDown without a touch-event-related target? > > > > > > I don't think so - we only send GestureTapDown for touch gestures. > > > > Ahh, ok. I had thought the structure of the gesture sequences would be the > same. > > But it still seems to me we should be treating the non-touch sequence > > differently, no? For example, it should need to have the ability to route to a > > different target, and we should be checking the source of the events to > > determine which sequence we're dealing with when we have multiple sequences in > > flight. Or am I misunderstanding. > > Hmmm, that does sound about right. What will the current behavior be? It would > be worth making sure this is sane. At present, I think we'd be in danger of throwing away non-touch gestures (if no touch gesture sequence is currnetly active), or potentially mis-targeting the events (if a touch gesture sequence *is* currently active).
On 2016/04/04 15:59:42, wjmaclean wrote: > On 2016/04/04 15:57:10, tdresser wrote: > > On 2016/04/04 15:48:01, wjmaclean wrote: > > > On 2016/04/04 15:37:55, tdresser wrote: > > > > On 2016/04/04 15:34:37, wjmaclean wrote: > > > > > On 2016/04/04 14:53:15, dtapuska wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > > > > File content/browser/renderer_host/input/mouse_wheel_event_queue.cc > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > > > > content/browser/renderer_host/input/mouse_wheel_event_queue.cc:219: > > > > > > scroll_end_timer_.Reset(); > > > > > > Although this typically wouldn't get hit. The one unit test was > > generating > > > a > > > > > GFS > > > > > > while a GSE was in the to be run state. That would generate a GSE > after > > > the > > > > > GFS > > > > > > would have been processed. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > > > > File > > > content/browser/renderer_host/render_widget_host_view_aura_unittest.cc > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > > > > > > > content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3946: > > > > > > SimulateGestureEvent(WebInputEvent::GestureScrollEnd, > > > > > > This test was just wrong it would generate two back to back > > > > GestureScrollBegin > > > > > > with sources as touchscreen. It needs to terminate the > GestureScrollEnd > > of > > > > the > > > > > > one before sending the second. > > > > > > > > > > LGTM. You're right, the original code never considered the possibility > > that > > > > > there might be multiple gestures with different sources in flight. > > > > > > > > > > tdresser@ - I wonder if that might have bearing on the other Gesture > issue > > > > we're > > > > > seeing, namely GestureTapDown without a touch-event-related target? > > > > > > > > I don't think so - we only send GestureTapDown for touch gestures. > > > > > > Ahh, ok. I had thought the structure of the gesture sequences would be the > > same. > > > But it still seems to me we should be treating the non-touch sequence > > > differently, no? For example, it should need to have the ability to route to > a > > > different target, and we should be checking the source of the events to > > > determine which sequence we're dealing with when we have multiple sequences > in > > > flight. Or am I misunderstanding. > > > > Hmmm, that does sound about right. What will the current behavior be? It would > > be worth making sure this is sane. > > At present, I think we'd be in danger of throwing away non-touch gestures (if no > touch gesture sequence is currnetly active), or potentially mis-targeting the > events (if a touch gesture sequence *is* currently active). Throwing away the non-touch gestures would likely be fine. Mis-targeting the events probably wouldn't be - we should ensure this doesn't happen. Dave, did we have some logic to prevent wheel gestures while a touch gesture is in progress? I remember discussing it.
On 2016/04/04 16:04:29, tdresser wrote: > On 2016/04/04 15:59:42, wjmaclean wrote: > > On 2016/04/04 15:57:10, tdresser wrote: > > > On 2016/04/04 15:48:01, wjmaclean wrote: > > > > On 2016/04/04 15:37:55, tdresser wrote: > > > > > On 2016/04/04 15:34:37, wjmaclean wrote: > > > > > > On 2016/04/04 14:53:15, dtapuska wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > > > > > File content/browser/renderer_host/input/mouse_wheel_event_queue.cc > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > > > > > content/browser/renderer_host/input/mouse_wheel_event_queue.cc:219: > > > > > > > scroll_end_timer_.Reset(); > > > > > > > Although this typically wouldn't get hit. The one unit test was > > > generating > > > > a > > > > > > GFS > > > > > > > while a GSE was in the to be run state. That would generate a GSE > > after > > > > the > > > > > > GFS > > > > > > > would have been processed. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > > > > > File > > > > content/browser/renderer_host/render_widget_host_view_aura_unittest.cc > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > > > > > > > > > > content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3946: > > > > > > > SimulateGestureEvent(WebInputEvent::GestureScrollEnd, > > > > > > > This test was just wrong it would generate two back to back > > > > > GestureScrollBegin > > > > > > > with sources as touchscreen. It needs to terminate the > > GestureScrollEnd > > > of > > > > > the > > > > > > > one before sending the second. > > > > > > > > > > > > LGTM. You're right, the original code never considered the possibility > > > that > > > > > > there might be multiple gestures with different sources in flight. > > > > > > > > > > > > tdresser@ - I wonder if that might have bearing on the other Gesture > > issue > > > > > we're > > > > > > seeing, namely GestureTapDown without a touch-event-related target? > > > > > > > > > > I don't think so - we only send GestureTapDown for touch gestures. > > > > > > > > Ahh, ok. I had thought the structure of the gesture sequences would be the > > > same. > > > > But it still seems to me we should be treating the non-touch sequence > > > > differently, no? For example, it should need to have the ability to route > to > > a > > > > different target, and we should be checking the source of the events to > > > > determine which sequence we're dealing with when we have multiple > sequences > > in > > > > flight. Or am I misunderstanding. > > > > > > Hmmm, that does sound about right. What will the current behavior be? It > would > > > be worth making sure this is sane. > > > > At present, I think we'd be in danger of throwing away non-touch gestures (if > no > > touch gesture sequence is currnetly active), or potentially mis-targeting the > > events (if a touch gesture sequence *is* currently active). > > Throwing away the non-touch gestures would likely be fine. > > Mis-targeting the events probably wouldn't be - we should ensure this doesn't > happen. > > Dave, did we have some logic to prevent wheel gestures while a touch gesture is > in progress? I remember discussing it. Yes we do have some logic to prevent that; in fact that this is how this method gets called via recursion.. Here is a call flow for an active touchpad gesture scroll ForwardGestureEventWithLatencyInfo(GSB; touch). InputRouter::SendGesture MouseWheelEventQueue::OnGestureScrollEvent ForwardGestureEventWithLatencyInfo(GSE, touchpad) InputRouter::SendGesture GestureEventQueue::QueueEvent GestureEventQueue::QueueEvent
On 2016/04/04 16:49:58, dtapuska wrote: > On 2016/04/04 16:04:29, tdresser wrote: > > On 2016/04/04 15:59:42, wjmaclean wrote: > > > On 2016/04/04 15:57:10, tdresser wrote: > > > > On 2016/04/04 15:48:01, wjmaclean wrote: > > > > > On 2016/04/04 15:37:55, tdresser wrote: > > > > > > On 2016/04/04 15:34:37, wjmaclean wrote: > > > > > > > On 2016/04/04 14:53:15, dtapuska wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > > > > > > File > content/browser/renderer_host/input/mouse_wheel_event_queue.cc > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > > > > > > > content/browser/renderer_host/input/mouse_wheel_event_queue.cc:219: > > > > > > > > scroll_end_timer_.Reset(); > > > > > > > > Although this typically wouldn't get hit. The one unit test was > > > > generating > > > > > a > > > > > > > GFS > > > > > > > > while a GSE was in the to be run state. That would generate a GSE > > > after > > > > > the > > > > > > > GFS > > > > > > > > would have been processed. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > > > > > > File > > > > > content/browser/renderer_host/render_widget_host_view_aura_unittest.cc > > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1847823005/diff/60001/content/browser/rendere... > > > > > > > > > > > > > > > content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3946: > > > > > > > > SimulateGestureEvent(WebInputEvent::GestureScrollEnd, > > > > > > > > This test was just wrong it would generate two back to back > > > > > > GestureScrollBegin > > > > > > > > with sources as touchscreen. It needs to terminate the > > > GestureScrollEnd > > > > of > > > > > > the > > > > > > > > one before sending the second. > > > > > > > > > > > > > > LGTM. You're right, the original code never considered the > possibility > > > > that > > > > > > > there might be multiple gestures with different sources in flight. > > > > > > > > > > > > > > tdresser@ - I wonder if that might have bearing on the other Gesture > > > issue > > > > > > we're > > > > > > > seeing, namely GestureTapDown without a touch-event-related target? > > > > > > > > > > > > I don't think so - we only send GestureTapDown for touch gestures. > > > > > > > > > > Ahh, ok. I had thought the structure of the gesture sequences would be > the > > > > same. > > > > > But it still seems to me we should be treating the non-touch sequence > > > > > differently, no? For example, it should need to have the ability to > route > > to > > > a > > > > > different target, and we should be checking the source of the events to > > > > > determine which sequence we're dealing with when we have multiple > > sequences > > > in > > > > > flight. Or am I misunderstanding. > > > > > > > > Hmmm, that does sound about right. What will the current behavior be? It > > would > > > > be worth making sure this is sane. > > > > > > At present, I think we'd be in danger of throwing away non-touch gestures > (if > > no > > > touch gesture sequence is currnetly active), or potentially mis-targeting > the > > > events (if a touch gesture sequence *is* currently active). > > > > Throwing away the non-touch gestures would likely be fine. > > > > Mis-targeting the events probably wouldn't be - we should ensure this doesn't > > happen. > > > > Dave, did we have some logic to prevent wheel gestures while a touch gesture > is > > in progress? I remember discussing it. > > Yes we do have some logic to prevent that; in fact that this is how this method > gets called > via recursion.. > > Here is a call flow for an active touchpad gesture scroll > > ForwardGestureEventWithLatencyInfo(GSB; touch). > InputRouter::SendGesture > MouseWheelEventQueue::OnGestureScrollEvent > ForwardGestureEventWithLatencyInfo(GSE, touchpad) > InputRouter::SendGesture > GestureEventQueue::QueueEvent > GestureEventQueue::QueueEvent Right, yeah, I think this should be fine. LGTM
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1847823005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1847823005/80001
Message was sent while issue was closed.
Description was changed from ========== Fix Event Latency Info on generate gesture events. Generated gesture events weren't marking the begin component correctly which caused the tag not to be set correctly. In order to achieve this proper marking the events need to propagate up from the InputRouterImpl into the RenderWidgetHostImpl which adds the latency information. BUG=596823 ========== to ========== Fix Event Latency Info on generate gesture events. Generated gesture events weren't marking the begin component correctly which caused the tag not to be set correctly. In order to achieve this proper marking the events need to propagate up from the InputRouterImpl into the RenderWidgetHostImpl which adds the latency information. BUG=596823 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix Event Latency Info on generate gesture events. Generated gesture events weren't marking the begin component correctly which caused the tag not to be set correctly. In order to achieve this proper marking the events need to propagate up from the InputRouterImpl into the RenderWidgetHostImpl which adds the latency information. BUG=596823 ========== to ========== Fix Event Latency Info on generate gesture events. Generated gesture events weren't marking the begin component correctly which caused the tag not to be set correctly. In order to achieve this proper marking the events need to propagate up from the InputRouterImpl into the RenderWidgetHostImpl which adds the latency information. BUG=596823 Committed: https://crrev.com/581656043e63a1e26fb6b038823b2d0bb2a28710 Cr-Commit-Position: refs/heads/master@{#384951} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/581656043e63a1e26fb6b038823b2d0bb2a28710 Cr-Commit-Position: refs/heads/master@{#384951} |
