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

Issue 1780953003: Change the non-blocking event queue to the main thread event queue. (Closed)

Created:
4 years, 9 months ago by dtapuska
Modified:
4 years, 9 months ago
Reviewers:
Charlie Reis, tdresser
CC:
chromium-reviews, mkwst+moarreviews-renderer_chromium.org, darin-cc_chromium.org, jam, mlamouri+watch-content_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master_wheel_regression_5
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Change the non-blocking event queue to the main thread event queue. A problem with the design in that blocking touch events could get ahead of the non-blocking events was revealed in discussions. Change the queue so that it processes all events going to the main thread. It may elect to put blocking events in queues that it maintains so that events are not re-ordered when dispatched to the main thread. Force the ack behavior in the renderer to match the disposition of the event. The benefit of this is uncancelable touch events now can be coalesced if the main thread is behind. BUG=489802 Committed: https://crrev.com/46616920c2a0d3e15381fff7111143574c60c7d1 Cr-Commit-Position: refs/heads/master@{#381821}

Patch Set 1 #

Total comments: 14

Patch Set 2 : Force ack in renderer to be solely based on the DispatchEventType #

Total comments: 14

Patch Set 3 : Fix up issues from patch set 2 #

Total comments: 5

Patch Set 4 : Rebase and adjust comments as per tdresser's request #

Total comments: 2

Patch Set 5 : Fix formatting nit #

Patch Set 6 : Fix perf test (it only in gn build) #

Patch Set 7 : Fix android #

Unified diffs Side-by-side diffs Delta from patch set Stats (+498 lines, -423 lines) Patch
M content/browser/android/in_process/synchronous_input_event_filter.h View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download
M content/browser/android/in_process/synchronous_input_event_filter.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M content/browser/renderer_host/input/input_router_impl.h View 1 2 chunks +3 lines, -1 line 0 comments Download
M content/browser/renderer_host/input/input_router_impl.cc View 1 2 3 chunks +14 lines, -12 lines 0 comments Download
M content/browser/renderer_host/input/input_router_impl_perftest.cc View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M content/browser/renderer_host/input/input_router_impl_unittest.cc View 1 2 7 chunks +9 lines, -10 lines 0 comments Download
M content/browser/renderer_host/input/touch_event_queue.cc View 1 2 1 chunk +1 line, -2 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura_unittest.cc View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M content/common/input/input_event_dispatch_type.h View 1 1 chunk +11 lines, -3 lines 0 comments Download
M content/common/input/web_input_event_traits.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M content/common/input/web_input_event_traits.cc View 1 2 1 chunk +1 line, -2 lines 0 comments Download
M content/content_renderer.gypi View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M content/content_tests.gypi View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M content/public/test/render_view_test.cc View 1 4 chunks +21 lines, -21 lines 0 comments Download
M content/renderer/android/synchronous_compositor_filter.h View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download
M content/renderer/android/synchronous_compositor_filter.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M content/renderer/input/input_event_filter.h View 1 5 chunks +10 lines, -9 lines 0 comments Download
M content/renderer/input/input_event_filter.cc View 1 5 chunks +19 lines, -24 lines 0 comments Download
M content/renderer/input/input_event_filter_unittest.cc View 1 2 11 chunks +100 lines, -27 lines 0 comments Download
M content/renderer/input/input_handler_manager.h View 1 2 3 2 chunks +4 lines, -5 lines 0 comments Download
M content/renderer/input/input_handler_manager.cc View 1 2 chunks +4 lines, -4 lines 0 comments Download
M content/renderer/input/input_handler_manager_client.h View 1 1 chunk +2 lines, -3 lines 0 comments Download
A content/renderer/input/main_thread_event_queue.h View 1 2 3 1 chunk +121 lines, -0 lines 0 comments Download
A content/renderer/input/main_thread_event_queue.cc View 1 2 3 1 chunk +99 lines, -0 lines 0 comments Download
A + content/renderer/input/main_thread_event_queue_unittest.cc View 1 6 chunks +33 lines, -22 lines 0 comments Download
D content/renderer/input/non_blocking_event_queue.h View 1 chunk +0 lines, -62 lines 0 comments Download
D content/renderer/input/non_blocking_event_queue.cc View 1 chunk +0 lines, -60 lines 0 comments Download
D content/renderer/input/non_blocking_event_queue_unittest.cc View 1 chunk +0 lines, -120 lines 0 comments Download
M content/renderer/input/render_widget_input_handler.cc View 1 2 3 chunks +13 lines, -7 lines 0 comments Download
M content/renderer/input/render_widget_input_handler_delegate.h View 1 1 chunk +4 lines, -3 lines 0 comments Download
M content/renderer/mus/compositor_mus_connection.cc View 1 2 3 1 chunk +1 line, -2 lines 0 comments Download
M content/renderer/mus/compositor_mus_connection_unittest.cc View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M content/renderer/mus/render_widget_mus_connection.h View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M content/renderer/mus/render_widget_mus_connection.cc View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M content/renderer/render_widget.h View 1 chunk +1 line, -1 line 0 comments Download
M content/renderer/render_widget.cc View 1 1 chunk +3 lines, -3 lines 0 comments Download
M content/renderer/render_widget_unittest.cc View 1 2 2 chunks +6 lines, -2 lines 0 comments Download

