|
|
Created:
3 years, 7 months ago by chongz Modified:
3 years, 6 months ago CC:
chromium-reviews, jam, dtapuska+chromiumwatch_chromium.org, dglazkov+blink, darin-cc_chromium.org, blink-reviews, kinuko+watch, blink-reviews-api_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[VSync Queue] Plug touch ack to gesture events and flush vsync queue if necessary
For a gesture event, we want to know whether the source touch event was handled
blocking or non-blocking, and we can flush the vsync queue immediately if it was
blocking.
This patch will help avoid regression in |first_gesture_scroll_update_latency|
in |smoothness.tough_scrolling_cases|.
After CL the process would be:
1. Browser -> Renderer: Send touch
2. Renderer -> Browser: Touch Ack
3. Browser: Set |is_source_touch_event_set_non_blocking == true| if the ack was
|INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING*|
4. Browser -> Renderer: Send generated gesture events with
|is_source_touch_event_set_non_blocking| bit
5. Renderer: Flush vsync queue if |is_source_touch_event_set_non_blocking == true|.
BUG=704732
Review-Url: https://codereview.chromium.org/2869823003
Cr-Original-Commit-Position: refs/heads/master@{#480202}
Committed: https://chromium.googlesource.com/chromium/src/+/80553a96dc6712a24a7d0a3e3074d5b7f5f65338
Review-Url: https://codereview.chromium.org/2869823003
Cr-Commit-Position: refs/heads/master@{#481130}
Committed: https://chromium.googlesource.com/chromium/src/+/44f78150b462c4c9daaba7ae47a772fba104e56a
Patch Set 1 #
Total comments: 2
Patch Set 2 : dtapuska's comment: Use ack to determine blocking vs. non-blocking #
Total comments: 6
Patch Set 3 : Use seperate bool for set_non_blocking #
Total comments: 4
Patch Set 4 : Address sadrul's commments #
Total comments: 2
Patch Set 5 : pfeldman's comment: Initialize |is_source_touch_event_set_non_blocking| in ctor #
Total comments: 3
Patch Set 6 : dtapuska's comment: Remove |is_source_touch_event_set_non_blocking| from ctor #Patch Set 7 : Remove empty test "stylus-generated-tap.html", added by mistake #Patch Set 8 : Fix MSAN Use-of-uninitialized-value: Initialize GestureEventDetails #
Total comments: 3
Messages
Total messages: 101 (62 generated)
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Forward gesture events generated by blocking touch events immediately BUG=704732 ========== to ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based |TouchEventQueue::send_touch_events_async_|. 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ==========
Description was changed from ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based |TouchEventQueue::send_touch_events_async_|. 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ========== to ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. *2. Set |source_touch_event_dispatch_type| based |TouchEventQueue::send_touch_events_async_|. 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. * Alternative approaches for 2. could be: a. Cache |InputEventAckState| based on |unique_touch_event_id|. b. Set |source_touch_event_dispatch_type| in |WindowEventDispatcher::ProcessedTouchEvent()|. BUG=704732 ==========
Description was changed from ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. *2. Set |source_touch_event_dispatch_type| based |TouchEventQueue::send_touch_events_async_|. 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. * Alternative approaches for 2. could be: a. Cache |InputEventAckState| based on |unique_touch_event_id|. b. Set |source_touch_event_dispatch_type| in |WindowEventDispatcher::ProcessedTouchEvent()|. BUG=704732 ========== to ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. *2. Set |source_touch_event_dispatch_type| based on |TouchEventQueue::send_touch_events_async_|. 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. * Alternative approaches for 2. could be: a. Cache |InputEventAckState| based on |unique_touch_event_id|. b. Set |source_touch_event_dispatch_type| in |WindowEventDispatcher::ProcessedTouchEvent()|. BUG=704732 ==========
Description was changed from ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. *2. Set |source_touch_event_dispatch_type| based on |TouchEventQueue::send_touch_events_async_|. 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. * Alternative approaches for 2. could be: a. Cache |InputEventAckState| based on |unique_touch_event_id|. b. Set |source_touch_event_dispatch_type| in |WindowEventDispatcher::ProcessedTouchEvent()|. BUG=704732 ========== to ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. *2. Set |source_touch_event_dispatch_type| based on |TouchEventQueue::send_touch_events_async_|. 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. * Alternative approaches for 2.: a. Cache |InputEventAckState| based on |unique_touch_event_id| * Might be overkill? Acks would always be |INPUT_EVENT_ACK_STATE_IGNORED| when |send_touch_events_async_ == true|. b. Set |source_touch_event_dispatch_type| in |WindowEventDispatcher::ProcessedTouchEvent()|. * We probably don't want to expose too much logic to |WindowEventDispatcher|? BUG=704732 ==========
Description was changed from ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. *2. Set |source_touch_event_dispatch_type| based on |TouchEventQueue::send_touch_events_async_|. 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. * Alternative approaches for 2.: a. Cache |InputEventAckState| based on |unique_touch_event_id| * Might be overkill? Acks would always be |INPUT_EVENT_ACK_STATE_IGNORED| when |send_touch_events_async_ == true|. b. Set |source_touch_event_dispatch_type| in |WindowEventDispatcher::ProcessedTouchEvent()|. * We probably don't want to expose too much logic to |WindowEventDispatcher|? BUG=704732 ========== to ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. *2. Set |source_touch_event_dispatch_type| based on |TouchEventQueue::send_touch_events_async_|. 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. * Alternative approaches for 2.: a. Cache |InputEventAckState| based on |unique_touch_event_id| * Might be overkill? We always simulate |INPUT_EVENT_ACK_STATE_IGNORED| when |send_touch_events_async_ == true|. b. Set |source_touch_event_dispatch_type| in |WindowEventDispatcher::ProcessedTouchEvent()|. * We probably don't want to expose too much logic to |WindowEventDispatcher|? BUG=704732 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
chongz@chromium.org changed reviewers: + dtapuska@chromium.org
dtapuska@ PTAL, thanks!
On 2017/05/09 14:40:34, chongz wrote: > dtapuska@ PTAL, thanks! dtapuska@ Can you take a look at this patch? Thanks!
https://codereview.chromium.org/2869823003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/input_router_impl.cc (right): https://codereview.chromium.org/2869823003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/input_router_impl.cc:192: touch_event_queue_->SendTouchEventsAsync() I don't think we really should query the state here. But instead use the ack result of the message to determine if we should send the gesture as blocking or not.
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. *2. Set |source_touch_event_dispatch_type| based on |TouchEventQueue::send_touch_events_async_|. 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. * Alternative approaches for 2.: a. Cache |InputEventAckState| based on |unique_touch_event_id| * Might be overkill? We always simulate |INPUT_EVENT_ACK_STATE_IGNORED| when |send_touch_events_async_ == true|. b. Set |source_touch_event_dispatch_type| in |WindowEventDispatcher::ProcessedTouchEvent()|. * We probably don't want to expose too much logic to |WindowEventDispatcher|? BUG=704732 ========== to ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based on the ack result of the touch event. * e.g. |INPUT_EVENT_ACK_STATE_IGNORED| => |ui::EventResult::ER_UNKNOWN_NON_BLOCKING| 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ==========
Description was changed from ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based on the ack result of the touch event. * e.g. |INPUT_EVENT_ACK_STATE_IGNORED| => |ui::EventResult::ER_UNKNOWN_NON_BLOCKING| 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ========== to ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based on the ack result of the touch event. * e.g. |INPUT_EVENT_ACK_STATE_IGNORED| => |ui::EventResult::ER_UNKNOWN_NON_BLOCKING| 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ==========
Description was changed from ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based on the ack result of the touch event. * e.g. |INPUT_EVENT_ACK_STATE_IGNORED| => |ui::EventResult::ER_UNKNOWN_NON_BLOCKING| 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ========== to ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based on the ack result of the touch event. * e.g. |INPUT_EVENT_ACK_STATE_IGNORED| => |ui::EventResult::ER_UNKNOWN_NON_BLOCKING| 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ==========
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Description was changed from ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based on the ack result of the touch event. * e.g. |INPUT_EVENT_ACK_STATE_IGNORED| => |ui::EventResult::ER_UNKNOWN_NON_BLOCKING| 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ========== to ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based on the ack result of the touch event. * e.g. |INPUT_EVENT_ACK_STATE_IGNORED| => |ui::EventResult::ER_IGNORED_NON_BLOCKING| 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based on the ack result of the touch event. * e.g. |INPUT_EVENT_ACK_STATE_IGNORED| => |ui::EventResult::ER_IGNORED_NON_BLOCKING| 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ========== to ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based on the ack result of the touch event. * e.g. |INPUT_EVENT_ACK_STATE_IGNORED| => |ui::EventResult::ER_IGNORED_NON_BLOCKING| => |WebInputEvent::DispatchType::kEventNonBlocking| 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ==========
Description was changed from ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based on the ack result of the touch event. * e.g. |INPUT_EVENT_ACK_STATE_IGNORED| => |ui::EventResult::ER_IGNORED_NON_BLOCKING| => |WebInputEvent::DispatchType::kEventNonBlocking| 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ========== to ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based on the ack result of the touch event. * e.g. |INPUT_EVENT_ACK_STATE_IGNORED| => |ui::EventResult::ER_IGNORED_NON_BLOCKING| => |WebInputEvent::DispatchType::kEventNonBlocking| 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ==========
Description was changed from ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based on the ack result of the touch event. * e.g. |INPUT_EVENT_ACK_STATE_IGNORED| => |ui::EventResult::ER_IGNORED_NON_BLOCKING| => |WebInputEvent::DispatchType::kEventNonBlocking| 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ========== to ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based on the ack result of the touch event. * e.g. |INPUT_EVENT_ACK_STATE_IGNORED| => |ui::EventResult::ER_IGNORED_NON_BLOCKING| => |WebInputEvent::DispatchType::kEventNonBlocking| 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ==========
Description was changed from ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based on the ack result of the touch event. * e.g. |INPUT_EVENT_ACK_STATE_IGNORED| => |ui::EventResult::ER_IGNORED_NON_BLOCKING| => |WebInputEvent::DispatchType::kEventNonBlocking| 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ========== to ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based on the ack result of the touch event. * e.g. |InputEventAckState::INPUT_EVENT_ACK_STATE_IGNORED| => |ui::EventResult::ER_IGNORED_NON_BLOCKING| => |WebInputEvent::DispatchType::kEventNonBlocking| 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ==========
Description was changed from ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based on the ack result of the touch event. * e.g. |InputEventAckState::INPUT_EVENT_ACK_STATE_IGNORED| => |ui::EventResult::ER_IGNORED_NON_BLOCKING| => |WebInputEvent::DispatchType::kEventNonBlocking| 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ========== to ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based on the ack of the touch event. * e.g. |InputEventAckState::INPUT_EVENT_ACK_STATE_IGNORED| => |ui::EventResult::ER_IGNORED_NON_BLOCKING| => |WebInputEvent::DispatchType::kEventNonBlocking| 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
dtapuska@ I've updated CL to use the ack. PTAL again, thanks! https://codereview.chromium.org/2869823003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/input_router_impl.cc (right): https://codereview.chromium.org/2869823003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/input_router_impl.cc:192: touch_event_queue_->SendTouchEventsAsync() On 2017/05/10 14:49:00, dtapuska wrote: > I don't think we really should query the state here. But instead use the ack > result of the message to determine if we should send the gesture as blocking or > not. Done.
https://codereview.chromium.org/2869823003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2869823003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1668: const ui::EventResult event_result = Why is this different than aura? https://codereview.chromium.org/2869823003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2869823003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1044: case INPUT_EVENT_ACK_STATE_IGNORED: I'm not sure we should map Ignored to ignored_non_blocking; I believe the Renderer can still return IGNORED. I think you really want to map the SET_NON_BLOCKING/SET_NON_BLOCKING_FLING states don't you? https://codereview.chromium.org/2869823003/diff/60001/ui/events/event_constan... File ui/events/event_constants.h (right): https://codereview.chromium.org/2869823003/diff/60001/ui/events/event_constan... ui/events/event_constants.h:156: ER_IGNORED_NON_BLOCKING = The name is slightly confusing because this is a bit set. sadrul@ might have a comment here. https://codereview.chromium.org/2869823003/diff/60001/ui/events/gesture_detec... File ui/events/gesture_detection/gesture_event_data_packet.cc (right): https://codereview.chromium.org/2869823003/diff/60001/ui/events/gesture_detec... ui/events/gesture_detection/gesture_event_data_packet.cc:116: ack_state_ = (result == ER_UNHANDLED || result == ER_IGNORED_NON_BLOCKING) I think these are bit sets.. so comparing them directly seems wrong.
Description was changed from ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based on the ack of the touch event. * e.g. |InputEventAckState::INPUT_EVENT_ACK_STATE_IGNORED| => |ui::EventResult::ER_IGNORED_NON_BLOCKING| => |WebInputEvent::DispatchType::kEventNonBlocking| 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ========== to ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately Queuing all events could increase |first_gesture_scroll_update_latency| in |smoothness.tough_scrolling_cases|. This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based on the ack of the touch event. * e.g. |InputEventAckState::INPUT_EVENT_ACK_STATE_IGNORED| => |ui::EventResult::ER_IGNORED_NON_BLOCKING| => |WebInputEvent::DispatchType::kEventNonBlocking| 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ==========
chongz@chromium.org changed reviewers: + sadrul@chromium.org
sadrul@ Thanks for the help! Can you take another look at the additional question below? Thanks! https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_h... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_h... ui/events/blink/input_handler_proxy.cc:387: HandleInputEvent(event_with_callback->event()); sadrul@ Just to recall: During our in-person discussion you suggested that we should store |InputEventAckState| (same as |EventDisposition|) on renderer for the Touch event and use on next Gesture event. However events could switch between blocking and non-blocking in one scroll, and it seems to me that the following sequence may happen: 1. TouchMove1 2. TouchMove2 3. GestureScroll1 (from TouchMove1) 4. GestureScroll2 (from TouchMove2) Or is there anything I'm missing?
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
sadrul@ I'm still concerned about the out-of-order event sequence... I've made |set_non_blocking| a separate parameter as you've mentioned last time, PTAL, thanks!
https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_h... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_h... ui/events/blink/input_handler_proxy.cc:387: HandleInputEvent(event_with_callback->event()); On 2017/05/25 20:25:28, chongz wrote: > sadrul@ Just to recall: During our in-person discussion you suggested that we > should store |InputEventAckState| (same as |EventDisposition|) on renderer for > the Touch event and use on next Gesture event. > > However events could switch between blocking and non-blocking in one scroll, and > it seems to me that the following sequence may happen: > 1. TouchMove1 > 2. TouchMove2 > 3. GestureScroll1 (from TouchMove1) > 4. GestureScroll2 (from TouchMove2) > > Or is there anything I'm missing? @dtapuska@: Can you confirm if this can happen? Actually, even if this happens, you still want to release the queue immediately, right? So shouldn't you just do a PostTask (and from RenderWidgetInputHandler too, maybe?) to DispatchQueuedInputEvents()? My feedback here is that sending the relevant information from main-thread to compositor-thread should happen directly, instead of having to route through all of browser/aura.
On 2017/06/01 19:57:18, sadrul wrote: > https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_h... > File ui/events/blink/input_handler_proxy.cc (right): > > https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_h... > ui/events/blink/input_handler_proxy.cc:387: > HandleInputEvent(event_with_callback->event()); > On 2017/05/25 20:25:28, chongz wrote: > > sadrul@ Just to recall: During our in-person discussion you suggested that we > > should store |InputEventAckState| (same as |EventDisposition|) on renderer for > > the Touch event and use on next Gesture event. > > > > However events could switch between blocking and non-blocking in one scroll, > and > > it seems to me that the following sequence may happen: > > 1. TouchMove1 > > 2. TouchMove2 > > 3. GestureScroll1 (from TouchMove1) > > 4. GestureScroll2 (from TouchMove2) > > > > Or is there anything I'm missing? > > @dtapuska@: Can you confirm if this can happen? > > Actually, even if this happens, you still want to release the queue immediately, > right? So shouldn't you just do a PostTask (and from RenderWidgetInputHandler > too, maybe?) to DispatchQueuedInputEvents()? > > My feedback here is that sending the relevant information from main-thread to > compositor-thread should happen directly, instead of having to route through all > of browser/aura. Yes that sequence can happen. The filtering of the event is done on the browser side so the gesture isn't released from the queue until the ACK is returned for the event as being unhandled. I'm not sure what dispatching between the main thread and compositor you are talking about but the compositor won't even have the relevant event until the ack is returned and processed by the browser. An alternate implementation of this is to have a table in the renderer compositor that knows about the dispatch type of previous touch events and maps those into whether they should be aligned or not. But I'd prefer not having a table of IDs as that can get complicated to manage and lead to memory leaks.
On 2017/06/05 18:11:42, dtapuska wrote: > On 2017/06/01 19:57:18, sadrul wrote: > > > https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_h... > > File ui/events/blink/input_handler_proxy.cc (right): > > > > > https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_h... > > ui/events/blink/input_handler_proxy.cc:387: > > HandleInputEvent(event_with_callback->event()); > > On 2017/05/25 20:25:28, chongz wrote: > > > sadrul@ Just to recall: During our in-person discussion you suggested that > we > > > should store |InputEventAckState| (same as |EventDisposition|) on renderer > for > > > the Touch event and use on next Gesture event. > > > > > > However events could switch between blocking and non-blocking in one scroll, > > and > > > it seems to me that the following sequence may happen: > > > 1. TouchMove1 > > > 2. TouchMove2 > > > 3. GestureScroll1 (from TouchMove1) > > > 4. GestureScroll2 (from TouchMove2) > > > > > > Or is there anything I'm missing? > > > > @dtapuska@: Can you confirm if this can happen? > > > > Actually, even if this happens, you still want to release the queue > immediately, > > right? So shouldn't you just do a PostTask (and from RenderWidgetInputHandler > > too, maybe?) to DispatchQueuedInputEvents()? > > > > My feedback here is that sending the relevant information from main-thread to > > compositor-thread should happen directly, instead of having to route through > all > > of browser/aura. > > Yes that sequence can happen. > > The filtering of the event is done on the browser side so the gesture isn't > released from the queue until the ACK is returned for the event as being > unhandled. Of course, the browser will need to continue to receive the acks. I am not saying it should not. > I'm not sure what dispatching between the main thread and compositor > you are talking about but the compositor won't even have the relevant event > until the ack is returned and processed by the browser. The new information being added in ui in this CL (i.e. touch-event non-blocking) is never set, or used, from ui (aura, browser), as far as I can see. It is set only when the renderer tells it to set it, and this information is sent back to the renderer. This is what I was referring to when I said communicating between main-thread and compositor (but I guess it can be compositor-thread back to compositor-thread too). > An alternate implementation of this is to have a table in the renderer > compositor that knows about the dispatch type of previous touch events and maps > those into whether they should be aligned or not. But I'd prefer not having a > table of IDs as that can get complicated to manage and lead to memory leaks. I would much rather isolate the complexity to where it is actually needed, and not spread it across all layers. Having said that, adding a table does sound ... not very nice.
OK, I looked at the CL, and it does not actually look too bad. Would you mind updating the CL description please?
On 2017/06/05 20:51:08, sadrul wrote: > OK, I looked at the CL, and it does not actually look too bad + anymore (for context) :) > . Would you mind > updating the CL description please?
Description was changed from ========== [VSync Queue] Dispatch gesture events generated by blocking touch events immediately Queuing all events could increase |first_gesture_scroll_update_latency| in |smoothness.tough_scrolling_cases|. This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based on the ack of the touch event. * e.g. |InputEventAckState::INPUT_EVENT_ACK_STATE_IGNORED| => |ui::EventResult::ER_IGNORED_NON_BLOCKING| => |WebInputEvent::DispatchType::kEventNonBlocking| 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ========== to ========== [VSync Queue] Plug touch ack to gesture events and flush vsync queue if necessary For a gesture event, we want to know whether the source touch event was handled blocking or non-blocking, and we want to flush the vsync queue immediately if it was blocking. This patch will help avoid regression in |first_gesture_scroll_update_latency| in |smoothness.tough_scrolling_cases|. This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based on the ack of the touch event. * e.g. |InputEventAckState::INPUT_EVENT_ACK_STATE_IGNORED| => |ui::EventResult::ER_IGNORED_NON_BLOCKING| => |WebInputEvent::DispatchType::kEventNonBlocking| 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ==========
Description was changed from ========== [VSync Queue] Plug touch ack to gesture events and flush vsync queue if necessary For a gesture event, we want to know whether the source touch event was handled blocking or non-blocking, and we want to flush the vsync queue immediately if it was blocking. This patch will help avoid regression in |first_gesture_scroll_update_latency| in |smoothness.tough_scrolling_cases|. This CL: 1. Adds |DispatchType source_touch_event_dispatch_type| to |WebGestureEvent|. 2. Set |source_touch_event_dispatch_type| based on the ack of the touch event. * e.g. |InputEventAckState::INPUT_EVENT_ACK_STATE_IGNORED| => |ui::EventResult::ER_IGNORED_NON_BLOCKING| => |WebInputEvent::DispatchType::kEventNonBlocking| 3. Dispatch gesture events immediately if |source_touch_event_dispatch_type == kBlocking|. BUG=704732 ========== to ========== [VSync Queue] Plug touch ack to gesture events and flush vsync queue if necessary For a gesture event, we want to know whether the source touch event was handled blocking or non-blocking, and we can flush the vsync queue immediately if it was blocking. This patch will help avoid regression in |first_gesture_scroll_update_latency| in |smoothness.tough_scrolling_cases|. After CL the process would be: 1. Browser -> Renderer: Send touch 2. Renderer -> Browser: Touch Ack 3. Browser: Set |is_source_touch_event_set_non_blocking == true| if the ack was |INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING*| 4. Browser -> Renderer: Send generated gesture events with |is_source_touch_event_set_non_blocking| bit 5. Renderer: Flush vsync queue if |is_source_touch_event_set_non_blocking == true|. BUG=704732 ==========
On 2017/06/05 20:48:20, sadrul wrote: > On 2017/06/05 18:11:42, dtapuska wrote: > > On 2017/06/01 19:57:18, sadrul wrote: > > > > > > https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_h... > > > File ui/events/blink/input_handler_proxy.cc (right): > > > > > > > > > https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_h... > > > ui/events/blink/input_handler_proxy.cc:387: > > > HandleInputEvent(event_with_callback->event()); > > > On 2017/05/25 20:25:28, chongz wrote: > > > > sadrul@ Just to recall: During our in-person discussion you suggested that > > we > > > > should store |InputEventAckState| (same as |EventDisposition|) on renderer > > for > > > > the Touch event and use on next Gesture event. > > > > > > > > However events could switch between blocking and non-blocking in one > scroll, > > > and > > > > it seems to me that the following sequence may happen: > > > > 1. TouchMove1 > > > > 2. TouchMove2 > > > > 3. GestureScroll1 (from TouchMove1) > > > > 4. GestureScroll2 (from TouchMove2) > > > > > > > > Or is there anything I'm missing? > > > > > > @dtapuska@: Can you confirm if this can happen? > > > > > > Actually, even if this happens, you still want to release the queue > > immediately, > > > right? So shouldn't you just do a PostTask (and from > RenderWidgetInputHandler > > > too, maybe?) to DispatchQueuedInputEvents()? > > > > > > My feedback here is that sending the relevant information from main-thread > to > > > compositor-thread should happen directly, instead of having to route through > > all > > > of browser/aura. > > > > Yes that sequence can happen. > > > > The filtering of the event is done on the browser side so the gesture isn't > > released from the queue until the ACK is returned for the event as being > > unhandled. > > Of course, the browser will need to continue to receive the acks. I am not > saying it should not. > > > I'm not sure what dispatching between the main thread and compositor > > you are talking about but the compositor won't even have the relevant event > > until the ack is returned and processed by the browser. > > The new information being added in ui in this CL (i.e. touch-event non-blocking) > is never set, or used, from ui (aura, browser), as far as I can see. It is set > only when the renderer tells it to set it, and this information is sent back to > the renderer. This is what I was referring to when I said communicating between > main-thread and compositor (but I guess it can be compositor-thread back to > compositor-thread too). > > > An alternate implementation of this is to have a table in the renderer > > compositor that knows about the dispatch type of previous touch events and > maps > > those into whether they should be aligned or not. But I'd prefer not having a > > table of IDs as that can get complicated to manage and lead to memory leaks. > > I would much rather isolate the complexity to where it is actually needed, and > not spread it across all layers. Having said that, adding a table does sound ... > not very nice. I'm not sure if I fully understand your suggestion: Do you prefer a table [touch_id -> ack] on the renderer side (so it won't be exposed to the browser), or are you ok with the current approach? Or actually you'd like something different than these two? On 2017/06/05 20:51:53, sadrul wrote: > On 2017/06/05 20:51:08, sadrul wrote: > > OK, I looked at the CL, and it does not actually look too bad > > + anymore (for context) :) > > > . Would you mind > > updating the CL description please? Done. Updated CL description to put more focus on event passing.
> I'm not sure if I fully understand your suggestion: Do you prefer a table > [touch_id -> ack] on the renderer side (so it won't be exposed to the browser), > or are you ok with the current approach? > Or actually you'd like something different than these two? Sorry, I wasn't clear. The current approach in patchset-3 looks reasonable. I will look at it more closely later tonight.
On 2017/06/05 21:37:07, sadrul wrote: > > I'm not sure if I fully understand your suggestion: Do you prefer a table > > [touch_id -> ack] on the renderer side (so it won't be exposed to the > browser), > > or are you ok with the current approach? > > Or actually you'd like something different than these two? > > Sorry, I wasn't clear. The current approach in patchset-3 looks reasonable. I > will look at it more closely later tonight. Gentle ping.
On 2017/06/07 17:41:59, chongz wrote: > On 2017/06/05 21:37:07, sadrul wrote: > > > I'm not sure if I fully understand your suggestion: Do you prefer a table > > > [touch_id -> ack] on the renderer side (so it won't be exposed to the > > browser), > > > or are you ok with the current approach? > > > Or actually you'd like something different than these two? > > > > Sorry, I wasn't clear. The current approach in patchset-3 looks reasonable. I > > will look at it more closely later tonight. > > Gentle ping. Sorry about the review delay :( The non-blink code lgtm
lgtm https://codereview.chromium.org/2869823003/diff/100001/content/browser/render... File content/browser/renderer_host/ui_events_helper.h (right): https://codereview.chromium.org/2869823003/diff/100001/content/browser/render... content/browser/renderer_host/ui_events_helper.h:41: bool InputEventAckStateIsSetNonBlocking(InputEventAckState); Document. https://codereview.chromium.org/2869823003/diff/100001/ui/events/gesture_dete... File ui/events/gesture_detection/gesture_event_data_packet.cc (right): https://codereview.chromium.org/2869823003/diff/100001/ui/events/gesture_dete... ui/events/gesture_detection/gesture_event_data_packet.cc:118: for (auto& gesture : gestures_.container()) Add {}
dtapuska@ PTAL again, thanks! https://codereview.chromium.org/2869823003/diff/100001/content/browser/render... File content/browser/renderer_host/ui_events_helper.h (right): https://codereview.chromium.org/2869823003/diff/100001/content/browser/render... content/browser/renderer_host/ui_events_helper.h:41: bool InputEventAckStateIsSetNonBlocking(InputEventAckState); On 2017/06/08 20:31:48, sadrul wrote: > Document. Done. https://codereview.chromium.org/2869823003/diff/100001/ui/events/gesture_dete... File ui/events/gesture_detection/gesture_event_data_packet.cc (right): https://codereview.chromium.org/2869823003/diff/100001/ui/events/gesture_dete... ui/events/gesture_detection/gesture_event_data_packet.cc:118: for (auto& gesture : gestures_.container()) On 2017/06/08 20:31:48, sadrul wrote: > Add {} Done.
On 2017/06/09 14:51:44, chongz wrote: > dtapuska@ PTAL again, thanks! > > https://codereview.chromium.org/2869823003/diff/100001/content/browser/render... > File content/browser/renderer_host/ui_events_helper.h (right): > > https://codereview.chromium.org/2869823003/diff/100001/content/browser/render... > content/browser/renderer_host/ui_events_helper.h:41: bool > InputEventAckStateIsSetNonBlocking(InputEventAckState); > On 2017/06/08 20:31:48, sadrul wrote: > > Document. > > Done. > > https://codereview.chromium.org/2869823003/diff/100001/ui/events/gesture_dete... > File ui/events/gesture_detection/gesture_event_data_packet.cc (right): > > https://codereview.chromium.org/2869823003/diff/100001/ui/events/gesture_dete... > ui/events/gesture_detection/gesture_event_data_packet.cc:118: for (auto& gesture > : gestures_.container()) > On 2017/06/08 20:31:48, sadrul wrote: > > Add {} > > Done. lgtm
chongz@chromium.org changed reviewers: + pfeldman@chromium.org
pfeldman@ PTAL at "content/browser/BUILD.gn" and 2 "third_party/WebKit/" files, thanks!
https://codereview.chromium.org/2869823003/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebGestureEvent.h (right): https://codereview.chromium.org/2869823003/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebGestureEvent.h:40: bool is_source_touch_event_set_non_blocking; Are you relying upon all the call sites to initialize these? Looks like an anti-pattern to me.
pfeldman@ I've updated as per your comments, PTAL again, thanks! https://codereview.chromium.org/2869823003/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebGestureEvent.h (right): https://codereview.chromium.org/2869823003/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebGestureEvent.h:40: bool is_source_touch_event_set_non_blocking; On 2017/06/09 18:38:08, pfeldman wrote: > Are you relying upon all the call sites to initialize these? Looks like an > anti-pattern to me. Done. Thanks for catching! Yes it would be safer to initialize it in ctors.
On 2017/06/09 18:58:58, chongz wrote: > pfeldman@ I've updated as per your comments, PTAL again, thanks! > > https://codereview.chromium.org/2869823003/diff/120001/third_party/WebKit/pub... > File third_party/WebKit/public/platform/WebGestureEvent.h (right): > > https://codereview.chromium.org/2869823003/diff/120001/third_party/WebKit/pub... > third_party/WebKit/public/platform/WebGestureEvent.h:40: bool > is_source_touch_event_set_non_blocking; > On 2017/06/09 18:38:08, pfeldman wrote: > > Are you relying upon all the call sites to initialize these? Looks like an > > anti-pattern to me. > > Done. Thanks for catching! Yes it would be safer to initialize it in ctors. pfeldman@ Can you take a look again? Thanks!
https://codereview.chromium.org/2869823003/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebGestureEvent.h (right): https://codereview.chromium.org/2869823003/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebGestureEvent.h:40: bool is_source_touch_event_set_non_blocking; lgtm. btw, in c++11 you can do bool is_source_touch_event_set_non_blocking = false;
https://codereview.chromium.org/2869823003/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebGestureEvent.h (right): https://codereview.chromium.org/2869823003/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebGestureEvent.h:40: bool is_source_touch_event_set_non_blocking; On 2017/06/15 18:56:42, pfeldman wrote: > lgtm. btw, in c++11 you can do bool is_source_touch_event_set_non_blocking = > false; Actually the initializer false, isn't needed here... This struct is actually memset(0) in the WebInputEvent ctor.. So chong you can remove the is_source_touch_event_set_non_blocking in the ctors...
https://codereview.chromium.org/2869823003/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebGestureEvent.h (right): https://codereview.chromium.org/2869823003/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebGestureEvent.h:40: bool is_source_touch_event_set_non_blocking; On 2017/06/15 19:18:29, dtapuska wrote: > On 2017/06/15 18:56:42, pfeldman wrote: > > lgtm. btw, in c++11 you can do bool is_source_touch_event_set_non_blocking = > > false; > > Actually the initializer false, isn't needed here... This struct is actually > memset(0) in the WebInputEvent ctor.. So chong you can remove the > is_source_touch_event_set_non_blocking in the ctors... Removed. Confirmed with dtapuska offline and the TODO about removing |memset| won't affect decision here.
The CQ bit was checked by chongz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, dtapuska@chromium.org, pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/2869823003/#ps160001 (title: "dtapuska's comment: Remove |is_source_touch_event_set_non_blocking| from ctor")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chongz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtapuska@chromium.org, sadrul@chromium.org, pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/2869823003/#ps180001 (title: "Remove empty test "stylus-generated-tap.html", added by mistake")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 180001, "attempt_start_ts": 1497643542288960, "parent_rev": "8b52edff46258108f273b22906c9e800e7787024", "commit_rev": "80553a96dc6712a24a7d0a3e3074d5b7f5f65338"}
Message was sent while issue was closed.
Description was changed from ========== [VSync Queue] Plug touch ack to gesture events and flush vsync queue if necessary For a gesture event, we want to know whether the source touch event was handled blocking or non-blocking, and we can flush the vsync queue immediately if it was blocking. This patch will help avoid regression in |first_gesture_scroll_update_latency| in |smoothness.tough_scrolling_cases|. After CL the process would be: 1. Browser -> Renderer: Send touch 2. Renderer -> Browser: Touch Ack 3. Browser: Set |is_source_touch_event_set_non_blocking == true| if the ack was |INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING*| 4. Browser -> Renderer: Send generated gesture events with |is_source_touch_event_set_non_blocking| bit 5. Renderer: Flush vsync queue if |is_source_touch_event_set_non_blocking == true|. BUG=704732 ========== to ========== [VSync Queue] Plug touch ack to gesture events and flush vsync queue if necessary For a gesture event, we want to know whether the source touch event was handled blocking or non-blocking, and we can flush the vsync queue immediately if it was blocking. This patch will help avoid regression in |first_gesture_scroll_update_latency| in |smoothness.tough_scrolling_cases|. After CL the process would be: 1. Browser -> Renderer: Send touch 2. Renderer -> Browser: Touch Ack 3. Browser: Set |is_source_touch_event_set_non_blocking == true| if the ack was |INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING*| 4. Browser -> Renderer: Send generated gesture events with |is_source_touch_event_set_non_blocking| bit 5. Renderer: Flush vsync queue if |is_source_touch_event_set_non_blocking == true|. BUG=704732 Review-Url: https://codereview.chromium.org/2869823003 Cr-Commit-Position: refs/heads/master@{#480202} Committed: https://chromium.googlesource.com/chromium/src/+/80553a96dc6712a24a7d0a3e3074... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001) as https://chromium.googlesource.com/chromium/src/+/80553a96dc6712a24a7d0a3e3074...
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) identified this CL at revision 480202 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
Probably should have initialized |is_source_touch_event_set_non_blocking_| normally rather than relying on some other code to memset() the struct. Are all the users going to memset() the struct? Looks like no because the MSAN bots are unhappy.
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:180001) has been created in https://codereview.chromium.org/2943883002/ by dcheng@chromium.org. The reason for reverting is: Likely causing failures on Linux ChromiumOS MSan Tests..
Message was sent while issue was closed.
Description was changed from ========== [VSync Queue] Plug touch ack to gesture events and flush vsync queue if necessary For a gesture event, we want to know whether the source touch event was handled blocking or non-blocking, and we can flush the vsync queue immediately if it was blocking. This patch will help avoid regression in |first_gesture_scroll_update_latency| in |smoothness.tough_scrolling_cases|. After CL the process would be: 1. Browser -> Renderer: Send touch 2. Renderer -> Browser: Touch Ack 3. Browser: Set |is_source_touch_event_set_non_blocking == true| if the ack was |INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING*| 4. Browser -> Renderer: Send generated gesture events with |is_source_touch_event_set_non_blocking| bit 5. Renderer: Flush vsync queue if |is_source_touch_event_set_non_blocking == true|. BUG=704732 Review-Url: https://codereview.chromium.org/2869823003 Cr-Commit-Position: refs/heads/master@{#480202} Committed: https://chromium.googlesource.com/chromium/src/+/80553a96dc6712a24a7d0a3e3074... ========== to ========== [VSync Queue] Plug touch ack to gesture events and flush vsync queue if necessary For a gesture event, we want to know whether the source touch event was handled blocking or non-blocking, and we can flush the vsync queue immediately if it was blocking. This patch will help avoid regression in |first_gesture_scroll_update_latency| in |smoothness.tough_scrolling_cases|. After CL the process would be: 1. Browser -> Renderer: Send touch 2. Renderer -> Browser: Touch Ack 3. Browser: Set |is_source_touch_event_set_non_blocking == true| if the ack was |INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING*| 4. Browser -> Renderer: Send generated gesture events with |is_source_touch_event_set_non_blocking| bit 5. Renderer: Flush vsync queue if |is_source_touch_event_set_non_blocking == true|. BUG=704732 Review-Url: https://codereview.chromium.org/2869823003 Cr-Commit-Position: refs/heads/master@{#480202} Committed: https://chromium.googlesource.com/chromium/src/+/80553a96dc6712a24a7d0a3e3074... ==========
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dtapuska@ PTAL again, thanks! It turns out that |WebGestureEvent| was initialized correctly, but I was then setting its value from an un-initialized |GestureEventDetail|. https://codereview.chromium.org/2869823003/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebGestureEvent.h (right): https://codereview.chromium.org/2869823003/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebGestureEvent.h:40: bool is_source_touch_event_set_non_blocking; Initialize |is_source_touch_event_set_non_blocking| here Won't fix the issue. https://codereview.chromium.org/2869823003/diff/200001/ui/events/blink/blink_... File ui/events/blink/blink_event_util.cc (right): https://codereview.chromium.org/2869823003/diff/200001/ui/events/blink/blink_... ui/events/blink/blink_event_util.cc:631: details.is_source_touch_event_set_non_blocking(); Remove this code Will fix the issue. https://codereview.chromium.org/2869823003/diff/200001/ui/events/gesture_even... File ui/events/gesture_event_details.h (right): https://codereview.chromium.org/2869823003/diff/200001/ui/events/gesture_even... ui/events/gesture_event_details.h:237: bool is_source_touch_event_set_non_blocking_ = false; |GestureEventDetails.is_source_touch_event_set_non_blocking_ = false| is the actual fix.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/19 20:24:40, chongz wrote: > dtapuska@ PTAL again, thanks! > > It turns out that |WebGestureEvent| was initialized correctly, but I was then > setting its value from an un-initialized |GestureEventDetail|. > > https://codereview.chromium.org/2869823003/diff/200001/third_party/WebKit/pub... > File third_party/WebKit/public/platform/WebGestureEvent.h (right): > > https://codereview.chromium.org/2869823003/diff/200001/third_party/WebKit/pub... > third_party/WebKit/public/platform/WebGestureEvent.h:40: bool > is_source_touch_event_set_non_blocking; > Initialize |is_source_touch_event_set_non_blocking| here Won't fix the issue. > > https://codereview.chromium.org/2869823003/diff/200001/ui/events/blink/blink_... > File ui/events/blink/blink_event_util.cc (right): > > https://codereview.chromium.org/2869823003/diff/200001/ui/events/blink/blink_... > ui/events/blink/blink_event_util.cc:631: > details.is_source_touch_event_set_non_blocking(); > Remove this code Will fix the issue. > > https://codereview.chromium.org/2869823003/diff/200001/ui/events/gesture_even... > File ui/events/gesture_event_details.h (right): > > https://codereview.chromium.org/2869823003/diff/200001/ui/events/gesture_even... > ui/events/gesture_event_details.h:237: bool > is_source_touch_event_set_non_blocking_ = false; > |GestureEventDetails.is_source_touch_event_set_non_blocking_ = false| is the > actual fix. lgtm, you should be able to reland this.
The CQ bit was checked by chongz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/2869823003/#ps200001 (title: "Fix MSAN Use-of-uninitialized-value: Initialize GestureEventDetails")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 200001, "attempt_start_ts": 1498017168325540, "parent_rev": "b8247ff416deaf64d757907aabe6d9931a444fed", "commit_rev": "44f78150b462c4c9daaba7ae47a772fba104e56a"}
Message was sent while issue was closed.
Description was changed from ========== [VSync Queue] Plug touch ack to gesture events and flush vsync queue if necessary For a gesture event, we want to know whether the source touch event was handled blocking or non-blocking, and we can flush the vsync queue immediately if it was blocking. This patch will help avoid regression in |first_gesture_scroll_update_latency| in |smoothness.tough_scrolling_cases|. After CL the process would be: 1. Browser -> Renderer: Send touch 2. Renderer -> Browser: Touch Ack 3. Browser: Set |is_source_touch_event_set_non_blocking == true| if the ack was |INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING*| 4. Browser -> Renderer: Send generated gesture events with |is_source_touch_event_set_non_blocking| bit 5. Renderer: Flush vsync queue if |is_source_touch_event_set_non_blocking == true|. BUG=704732 Review-Url: https://codereview.chromium.org/2869823003 Cr-Commit-Position: refs/heads/master@{#480202} Committed: https://chromium.googlesource.com/chromium/src/+/80553a96dc6712a24a7d0a3e3074... ========== to ========== [VSync Queue] Plug touch ack to gesture events and flush vsync queue if necessary For a gesture event, we want to know whether the source touch event was handled blocking or non-blocking, and we can flush the vsync queue immediately if it was blocking. This patch will help avoid regression in |first_gesture_scroll_update_latency| in |smoothness.tough_scrolling_cases|. After CL the process would be: 1. Browser -> Renderer: Send touch 2. Renderer -> Browser: Touch Ack 3. Browser: Set |is_source_touch_event_set_non_blocking == true| if the ack was |INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING*| 4. Browser -> Renderer: Send generated gesture events with |is_source_touch_event_set_non_blocking| bit 5. Renderer: Flush vsync queue if |is_source_touch_event_set_non_blocking == true|. BUG=704732 Review-Url: https://codereview.chromium.org/2869823003 Cr-Original-Commit-Position: refs/heads/master@{#480202} Committed: https://chromium.googlesource.com/chromium/src/+/80553a96dc6712a24a7d0a3e3074... Review-Url: https://codereview.chromium.org/2869823003 Cr-Commit-Position: refs/heads/master@{#481130} Committed: https://chromium.googlesource.com/chromium/src/+/44f78150b462c4c9daaba7ae47a7... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:200001) as https://chromium.googlesource.com/chromium/src/+/44f78150b462c4c9daaba7ae47a7...
Message was sent while issue was closed.
The builder https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20x64%20Bu... fails with: FAILED: gesture_detection.dll gesture_detection.dll.lib gesture_detection.dll.pdb C:/b/depot_tools/python276_bin/python.exe ../../build/toolchain/win/tool_wrapper.py link-wrapper environment.x64 False link.exe /nologo /IMPLIB:./gesture_detection.dll.lib /DLL /OUT:./gesture_detection.dll /PDB:./gesture_detection.dll.pdb @./gesture_detection.dll.rsp LINK : fatal error LNK1104: cannot open file 'gfx_switches.dll.lib' Suspecting that it is related to this CL. Today's sheriff.
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:200001) has been created in https://codereview.chromium.org/2953563002/ by vabr@chromium.org. The reason for reverting is: Speculative revert. The builder https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20x64%20Bu... fails with: FAILED: gesture_detection.dll gesture_detection.dll.lib gesture_detection.dll.pdb C:/b/depot_tools/python276_bin/python.exe ../../build/toolchain/win/tool_wrapper.py link-wrapper environment.x64 False link.exe /nologo /IMPLIB:./gesture_detection.dll.lib /DLL /OUT:./gesture_detection.dll /PDB:./gesture_detection.dll.pdb @./gesture_detection.dll.rsp LINK : fatal error LNK1104: cannot open file 'gfx_switches.dll.lib' The reverted CL touches code with gestures and gfx. Today's sheriff.. |