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

Issue 1631963002: Plumb firing passive event listeners. (Closed)

Created:
4 years, 10 months ago by dtapuska
Modified:
4 years, 10 months ago
CC:
cc-bugs_chromium.org, chromium-reviews, darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, Rick Byers, tdresser+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master_wheel_passive_listeners_2a
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Plumb firing passive event listeners. Detect whether passive event listeners exist when handling wheel and touch events. If the events can be handled on the compositor thread and an event posted to main thread dispatch that. Add an event queue for wheel and touch so that we can coalesce pending events for the main thread. BUG=489802 Committed: https://crrev.com/0bd451acef1d65d36b09a60ffef3e8a6515434a6 Cr-Commit-Position: refs/heads/master@{#376182}

Patch Set 1 #

Patch Set 2 : Set dependency correctly #

Total comments: 32

Patch Set 3 : Rename to non-blocking and rebase against underlying changes #

Total comments: 18

Patch Set 4 : Add integration test #

Total comments: 6

Patch Set 5 : Attempt to fix windows build #

Patch Set 6 : Fix windows build #

Total comments: 13

Patch Set 7 : Another attempt at satisfying MSVC #

Patch Set 8 : Fix touch move event disposition and nits #

Patch Set 9 : Fix last touch sequence start to be last touch start #

Total comments: 13

Patch Set 10 : Attempt to fix android build #

Patch Set 11 : Fix Tim's comments #

Patch Set 12 : Fix build #

Total comments: 1

Patch Set 13 : Fix build #

Patch Set 14 : Fix mac build #

Total comments: 1

Patch Set 15 : Fix android and mac tests #

Patch Set 16 : Submitted #15 too early #

Patch Set 17 : Disable integration test on MacOS it still needs more work for wheel gesture events #

Total comments: 2

Patch Set 18 : Renamed enum value as per wez@ request #

Total comments: 19

