Bug 221030 – Make closestSnapOffset a method on ScrollSnapOffsetsInfo

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

[Martin Robinson]

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.

[Martin Robinson]

Created attachment 418521 [details]
Patch

[Martin Robinson]

Created attachment 418523 [details]
Patch

[Martin Robinson]

Created attachment 418527 [details]
Patch

[Simon Fraser (smfr)]

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?

[Martin Robinson]

Created attachment 418636 [details]
Patch

[Martin Robinson]

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

[Martin Robinson]

Created attachment 418638 [details]
Patch

[EWS]

Committed r272019: <https://trac.webkit.org/changeset/272019>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418638 [details].

[Radar WebKit Bug Importer]

<rdar://problem/73714375>