Bug 226538 – [css-scroll-snap] Snap offsets and active index are duplicated in ScrollController and ScrollableArea

• Status: RESOLVED
• Resolution: FIXED
• Component: Scrolling

[Martin Robinson]

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.

[Martin Robinson]

Created attachment 430450 [details]
Patch

[Martin Robinson]

Created attachment 430456 [details]
Patch

[Simon Fraser (smfr)]

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.

[Martin Robinson]

Created attachment 430558 [details]
Patch

[Martin Robinson]

(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.

[Simon Fraser (smfr)]

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

[Martin Robinson]

Created attachment 430594 [details]
Patch

[Martin Robinson]

(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.

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/78881467>