Patch Set 19 : Fix creis's comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1374 lines, -180 lines) Patch
M blimp/client/feature/compositor/blimp_input_handler_wrapper.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/android/in_process/synchronous_input_event_filter.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/android/in_process/synchronous_input_event_filter.cc View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download
M content/browser/renderer_host/input/input_router_impl.cc View 1 2 3 4 5 2 chunks +5 lines, -2 lines 0 comments Download
A content/browser/renderer_host/input/non_blocking_event_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +188 lines, -0 lines 0 comments Download
M content/browser/renderer_host/input/touch_input_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 7 chunks +24 lines, -77 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_mac_unittest.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +3 lines, -1 line 0 comments Download
M content/common/input/input_event_ack_state.h View 1 chunk +2 lines, -1 line 0 comments Download
A content/common/input/input_event_dispatch_type.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +18 lines, -0 lines 0 comments Download
A content/common/input/web_input_event_queue.h View 1 2 3 4 5 6 7 1 chunk +79 lines, -0 lines 0 comments Download
M content/common/input_messages.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +6 lines, -2 lines 0 comments Download
M content/content_common.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +2 lines, -0 lines 0 comments Download
M content/content_renderer.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +2 lines, -0 lines 0 comments Download
M content/content_tests.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +2 lines, -0 lines 0 comments Download
M content/public/test/browser_test_utils.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +32 lines, -1 line 0 comments Download
M content/public/test/browser_test_utils.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 4 chunks +60 lines, -3 lines 0 comments Download
M content/public/test/render_view_test.cc View 1 2 3 4 5 6 7 4 chunks +14 lines, -7 lines 0 comments Download
M content/renderer/android/synchronous_compositor_filter.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M content/renderer/android/synchronous_compositor_filter.cc View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download
M content/renderer/idle_user_detector.h View 1 2 2 chunks +3 lines, -1 line 0 comments Download
M content/renderer/idle_user_detector.cc View 1 2 1 chunk +4 lines, -2 lines 0 comments Download
M content/renderer/input/input_event_filter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 5 chunks +15 lines, -1 line 0 comments Download
M content/renderer/input/input_event_filter.cc View 1 2 3 4 5 6 7 5 chunks +37 lines, -6 lines 0 comments Download
M content/renderer/input/input_event_filter_unittest.cc View 1 2 3 8 chunks +174 lines, -10 lines 0 comments Download
M content/renderer/input/input_handler_manager.h View 1 2 3 2 chunks +14 lines, -6 lines 0 comments Download
M content/renderer/input/input_handler_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 5 chunks +33 lines, -9 lines 0 comments Download
M content/renderer/input/input_handler_manager_client.h View 1 2 3 chunks +4 lines, -4 lines 0 comments Download
M content/renderer/input/input_handler_wrapper.h View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
M content/renderer/input/input_handler_wrapper.cc View 1 2 3 4 2 chunks +4 lines, -1 line 0 comments Download
A content/renderer/input/non_blocking_event_queue.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +62 lines, -0 lines 0 comments Download
A content/renderer/input/non_blocking_event_queue.cc View 1 2 3 4 5 6 7 1 chunk +60 lines, -0 lines 0 comments Download
A content/renderer/input/non_blocking_event_queue_unittest.cc View 1 2 3 1 chunk +120 lines, -0 lines 0 comments Download
M content/renderer/input/render_widget_input_handler.h View 1 2 2 chunks +3 lines, -1 line 0 comments Download
M content/renderer/input/render_widget_input_handler.cc View 1 2 7 chunks +23 lines, -13 lines 0 comments Download
M content/renderer/input/render_widget_input_handler_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +5 lines, -0 lines 0 comments Download
M content/renderer/mus/render_widget_mus_connection.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M content/renderer/mus/render_widget_mus_connection.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +7 lines, -1 line 0 comments Download
M content/renderer/render_view_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +3 lines, -1 line 0 comments Download
M content/renderer/render_widget.h View 1 2 2 chunks +4 lines, -1 line 0 comments Download
M content/renderer/render_widget.cc View 1 2 3 2 chunks +14 lines, -2 lines 0 comments Download
M content/renderer/render_widget_unittest.cc View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M ui/events/blink/input_handler_proxy.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 4 chunks +14 lines, -3 lines 0 comments Download
M ui/events/blink/input_handler_proxy.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 6 chunks +122 lines, -19 lines 0 comments Download
M ui/events/blink/input_handler_proxy_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 20 chunks +193 lines, -3 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 49 (13 generated)
dtapuska
4 years, 10 months ago (2016-01-25 21:03:06 UTC) #4
dtapuska
On 2016/01/25 21:03:06, dtapuska wrote: Be advised; I haven't written unit tests for this yet. ...
4 years, 10 months ago (2016-01-25 21:04:18 UTC) #5
aelias_OOO_until_Jul13
Looks good overall, thanks. https://codereview.chromium.org/1631963002/diff/20001/content/renderer/input/input_event_filter.cc File content/renderer/input/input_event_filter.cc (right): https://codereview.chromium.org/1631963002/diff/20001/content/renderer/input/input_event_filter.cc#newcode74 content/renderer/input/input_event_filter.cc:74: delete iter->second; Can we avoid ...
4 years, 10 months ago (2016-01-26 04:50:41 UTC) #6
tdresser
Looks reasonable at a high level. I'll do a more thorough review once tests are ...
4 years, 10 months ago (2016-01-26 16:34:37 UTC) #7
dtapuska
Will re-post with fixes when done writing unit tests. https://codereview.chromium.org/1631963002/diff/20001/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/1631963002/diff/20001/cc/trees/layer_tree_host_impl.cc#newcode635 cc/trees/layer_tree_host_impl.cc:635: ...
4 years, 10 months ago (2016-01-26 16:53:15 UTC) #8
tdresser
https://codereview.chromium.org/1631963002/diff/20001/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/1631963002/diff/20001/cc/trees/layer_tree_host_impl.cc#newcode635 cc/trees/layer_tree_host_impl.cc:635: uint32_t result = EventListenerProperties::kNone; On 2016/01/26 16:53:15, dtapuska wrote: ...
4 years, 10 months ago (2016-01-26 18:45:42 UTC) #9
dtapuska
https://codereview.chromium.org/1631963002/diff/20001/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/1631963002/diff/20001/cc/trees/layer_tree_host_impl.cc#newcode635 cc/trees/layer_tree_host_impl.cc:635: uint32_t result = EventListenerProperties::kNone; On 2016/01/26 18:45:42, tdresser wrote: ...
4 years, 10 months ago (2016-01-26 18:56:50 UTC) #10
tdresser
https://codereview.chromium.org/1631963002/diff/20001/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/1631963002/diff/20001/cc/trees/layer_tree_host_impl.cc#newcode635 cc/trees/layer_tree_host_impl.cc:635: uint32_t result = EventListenerProperties::kNone; On 2016/01/26 18:56:49, dtapuska wrote: ...
4 years, 10 months ago (2016-01-26 18:58:19 UTC) #11
Rick Byers
High level looks reasonable to me too. https://codereview.chromium.org/1631963002/diff/20001/cc/input/input_handler.h File cc/input/input_handler.h (right): https://codereview.chromium.org/1631963002/diff/20001/cc/input/input_handler.h#newcode178 cc/input/input_handler.h:178: virtual uint32_t ...
4 years, 10 months ago (2016-01-26 19:17:21 UTC) #12
Rick Byers
https://codereview.chromium.org/1631963002/diff/20001/content/renderer/input/passive_event_queue.cc File content/renderer/input/passive_event_queue.cc (right): https://codereview.chromium.org/1631963002/diff/20001/content/renderer/input/passive_event_queue.cc#newcode22 content/renderer/input/passive_event_queue.cc:22: wheel_events_.Queue(MouseWheelEventWithLatencyInfo( On 2016/01/26 19:17:21, Rick Byers wrote: > I ...
4 years, 10 months ago (2016-01-26 19:20:52 UTC) #13
dtapuska
On 2016/01/26 19:20:52, Rick Byers wrote: > https://codereview.chromium.org/1631963002/diff/20001/content/renderer/input/passive_event_queue.cc > File content/renderer/input/passive_event_queue.cc (right): > > https://codereview.chromium.org/1631963002/diff/20001/content/renderer/input/passive_event_queue.cc#newcode22 ...
4 years, 10 months ago (2016-02-04 22:23:13 UTC) #14
tdresser
Looks pretty solid - I'll take another look once the integration test in in place. ...
4 years, 10 months ago (2016-02-08 17:51:32 UTC) #15
dtapuska
PTAL. I am ready for a formal review. https://codereview.chromium.org/1631963002/diff/40001/content/common/input/web_input_event_queue.h File content/common/input/web_input_event_queue.h (right): https://codereview.chromium.org/1631963002/diff/40001/content/common/input/web_input_event_queue.h#newcode21 content/common/input/web_input_event_queue.h:21: // ...
4 years, 10 months ago (2016-02-09 19:40:12 UTC) #16
tdresser
Compile issues look real.
4 years, 10 months ago (2016-02-09 21:41:40 UTC) #17
tdresser
More thoughts coming... https://codereview.chromium.org/1631963002/diff/60001/ui/events/blink/input_handler_proxy.cc File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/1631963002/diff/60001/ui/events/blink/input_handler_proxy.cc#newcode775 ui/events/blink/input_handler_proxy.cc:775: if (input_handler_->DoTouchEventsBlockScrollAt( Does this correctly respect ...
4 years, 10 months ago (2016-02-09 22:00:14 UTC) #18
dtapuska
https://codereview.chromium.org/1631963002/diff/60001/ui/events/blink/input_handler_proxy.cc File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/1631963002/diff/60001/ui/events/blink/input_handler_proxy.cc#newcode775 ui/events/blink/input_handler_proxy.cc:775: if (input_handler_->DoTouchEventsBlockScrollAt( On 2016/02/09 22:00:14, tdresser wrote: > Does ...
4 years, 10 months ago (2016-02-09 22:23:57 UTC) #19
aelias_OOO_until_Jul13
lgtm overall, largely minor comments below. tdresser's observation about touch-start behavior is a substantive issue ...
4 years, 10 months ago (2016-02-10 03:30:24 UTC) #20
tdresser
https://codereview.chromium.org/1631963002/diff/100001/content/browser/renderer_host/input/non_blocking_event_browsertest.cc File content/browser/renderer_host/input/non_blocking_event_browsertest.cc (right): https://codereview.chromium.org/1631963002/diff/100001/content/browser/renderer_host/input/non_blocking_event_browsertest.cc#newcode126 content/browser/renderer_host/input/non_blocking_event_browsertest.cc:126: EXPECT_LT(0, frame_watcher->LastMetaData().root_scroll_offset.y()); On 2016/02/10 03:30:23, aelias wrote: > Why ...
4 years, 10 months ago (2016-02-10 14:09:41 UTC) #21
dtapuska
https://codereview.chromium.org/1631963002/diff/100001/content/browser/renderer_host/input/non_blocking_event_browsertest.cc File content/browser/renderer_host/input/non_blocking_event_browsertest.cc (right): https://codereview.chromium.org/1631963002/diff/100001/content/browser/renderer_host/input/non_blocking_event_browsertest.cc#newcode126 content/browser/renderer_host/input/non_blocking_event_browsertest.cc:126: EXPECT_LT(0, frame_watcher->LastMetaData().root_scroll_offset.y()); On 2016/02/10 14:09:41, tdresser wrote: > On ...
4 years, 10 months ago (2016-02-10 16:46:26 UTC) #22
tdresser
A few minor comments. I'll take one more small look once you've fixed the issues ...
4 years, 10 months ago (2016-02-10 19:37:18 UTC) #23
dtapuska
https://codereview.chromium.org/1631963002/diff/160001/content/public/test/browser_test_utils.h File content/public/test/browser_test_utils.h (right): https://codereview.chromium.org/1631963002/diff/160001/content/public/test/browser_test_utils.h#newcode477 content/public/test/browser_test_utils.h:477: uint32_t Wait(); On 2016/02/10 19:37:17, tdresser wrote: > Perhaps ...
4 years, 10 months ago (2016-02-10 22:05:22 UTC) #24
tdresser
LGTM! https://codereview.chromium.org/1631963002/diff/160001/ui/events/blink/input_handler_proxy.cc File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/1631963002/diff/160001/ui/events/blink/input_handler_proxy.cc#newcode816 ui/events/blink/input_handler_proxy.cc:816: return DID_NOT_HANDLE; On 2016/02/10 22:05:22, dtapuska wrote: > ...
4 years, 10 months ago (2016-02-11 14:24:02 UTC) #25
dtapuska
https://codereview.chromium.org/1631963002/diff/260001/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm (right): https://codereview.chromium.org/1631963002/diff/260001/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm#newcode998 content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:998: data; This is how git cl formatted this for ...
4 years, 10 months ago (2016-02-12 16:19:04 UTC) #27
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1631963002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1631963002/320001
4 years, 10 months ago (2016-02-16 13:43:42 UTC) #31
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/23688)
4 years, 10 months ago (2016-02-16 15:49:39 UTC) #33
dtapuska
On 2016/02/16 15:49:39, commit-bot: I haz the power wrote: > Dry run: Try jobs failed ...
4 years, 10 months ago (2016-02-16 16:23:29 UTC) #35
Wez
https://codereview.chromium.org/1631963002/diff/320001/ui/events/blink/input_handler_proxy.h File ui/events/blink/input_handler_proxy.h (right): https://codereview.chromium.org/1631963002/diff/320001/ui/events/blink/input_handler_proxy.h#newcode56 ui/events/blink/input_handler_proxy.h:56: NON_BLOCKING, Is there a more descriptive name we can ...
4 years, 10 months ago (2016-02-16 21:21:04 UTC) #36
dtapuska
https://codereview.chromium.org/1631963002/diff/320001/ui/events/blink/input_handler_proxy.h File ui/events/blink/input_handler_proxy.h (right): https://codereview.chromium.org/1631963002/diff/320001/ui/events/blink/input_handler_proxy.h#newcode56 ui/events/blink/input_handler_proxy.h:56: NON_BLOCKING, On 2016/02/16 21:21:04, Wez wrote: > Is there ...
4 years, 10 months ago (2016-02-16 21:49:25 UTC) #37
Wez
blimp/ LGTM
4 years, 10 months ago (2016-02-16 22:01:41 UTC) #38
Charlie Reis
This is mostly beyond my domain, so I'm deferring to the input reviewers and just ...
4 years, 10 months ago (2016-02-16 22:25:22 UTC) #39
dtapuska
https://codereview.chromium.org/1631963002/diff/340001/content/browser/renderer_host/input/non_blocking_event_browsertest.cc File content/browser/renderer_host/input/non_blocking_event_browsertest.cc (right): https://codereview.chromium.org/1631963002/diff/340001/content/browser/renderer_host/input/non_blocking_event_browsertest.cc#newcode66 content/browser/renderer_host/input/non_blocking_event_browsertest.cc:66: shell()->web_contents()->GetRenderViewHost()->GetWidget()); On 2016/02/16 22:25:22, Charlie Reis (catching up) wrote: ...
4 years, 10 months ago (2016-02-17 03:34:57 UTC) #40
Charlie Reis
Thanks for the additional comments. content/ LGTM, though I'd suggest chatting with Ken about what ...
4 years, 10 months ago (2016-02-17 06:34:13 UTC) #41
kenrb
ipc lgtm
4 years, 10 months ago (2016-02-18 15:33:12 UTC) #42
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1631963002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1631963002/360001
4 years, 10 months ago (2016-02-18 15:35:07 UTC) #45
commit-bot: I haz the power
Committed patchset #19 (id:360001)
4 years, 10 months ago (2016-02-18 17:08:19 UTC) #47
commit-bot: I haz the power
4 years, 10 months ago (2016-02-18 17:09:31 UTC) #49
Message was sent while issue was closed.
Patchset 19 (id:??) landed as
https://crrev.com/0bd451acef1d65d36b09a60ffef3e8a6515434a6
Cr-Commit-Position: refs/heads/master@{#376182}

Powered by Google App Engine
This is Rietveld 408576698