Bug 226538 - [css-scroll-snap] Snap offsets and active index are duplicated in ScrollController and ScrollableArea
Summary: [css-scroll-snap] Snap offsets and active index are duplicated in ScrollContr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords: InRadar
Depends on:
Blocks: 218115 226630
  Show dependency treegraph
 
Reported: 2021-06-02 08:21 PDT by Martin Robinson
Modified: 2021-06-04 12:51 PDT (History)
13 users (show)

See Also:


Attachments
Patch (25.90 KB, patch)
2021-06-03 01:29 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (25.97 KB, patch)
2021-06-03 03:19 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (25.56 KB, patch)
2021-06-04 01:03 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (25.50 KB, patch)
2021-06-04 11:11 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>