Bug 226538

Summary: [css-scroll-snap] Snap offsets and active index are duplicated in ScrollController and ScrollableArea
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: ScrollingAssignee: Martin Robinson <mrobinson>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, cmarcelo, esprehn+autocc, ews-watchlist, fred.wang, glenn, jamesr, kondapallykalyan, luiz, pdr, simon.fraser, tonikitoo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 218115, 226630    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Martin Robinson 2021-06-02 08:21:46 PDT 
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.
Comment 1 Martin Robinson 2021-06-03 01:29:40 PDT 
Created attachment 430450 [details]
Patch
Comment 2 Martin Robinson 2021-06-03 03:19:51 PDT 
Created attachment 430456 [details]
Patch
Comment 3 Simon Fraser (smfr) 2021-06-03 10:14:49 PDT 
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.
Comment 4 Martin Robinson 2021-06-04 01:03:28 PDT 
Created attachment 430558 [details]
Patch
Comment 5 Martin Robinson 2021-06-04 01:04:58 PDT 
(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 6 Simon Fraser (smfr) 2021-06-04 10:41:02 PDT 
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
Comment 7 Martin Robinson 2021-06-04 11:11:12 PDT 
Created attachment 430594 [details]
Patch
Comment 8 Martin Robinson 2021-06-04 11:48:31 PDT 
(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.
Comment 9 EWS 2021-06-04 12:50:40 PDT 
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].
Comment 10 Radar WebKit Bug Importer 2021-06-04 12:51:19 PDT 
<rdar://problem/78881467>