Summary: | Make closestSnapOffset a method on ScrollSnapOffsetsInfo | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Martin Robinson <mrobinson> | ||||||||||||
Component: | Scrolling | Assignee: | Martin Robinson <mrobinson> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | cmarcelo, ews-watchlist, fred.wang, jamesr, luiz, simon.fraser, tonikitoo, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 145099 | ||||||||||||||
Attachments: |
|
Description
Martin Robinson
2021-01-27 02:55:53 PST
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]. |