Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2882443002:
Tiimer based phase info generated for mouse wheel events. (Closed)
|
Created:
3 years, 7 months ago by sahel Modified:
3 years, 7 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, dtapuska+chromiumwatch_chromium.org, blink-reviews-api_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, dglazkov+blink, darin-cc_chromium.org, piman+watch_chromium.org, blink-reviews, kalyank, danakj+watch_chromium.org, James Su, kinuko+watch Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionTimer based phase info generated for mouse wheel events.
In RenderWidgetHostViewAura and RenderWidgetHostViewEventHandler
timer based phase info is added to WebMouseWheelEvents when they are
generated from ui events. This phase info is used in MouseWheelEventQueue
to generate GSB and GSE events when wheel scroll latching is enabled.
When latching is enabled, no wheel event with phase = |kPhaseEnded| will
be sent before a wheel event with momentum_phase = |kPhaseBegan|.
This cl contains aura only changes. Mac specific changes will be
implemented as a different cl.
BUG=526463
TEST=RenderWidgetHostViewAuraWheelScrollLatchingEnabledTest.
TimerBasedWheelEventPhaseInfo/TouchpadFlingStartStopsWheelPhaseTimer/
GSBWithTouchSourceStopsWheelScrollSequence
Review-Url: https://codereview.chromium.org/2882443002
Cr-Commit-Position: refs/heads/master@{#473288}
Committed: https://chromium.googlesource.com/chromium/src/+/d18f2226063d54e2f67afbcdf63347a70a94e66b
Patch Set 1 #Patch Set 2 : use of phase instead of syntheticPhase #Patch Set 3 : failing input_router_impl_unittest fixed. #Patch Set 4 : debouncing queue disabled to count sent messages properly #
Total comments: 32
Patch Set 5 : comments addressed #
Total comments: 6
Patch Set 6 : review comments addressed. #Messages
Total messages: 50 (34 generated)
sahel@chromium.org changed reviewers: + bokan@chromium.org, dtapuska@chromium.org
dtapuska@chromium.org: Please review changes in bokan@chromium.org: Please review changes in
Hey Sahel, So you mentioned that the reason you added a separate synthetic field was because on Mac we don't know at the time of a phase end whether we'll get a momentum phase begin. But does that matter now that you've moved the timer into the RWHVEventHandler? Presumably we don't need the timer at all on Mac, right? So if you avoid starting/creating the timer if we have phase info on the incoming wheel event, you don't have to do anything additional. You just have to add phase (and start the timer) if the incoming event has no phase. WDYT? BTW, for my own benefit, when do we get ET_MOUSEWHEEL vs ET_SCROLL?
On 2017/05/11 20:46:03, bokan wrote: > Hey Sahel, > > So you mentioned that the reason you added a separate synthetic field was > because on Mac we don't know at the time of a phase end whether we'll get a > momentum phase begin. But does that matter now that you've moved the timer into > the RWHVEventHandler? Presumably we don't need the timer at all on Mac, right? > So if you avoid starting/creating the timer if we have phase info on the > incoming wheel event, you don't have to do anything additional. You just have to > add phase (and start the timer) if the incoming event has no phase. WDYT? > > BTW, for my own benefit, when do we get ET_MOUSEWHEEL vs ET_SCROLL? In case of mac, I only start the timer when a wheel event with phaseEnded arrives. If a next event with momentum_phase Began arrives while the timer is running, I stop the timer, otherwise when the timer expires It will send a synthetic wheel with syntheticPhase ended. I still need the timer because I don't want to generate a GSE right away when a wheel event with phase Ended arrives. ET_SCROLL is on trackpad scrolling on chromeOS, and Mousewheel is for the rest of the platforms.
On 2017/05/11 20:54:48, sahel wrote: > On 2017/05/11 20:46:03, bokan wrote: > > Hey Sahel, > > > > So you mentioned that the reason you added a separate synthetic field was > > because on Mac we don't know at the time of a phase end whether we'll get a > > momentum phase begin. But does that matter now that you've moved the timer > into > > the RWHVEventHandler? Presumably we don't need the timer at all on Mac, right? > > So if you avoid starting/creating the timer if we have phase info on the > > incoming wheel event, you don't have to do anything additional. You just have > to > > add phase (and start the timer) if the incoming event has no phase. WDYT? > > > > BTW, for my own benefit, when do we get ET_MOUSEWHEEL vs ET_SCROLL? > > In case of mac, I only start the timer when a wheel event with phaseEnded > arrives. > If a next event with momentum_phase Began arrives while the timer is running, I > stop the timer, otherwise when the timer expires It will send a synthetic wheel > with syntheticPhase ended. I still need the timer because I don't want to > generate a GSE right away when a wheel event with phase Ended arrives. Ah, ok thanks, I didn't understand how the fling sequence here worked. So in this case, it looks like when an ET_MOUSEWHEEL[phase=ended] arrives, you forward a WebMouseEvent [syntheticPhase=InScroll]. Then when the timer fires you synthesise a WebMouseEvent[syntheticPhase=ended], is that right? It looks to me like this is where the complexity is rooted because we're trying to have two notions of ending the gesture (i.e. the user let go of the trackpad so end an elastic overscroll immediately but don't retarget a scroll in case we're about to start a fling). IMHO, instead of turning the ET_MOUSEWHEEL[phase=ended] into a WebMouseEvent[syntheticPhase=InScroll], we should just drop it and wait for the timer to pass along the WebMouseEvent[syntheticPhase=ended]. It means letting go of an elastic overscroll will be delayed by 100ms but I think we'd prefer that to the additional complexity. In that case, I think we don't need to differentiate between synthetic and non-synthetic phase. So, on Mac, if you get an ET_MOUSEWHEEL and the timer is running it means that either we've recently had another phase=none event or we recently had a phase=ended. In either case it would be valid to pass along a WebMouseEvent[phase=Changed]. We may want to differentiate the two but it's an edge case (user using both trackpad and external mouse at the same time) that we can deal with later. On non-Mac the timer running always means we had another phase=none recently so add the Changed phase and forward it. Does that work? Aside: an alternative might be to add a new phase MaybeEnded and forward that when you get a phase=Ended (and forward the real phase=Ended when the timer fires), and using that in the overscroll controller. But I'd avoid trying to handle this unless we find that the overscroll delay is bad in practice. > ET_SCROLL is on trackpad scrolling on chromeOS, and Mousewheel is for the rest > of the platforms. Got it, thanks.
On 2017/05/11 21:53:56, bokan wrote: > On 2017/05/11 20:54:48, sahel wrote: > > On 2017/05/11 20:46:03, bokan wrote: > > > Hey Sahel, > > > > > > So you mentioned that the reason you added a separate synthetic field was > > > because on Mac we don't know at the time of a phase end whether we'll get a > > > momentum phase begin. But does that matter now that you've moved the timer > > into > > > the RWHVEventHandler? Presumably we don't need the timer at all on Mac, > right? > > > So if you avoid starting/creating the timer if we have phase info on the > > > incoming wheel event, you don't have to do anything additional. You just > have > > to > > > add phase (and start the timer) if the incoming event has no phase. WDYT? > > > > > > BTW, for my own benefit, when do we get ET_MOUSEWHEEL vs ET_SCROLL? > > > > In case of mac, I only start the timer when a wheel event with phaseEnded > > arrives. > > If a next event with momentum_phase Began arrives while the timer is running, > I > > stop the timer, otherwise when the timer expires It will send a synthetic > wheel > > with syntheticPhase ended. I still need the timer because I don't want to > > generate a GSE right away when a wheel event with phase Ended arrives. > > Ah, ok thanks, I didn't understand how the fling sequence here worked. So in > this case, it looks like when an ET_MOUSEWHEEL[phase=ended] arrives, you forward > a WebMouseEvent [syntheticPhase=InScroll]. Then when the timer fires you > synthesise a WebMouseEvent[syntheticPhase=ended], is that right? It looks to me > like this is where the complexity is rooted because we're trying to have two > notions of ending the gesture (i.e. the user let go of the trackpad so end an > elastic overscroll immediately but don't retarget a scroll in case we're about > to start a fling). > > IMHO, instead of turning the ET_MOUSEWHEEL[phase=ended] into a > WebMouseEvent[syntheticPhase=InScroll], we should just drop it and wait for the > timer to pass along the WebMouseEvent[syntheticPhase=ended]. It means letting go > of an elastic overscroll will be delayed by 100ms but I think we'd prefer that > to the additional complexity. In that case, I think we don't need to > differentiate between synthetic and non-synthetic phase. > > So, on Mac, if you get an ET_MOUSEWHEEL and the timer is running it means that > either we've recently had another phase=none event or we recently had a > phase=ended. In either case it would be valid to pass along a > WebMouseEvent[phase=Changed]. We may want to differentiate the two but it's an > edge case (user using both trackpad and external mouse at the same time) that we > can deal with later. On non-Mac the timer running always means we had another > phase=none recently so add the Changed phase and forward it. > > Does that work? > > Aside: an alternative might be to add a new phase MaybeEnded and forward that > when you get a phase=Ended (and forward the real phase=Ended when the timer > fires), and using that in the overscroll controller. But I'd avoid trying to > handle this unless we find that the overscroll delay is bad in practice. > > > ET_SCROLL is on trackpad scrolling on chromeOS, and Mousewheel is for the rest > > of the platforms. > > Got it, thanks. Yes, I think it does work and dropping the event (I checked that when phase is PhaseEnded, deltaX and deltaY are zero and dropping will be fine) simplifies things a lot, thank you:)
On 2017/05/12 18:28:27, sahel wrote: > On 2017/05/11 21:53:56, bokan wrote: > > On 2017/05/11 20:54:48, sahel wrote: > > > On 2017/05/11 20:46:03, bokan wrote: > > > > Hey Sahel, > > > > > > > > So you mentioned that the reason you added a separate synthetic field was > > > > because on Mac we don't know at the time of a phase end whether we'll get > a > > > > momentum phase begin. But does that matter now that you've moved the timer > > > into > > > > the RWHVEventHandler? Presumably we don't need the timer at all on Mac, > > right? > > > > So if you avoid starting/creating the timer if we have phase info on the > > > > incoming wheel event, you don't have to do anything additional. You just > > have > > > to > > > > add phase (and start the timer) if the incoming event has no phase. WDYT? > > > > > > > > BTW, for my own benefit, when do we get ET_MOUSEWHEEL vs ET_SCROLL? > > > > > > In case of mac, I only start the timer when a wheel event with phaseEnded > > > arrives. > > > If a next event with momentum_phase Began arrives while the timer is > running, > > I > > > stop the timer, otherwise when the timer expires It will send a synthetic > > wheel > > > with syntheticPhase ended. I still need the timer because I don't want to > > > generate a GSE right away when a wheel event with phase Ended arrives. > > > > Ah, ok thanks, I didn't understand how the fling sequence here worked. So in > > this case, it looks like when an ET_MOUSEWHEEL[phase=ended] arrives, you > forward > > a WebMouseEvent [syntheticPhase=InScroll]. Then when the timer fires you > > synthesise a WebMouseEvent[syntheticPhase=ended], is that right? It looks to > me > > like this is where the complexity is rooted because we're trying to have two > > notions of ending the gesture (i.e. the user let go of the trackpad so end an > > elastic overscroll immediately but don't retarget a scroll in case we're about > > to start a fling). > > > > IMHO, instead of turning the ET_MOUSEWHEEL[phase=ended] into a > > WebMouseEvent[syntheticPhase=InScroll], we should just drop it and wait for > the > > timer to pass along the WebMouseEvent[syntheticPhase=ended]. It means letting > go > > of an elastic overscroll will be delayed by 100ms but I think we'd prefer that > > to the additional complexity. In that case, I think we don't need to > > differentiate between synthetic and non-synthetic phase. > > > > So, on Mac, if you get an ET_MOUSEWHEEL and the timer is running it means that > > either we've recently had another phase=none event or we recently had a > > phase=ended. In either case it would be valid to pass along a > > WebMouseEvent[phase=Changed]. We may want to differentiate the two but it's an > > edge case (user using both trackpad and external mouse at the same time) that > we > > can deal with later. On non-Mac the timer running always means we had another > > phase=none recently so add the Changed phase and forward it. > > > > Does that work? > > > > Aside: an alternative might be to add a new phase MaybeEnded and forward that > > when you get a phase=Ended (and forward the real phase=Ended when the timer > > fires), and using that in the overscroll controller. But I'd avoid trying to > > handle this unless we find that the overscroll delay is bad in practice. > > > > > ET_SCROLL is on trackpad scrolling on chromeOS, and Mousewheel is for the > rest > > > of the platforms. > > > > Got it, thanks. > > Yes, I think it does work and dropping the event (I checked that when phase is > PhaseEnded, deltaX and deltaY are zero and dropping will be fine) simplifies > things a lot, thank you:) Great, Dave: does the above sound ok to you? Basically, as far as the renderer is concerned, it won't know the user lifted their fingers from the trackpad until the time expires. I expect users wont notice but if it turns out to be perceptible in a negative way we can either add a MaybeEnded phase which says "fingers lifted but don't retarget since a fling might come", or we can try to see if the Mac API gives us some way to tell a fling is about to happen.
On 2017/05/12 18:41:00, bokan wrote: > On 2017/05/12 18:28:27, sahel wrote: > > On 2017/05/11 21:53:56, bokan wrote: > > > On 2017/05/11 20:54:48, sahel wrote: > > > > On 2017/05/11 20:46:03, bokan wrote: > > > > > Hey Sahel, > > > > > > > > > > So you mentioned that the reason you added a separate synthetic field > was > > > > > because on Mac we don't know at the time of a phase end whether we'll > get > > a > > > > > momentum phase begin. But does that matter now that you've moved the > timer > > > > into > > > > > the RWHVEventHandler? Presumably we don't need the timer at all on Mac, > > > right? > > > > > So if you avoid starting/creating the timer if we have phase info on the > > > > > incoming wheel event, you don't have to do anything additional. You just > > > have > > > > to > > > > > add phase (and start the timer) if the incoming event has no phase. > WDYT? > > > > > > > > > > BTW, for my own benefit, when do we get ET_MOUSEWHEEL vs ET_SCROLL? > > > > > > > > In case of mac, I only start the timer when a wheel event with phaseEnded > > > > arrives. > > > > If a next event with momentum_phase Began arrives while the timer is > > running, > > > I > > > > stop the timer, otherwise when the timer expires It will send a synthetic > > > wheel > > > > with syntheticPhase ended. I still need the timer because I don't want to > > > > generate a GSE right away when a wheel event with phase Ended arrives. > > > > > > Ah, ok thanks, I didn't understand how the fling sequence here worked. So in > > > this case, it looks like when an ET_MOUSEWHEEL[phase=ended] arrives, you > > forward > > > a WebMouseEvent [syntheticPhase=InScroll]. Then when the timer fires you > > > synthesise a WebMouseEvent[syntheticPhase=ended], is that right? It looks to > > me > > > like this is where the complexity is rooted because we're trying to have two > > > notions of ending the gesture (i.e. the user let go of the trackpad so end > an > > > elastic overscroll immediately but don't retarget a scroll in case we're > about > > > to start a fling). > > > > > > IMHO, instead of turning the ET_MOUSEWHEEL[phase=ended] into a > > > WebMouseEvent[syntheticPhase=InScroll], we should just drop it and wait for > > the > > > timer to pass along the WebMouseEvent[syntheticPhase=ended]. It means > letting > > go > > > of an elastic overscroll will be delayed by 100ms but I think we'd prefer > that > > > to the additional complexity. In that case, I think we don't need to > > > differentiate between synthetic and non-synthetic phase. > > > > > > So, on Mac, if you get an ET_MOUSEWHEEL and the timer is running it means > that > > > either we've recently had another phase=none event or we recently had a > > > phase=ended. In either case it would be valid to pass along a > > > WebMouseEvent[phase=Changed]. We may want to differentiate the two but it's > an > > > edge case (user using both trackpad and external mouse at the same time) > that > > we > > > can deal with later. On non-Mac the timer running always means we had > another > > > phase=none recently so add the Changed phase and forward it. > > > > > > Does that work? > > > > > > Aside: an alternative might be to add a new phase MaybeEnded and forward > that > > > when you get a phase=Ended (and forward the real phase=Ended when the timer > > > fires), and using that in the overscroll controller. But I'd avoid trying to > > > handle this unless we find that the overscroll delay is bad in practice. > > > > > > > ET_SCROLL is on trackpad scrolling on chromeOS, and Mousewheel is for the > > rest > > > > of the platforms. > > > > > > Got it, thanks. > > > > Yes, I think it does work and dropping the event (I checked that when phase is > > PhaseEnded, deltaX and deltaY are zero and dropping will be fine) simplifies > > things a lot, thank you:) > > Great, Dave: does the above sound ok to you? Basically, as far as the renderer > is concerned, it won't know the user lifted their fingers from the trackpad > until the time expires. I expect users wont notice but if it turns out to be > perceptible in a negative way we can either add a MaybeEnded phase which says > "fingers lifted but don't retarget since a fling might come", or we can try to > see if the Mac API gives us some way to tell a fling is about to happen. Ya this sgtm
The CQ bit was checked by sahel@chromium.org to run a CQ dry run
Description was changed from ========== Synthetic timer based phase info generated for mouse wheel events. This is a WIP for generating synthetic timer based phase info for mousewheel events. When wheel scroll latching is enabled, in render_widget_host_view_event_handler, synthetic phase info is added to the mousewheel events; In mouse_Wheel_event_queue GSB and GSE events are sent only based on the synthetic phase info. This change will make mouse_wheel events handling similar to touch event handling both for hittesting/targetting in render_widget_host_input_router and async wheel events with only the first one cancellable. BUG=526463 ========== to ========== Timer based phase info generated for mouse wheel events. BUG=526463 TEST=RenderWidgetHostViewAuraWheelScrollLatchingEnabledTest.TimerBasedWheelEventPhaseInfo, RenderWidgetHostViewAuraWheelScrollLatchingEnabledTest.TouchpadFlingStartStopsWheelPhaseTimer, RenderWidgetHostViewAuraWheelScrollLatchingEnabledTest.GSBWithTouchSourceStopsWheelScrollSequence ==========
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_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
sahel@chromium.org changed reviewers: + tdresser@chromium.org
tdresser@chromium.org: Please review changes in
Description was changed from ========== Timer based phase info generated for mouse wheel events. BUG=526463 TEST=RenderWidgetHostViewAuraWheelScrollLatchingEnabledTest.TimerBasedWheelEventPhaseInfo, RenderWidgetHostViewAuraWheelScrollLatchingEnabledTest.TouchpadFlingStartStopsWheelPhaseTimer, RenderWidgetHostViewAuraWheelScrollLatchingEnabledTest.GSBWithTouchSourceStopsWheelScrollSequence ========== to ========== Timer based phase info generated for mouse wheel events. In RenderWidgetHostViewAura and RenderWidgetHostViewEventHandler timer based phase info is added to WebMouseWheelEvents when they are generated from ui events. This phase info is used in MouseWheelEventQueue to generate GSB and GSE events when wheel scroll latching is enabled. This cl contains aura only changes. Mac specific changes will be implemented as a different cl. BUG=526463 TEST=RenderWidgetHostViewAuraWheelScrollLatchingEnabledTest.TimerBasedWheelEventPhaseInfo, RenderWidgetHostViewAuraWheelScrollLatchingEnabledTest.TouchpadFlingStartStopsWheelPhaseTimer, RenderWidgetHostViewAuraWheelScrollLatchingEnabledTest.GSBWithTouchSourceStopsWheelScrollSequence ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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_clang on master.tryserver.chromium.win (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc (right): https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:320: kWheelScrollGlobalY, 5, 5, 0, high_precision); Would it make sense to have a method along the lines of SendMouseWheelPossiblyIncludingPhase, which took the same parameters as SendMouseWheelEventWithPhase, as well as a bool |ignore_phase|? That would let you remove a bunch of duplication here. https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_event_handler.cc (right): https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.cc:941: void RenderWidgetHostViewEventHandler::AddSyntheticPhase( The name of this function doesn't make it clear that it might start a timer which dispatches an event. I haven't come up with a name I really like though. Any ideas? https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_event_handler.h (right): https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.h:12: #include "base/timer/timer.h" Are both of these being used? https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.h:37: // The duration in which a synthetic wheel with zero deltas and in which -> after which https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.h:262: base::OneShotTimer mouse_wheel_phase_timer_; Maybe |dispatch_synthetic_mouse_wheel_end_timer_|?
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/2882443002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.cc:44: wheel_scroll_latching_enabled_(base::FeatureList::IsEnabled( Why do we need to store this? Can we just query FeatureList when you need it - I think that should be cheap? https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_event_handler.cc (right): https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.cc:339: bool should_route_event = ShouldRouteEvent(event); Nit: move this inside the `if` block. https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.cc:430: bool should_route_event = ShouldRouteEvent(event); Define this variable outside only once and use in both blocks. https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.cc:444: mouse_wheel_phase_timer_.Stop(); Is there any harm in calling Stop on a timer that isn't running? If not, remove the `if`. https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.cc:553: if (mouse_wheel_phase_timer_.IsRunning()) { If the wheel events are ignored while a touchscreen scroll is in progress, and we stopped the timer when the touchscreen scroll started, how can this happen? https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.cc:922: void RenderWidgetHostViewEventHandler::SendSyntheticWheelEventWithPhaseEnded( Please add a DCHECK that touchpad latching is enabled here and in AddSyntheticPhase. https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.cc:941: void RenderWidgetHostViewEventHandler::AddSyntheticPhase( On 2017/05/18 13:49:05, tdresser wrote: > The name of this function doesn't make it clear that it might start a timer > which dispatches an event. > > I haven't come up with a name I really like though. > Any ideas? IMO, it doesn't matter that the phase is "synthetic" (it's synthetic by virtue of us adding it here), I'd call this AddPhaseAndScheduleEndEvent. I'd also rename the method to just "AddPhase". https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.cc:944: DCHECK(mouse_wheel_event.phase == blink::WebMouseWheelEvent::kPhaseNone && Where do the Mac events that do have this go? https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_event_handler.h (right): https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.h:262: base::OneShotTimer mouse_wheel_phase_timer_; On 2017/05/18 13:49:06, tdresser wrote: > Maybe |dispatch_synthetic_mouse_wheel_end_timer_|? I like mouse_wheel_end_dispatch_timer_. https://codereview.chromium.org/2882443002/diff/60001/ui/events/blink/web_inp... File ui/events/blink/web_input_event_traits.cc (right): https://codereview.chromium.org/2882443002/diff/60001/ui/events/blink/web_inp... ui/events/blink/web_input_event_traits.cc:225: return static_cast<const WebMouseWheelEvent&>(event).dispatch_type == How is this related to this change?
Patchset #5 (id:80001) 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...
https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc (right): https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:320: kWheelScrollGlobalY, 5, 5, 0, high_precision); On 2017/05/18 13:49:05, tdresser wrote: > Would it make sense to have a method along the lines of > > SendMouseWheelPossiblyIncludingPhase, which took the same parameters as > SendMouseWheelEventWithPhase, as well as a bool |ignore_phase|? > > That would let you remove a bunch of duplication here. Done. https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_base.cc (right): https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_base.cc:44: wheel_scroll_latching_enabled_(base::FeatureList::IsEnabled( On 2017/05/18 16:47:00, bokan wrote: > Why do we need to store this? Can we just query FeatureList when you need it - I > think that should be cheap? This way we need to include the content_features.h only here and use the getter for wheel_scroll_latching_enabled_ variable in render_widget_host_view_event_handler.cc (two places) and render_widget_host_view_mac.mm (two places). I can change it to direct query if you prefer not having a new variable here. https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_event_handler.cc (right): https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.cc:339: bool should_route_event = ShouldRouteEvent(event); On 2017/05/18 16:47:00, bokan wrote: > Nit: move this inside the `if` block. Done. https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.cc:430: bool should_route_event = ShouldRouteEvent(event); On 2017/05/18 16:47:00, bokan wrote: > Define this variable outside only once and use in both blocks. Done. https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.cc:444: mouse_wheel_phase_timer_.Stop(); On 2017/05/18 16:47:00, bokan wrote: > Is there any harm in calling Stop on a timer that isn't running? If not, remove > the `if`. Done. https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.cc:553: if (mouse_wheel_phase_timer_.IsRunning()) { On 2017/05/18 16:47:00, bokan wrote: > If the wheel events are ignored while a touchscreen scroll is in progress, and > we stopped the timer when the touchscreen scroll started, how can this happen? They will be still generated (with phase info) and sent to mouse_wheel_event_queue, but there they will be ignored. It can happen when in the middle of touch screen scrolling sequence we start wheel scrolling and then we stop touch screen scrolling. https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.cc:922: void RenderWidgetHostViewEventHandler::SendSyntheticWheelEventWithPhaseEnded( On 2017/05/18 16:47:00, bokan wrote: > Please add a DCHECK that touchpad latching is enabled here and in > AddSyntheticPhase. Done. https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.cc:941: void RenderWidgetHostViewEventHandler::AddSyntheticPhase( On 2017/05/18 13:49:05, tdresser wrote: > The name of this function doesn't make it clear that it might start a timer > which dispatches an event. > > I haven't come up with a name I really like though. > Any ideas? I changed the name, it might be too long but is descriptive. https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.cc:941: void RenderWidgetHostViewEventHandler::AddSyntheticPhase( On 2017/05/18 16:47:00, bokan wrote: > On 2017/05/18 13:49:05, tdresser wrote: > > The name of this function doesn't make it clear that it might start a timer > > which dispatches an event. > > > > I haven't come up with a name I really like though. > > Any ideas? > > IMO, it doesn't matter that the phase is "synthetic" (it's synthetic by virtue > of us adding it here), I'd call this AddPhaseAndScheduleEndEvent. I'd also > rename the method to just "AddPhase". Done. https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.cc:944: DCHECK(mouse_wheel_event.phase == blink::WebMouseWheelEvent::kPhaseNone && On 2017/05/18 16:47:00, bokan wrote: > Where do the Mac events that do have this go? This event handler is aura only, mac events are handled in render_widget_host_view_mac.mm https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_event_handler.h (right): https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.h:12: #include "base/timer/timer.h" On 2017/05/18 13:49:06, tdresser wrote: > Are both of these being used? Done. https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.h:37: // The duration in which a synthetic wheel with zero deltas and On 2017/05/18 13:49:06, tdresser wrote: > in which -> after which Done. https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.h:262: base::OneShotTimer mouse_wheel_phase_timer_; On 2017/05/18 16:47:01, bokan wrote: > On 2017/05/18 13:49:06, tdresser wrote: > > Maybe |dispatch_synthetic_mouse_wheel_end_timer_|? > > I like mouse_wheel_end_dispatch_timer_. Done. https://codereview.chromium.org/2882443002/diff/60001/ui/events/blink/web_inp... File ui/events/blink/web_input_event_traits.cc (right): https://codereview.chromium.org/2882443002/diff/60001/ui/events/blink/web_inp... ui/events/blink/web_input_event_traits.cc:225: return static_cast<const WebMouseWheelEvent&>(event).dispatch_type == On 2017/05/18 16:47:01, bokan wrote: > How is this related to this change? When a GSB with touch source arrives, we should stop the current wheel based scrolling sequence. To implement this, we stop the timer and send a synthetic wheel event with dispatch type nonblocking to send early ack. This is because we don't want to generate the GSE with touchpad source after GSB with touchscreen source.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please wrap the commit message at 72 columns. Otherwise, lgtm https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_event_handler.cc (right): https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.cc:553: if (mouse_wheel_phase_timer_.IsRunning()) { On 2017/05/18 18:15:15, sahel wrote: > On 2017/05/18 16:47:00, bokan wrote: > > If the wheel events are ignored while a touchscreen scroll is in progress, and > > we stopped the timer when the touchscreen scroll started, how can this happen? > > They will be still generated (with phase info) and sent to > mouse_wheel_event_queue, but there they will be ignored. It can happen when in > the middle of touch screen scrolling sequence we start wheel scrolling and then > we stop touch screen scrolling. Acknowledged. https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.cc:941: void RenderWidgetHostViewEventHandler::AddSyntheticPhase( On 2017/05/18 18:15:15, sahel wrote: > On 2017/05/18 13:49:05, tdresser wrote: > > The name of this function doesn't make it clear that it might start a timer > > which dispatches an event. > > > > I haven't come up with a name I really like though. > > Any ideas? > > I changed the name, it might be too long but is descriptive. We have (much) longer :) https://codereview.chromium.org/2882443002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_event_handler.cc:944: DCHECK(mouse_wheel_event.phase == blink::WebMouseWheelEvent::kPhaseNone && On 2017/05/18 18:15:15, sahel wrote: > On 2017/05/18 16:47:00, bokan wrote: > > Where do the Mac events that do have this go? > > This event handler is aura only, mac events are handled in > render_widget_host_view_mac.mm Acknowledged. https://codereview.chromium.org/2882443002/diff/100001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc (right): https://codereview.chromium.org/2882443002/diff/100001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:422: // |kPhaseEnded| will be sent before a wheel event with momentum_phase = Please also mention in the commit message that this changed.
Description was changed from
==========
Timer based phase info generated for mouse wheel events.
In RenderWidgetHostViewAura and RenderWidgetHostViewEventHandler
timer based phase info is added to WebMouseWheelEvents when they are generated
from ui events. This phase info is used in MouseWheelEventQueue to generate GSB
and GSE events when wheel scroll latching is enabled.
This cl contains aura only changes. Mac specific changes will be implemented as
a different cl.
BUG=526463
TEST=RenderWidgetHostViewAuraWheelScrollLatchingEnabledTest.TimerBasedWheelEventPhaseInfo,
RenderWidgetHostViewAuraWheelScrollLatchingEnabledTest.TouchpadFlingStartStopsWheelPhaseTimer,
RenderWidgetHostViewAuraWheelScrollLatchingEnabledTest.GSBWithTouchSourceStopsWheelScrollSequence
==========
to
==========
Timer based phase info generated for mouse wheel events.
In RenderWidgetHostViewAura and RenderWidgetHostViewEventHandler
timer based phase info is added to WebMouseWheelEvents when they are
generated from ui events. This phase info is used in MouseWheelEventQueue
to generate GSB and GSE events when wheel scroll latching is enabled.
This cl contains aura only changes. Mac specific changes will be
implemented as a different cl.
BUG=526463
TEST=RenderWidgetHostViewAuraWheelScrollLatchingEnabledTest.
TimerBasedWheelEventPhaseInfo/TouchpadFlingStartStopsWheelPhaseTimer/
GSBWithTouchSourceStopsWheelScrollSequence
==========
LGTM https://codereview.chromium.org/2882443002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/2882443002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4779: WebMouseWheelEvent::kPhaseChanged); Is it worth using the same trick here of adding a parameter indicating whether or not phase information should be ignored? https://codereview.chromium.org/2882443002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_event_handler.cc (right): https://codereview.chromium.org/2882443002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.cc:939: blink::WebMouseWheelEvent::kPhaseNone); I'd split this into two DCHECKs, as it'll be easier to read, and easier to debug if one fails.
Description was changed from
==========
Timer based phase info generated for mouse wheel events.
In RenderWidgetHostViewAura and RenderWidgetHostViewEventHandler
timer based phase info is added to WebMouseWheelEvents when they are
generated from ui events. This phase info is used in MouseWheelEventQueue
to generate GSB and GSE events when wheel scroll latching is enabled.
This cl contains aura only changes. Mac specific changes will be
implemented as a different cl.
BUG=526463
TEST=RenderWidgetHostViewAuraWheelScrollLatchingEnabledTest.
TimerBasedWheelEventPhaseInfo/TouchpadFlingStartStopsWheelPhaseTimer/
GSBWithTouchSourceStopsWheelScrollSequence
==========
to
==========
Timer based phase info generated for mouse wheel events.
In RenderWidgetHostViewAura and RenderWidgetHostViewEventHandler
timer based phase info is added to WebMouseWheelEvents when they are
generated from ui events. This phase info is used in MouseWheelEventQueue
to generate GSB and GSE events when wheel scroll latching is enabled.
When latching is enabled, no wheel event with phase = |kPhaseEnded| will be sent
before a wheel event with momentum_phase = |kPhaseBegan|.
This cl contains aura only changes. Mac specific changes will be
implemented as a different cl.
BUG=526463
TEST=RenderWidgetHostViewAuraWheelScrollLatchingEnabledTest.
TimerBasedWheelEventPhaseInfo/TouchpadFlingStartStopsWheelPhaseTimer/
GSBWithTouchSourceStopsWheelScrollSequence
==========
Description was changed from
==========
Timer based phase info generated for mouse wheel events.
In RenderWidgetHostViewAura and RenderWidgetHostViewEventHandler
timer based phase info is added to WebMouseWheelEvents when they are
generated from ui events. This phase info is used in MouseWheelEventQueue
to generate GSB and GSE events when wheel scroll latching is enabled.
When latching is enabled, no wheel event with phase = |kPhaseEnded| will be sent
before a wheel event with momentum_phase = |kPhaseBegan|.
This cl contains aura only changes. Mac specific changes will be
implemented as a different cl.
BUG=526463
TEST=RenderWidgetHostViewAuraWheelScrollLatchingEnabledTest.
TimerBasedWheelEventPhaseInfo/TouchpadFlingStartStopsWheelPhaseTimer/
GSBWithTouchSourceStopsWheelScrollSequence
==========
to
==========
Timer based phase info generated for mouse wheel events.
In RenderWidgetHostViewAura and RenderWidgetHostViewEventHandler
timer based phase info is added to WebMouseWheelEvents when they are
generated from ui events. This phase info is used in MouseWheelEventQueue
to generate GSB and GSE events when wheel scroll latching is enabled.
When latching is enabled, no wheel event with phase = |kPhaseEnded| will
be sent before a wheel event with momentum_phase = |kPhaseBegan|.
This cl contains aura only changes. Mac specific changes will be
implemented as a different cl.
BUG=526463
TEST=RenderWidgetHostViewAuraWheelScrollLatchingEnabledTest.
TimerBasedWheelEventPhaseInfo/TouchpadFlingStartStopsWheelPhaseTimer/
GSBWithTouchSourceStopsWheelScrollSequence
==========
The CQ bit was checked by sahel@chromium.org to run a CQ dry run
https://codereview.chromium.org/2882443002/diff/100001/content/browser/render... File content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc (right): https://codereview.chromium.org/2882443002/diff/100001/content/browser/render... content/browser/renderer_host/input/mouse_wheel_event_queue_unittest.cc:422: // |kPhaseEnded| will be sent before a wheel event with momentum_phase = On 2017/05/19 15:52:59, bokan wrote: > Please also mention in the commit message that this changed. Done. https://codereview.chromium.org/2882443002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/2882443002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:4779: WebMouseWheelEvent::kPhaseChanged); On 2017/05/19 16:17:33, tdresser wrote: > Is it worth using the same trick here of adding a parameter indicating whether > or not phase information should be ignored? Done. https://codereview.chromium.org/2882443002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_event_handler.cc (right): https://codereview.chromium.org/2882443002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_event_handler.cc:939: blink::WebMouseWheelEvent::kPhaseNone); On 2017/05/19 16:17:33, tdresser wrote: > I'd split this into two DCHECKs, as it'll be easier to read, and easier to debug > if one fails. 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 bokan@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2882443002/#ps120001 (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": 120001, "attempt_start_ts": 1495222960868080,
"parent_rev": "9c3c202169fe21903913e25f2a44f3353b72d4d6", "commit_rev":
"d18f2226063d54e2f67afbcdf63347a70a94e66b"}
Message was sent while issue was closed.
Description was changed from
==========
Timer based phase info generated for mouse wheel events.
In RenderWidgetHostViewAura and RenderWidgetHostViewEventHandler
timer based phase info is added to WebMouseWheelEvents when they are
generated from ui events. This phase info is used in MouseWheelEventQueue
to generate GSB and GSE events when wheel scroll latching is enabled.
When latching is enabled, no wheel event with phase = |kPhaseEnded| will
be sent before a wheel event with momentum_phase = |kPhaseBegan|.
This cl contains aura only changes. Mac specific changes will be
implemented as a different cl.
BUG=526463
TEST=RenderWidgetHostViewAuraWheelScrollLatchingEnabledTest.
TimerBasedWheelEventPhaseInfo/TouchpadFlingStartStopsWheelPhaseTimer/
GSBWithTouchSourceStopsWheelScrollSequence
==========
to
==========
Timer based phase info generated for mouse wheel events.
In RenderWidgetHostViewAura and RenderWidgetHostViewEventHandler
timer based phase info is added to WebMouseWheelEvents when they are
generated from ui events. This phase info is used in MouseWheelEventQueue
to generate GSB and GSE events when wheel scroll latching is enabled.
When latching is enabled, no wheel event with phase = |kPhaseEnded| will
be sent before a wheel event with momentum_phase = |kPhaseBegan|.
This cl contains aura only changes. Mac specific changes will be
implemented as a different cl.
BUG=526463
TEST=RenderWidgetHostViewAuraWheelScrollLatchingEnabledTest.
TimerBasedWheelEventPhaseInfo/TouchpadFlingStartStopsWheelPhaseTimer/
GSBWithTouchSourceStopsWheelScrollSequence
Review-Url: https://codereview.chromium.org/2882443002
Cr-Commit-Position: refs/heads/master@{#473288}
Committed:
https://chromium.googlesource.com/chromium/src/+/d18f2226063d54e2f67afbcdf633...
==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/d18f2226063d54e2f67afbcdf633... |
