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.
Created attachment 418521 [details] Patch
Created attachment 418523 [details] Patch
Created attachment 418527 [details] Patch
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?
Created attachment 418636 [details] Patch
(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.
Created attachment 418638 [details] Patch
Committed r272019: <https://trac.webkit.org/changeset/272019> All reviewed patches have been landed. Closing bug and clearing flags on attachment 418638 [details].
<rdar://problem/73714375>