[webkit-reviews] review granted: [Bug 233653] Add a momentum event synthesizer : [Attachment 445459] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Tim Horton
<thorton at apple.com>'s request for review:
Bug 233653: Add a momentum event synthesizer
https://bugs.webkit.org/show_bug.cgi?id=233653

Attachment 445459: Patch

https://bugs.webkit.org/attachment.cgi?id=445459&action=review




--- Comment #5 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 445459
  --> https://bugs.webkit.org/attachment.cgi?id=445459
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445459&action=review

> Source/WebKit/Shared/ScrollingAccelerationCurve.cpp:76
> +	   return (4 * std::pow(x, 3) * std::pow(m_gainQuartic, 4)) + (3 *
std::pow(x, 2) * std::pow(m_gainCubic, 3)) + (2 * x * std::pow(m_gainParabolic,
2)) + m_gainLinear;

This calls std::pow(m_gainQuartic, 4) and std::pow(m_gainCubic, 3) on each
call, but that probably doesn't matter.

> Source/WebKit/Shared/ScrollingAccelerationCurve.cpp:107
> +    constexpr float cursorScale = 96.f / frameRate;

Any idea where the 96 comes from? The word "cursor" here is confusing.

> Source/WebKit/Shared/ScrollingAccelerationCurve.h:42
> +class ScrollingAccelerationCurve {

AccelerationCurve or DecelerationCurve?

> Source/WebKit/Shared/ScrollingAccelerationCurve.h:50
> +    float accelerationFactor(float);

const

> Source/WebKit/Shared/ScrollingAccelerationCurve.h:73
> +    float evaluateQuartic(float);

const

> Source/WebKit/Shared/ScrollingAccelerationCurve.h:91
> +    // Input parameters.
> +    float m_gainLinear { 0 };
> +    float m_gainParabolic { 0 };
> +    float m_gainCubic { 0 };
> +    float m_gainQuartic { 0 };
> +    float m_tangentSpeedLinear { 0 };
> +    float m_tangentSpeedParabolicRoot { 0 };
> +    float m_resolution { 0 };
> +
> +    // Computed intermediate values.
> +    bool m_hasComputedIntermediateValues { false };
> +    float m_tangentStartX { 0 };
> +    float m_tangentSlope { 0 };
> +    float m_tangentIntercept { 0 };
> +    float m_falloffStartX { 0 };
> +    float m_falloffSlope { 0 };
> +    float m_falloffIntercept { 0 };

You could group these into structs to make the comments obsolete.

> Source/WebKit/Shared/mac/ScrollingAccelerationCurveMac.mm:60
> +static ScrollingAccelerationCurve
fromIOHIDCurveArrayWithAcceleration(NSArray *ioHIDCurves, float
desiredAcceleration, float resolution)

Can the NSArray * be typed?

> Source/WebKit/Shared/mac/ScrollingAccelerationCurveMac.mm:86
> +std::optional<ScrollingAccelerationCurve>
ScrollingAccelerationCurve::fromNativeWheelEvent(const NativeWebWheelEvent&
nativeWebWheelEvent)

It's a bit weird to say that you're getting a curve from an event. I guess
you're really getting a curve from the device associated with the event, so
maybe split this into two functions?

> Source/WebKit/Shared/mac/ScrollingAccelerationCurveMac.mm:93
> +    auto hidEvent = adoptCF(CGEventCopyIOHIDEvent(event.CGEvent));

Is CGEventCopyIOHIDEvent OK with a null CGEvent?

> Source/WebKit/UIProcess/WebPageProxy.h:3167
> +#if ENABLE(MOMENTUM_EVENT_DISPATCHER)
> +    std::optional<ScrollingAccelerationCurve> m_scrollingAccelerationCurve;
> +    std::optional<ScrollingAccelerationCurve>
m_lastSentScrollingAccelerationCurve;
> +#endif

Maybe we should bundle these up with:
    std::unique_ptr<WebWheelEventCoalescer> m_wheelEventCoalescer;
    PAL::HysteresisActivity m_wheelEventActivityHysteresis;

into a wheel-event-related helper class.

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:50
> +    stopDisplayLink();

You're sure this gets hit (no retain cycles?)

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:108
> +    return isMomentumEventDuringSyntheticGesture;

I wish we used an enum value for "did this function handle the event" return
values.

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:138
> +    WebWheelEvent syntheticEvent(WebEvent::Wheel,
m_currentGesture.initiatingEvent->position(),
m_currentGesture.initiatingEvent->globalPosition(), appKitAcceleratedDelta,
wheelTicks, WebWheelEvent::ScrollByPixelWheelEvent,
m_currentGesture.initiatingEvent->directionInvertedFromDevice(),
WebWheelEvent::PhaseNone, phase, true,
m_currentGesture.initiatingEvent->scrollCount(), delta,
m_lastIncomingEvent->modifiers(), time, time, { });

I would be inclined to put one arg on each line

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:172
> +    RELEASE_LOG(ScrollAnimations, "MomentumEventDispatcher saw momentum end
phase with total offset %f %f, duration %f (event offset would have been %f
%f)", m_currentGesture.currentOffset.width(),
m_currentGesture.currentOffset.height(), (WallTime::now() -
m_currentGesture.startTime).seconds(),
m_currentGesture.accumulatedEventOffset.width(),
m_currentGesture.accumulatedEventOffset.height());

Maybe %.1f since they are all integral right?

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:174
> +    stopDisplayLink();

Is mismatched start/stop a problem, if that ever happens?

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:184
> +    WTF::TextStream stream(WTF::TextStream::LineMode::SingleLine);
> +    stream << curve;

Should remove this at some point. Maybe a TEMPORARY_LOGGING ifdef or something?

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:222
> +void MomentumEventDispatcher::windowScreenDidChange(WebCore::PageIdentifier
pageID, WebCore::PlatformDisplayID displayID)

A bit odd that no argument references a window.

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:415
> +	   constexpr float velocityGainA = 2.f / 65536.0f;
> +	   constexpr float velocityGainB = 955.f / 65536.0f;
> +	   constexpr float velocityConstant = 98369.f / 65536.0f;
> +	   constexpr float minimumVelocity = 1.f / 65536.0f;

Use fromFixedPoint()?

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.h:84
> +    void didScrollWithInterval(FloatSize, Seconds);
> +    void didScroll(const WebWheelEvent&);

These aren't about scrolling (we have no idea if a scroll happened). They are
about event processing I think?

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.h:93
> +    typedef Deque<Delta, deltaHistoryQueueSize> HistoricalDeltas;
> +    HistoricalDeltas m_deltaHistoryX;
> +    HistoricalDeltas m_deltaHistoryY;

Can we use one deque of struct	{ FloatSize, Seconds }?