[webkit-reviews] review granted: [Bug 232077] [GTK][WPE] Use the display refresh to drive scrolling animations (sync scroll) : [Attachment 443081] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Chris Lord
<clord at igalia.com>'s request for review:
Bug 232077: [GTK][WPE] Use the display refresh to drive scrolling animations
(sync scroll)
https://bugs.webkit.org/show_bug.cgi?id=232077

Attachment 443081: Patch

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




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

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

> Source/WebCore/dom/Document.cpp:4358
> +    if (auto* animator = scrollableArea->existingScrollAnimator())
> +	   animator->serviceScrollAnimations();
> +    return scrollableArea->userScrollAnimationInProgress();

Maybe serviceScrollAnimations should return an enum that's like "stop" or "keep
going". Not necessary in this patch.

> Source/WebCore/platform/ScrollAnimator.cpp:462
> +	   m_scrollController.animationCallback(MonotonicTime::now());

Ideally the entire rendering update would use the same snapshotted time. As a
stepping stone, pass the MonotonicTime in to serviceScrollAnimations().

> Source/WebCore/platform/ScrollableArea.h:356
> +    bool userScrollAnimationInProgress() { return
m_userScrollAnimationInProgress; }

Let's make this
bool hasActiveScrollAnimation() const

> Source/WebCore/platform/ScrollableArea.h:357
> +    virtual void setUserScrollAnimationInProgress(bool inProgress) {
m_userScrollAnimationInProgress = inProgress; }

I think it would be better named didStartScrollAnimation() - I don't think you
care about the "did end" case.

> Source/WebCore/platform/ScrollableArea.h:419
> +    bool m_userScrollAnimationInProgress { false };

I don't think we need this state here; ScrollingEffectsController already has
m_isRunningAnimatingCallback, so ScrollableArea can just call through
ScrollAnimator to get this state from its ScrollingEffectsController.