Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2928793003:
Re-target wheel events only when a new scroll sequence has started. (Closed)
DescriptionRe-target wheel events only when a new scroll sequence has started.
In RouteMouseWheelEvent, when a wheel event with phase began arrives,
find the target for wheel event. While no wheel with phase end has
arrived (during a scroll sequence) send the wheel events to the saved
target.
In EventHandler, do the same targetting to dispatch the events to js.
BUG=526463
TEST=WheelScrollLatchingBrowserTest.WheelEventTarget,
WheelScrollLatchingDisabledBrowserTest.WheelEventTarget
Review-Url: https://codereview.chromium.org/2928793003
Cr-Commit-Position: refs/heads/master@{#480052}
Committed: https://chromium.googlesource.com/chromium/src/+/8aa8c38005ee36bc441138c81a5cf165fb6514a5
Patch Set 1 #Patch Set 2 : browser test updated. #Patch Set 3 : browser test updated. #
Total comments: 12
Patch Set 4 : MouseWheelEventManager handles wheel events. #Patch Set 5 : Landed migration of wheel handling to MouseWheelEventManager separately. #
Total comments: 18
Patch Set 6 : review comments addressed. #Patch Set 7 : review comments addressed. #Messages
Total messages: 62 (50 generated)
The CQ bit was checked by sahel@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by sahel@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sahel@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: This issue passed the CQ dry run.
The CQ bit was checked by sahel@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by sahel@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: This issue passed the CQ dry run.
sahel@chromium.org changed reviewers: + bokan@chromium.org, tdresser@chromium.org
bokan@chromium.org: Please review changes in tdresser@chromium.org: Please review changes in
https://codereview.chromium.org/2928793003/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/wheel_scroll_latching_browsertest.cc (right): https://codereview.chromium.org/2928793003/diff/60001/content/browser/rendere... content/browser/renderer_host/input/wheel_scroll_latching_browsertest.cc:35: "body { height: 10000px;" This brace style is atypical - we'd normally put the text after the { on a new line, wouldn't we? https://codereview.chromium.org/2928793003/diff/60001/content/browser/rendere... content/browser/renderer_host/input/wheel_scroll_latching_browsertest.cc:161: // Runs until we get the InputMsgAck callback Trailing . https://codereview.chromium.org/2928793003/diff/60001/content/browser/rendere... content/browser/renderer_host/input/wheel_scroll_latching_browsertest.cc:178: // Runs until we get the InputMsgAck callback Trailing . https://codereview.chromium.org/2928793003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2928793003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.cc:324: wheel_target_.target = nullptr; This isn't needed, is it? https://codereview.chromium.org/2928793003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandler.h (right): https://codereview.chromium.org/2928793003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.h:369: Member<Node> wheel_target_; Should we move wheel logic out to a separate WheelEventManager, like we did for touch? There's a fair bit of wheel specific logic here.
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by sahel@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
The CQ bit was checked by sahel@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2928793003/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/wheel_scroll_latching_browsertest.cc (right): https://codereview.chromium.org/2928793003/diff/60001/content/browser/rendere... content/browser/renderer_host/input/wheel_scroll_latching_browsertest.cc:35: "body { height: 10000px;" On 2017/06/09 18:13:11, tdresser wrote: > This brace style is atypical - we'd normally put the text after the { on a new > line, wouldn't we? Done. https://codereview.chromium.org/2928793003/diff/60001/content/browser/rendere... content/browser/renderer_host/input/wheel_scroll_latching_browsertest.cc:161: // Runs until we get the InputMsgAck callback On 2017/06/09 18:13:11, tdresser wrote: > Trailing . Done. https://codereview.chromium.org/2928793003/diff/60001/content/browser/rendere... content/browser/renderer_host/input/wheel_scroll_latching_browsertest.cc:178: // Runs until we get the InputMsgAck callback On 2017/06/09 18:13:11, tdresser wrote: > Trailing . Done. https://codereview.chromium.org/2928793003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2928793003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.cc:324: wheel_target_.target = nullptr; On 2017/06/09 18:13:11, tdresser wrote: > This isn't needed, is it? By resetting the target on end events we won't send the rest of the wheel events to a stale target if the event with phaseBegan gets missed for any reason. I agree that it shouldn't happen normally, so if you think it is not needed I can get rid of it. https://codereview.chromium.org/2928793003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandler.h (right): https://codereview.chromium.org/2928793003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.h:369: Member<Node> wheel_target_; On 2017/06/09 18:13:11, tdresser wrote: > Should we move wheel logic out to a separate WheelEventManager, like we did for > touch? > > There's a fair bit of wheel specific logic here. Done.
Thanks! Sorry, I should have thought of this when asking you to split out the wheel event manager - any chance you could land this as two patches, one which splits out the wheel event manager, and then another which does the actual change? Sorry for not requesting this up front... https://codereview.chromium.org/2928793003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2928793003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.cc:324: wheel_target_.target = nullptr; On 2017/06/13 15:43:20, sahel wrote: > On 2017/06/09 18:13:11, tdresser wrote: > > This isn't needed, is it? > > By resetting the target on end events we won't send the rest of the wheel events > to a stale target if the event with phaseBegan gets missed for any reason. I > agree that it shouldn't happen normally, so if you think it is not needed I can > get rid of it. Maybe leave it, and add a DCHECK that the target isn't null for wheel events other than phase begin?
On 2017/06/13 15:48:51, tdresser wrote: > Thanks! > > Sorry, I should have thought of this when asking you to split out the wheel > event manager - any chance you could land this as two patches, one which splits > out the wheel event manager, and then another which does the actual change? > > Sorry for not requesting this up front... > > https://codereview.chromium.org/2928793003/diff/60001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_input_event_router.cc > (right): > > https://codereview.chromium.org/2928793003/diff/60001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_input_event_router.cc:324: > wheel_target_.target = nullptr; > On 2017/06/13 15:43:20, sahel wrote: > > On 2017/06/09 18:13:11, tdresser wrote: > > > This isn't needed, is it? > > > > By resetting the target on end events we won't send the rest of the wheel > events > > to a stale target if the event with phaseBegan gets missed for any reason. I > > agree that it shouldn't happen normally, so if you think it is not needed I > can > > get rid of it. > > Maybe leave it, and add a DCHECK that the target isn't null for wheel events > other than phase begin? The MouseWheelEventManager has been added in this cl: https://codereview.chromium.org/2936023002/
https://codereview.chromium.org/2928793003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2928793003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_input_event_router.cc:324: wheel_target_.target = nullptr; On 2017/06/13 15:48:50, tdresser wrote: > On 2017/06/13 15:43:20, sahel wrote: > > On 2017/06/09 18:13:11, tdresser wrote: > > > This isn't needed, is it? > > > > By resetting the target on end events we won't send the rest of the wheel > events > > to a stale target if the event with phaseBegan gets missed for any reason. I > > agree that it shouldn't happen normally, so if you think it is not needed I > can > > get rid of it. > > Maybe leave it, and add a DCHECK that the target isn't null for wheel events > other than phase begin? We agreed to leave it as it is since sometimes FindEventTarget fails to find any targets.
The CQ bit was checked by sahel@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: This issue passed the CQ dry run.
LGTM with nits. https://codereview.chromium.org/2928793003/diff/180001/content/browser/render... File content/browser/renderer_host/input/wheel_scroll_latching_browsertest.cc (right): https://codereview.chromium.org/2928793003/diff/180001/content/browser/render... content/browser/renderer_host/input/wheel_scroll_latching_browsertest.cc:160: wheel_event.phase = blink::WebMouseWheelEvent::kPhaseBegan; Couldn't you set this phase information in both cases? https://codereview.chromium.org/2928793003/diff/180001/content/browser/render... content/browser/renderer_host/input/wheel_scroll_latching_browsertest.cc:169: -delta_y) I prefer using {} when the condition is multi-line. https://codereview.chromium.org/2928793003/diff/180001/content/browser/render... content/browser/renderer_host/input/wheel_scroll_latching_browsertest.cc:187: -2 * delta_y) {}
The CQ bit was checked by sahel@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...
Some cleanup suggestions but nothing significant. lgtm % comments https://codereview.chromium.org/2928793003/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2928793003/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_input_event_router.cc:288: if (root_view->wheel_scroll_latching_enabled()) { Nit: make this an else if in the outer block https://codereview.chromium.org/2928793003/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_input_event_router.cc:321: if (root_view->wheel_scroll_latching_enabled() && Nit: doesn't hurt to clear this even if latching is off, right? I'd just add a DCHECK(wheel_scroll_latching_enabled || !wheel_target_.target) and remove the enabled check. https://codereview.chromium.org/2928793003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2928793003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.cpp:953: Nit: remove newline https://codereview.chromium.org/2928793003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/MouseWheelEventManager.cpp (right): https://codereview.chromium.org/2928793003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/MouseWheelEventManager.cpp:41: WebMouseWheelEvent::kPhaseMayBegin; Do we really need to clear the target on MayBegin? If we get one it should always be after a PhaseEnded or PhaseCancelled and before a Begin, right? In that case, I'd rather we DCHECK on MayBegin that wheel_target_ == nullptr. https://codereview.chromium.org/2928793003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/MouseWheelEventManager.cpp:62: return WebInputEventResult::kNotHandled; Add the early out at the top of the method if there's no Document, FrameView, or LayoutView. https://codereview.chromium.org/2928793003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/MouseWheelEventManager.cpp:105: LayoutPoint v_point = Everything from here to end of the block is identical between the two branches. Please factor it out into a private helper method and use it in both branches.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sahel@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2928793003/diff/180001/content/browser/render... File content/browser/renderer_host/input/wheel_scroll_latching_browsertest.cc (right): https://codereview.chromium.org/2928793003/diff/180001/content/browser/render... content/browser/renderer_host/input/wheel_scroll_latching_browsertest.cc:160: wheel_event.phase = blink::WebMouseWheelEvent::kPhaseBegan; On 2017/06/15 13:40:01, tdresser wrote: > Couldn't you set this phase information in both cases? Done. https://codereview.chromium.org/2928793003/diff/180001/content/browser/render... content/browser/renderer_host/input/wheel_scroll_latching_browsertest.cc:169: -delta_y) On 2017/06/15 13:40:01, tdresser wrote: > I prefer using {} when the condition is multi-line. Done. https://codereview.chromium.org/2928793003/diff/180001/content/browser/render... content/browser/renderer_host/input/wheel_scroll_latching_browsertest.cc:187: -2 * delta_y) On 2017/06/15 13:40:01, tdresser wrote: > {} Done. https://codereview.chromium.org/2928793003/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/2928793003/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_input_event_router.cc:288: if (root_view->wheel_scroll_latching_enabled()) { On 2017/06/15 15:47:17, bokan wrote: > Nit: make this an else if in the outer block Done. https://codereview.chromium.org/2928793003/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_input_event_router.cc:321: if (root_view->wheel_scroll_latching_enabled() && On 2017/06/15 15:47:17, bokan wrote: > Nit: doesn't hurt to clear this even if latching is off, right? I'd just add a > DCHECK(wheel_scroll_latching_enabled || !wheel_target_.target) and remove the > enabled check. Done. https://codereview.chromium.org/2928793003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2928793003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.cpp:953: On 2017/06/15 15:47:17, bokan wrote: > Nit: remove newline Done. https://codereview.chromium.org/2928793003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/MouseWheelEventManager.cpp (right): https://codereview.chromium.org/2928793003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/MouseWheelEventManager.cpp:41: WebMouseWheelEvent::kPhaseMayBegin; On 2017/06/15 15:47:18, bokan wrote: > Do we really need to clear the target on MayBegin? If we get one it should > always be after a PhaseEnded or PhaseCancelled and before a Begin, right? In > that case, I'd rather we DCHECK on MayBegin that wheel_target_ == nullptr. Done. https://codereview.chromium.org/2928793003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/MouseWheelEventManager.cpp:62: return WebInputEventResult::kNotHandled; On 2017/06/15 15:47:18, bokan wrote: > Add the early out at the top of the method if there's no Document, FrameView, or > LayoutView. Done. https://codereview.chromium.org/2928793003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/MouseWheelEventManager.cpp:105: LayoutPoint v_point = On 2017/06/15 15:47:17, bokan wrote: > Everything from here to end of the block is identical between the two branches. > Please factor it out into a private helper method and use it in both branches. Done.
The CQ bit was checked by sahel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, bokan@chromium.org Link to the patchset: https://codereview.chromium.org/2928793003/#ps220001 (title: "review comments addressed.")
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": 220001, "attempt_start_ts": 1497626808214790,
"parent_rev": "0c99ff881c2da77c5afb5baaab2ef22c9fde8c1b", "commit_rev":
"8aa8c38005ee36bc441138c81a5cf165fb6514a5"}
Message was sent while issue was closed.
Description was changed from
==========
Re-target wheel events only when a new scroll sequence has started.
In RouteMouseWheelEvent, when a wheel event with phase began arrives,
find the target for wheel event. While no wheel with phase end has
arrived (during a scroll sequence) send the wheel events to the saved
target.
In EventHandler, do the same targetting to dispatch the events to js.
BUG=526463
TEST=WheelScrollLatchingBrowserTest.WheelEventTarget,
WheelScrollLatchingDisabledBrowserTest.WheelEventTarget
==========
to
==========
Re-target wheel events only when a new scroll sequence has started.
In RouteMouseWheelEvent, when a wheel event with phase began arrives,
find the target for wheel event. While no wheel with phase end has
arrived (during a scroll sequence) send the wheel events to the saved
target.
In EventHandler, do the same targetting to dispatch the events to js.
BUG=526463
TEST=WheelScrollLatchingBrowserTest.WheelEventTarget,
WheelScrollLatchingDisabledBrowserTest.WheelEventTarget
Review-Url: https://codereview.chromium.org/2928793003
Cr-Commit-Position: refs/heads/master@{#480052}
Committed:
https://chromium.googlesource.com/chromium/src/+/8aa8c38005ee36bc441138c81a5c...
==========
Message was sent while issue was closed.
Committed patchset #7 (id:220001) as https://chromium.googlesource.com/chromium/src/+/8aa8c38005ee36bc441138c81a5c...
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:220001) has been created in https://codereview.chromium.org/2943183002/ by nyquist@chromium.org. The reason for reverting is: Flaky on Android Tests buildbot. First breaking build: https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test.... |
