Various information about the current snap "situation" is duplicated between these two classes needlessly. There is also some complicated code keeping the two copies of the information in sync.
Created attachment 430450 [details] Patch
Created attachment 430456 [details] Patch
Comment on attachment 430456 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430456&action=review > Source/WebCore/platform/ScrollAnimator.h:162 > + void setSnapOffsetsInfo(const LayoutScrollSnapOffsetsInfo*); Using a pointer argument makes it unclear about whether the implementation needs to copy the data. In some places, you pass a stack pointer to this function.
Created attachment 430558 [details] Patch
(In reply to Simon Fraser (smfr) from comment #3) > Comment on attachment 430456 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=430456&action=review > > > Source/WebCore/platform/ScrollAnimator.h:162 > > + void setSnapOffsetsInfo(const LayoutScrollSnapOffsetsInfo*); > > Using a pointer argument makes it unclear about whether the implementation > needs to copy the data. In some places, you pass a stack pointer to this > function. This is a good point. I've modified all of these methods to accept a const LayoutScrollSnapOffsetsInfo&. An empty offsets info signals the inner classes to clear the data. I believe that the scrolling tree already relies on this behavior.
Comment on attachment 430558 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430558&action=review > Source/WebCore/platform/ScrollableArea.cpp:489 > + if (ScrollAnimator* scrollAnimator = existingScrollAnimator()) auto > Source/WebCore/platform/ScrollableArea.cpp:495 > + if (ScrollAnimator* scrollAnimator = existingScrollAnimator()) auto > Source/WebCore/platform/ScrollableArea.cpp:502 > + if (ScrollAnimator* scrollAnimator = existingScrollAnimator()) auto > Source/WebCore/platform/ScrollableArea.cpp:504 > + return 0; // FIXME: This should really be invalidSnapOffsetIndex. Or a std::optional<> > Source/WebCore/platform/ScrollableArea.cpp:531 > + size_t activeHorizontalIndex = currentHorizontalSnapPointIndex(); auto > Source/WebCore/platform/ScrollableArea.cpp:536 > + size_t activeVerticalIndex = currentVerticalSnapPointIndex(); auto > Source/WebCore/platform/ScrollableArea.cpp:548 > + ScrollAnimator* scrollAnimator = existingScrollAnimator(); auto
Created attachment 430594 [details] Patch
(In reply to Simon Fraser (smfr) from comment #6) > Comment on attachment 430558 [details] > Patch Thanks for the review! > > View in context: > https://bugs.webkit.org/attachment.cgi?id=430558&action=review > > > Source/WebCore/platform/ScrollableArea.cpp:489 > > + if (ScrollAnimator* scrollAnimator = existingScrollAnimator()) > > auto Fixed. > > > Source/WebCore/platform/ScrollableArea.cpp:495 > > + if (ScrollAnimator* scrollAnimator = existingScrollAnimator()) > > auto Fixed. > > > Source/WebCore/platform/ScrollableArea.cpp:502 > > + if (ScrollAnimator* scrollAnimator = existingScrollAnimator()) > > auto Fixed. > > > Source/WebCore/platform/ScrollableArea.cpp:504 > > + return 0; // FIXME: This should really be invalidSnapOffsetIndex. > > Or a std::optional<> This is a great idea! I've opened https://bugs.webkit.org/show_bug.cgi?id=226654 for this. > > > Source/WebCore/platform/ScrollableArea.cpp:531 > > + size_t activeHorizontalIndex = currentHorizontalSnapPointIndex(); > > auto Fixed. > > > Source/WebCore/platform/ScrollableArea.cpp:536 > > + size_t activeVerticalIndex = currentVerticalSnapPointIndex(); > > auto Fixed. > > > Source/WebCore/platform/ScrollableArea.cpp:548 > > + ScrollAnimator* scrollAnimator = existingScrollAnimator(); > > auto Fixed.
Committed r278484 (238502@main): <https://commits.webkit.org/238502@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 430594 [details].
<rdar://problem/78881467>