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

Martin Robinson
Reported 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.
Attachments
Patch (25.90 KB, patch)
2021-06-03 01:29 PDT, Martin Robinson 		no flags
DetailsFormatted DiffDiff
Patch (25.97 KB, patch)
2021-06-03 03:19 PDT, Martin Robinson 		no flags
DetailsFormatted DiffDiff
Patch (25.56 KB, patch)
2021-06-04 01:03 PDT, Martin Robinson 		no flags
DetailsFormatted DiffDiff
Patch (25.50 KB, patch)
2021-06-04 11:11 PDT, Martin Robinson 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Martin Robinson
Comment 1 2021-06-03 01:29:40 PDT
Created attachment 430450 [details] Patch
Martin Robinson
Comment 2 2021-06-03 03:19:51 PDT
Created attachment 430456 [details] Patch
Simon Fraser (smfr)
Comment 3 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.
Martin Robinson
Comment 4 2021-06-04 01:03:28 PDT
Created attachment 430558 [details] Patch
Martin Robinson
Comment 5 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.
Simon Fraser (smfr)
Comment 6 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
Martin Robinson
Comment 7 2021-06-04 11:11:12 PDT
Created attachment 430594 [details] Patch
Martin Robinson
Comment 8 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.
EWS
Comment 9 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].
Radar WebKit Bug Importer
Comment 10 2021-06-04 12:51:19 PDT
<rdar://problem/78881467>
Note You need to log in before you can comment on or make changes to this bug.