Commit 35699cea authored by David Bokan's avatar David Bokan Committed by Commit Bot

Reland "Do not coalesce wheel with phaseBegan to wheel with phaseEnded."

This reverts commit 5dd16566.

Reason for revert: This CL was confirmed to be the cause of the perf
regression in crbug.com/978980 - this is artificial so we can reland

Original change's description:
> Revert "Do not coalesce wheel with phaseBegan to wheel with phaseEnded."
>
> This reverts commit 2ca01526.
>
> Reason for revert: Speculative revert for https://crbug.com/978980
>
> Original change's description:
> > Do not coalesce wheel with phaseBegan to wheel with phaseEnded.
> >
> > If no element in the scrolling chain is not scrollable in the given direction
> > (delta_hints) on GSB we latch to the viewport. This is not user friendly on
> > platforms that do not have any scroll affordance animations (e.g. Windows)
> > since if the user changes the scrolling direction and an element in the chain
> > is actually scrollable in the new direction, the user expects it to scroll and
> > has no idea that we have latched to the (possibly unscrollable) viewport for
> > the rest of the scroll sequence. To address the issue for timer based wheel
> > scroll sequence we end the sequence and start a new one upon arrival of a wheel
> > event with different delta directions if no GSU has been consumed since the
> > beginning of the sequence. This cl ensures that the GSE/GSB generated for
> > breaking the latching seqeunce are not coalesced with each other.
> >
> > Bug: 882998
> > Change-Id: Icf703c935475fffd69b899c59abc03ec1b548784
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1524703
> > Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org>
> > Commit-Queue: Sahel Sharify <sahel@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#641719}
>
> TBR=nzolghadr@chromium.org,sahel@chromium.org
>
> # Not skipping CQ checks because original CL landed > 1 day ago.
>
> Bug: 882998,978980
> Change-Id: I85799b6e6eace029b4c2ef00e142f035ac9e8e4c
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1688230
> Reviewed-by: David Bokan <bokan@chromium.org>
> Commit-Queue: David Bokan <bokan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#674615}