Messages

Total messages: 36 (16 generated)
dtapuska
PTAL; I wasn't necessarily sold on the NotifyInputEventHandled names; I tried other names like MainThreadHandledInputEvent ...
4 years, 9 months ago (2016-03-10 19:41:30 UTC) #3
tdresser
https://codereview.chromium.org/1780953003/diff/1/content/common/input/input_event_dispatch_type.h File content/common/input/input_event_dispatch_type.h (right): https://codereview.chromium.org/1780953003/diff/1/content/common/input/input_event_dispatch_type.h#newcode10 content/common/input/input_event_dispatch_type.h:10: enum InputEventDispatchType { InputEventMainThreadDispatchType https://codereview.chromium.org/1780953003/diff/1/content/common/input/input_event_dispatch_type.h#newcode11 content/common/input/input_event_dispatch_type.h:11: DISPATCH_TYPE_NORMAL, // Dispatch ...
4 years, 9 months ago (2016-03-10 20:12:05 UTC) #4
tdresser
Change Subject to reflect that this isn't just a straight rename.
4 years, 9 months ago (2016-03-11 15:48:59 UTC) #5
tdresser
Pending comments I never sent: https://codereview.chromium.org/1780953003/diff/1/content/renderer/input/main_thread_event_queue.h File content/renderer/input/main_thread_event_queue.h (right): https://codereview.chromium.org/1780953003/diff/1/content/renderer/input/main_thread_event_queue.h#newcode72 content/renderer/input/main_thread_event_queue.h:72: // Called once compositor ...
4 years, 9 months ago (2016-03-14 14:45:03 UTC) #8
dtapuska
https://codereview.chromium.org/1780953003/diff/1/content/common/input/input_event_dispatch_type.h File content/common/input/input_event_dispatch_type.h (right): https://codereview.chromium.org/1780953003/diff/1/content/common/input/input_event_dispatch_type.h#newcode10 content/common/input/input_event_dispatch_type.h:10: enum InputEventDispatchType { On 2016/03/10 20:12:05, tdresser wrote: > ...
4 years, 9 months ago (2016-03-14 17:33:27 UTC) #10
tdresser
https://codereview.chromium.org/1780953003/diff/1/content/common/input/input_event_dispatch_type.h File content/common/input/input_event_dispatch_type.h (right): https://codereview.chromium.org/1780953003/diff/1/content/common/input/input_event_dispatch_type.h#newcode10 content/common/input/input_event_dispatch_type.h:10: enum InputEventDispatchType { On 2016/03/14 17:33:27, dtapuska wrote: > ...
4 years, 9 months ago (2016-03-15 13:41:22 UTC) #11
dtapuska
https://codereview.chromium.org/1780953003/diff/20001/content/browser/renderer_host/input/input_router_impl.cc File content/browser/renderer_host/input/input_router_impl.cc (right): https://codereview.chromium.org/1780953003/diff/20001/content/browser/renderer_host/input/input_router_impl.cc#newcode370 content/browser/renderer_host/input/input_router_impl.cc:370: !WebInputEventTraits::ShouldBlockEventOnRenderer(input_event); On 2016/03/15 13:41:21, tdresser wrote: > I'd consider ...
4 years, 9 months ago (2016-03-15 19:46:05 UTC) #12
tdresser
LGTM https://codereview.chromium.org/1780953003/diff/20001/content/browser/renderer_host/input/input_router_impl.cc File content/browser/renderer_host/input/input_router_impl.cc (right): https://codereview.chromium.org/1780953003/diff/20001/content/browser/renderer_host/input/input_router_impl.cc#newcode370 content/browser/renderer_host/input/input_router_impl.cc:370: !WebInputEventTraits::ShouldBlockEventOnRenderer(input_event); On 2016/03/15 19:46:04, dtapuska wrote: > On ...
4 years, 9 months ago (2016-03-16 15:34:47 UTC) #13
dtapuska
https://codereview.chromium.org/1780953003/diff/40001/content/renderer/input/main_thread_event_queue.cc File content/renderer/input/main_thread_event_queue.cc (right): https://codereview.chromium.org/1780953003/diff/40001/content/renderer/input/main_thread_event_queue.cc#newcode44 content/renderer/input/main_thread_event_queue.cc:44: // blocking we can avoid having the main thread ...
4 years, 9 months ago (2016-03-17 13:32:36 UTC) #14
dtapuska
On 2016/03/17 13:32:36, dtapuska wrote: > https://codereview.chromium.org/1780953003/diff/40001/content/renderer/input/main_thread_event_queue.cc > File content/renderer/input/main_thread_event_queue.cc (right): > > https://codereview.chromium.org/1780953003/diff/40001/content/renderer/input/main_thread_event_queue.cc#newcode44 > ...
4 years, 9 months ago (2016-03-17 13:36:50 UTC) #16
tdresser
https://codereview.chromium.org/1780953003/diff/40001/content/renderer/input/main_thread_event_queue.h File content/renderer/input/main_thread_event_queue.h (right): https://codereview.chromium.org/1780953003/diff/40001/content/renderer/input/main_thread_event_queue.h#newcode65 content/renderer/input/main_thread_event_queue.h:65: // from the compositor thread. On 2016/03/17 13:32:36, dtapuska ...
4 years, 9 months ago (2016-03-17 14:07:31 UTC) #17
Charlie Reis
Rubber stamp LGTM. https://codereview.chromium.org/1780953003/diff/60001/content/renderer/mus/compositor_mus_connection_unittest.cc File content/renderer/mus/compositor_mus_connection_unittest.cc (right): https://codereview.chromium.org/1780953003/diff/60001/content/renderer/mus/compositor_mus_connection_unittest.cc#newcode129 content/renderer/mus/compositor_mus_connection_unittest.cc:129: blink::WebInputEvent::Type type) override {} nit: Update ...
4 years, 9 months ago (2016-03-17 17:30:37 UTC) #18
dtapuska
https://codereview.chromium.org/1780953003/diff/60001/content/renderer/mus/compositor_mus_connection_unittest.cc File content/renderer/mus/compositor_mus_connection_unittest.cc (right): https://codereview.chromium.org/1780953003/diff/60001/content/renderer/mus/compositor_mus_connection_unittest.cc#newcode129 content/renderer/mus/compositor_mus_connection_unittest.cc:129: blink::WebInputEvent::Type type) override {} On 2016/03/17 17:30:36, Charlie Reis ...
4 years, 9 months ago (2016-03-17 18:09:08 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1780953003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1780953003/80001
4 years, 9 months ago (2016-03-17 18:09:14 UTC) #22
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_compile_dbg_ng/builds/65400)
4 years, 9 months ago (2016-03-17 18:24:25 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1780953003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1780953003/100001
4 years, 9 months ago (2016-03-17 19:24:29 UTC) #27
commit-bot: I haz the power
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/37145)
4 years, 9 months ago (2016-03-17 19:39:48 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1780953003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1780953003/120001
4 years, 9 months ago (2016-03-17 20:44:02 UTC) #32
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years, 9 months ago (2016-03-17 22:52:13 UTC) #34
commit-bot: I haz the power
4 years, 9 months ago (2016-03-17 22:53:50 UTC) #36
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/46616920c2a0d3e15381fff7111143574c60c7d1
Cr-Commit-Position: refs/heads/master@{#381821}

Powered by Google App Engine
This is Rietveld 408576698