Bug 231347 - Clean up state maintenance around animated scrolls
Summary: Clean up state maintenance around animated scrolls
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-10-06 21:28 PDT by Simon Fraser (smfr)
Modified: 2021-10-07 09:22 PDT (History)
16 users (show)

See Also:

Attachments
Patch (21.34 KB, patch)
2021-10-06 21:35 PDT, Simon Fraser (smfr) 		mrobinson: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2021-10-06 21:28:55 PDT 
Clean up state maintenance around animated scrolls
Comment 1 Simon Fraser (smfr) 2021-10-06 21:35:00 PDT 
Created attachment 440466 [details]
Patch
Comment 2 Martin Robinson 2021-10-07 01:18:00 PDT 
Comment on attachment 440466 [details]
Patch

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

Really glad to see this simplification.

> Source/WebCore/platform/ScrollView.cpp:530
> +    if ((!delegatesScrolling() || currentScrollType() == ScrollType::User) && scrollAnimationStatus() == ScrollAnimationStatus::NotAnimating && newScrollPosition == this->scrollPosition())

I wonder about the combination of testing `scrollAnimationStatus() == ScrollAnimationStatus::NotAnimating` and the call to `scrollAnimator().cancelAnimations();` above. Perhaps `scrollAnimationStatus() == ScrollAnimationStatus::NotAnimating` is always true?
Comment 3 Simon Fraser (smfr) 2021-10-07 08:58:12 PDT 
Comment on attachment 440466 [details]
Patch

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

>> Source/WebCore/platform/ScrollView.cpp:530
>> +    if ((!delegatesScrolling() || currentScrollType() == ScrollType::User) && scrollAnimationStatus() == ScrollAnimationStatus::NotAnimating && newScrollPosition == this->scrollPosition())
> 
> I wonder about the combination of testing `scrollAnimationStatus() == ScrollAnimationStatus::NotAnimating` and the call to `scrollAnimator().cancelAnimations();` above. Perhaps `scrollAnimationStatus() == ScrollAnimationStatus::NotAnimating` is always true?

I think you're right.
Comment 4 Simon Fraser (smfr) 2021-10-07 09:21:07 PDT 
https://trac.webkit.org/changeset/283716/webkit
Comment 5 Radar WebKit Bug Importer 2021-10-07 09:22:20 PDT 
<rdar://problem/83984449>