Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(12)

Issue 2869823003: [VSync Queue] Plug touch ack to gesture events and flush vsync queue if necessary (Closed)

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
Unified diffs Side-by-side diffs Delta from patch set Stats (+145 lines, -52 lines) Patch
M content/browser/BUILD.gn View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
M content/browser/renderer_host/input/touch_emulator.cc View 1 2 3 4 chunks +9 lines, -4 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_android.cc View 1 2 3 4 5 6 7 2 chunks +4 lines, -2 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura.cc View 1 2 3 4 5 6 1 chunk +3 lines, -2 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura_unittest.cc View 1 2 3 4 5 6 1 chunk +8 lines, -4 lines 0 comments Download
M content/browser/renderer_host/ui_events_helper.h View 1 2 3 2 chunks +5 lines, -0 lines 0 comments Download
M content/browser/renderer_host/ui_events_helper.cc View 1 2 3 1 chunk +10 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/exported/WebInputEvent.cpp View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/public/platform/WebGestureEvent.h View 1 2 5 1 chunk +1 line, -0 lines 1 comment Download
M ui/aura/gestures/gesture_recognizer_unittest.cc View 1 2 2 chunks +3 lines, -2 lines 0 comments Download
M ui/aura/window_event_dispatcher.h View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M ui/aura/window_event_dispatcher.cc View 1 2 2 chunks +10 lines, -6 lines 0 comments Download
M ui/aura/window_event_dispatcher_unittest.cc View 1 2 3 4 5 2 chunks +6 lines, -3 lines 0 comments Download
M ui/chromeos/touch_exploration_controller.cc View 1 2 3 1 chunk +3 lines, -2 lines 0 comments Download
M ui/events/blink/blink_event_util.cc View 1 2 3 1 chunk +3 lines, -0 lines 1 comment Download
M ui/events/blink/input_handler_proxy.cc View 1 2 3 4 5 6 7 1 chunk +11 lines, -0 lines 0 comments Download
M ui/events/gesture_detection/filtered_gesture_provider.h View 1 2 1 chunk +3 lines, -1 line 0 comments Download
M ui/events/gesture_detection/filtered_gesture_provider.cc View 1 2 1 chunk +6 lines, -3 lines 0 comments Download
M ui/events/gesture_detection/gesture_event_data_packet.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M ui/events/gesture_detection/gesture_event_data_packet.cc View 1 2 3 1 chunk +6 lines, -1 line 0 comments Download
M ui/events/gesture_detection/touch_disposition_gesture_filter.h View 1 2 1 chunk +3 lines, -1 line 0 comments Download
M ui/events/gesture_detection/touch_disposition_gesture_filter.cc View 1 2 2 chunks +5 lines, -3 lines 0 comments Download
M ui/events/gesture_detection/touch_disposition_gesture_filter_unittest.cc View 1 2 1 chunk +13 lines, -6 lines 0 comments Download
M ui/events/gesture_event_details.h View 1 2 3 4 5 6 7 2 chunks +11 lines, -0 lines 1 comment Download
M ui/events/gestures/gesture_provider_aura.h View 1 2 1 chunk +3 lines, -1 line 0 comments Download
M ui/events/gestures/gesture_provider_aura.cc View 1 2 2 chunks +9 lines, -5 lines 0 comments Download
M ui/events/gestures/gesture_recognizer.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M ui/events/gestures/gesture_recognizer_impl.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M ui/events/gestures/gesture_recognizer_impl.cc View 1 2 2 chunks +3 lines, -1 line 0 comments Download
M ui/events/gestures/gesture_recognizer_impl_mac.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 101 (62 generated)
chongz
dtapuska@ PTAL, thanks!
3 years, 7 months ago (2017-05-09 14:40:34 UTC) #11
chongz
On 2017/05/09 14:40:34, chongz wrote: > dtapuska@ PTAL, thanks! dtapuska@ Can you take a look ...
3 years, 7 months ago (2017-05-10 14:36:44 UTC) #12
dtapuska
https://codereview.chromium.org/2869823003/diff/1/content/browser/renderer_host/input/input_router_impl.cc File content/browser/renderer_host/input/input_router_impl.cc (right): https://codereview.chromium.org/2869823003/diff/1/content/browser/renderer_host/input/input_router_impl.cc#newcode192 content/browser/renderer_host/input/input_router_impl.cc:192: touch_event_queue_->SendTouchEventsAsync() I don't think we really should query the ...
3 years, 7 months ago (2017-05-10 14:49:02 UTC) #13
chongz
dtapuska@ I've updated CL to use the ack. PTAL again, thanks! https://codereview.chromium.org/2869823003/diff/1/content/browser/renderer_host/input/input_router_impl.cc File content/browser/renderer_host/input/input_router_impl.cc (right): ...
3 years, 7 months ago (2017-05-24 13:47:09 UTC) #35
dtapuska
https://codereview.chromium.org/2869823003/diff/60001/content/browser/renderer_host/render_widget_host_view_android.cc File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2869823003/diff/60001/content/browser/renderer_host/render_widget_host_view_android.cc#newcode1668 content/browser/renderer_host/render_widget_host_view_android.cc:1668: const ui::EventResult event_result = Why is this different than ...
3 years, 7 months ago (2017-05-25 15:40:12 UTC) #36
chongz
sadrul@ Thanks for the help! Can you take another look at the additional question below? ...
3 years, 7 months ago (2017-05-25 20:25:29 UTC) #39
chongz
sadrul@ I'm still concerned about the out-of-order event sequence... I've made |set_non_blocking| a separate parameter ...
3 years, 6 months ago (2017-05-31 17:57:50 UTC) #49
sadrul
https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_handler_proxy.cc File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_handler_proxy.cc#newcode387 ui/events/blink/input_handler_proxy.cc:387: HandleInputEvent(event_with_callback->event()); On 2017/05/25 20:25:28, chongz wrote: > sadrul@ Just ...
3 years, 6 months ago (2017-06-01 19:57:18 UTC) #50
dtapuska
On 2017/06/01 19:57:18, sadrul wrote: > https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_handler_proxy.cc > File ui/events/blink/input_handler_proxy.cc (right): > > https://codereview.chromium.org/2869823003/diff/60001/ui/events/blink/input_handler_proxy.cc#newcode387 > ...
3 years, 6 months ago (2017-06-05 18:11:42 UTC) #51
sadrul
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_handler_proxy.cc ...
3 years, 6 months ago (2017-06-05 20:48:20 UTC) #52
sadrul
OK, I looked at the CL, and it does not actually look too bad. Would ...
3 years, 6 months ago (2017-06-05 20:51:08 UTC) #53
sadrul
On 2017/06/05 20:51:08, sadrul wrote: > OK, I looked at the CL, and it does ...
3 years, 6 months ago (2017-06-05 20:51:53 UTC) #54
chongz
On 2017/06/05 20:48:20, sadrul wrote: > On 2017/06/05 18:11:42, dtapuska wrote: > > On 2017/06/01 ...
3 years, 6 months ago (2017-06-05 21:33:49 UTC) #57
sadrul
> I'm not sure if I fully understand your suggestion: Do you prefer a table ...
3 years, 6 months ago (2017-06-05 21:37:07 UTC) #58
chongz
On 2017/06/05 21:37:07, sadrul wrote: > > I'm not sure if I fully understand your ...
3 years, 6 months ago (2017-06-07 17:41:59 UTC) #59
sadrul
On 2017/06/07 17:41:59, chongz wrote: > On 2017/06/05 21:37:07, sadrul wrote: > > > I'm ...
3 years, 6 months ago (2017-06-08 20:31:38 UTC) #60
sadrul
lgtm https://codereview.chromium.org/2869823003/diff/100001/content/browser/renderer_host/ui_events_helper.h File content/browser/renderer_host/ui_events_helper.h (right): https://codereview.chromium.org/2869823003/diff/100001/content/browser/renderer_host/ui_events_helper.h#newcode41 content/browser/renderer_host/ui_events_helper.h:41: bool InputEventAckStateIsSetNonBlocking(InputEventAckState); Document. https://codereview.chromium.org/2869823003/diff/100001/ui/events/gesture_detection/gesture_event_data_packet.cc File ui/events/gesture_detection/gesture_event_data_packet.cc (right): https://codereview.chromium.org/2869823003/diff/100001/ui/events/gesture_detection/gesture_event_data_packet.cc#newcode118 ...
3 years, 6 months ago (2017-06-08 20:31:48 UTC) #61
chongz
dtapuska@ PTAL again, thanks! https://codereview.chromium.org/2869823003/diff/100001/content/browser/renderer_host/ui_events_helper.h File content/browser/renderer_host/ui_events_helper.h (right): https://codereview.chromium.org/2869823003/diff/100001/content/browser/renderer_host/ui_events_helper.h#newcode41 content/browser/renderer_host/ui_events_helper.h:41: bool InputEventAckStateIsSetNonBlocking(InputEventAckState); On 2017/06/08 20:31:48, ...
3 years, 6 months ago (2017-06-09 14:51:44 UTC) #62
dtapuska
On 2017/06/09 14:51:44, chongz wrote: > dtapuska@ PTAL again, thanks! > > https://codereview.chromium.org/2869823003/diff/100001/content/browser/renderer_host/ui_events_helper.h > File ...
3 years, 6 months ago (2017-06-09 15:07:40 UTC) #63
chongz
pfeldman@ PTAL at "content/browser/BUILD.gn" and 2 "third_party/WebKit/" files, thanks!
3 years, 6 months ago (2017-06-09 15:27:43 UTC) #65
pfeldman
https://codereview.chromium.org/2869823003/diff/120001/third_party/WebKit/public/platform/WebGestureEvent.h File third_party/WebKit/public/platform/WebGestureEvent.h (right): https://codereview.chromium.org/2869823003/diff/120001/third_party/WebKit/public/platform/WebGestureEvent.h#newcode40 third_party/WebKit/public/platform/WebGestureEvent.h:40: bool is_source_touch_event_set_non_blocking; Are you relying upon all the call ...
3 years, 6 months ago (2017-06-09 18:38:08 UTC) #66
chongz
pfeldman@ I've updated as per your comments, PTAL again, thanks! https://codereview.chromium.org/2869823003/diff/120001/third_party/WebKit/public/platform/WebGestureEvent.h File third_party/WebKit/public/platform/WebGestureEvent.h (right): https://codereview.chromium.org/2869823003/diff/120001/third_party/WebKit/public/platform/WebGestureEvent.h#newcode40 ...
3 years, 6 months ago (2017-06-09 18:58:58 UTC) #67
chongz
On 2017/06/09 18:58:58, chongz wrote: > pfeldman@ I've updated as per your comments, PTAL again, ...
3 years, 6 months ago (2017-06-14 14:06:08 UTC) #68
pfeldman
https://codereview.chromium.org/2869823003/diff/140001/third_party/WebKit/public/platform/WebGestureEvent.h File third_party/WebKit/public/platform/WebGestureEvent.h (right): https://codereview.chromium.org/2869823003/diff/140001/third_party/WebKit/public/platform/WebGestureEvent.h#newcode40 third_party/WebKit/public/platform/WebGestureEvent.h:40: bool is_source_touch_event_set_non_blocking; lgtm. btw, in c++11 you can do ...
3 years, 6 months ago (2017-06-15 18:56:42 UTC) #69
dtapuska
https://codereview.chromium.org/2869823003/diff/140001/third_party/WebKit/public/platform/WebGestureEvent.h File third_party/WebKit/public/platform/WebGestureEvent.h (right): https://codereview.chromium.org/2869823003/diff/140001/third_party/WebKit/public/platform/WebGestureEvent.h#newcode40 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. ...
3 years, 6 months ago (2017-06-15 19:18:30 UTC) #70
chongz
https://codereview.chromium.org/2869823003/diff/140001/third_party/WebKit/public/platform/WebGestureEvent.h File third_party/WebKit/public/platform/WebGestureEvent.h (right): https://codereview.chromium.org/2869823003/diff/140001/third_party/WebKit/public/platform/WebGestureEvent.h#newcode40 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 ...
3 years, 6 months ago (2017-06-15 19:37:37 UTC) #71
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2869823003/160001
3 years, 6 months ago (2017-06-16 16:04:20 UTC) #74
commit-bot: I haz the power
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_rel_ng/builds/480973)
3 years, 6 months ago (2017-06-16 17:39:44 UTC) #76
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2869823003/180001
3 years, 6 months ago (2017-06-16 20:06:02 UTC) #79
commit-bot: I haz the power
Committed patchset #7 (id:180001) as https://chromium.googlesource.com/chromium/src/+/80553a96dc6712a24a7d0a3e3074d5b7f5f65338
3 years, 6 months ago (2017-06-16 22:37:56 UTC) #82
findit-for-me
Findit (https://goo.gl/kROfz5) identified this CL at revision 480202 as the culprit for failures in the ...
3 years, 6 months ago (2017-06-17 04:07:22 UTC) #83
Lei Zhang
Probably should have initialized |is_source_touch_event_set_non_blocking_| normally rather than relying on some other code to memset() ...
3 years, 6 months ago (2017-06-17 06:00:46 UTC) #85
dcheng
A revert of this CL (patchset #7 id:180001) has been created in https://codereview.chromium.org/2943883002/ by dcheng@chromium.org. ...
3 years, 6 months ago (2017-06-17 07:36:41 UTC) #86
chongz
dtapuska@ PTAL again, thanks! It turns out that |WebGestureEvent| was initialized correctly, but I was ...
3 years, 6 months ago (2017-06-19 20:24:40 UTC) #90
dtapuska
On 2017/06/19 20:24:40, chongz wrote: > dtapuska@ PTAL again, thanks! > > It turns out ...
3 years, 6 months ago (2017-06-20 21:04:11 UTC) #93
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2869823003/200001
3 years, 6 months ago (2017-06-21 03:52:57 UTC) #96
commit-bot: I haz the power
Committed patchset #8 (id:200001) as https://chromium.googlesource.com/chromium/src/+/44f78150b462c4c9daaba7ae47a772fba104e56a
3 years, 6 months ago (2017-06-21 07:04:59 UTC) #99
vabr (Chromium)
The builder https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20x64%20Builder%20%28dbg%29/builds/110096 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 ...
3 years, 6 months ago (2017-06-21 07:40:48 UTC) #100
vabr (Chromium)
3 years, 6 months ago (2017-06-21 07:43:31 UTC) #101
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..

Powered by Google App Engine
This is Rietveld 408576698