TBR=bokan@chromium.org,nzolghadr@chromium.org,sahel@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 882998, 978980
Change-Id: I3dfb3f3ec0467b7101597720325222899e46f10b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1716444Reviewed-by: default avatarDavid Bokan <bokan@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#680315}
parent da8fb16c
...@@ -317,17 +317,9 @@ bool HaveConsistentPhase(const WebMouseWheelEvent& event_to_coalesce, ...@@ -317,17 +317,9 @@ bool HaveConsistentPhase(const WebMouseWheelEvent& event_to_coalesce,
} }
if (event.has_synthetic_phase) { if (event.has_synthetic_phase) {
// Synthetic phase information is added based on a timer in // It is alright to coalesce a wheel event with synthetic phaseChanged to
// MouseWheelPhaseHandler. This information is for simulating scroll // its previous one with synthetic phaseBegan.
// sequences when the beginning and end of scrolls are not available. It is return (event.phase == WebMouseWheelEvent::kPhaseBegan &&
// alright to coalesce an event with synthetic phaseBegan to its previous
// event with synthetic phaseEnded since these phase values don't correspond
// with real start and end of the scroll sequences.
// It is also alright to coalesce a wheel event with synthetic phaseChanged
// to its previous one with synthetic phaseBegan.
return (event.phase == WebMouseWheelEvent::kPhaseEnded &&
event_to_coalesce.phase == WebMouseWheelEvent::kPhaseBegan) ||
(event.phase == WebMouseWheelEvent::kPhaseBegan &&
event_to_coalesce.phase == WebMouseWheelEvent::kPhaseChanged); event_to_coalesce.phase == WebMouseWheelEvent::kPhaseChanged);
} }
return false; return false;
...@@ -377,18 +369,11 @@ void Coalesce(const WebMouseWheelEvent& event_to_coalesce, ...@@ -377,18 +369,11 @@ void Coalesce(const WebMouseWheelEvent& event_to_coalesce,
MergeDispatchTypes(old_dispatch_type, event_to_coalesce.dispatch_type); MergeDispatchTypes(old_dispatch_type, event_to_coalesce.dispatch_type);
if (event_to_coalesce.has_synthetic_phase && if (event_to_coalesce.has_synthetic_phase &&
event_to_coalesce.phase != old_phase) { event_to_coalesce.phase != old_phase) {
if (event_to_coalesce.phase == WebMouseWheelEvent::kPhaseBegan) { // Coalesce a wheel event with synthetic phase changed to a wheel event
// Coalesce a wheel event with synthetic phase began with a wheel event // with synthetic phase began.
// with synthetic phase ended. DCHECK_EQ(WebMouseWheelEvent::kPhaseChanged, event_to_coalesce.phase);
DCHECK_EQ(WebMouseWheelEvent::kPhaseEnded, old_phase); DCHECK_EQ(WebMouseWheelEvent::kPhaseBegan, old_phase);
event->phase = WebMouseWheelEvent::kPhaseChanged; event->phase = WebMouseWheelEvent::kPhaseBegan;
} else {
// Coalesce a wheel event with synthetic phase changed to a wheel event
// with synthetic phase began.
DCHECK_EQ(WebMouseWheelEvent::kPhaseChanged, event_to_coalesce.phase);
DCHECK_EQ(WebMouseWheelEvent::kPhaseBegan, old_phase);
event->phase = WebMouseWheelEvent::kPhaseBegan;
}
} }
} }
......
...@@ -252,21 +252,24 @@ TEST(BlinkEventUtilTest, WebMouseWheelEventCoalescing) { ...@@ -252,21 +252,24 @@ TEST(BlinkEventUtilTest, WebMouseWheelEventCoalescing) {
coalesced_event.phase = blink::WebMouseWheelEvent::kPhaseEnded; coalesced_event.phase = blink::WebMouseWheelEvent::kPhaseEnded;
EXPECT_FALSE(CanCoalesce(event_to_be_coalesced, coalesced_event)); EXPECT_FALSE(CanCoalesce(event_to_be_coalesced, coalesced_event));
// With timer based wheel scroll latching, we break the latching sequence on
// direction change when all prior GSU events in the current sequence are
// ignored. To do so we dispatch the pending wheel event with phaseEnded and
// the first wheel event in the opposite direction will have phaseBegan. The
// GSB generated from this wheel event will cause a new hittesting. To make
// sure that a GSB will actually get created we should not coalesce the wheel
// event with synthetic kPhaseBegan to one with synthetic kPhaseEnded.
event_to_be_coalesced.has_synthetic_phase = true; event_to_be_coalesced.has_synthetic_phase = true;
coalesced_event.has_synthetic_phase = true; coalesced_event.has_synthetic_phase = true;
EXPECT_TRUE(CanCoalesce(event_to_be_coalesced, coalesced_event)); EXPECT_FALSE(CanCoalesce(event_to_be_coalesced, coalesced_event));
Coalesce(event_to_be_coalesced, &coalesced_event);
EXPECT_EQ(blink::WebMouseWheelEvent::kPhaseChanged, coalesced_event.phase);
EXPECT_EQ(7, coalesced_event.delta_x);
EXPECT_EQ(9, coalesced_event.delta_y);
event_to_be_coalesced.phase = blink::WebMouseWheelEvent::kPhaseChanged; event_to_be_coalesced.phase = blink::WebMouseWheelEvent::kPhaseChanged;
coalesced_event.phase = blink::WebMouseWheelEvent::kPhaseBegan; coalesced_event.phase = blink::WebMouseWheelEvent::kPhaseBegan;
EXPECT_TRUE(CanCoalesce(event_to_be_coalesced, coalesced_event)); EXPECT_TRUE(CanCoalesce(event_to_be_coalesced, coalesced_event));
Coalesce(event_to_be_coalesced, &coalesced_event); Coalesce(event_to_be_coalesced, &coalesced_event);
EXPECT_EQ(blink::WebMouseWheelEvent::kPhaseBegan, coalesced_event.phase); EXPECT_EQ(blink::WebMouseWheelEvent::kPhaseBegan, coalesced_event.phase);
EXPECT_EQ(10, coalesced_event.delta_x); EXPECT_EQ(7, coalesced_event.delta_x);
EXPECT_EQ(13, coalesced_event.delta_y); EXPECT_EQ(9, coalesced_event.delta_y);
event_to_be_coalesced.resending_plugin_id = 3; event_to_be_coalesced.resending_plugin_id = 3;
EXPECT_FALSE(CanCoalesce(event_to_be_coalesced, coalesced_event)); EXPECT_FALSE(CanCoalesce(event_to_be_coalesced, coalesced_event));
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment