Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2902303002:
phase based wheel scroll latching for mac (Closed)
|
Created:
3 years, 7 months ago by sahel Modified:
3 years, 6 months ago Reviewers:
tdresser CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, nona+watch_chromium.org, darin-cc_chromium.org, mac-reviews_chromium.org, James Su Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionphase based wheel scroll latching for mac
This cl is the mac version of the following cl:
https://codereview.chromium.org/2882443002/
When a mouse wheel event with phase ended arrives, instead of
dispatching it, a timer gets started. While the timer is running if a
wheel event with momentumPhase began arrives, the timer stops and no
wheel event with phase ended is sent. This is for latching the momentum
(fling) part of the scrolling to the same target that has handled
scrolling phase. If the timer expires, the pending wheel event with
phase ended is sent to indicate end of the current scrolling sequence
with a momentum phase happening.
It is possible that another wheel event with phase began arrives
when the timer is running. In this case the pending wheel event will
be sent immediately to show the end of the previous scroll sequence
before dispatching the event with phase began.
BUG=526463
TEST=RenderWidgetHostViewMacWithWheelScrollLatchingEnabledTest.
WheelWithPhaseEndedIsNotForwardedImmediately,
RenderWidgetHostViewMacWithWheelScrollLatchingEnabledTest.
WheelWithMomentumPhaseBeganStopsTheWheelEndDispatchTimer,
RenderWidgetHostViewMacWithWheelScrollLatchingEnabledTest.
WheelWithPhaseBeganDispatchesThePendingWheelEnd
Review-Url: https://codereview.chromium.org/2902303002
Cr-Commit-Position: refs/heads/master@{#475346}
Committed: https://chromium.googlesource.com/chromium/src/+/a71587c587597d64a7cd97d65826692f699bd881
Patch Set 1 #Patch Set 2 : in -> after #
Total comments: 30
Patch Set 3 : comments addressed #
Total comments: 2
Patch Set 4 : review comments addressed. #
Messages
Total messages: 30 (22 generated)
The CQ bit was checked by sahel@google.com 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.
Description was changed from
==========
phase based wheel scroll latching for mac
This cl is the mac version of the .
When a mouse wheel event with phase ended arrives, instead of
dispatching it, a timer gets started. While the timer is running if a
wheel event with momentumPhase began arrives, the timer stops and no
wheel event with phase ended is sent. This is for latching the momentum
(fling) part of the scrolling to the same target that has handled
scrolling phase. If the timer expires, the pending wheel event with
phase ended is sent to indicate end of the current scrolling sequence
with a momentum phase happening.
It is possible that another wheel event with phase began arrives
when the timer is running. In this case the pending wheel event will
be sent immediately to show the end of the previous scroll sequence
before dispatching the event with phase began.
BUG=526463
TEST=RenderWidgetHostViewMacWithWheelScrollLatchingEnabledTest.
WheelWithPhaseEndedIsNotForwardedImmediately,
RenderWidgetHostViewMacWithWheelScrollLatchingEnabledTest.
WheelWithMomentumPhaseBeganStopsTheWheelEndDispatchTimer,
RenderWidgetHostViewMacWithWheelScrollLatchingEnabledTest.
WheelWithPhaseBeganDispatchesThePendingWheelEnd
==========
to
==========
phase based wheel scroll latching for mac
This cl is the mac version of the following cl:
https://codereview.chromium.org/2882443002/
When a mouse wheel event with phase ended arrives, instead of
dispatching it, a timer gets started. While the timer is running if a
wheel event with momentumPhase began arrives, the timer stops and no
wheel event with phase ended is sent. This is for latching the momentum
(fling) part of the scrolling to the same target that has handled
scrolling phase. If the timer expires, the pending wheel event with
phase ended is sent to indicate end of the current scrolling sequence
with a momentum phase happening.
It is possible that another wheel event with phase began arrives
when the timer is running. In this case the pending wheel event will
be sent immediately to show the end of the previous scroll sequence
before dispatching the event with phase began.
BUG=526463
TEST=RenderWidgetHostViewMacWithWheelScrollLatchingEnabledTest.
WheelWithPhaseEndedIsNotForwardedImmediately,
RenderWidgetHostViewMacWithWheelScrollLatchingEnabledTest.
WheelWithMomentumPhaseBeganStopsTheWheelEndDispatchTimer,
RenderWidgetHostViewMacWithWheelScrollLatchingEnabledTest.
WheelWithPhaseBeganDispatchesThePendingWheelEnd
==========
sahel@chromium.org changed reviewers: - sahel@google.com
Description was changed from ========== phase based wheel scroll latching for mac This cl is the mac version of the following cl: https://codereview.chromium.org/2882443002/ When a mouse wheel event with phase ended arrives, instead of dispatching it, a timer gets started. While the timer is running if a wheel event with momentumPhase began arrives, the timer stops and no wheel event with phase ended is sent. This is for latching the momentum (fling) part of the scrolling to the same target that has handled scrolling phase. If the timer expires, the pending wheel event with phase ended is sent to indicate end of the current scrolling sequence with a momentum phase happening. It is possible that another wheel event with phase began arrives when the timer is running. In this case the pending wheel event will be sent immediately to show the end of the previous scroll sequence before dispatching the event with phase began. BUG=526463 TEST=RenderWidgetHostViewMacWithWheelScrollLatchingEnabledTest. WheelWithPhaseEndedIsNotForwardedImmediately, RenderWidgetHostViewMacWithWheelScrollLatchingEnabledTest. WheelWithMomentumPhaseBeganStopsTheWheelEndDispatchTimer, RenderWidgetHostViewMacWithWheelScrollLatchingEnabledTest. WheelWithPhaseBeganDispatchesThePendingWheelEnd ========== to ========== phase based wheel scroll latching for mac This cl is the mac version of the following cl: https://codereview.chromium.org/2882443002/ When a mouse wheel event with phase ended arrives, instead of dispatching it, a timer gets started. While the timer is running if a wheel event with momentumPhase began arrives, the timer stops and no wheel event with phase ended is sent. This is for latching the momentum (fling) part of the scrolling to the same target that has handled scrolling phase. If the timer expires, the pending wheel event with phase ended is sent to indicate end of the current scrolling sequence with a momentum phase happening. It is possible that another wheel event with phase began arrives when the timer is running. In this case the pending wheel event will be sent immediately to show the end of the previous scroll sequence before dispatching the event with phase began. BUG=526463 TEST=RenderWidgetHostViewMacWithWheelScrollLatchingEnabledTest. WheelWithPhaseEndedIsNotForwardedImmediately, RenderWidgetHostViewMacWithWheelScrollLatchingEnabledTest. WheelWithMomentumPhaseBeganStopsTheWheelEndDispatchTimer, RenderWidgetHostViewMacWithWheelScrollLatchingEnabledTest. WheelWithPhaseBeganDispatchesThePendingWheelEnd ==========
sahel@chromium.org changed reviewers: + tdresser@chromium.org
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...
https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.h (right): https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.h:490: bool WheelScrollLatchingEnabled() { return wheel_scroll_latching_enabled_; } Is this needed? It appears redundant with https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid... https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.h:492: base::OneShotTimer mouse_wheel_end_dispatch_timer_; Make private. https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.h:496: bool should_route_event); Can this be private? https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.h:498: bool should_route_event); Can this be private? https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2600: if (webEvent.phase == blink::WebMouseWheelEvent::kPhaseEnded) { Could you add a comment per condition here? https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2606: // A new scrolling sequence hase started, stop the timer and send its hase -> has https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm (right): https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:309: // Create a dum event with phaseNone. This is for resetting the phase info of dum -> dummy? https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:312: CGEventRef cg_event1 = CGEventCreateScrollWheelEvent( Why event1 instead of just event? (and below with event1). https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:317: // cg_momentum_phase); Should these lines be removed? https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1351: WheelWithPhaseEndedIsNotForwardedImmediately) { Can you add a comment briefly explaining why it shouldn't be forwarded immediately? https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1384: // the evnet will be dropped and the mouse_wheel_end_dispatch_timer_ will evnet -> event https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1430: // the evnet will be dropped and the mouse_wheel_end_dispatch_timer_ will evnet -> event https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1486: // the evnet will be dropped and the mouse_wheel_end_dispatch_timer_ will evnet -> event
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...
https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.h (right): https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.h:490: bool WheelScrollLatchingEnabled() { return wheel_scroll_latching_enabled_; } On 2017/05/25 15:18:36, tdresser wrote: > Is this needed? It appears redundant with > https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid... Done. https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.h:492: base::OneShotTimer mouse_wheel_end_dispatch_timer_; On 2017/05/25 15:18:36, tdresser wrote: > Make private. This is Stopped in RenderWidgetHostViewCocoa, should I make it private and add a public function that stops this timer? https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.h:496: bool should_route_event); On 2017/05/25 15:18:36, tdresser wrote: > Can this be private? This is called in RenderWidgetHostViewCocoa, cannot be private. https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.h:498: bool should_route_event); On 2017/05/25 15:18:36, tdresser wrote: > Can this be private? This is called in RenderWidgetHostViewCocoa, cannot be private. https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2600: if (webEvent.phase == blink::WebMouseWheelEvent::kPhaseEnded) { On 2017/05/25 15:18:36, tdresser wrote: > Could you add a comment per condition here? Done. https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:2606: // A new scrolling sequence hase started, stop the timer and send its On 2017/05/25 15:18:36, tdresser wrote: > hase -> has Done. https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac_unittest.mm (right): https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:309: // Create a dum event with phaseNone. This is for resetting the phase info of On 2017/05/25 15:18:37, tdresser wrote: > dum -> dummy? Done. https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:312: CGEventRef cg_event1 = CGEventCreateScrollWheelEvent( On 2017/05/25 15:18:36, tdresser wrote: > Why event1 instead of just event? (and below with event1). Done. https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:317: // cg_momentum_phase); On 2017/05/25 15:18:36, tdresser wrote: > Should these lines be removed? Done, I am sorry for the sloppy unittest file, I didn't mean to upload this version. https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1351: WheelWithPhaseEndedIsNotForwardedImmediately) { On 2017/05/25 15:18:36, tdresser wrote: > Can you add a comment briefly explaining why it shouldn't be forwarded > immediately? Done. https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1384: // the evnet will be dropped and the mouse_wheel_end_dispatch_timer_ will On 2017/05/25 15:18:37, tdresser wrote: > evnet -> event Done. https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1430: // the evnet will be dropped and the mouse_wheel_end_dispatch_timer_ will On 2017/05/25 15:18:37, tdresser wrote: > evnet -> event Done. https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac_unittest.mm:1486: // the evnet will be dropped and the mouse_wheel_end_dispatch_timer_ will On 2017/05/25 15:18:36, tdresser wrote: > evnet -> event Done.
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_...)
LGTM with nits. https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.h (right): https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.h:492: base::OneShotTimer mouse_wheel_end_dispatch_timer_; On 2017/05/25 16:00:04, sahel wrote: > On 2017/05/25 15:18:36, tdresser wrote: > > Make private. > > This is Stopped in RenderWidgetHostViewCocoa, should I make it private and add a > public function that stops this timer? Yup. The fact that this is implemented via a timer is arguably an implementation detail that shouldn't be exposed, so you might want to name it more semantically. Maybe we need two methods, like "DispatchPendingWheelEndEvent" and "IgnorePendingWheelEndEvent"? https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.h:496: bool should_route_event); On 2017/05/25 16:00:04, sahel wrote: > On 2017/05/25 15:18:36, tdresser wrote: > > Can this be private? > > This is called in RenderWidgetHostViewCocoa, cannot be private. It is? Isn't it only called in RenderWidgetHostViewMac::StartMouseWheelEndDispatchTimer? https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.h:498: bool should_route_event); On 2017/05/25 16:00:04, sahel wrote: > On 2017/05/25 15:18:36, tdresser wrote: > > Can this be private? > > This is called in RenderWidgetHostViewCocoa, cannot be private. Acknowledged. https://codereview.chromium.org/2902303002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.h (right): https://codereview.chromium.org/2902303002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.h:496: bool should_route_event); Since this is public, we might not want to refer to the implementation detail of this starting the timer, but should refer to what the method does and when to call it. Something like QueueMouseWheelEnd, or MouseWheelMaybeFlinging or something like that.
The CQ bit was checked by sahel@chromium.org to run a CQ dry run
https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.h (right): https://codereview.chromium.org/2902303002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.h:496: bool should_route_event); On 2017/05/25 19:34:48, tdresser wrote: > On 2017/05/25 16:00:04, sahel wrote: > > On 2017/05/25 15:18:36, tdresser wrote: > > > Can this be private? > > > > This is called in RenderWidgetHostViewCocoa, cannot be private. > > It is? Isn't it only called in > RenderWidgetHostViewMac::StartMouseWheelEndDispatchTimer? Done. https://codereview.chromium.org/2902303002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.h (right): https://codereview.chromium.org/2902303002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.h:496: bool should_route_event); On 2017/05/25 19:34:48, tdresser wrote: > Since this is public, we might not want to refer to the implementation detail of > this starting the timer, but should refer to what the method does and when to > call it. > > Something like QueueMouseWheelEnd, or MouseWheelMaybeFlinging or something like > that. Done.
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
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2902303002/#ps60001 (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": 60001, "attempt_start_ts": 1496063638058700,
"parent_rev": "de75f45d3842ce5270b49a0faaa8dc101e8c792f", "commit_rev":
"a71587c587597d64a7cd97d65826692f699bd881"}
Message was sent while issue was closed.
Description was changed from ========== phase based wheel scroll latching for mac This cl is the mac version of the following cl: https://codereview.chromium.org/2882443002/ When a mouse wheel event with phase ended arrives, instead of dispatching it, a timer gets started. While the timer is running if a wheel event with momentumPhase began arrives, the timer stops and no wheel event with phase ended is sent. This is for latching the momentum (fling) part of the scrolling to the same target that has handled scrolling phase. If the timer expires, the pending wheel event with phase ended is sent to indicate end of the current scrolling sequence with a momentum phase happening. It is possible that another wheel event with phase began arrives when the timer is running. In this case the pending wheel event will be sent immediately to show the end of the previous scroll sequence before dispatching the event with phase began. BUG=526463 TEST=RenderWidgetHostViewMacWithWheelScrollLatchingEnabledTest. WheelWithPhaseEndedIsNotForwardedImmediately, RenderWidgetHostViewMacWithWheelScrollLatchingEnabledTest. WheelWithMomentumPhaseBeganStopsTheWheelEndDispatchTimer, RenderWidgetHostViewMacWithWheelScrollLatchingEnabledTest. WheelWithPhaseBeganDispatchesThePendingWheelEnd ========== to ========== phase based wheel scroll latching for mac This cl is the mac version of the following cl: https://codereview.chromium.org/2882443002/ When a mouse wheel event with phase ended arrives, instead of dispatching it, a timer gets started. While the timer is running if a wheel event with momentumPhase began arrives, the timer stops and no wheel event with phase ended is sent. This is for latching the momentum (fling) part of the scrolling to the same target that has handled scrolling phase. If the timer expires, the pending wheel event with phase ended is sent to indicate end of the current scrolling sequence with a momentum phase happening. It is possible that another wheel event with phase began arrives when the timer is running. In this case the pending wheel event will be sent immediately to show the end of the previous scroll sequence before dispatching the event with phase began. BUG=526463 TEST=RenderWidgetHostViewMacWithWheelScrollLatchingEnabledTest. WheelWithPhaseEndedIsNotForwardedImmediately, RenderWidgetHostViewMacWithWheelScrollLatchingEnabledTest. WheelWithMomentumPhaseBeganStopsTheWheelEndDispatchTimer, RenderWidgetHostViewMacWithWheelScrollLatchingEnabledTest. WheelWithPhaseBeganDispatchesThePendingWheelEnd Review-Url: https://codereview.chromium.org/2902303002 Cr-Commit-Position: refs/heads/master@{#475346} Committed: https://chromium.googlesource.com/chromium/src/+/a71587c587597d64a7cd97d65826... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/a71587c587597d64a7cd97d65826...
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) identified this CL at revision 475346 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
