Bug 221030 - Make closestSnapOffset a method on ScrollSnapOffsetsInfo
Summary: Make closestSnapOffset a method on ScrollSnapOffsetsInfo
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: 145099
  Show dependency treegraph
 
Reported: 2021-01-27 02:55 PST by Martin Robinson
Modified: 2021-07-07 07:17 PDT (History)
8 users (show)

See Also:


Attachments
Patch (18.36 KB, patch)
2021-01-27 03:09 PST, Martin Robinson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (18.38 KB, patch)
2021-01-27 03:33 PST, Martin Robinson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (19.23 KB, patch)
2021-01-27 04:07 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (23.62 KB, patch)
2021-01-28 04:09 PST, Martin Robinson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (23.60 KB, patch)
2021-01-28 04:29 PST, 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-01-27 02:55:53 PST
ScrollSnapOffsetsInfo has all the information it needs to implement this method and this will prevent callers from having to pull out the horizontal and vertical offsets and offset ranges themselves.
Comment 1 Martin Robinson 2021-01-27 03:09:37 PST
Created attachment 418521 [details]
Patch
Comment 2 Martin Robinson 2021-01-27 03:33:48 PST
Created attachment 418523 [details]
Patch
Comment 3 Martin Robinson 2021-01-27 04:07:16 PST
Created attachment 418527 [details]
Patch
Comment 4 Simon Fraser (smfr) 2021-01-27 10:47:42 PST
Comment on attachment 418527 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418527&action=review

> Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:369
> +template <>
> +LayoutUnit ScrollSnapOffsetsInfo<LayoutUnit>::closestSnapOffset(ScrollEventAxis axis, LayoutUnit scrollDestinationOffset, float velocity, unsigned& activeSnapIndex, Optional<LayoutUnit> originalPositionForDirectionalSnapping) const
> +{

Would be nicer if these returned a std::pair<LayoutUnit, unsigned>

> Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:370
> +    return ::WebCore::closestSnapOffset(offsetsForAxis(axis), offsetRangesForAxis(axis), scrollDestinationOffset, velocity, activeSnapIndex, originalPositionForDirectionalSnapping);

Is the leading :: really necessary?
Comment 5 Martin Robinson 2021-01-28 04:09:18 PST
Created attachment 418636 [details]
Patch
Comment 6 Martin Robinson 2021-01-28 04:12:47 PST
(In reply to Simon Fraser (smfr) from comment #4)
> Comment on attachment 418527 [details]
> Patch

Thanks for the review.

> > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:369
> > +template <>
> > +LayoutUnit ScrollSnapOffsetsInfo<LayoutUnit>::closestSnapOffset(ScrollEventAxis axis, LayoutUnit scrollDestinationOffset, float velocity, unsigned& activeSnapIndex, Optional<LayoutUnit> originalPositionForDirectionalSnapping) const
> > +{
> 
> Would be nicer if these returned a std::pair<LayoutUnit, unsigned>

I've done this and have used c++17 structured binding at all call sites and std::tie in places that I was not able to.

> 
> > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:370
> > +    return ::WebCore::closestSnapOffset(offsetsForAxis(axis), offsetRangesForAxis(axis), scrollDestinationOffset, velocity, activeSnapIndex, originalPositionForDirectionalSnapping);
> 
> Is the leading :: really necessary?

I'm not sure, but I went ahead and renamed the free function to closestSnapOffsetWithOffetsAndRanges so that the namespace specifier isn't necessary at all now. This also makes the code a bit clearer, I think.
Comment 7 Martin Robinson 2021-01-28 04:29:37 PST
Created attachment 418638 [details]
Patch
Comment 8 EWS 2021-01-28 10:32:33 PST
Committed r272019: <https://trac.webkit.org/changeset/272019>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418638 [details].
Comment 9 Radar WebKit Bug Importer 2021-01-28 10:33:18 PST
<rdar://problem/73714375>