We can combine the two snap point selection helpers indicesOfNearestSnapOffsets and searchForPotentialSnapPoints, reducing the amount of times that we walk the snap point list and making it more obvious what the return values of the arguments are. In addition, we can have this method return the geometric next and previous snap offsets if they exist, rather than two offsets that might both be greater or lesser than the target. This is useful because parts of the specification refer to these two values when implementing certain features. This makes the code a lot easier to read and very slightly more efficient.
(In reply to Martin Robinson from comment #0) > We can combine the two snap point selection helpers > indicesOfNearestSnapOffsets and searchForPotentialSnapPoints ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Here I meant to write findFirstSnapStopOffsetBetweenOriginAndDestination.
Created attachment 431531 [details] Patch
Created attachment 431535 [details] Patch
Comment on attachment 431535 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431535&action=review > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:109 > + It seems this neither use viewportLength nor isNearEnoughToOffsetForProximity, so it can keep it at the place of the original "if (originalOffsetForDirectionalSnapping)".
<rdar://problem/79657705>
Created attachment 432041 [details] Patch
Comment on attachment 432041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432041&action=review > Source/WebCore/ChangeLog:16 > + most snap containers have a small number of snap points. Thus, it's unlikely that this "most snap containers have a small number of snap points". That's true until it isn't! What about a big table where every cell is a snap point? > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:59 > + // offset than the previously selected snap stop. We always want to stop at the closest snap stop to the original offset. Clearer as "We always want to stop at the snap stop closest to the original offset" > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:134 > + if (*originalOffsetForDirectionalSnapping < scrollDestinationOffset && previous && (*previous).first <= *originalOffsetForDirectionalSnapping) > + previous.reset(); > + if (*originalOffsetForDirectionalSnapping > scrollDestinationOffset && next && (*next).first >= *originalOffsetForDirectionalSnapping) > + next.reset(); Maybe put *originalOffsetForDirectionalSnapping in a local variable.
Created attachment 432244 [details] Patch
Comment on attachment 432244 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432244&action=review > Source/WebCore/ChangeLog:18 > + of snap points and performance does not seem to be changed. In addition, a future change should mean of of
Created attachment 432245 [details] Patch
(In reply to Simon Fraser (smfr) from comment #7) > Comment on attachment 432041 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=432041&action=review > > > Source/WebCore/ChangeLog:16 > > + most snap containers have a small number of snap points. Thus, it's unlikely that this > > "most snap containers have a small number of snap points". That's true until > it isn't! What about a big table where every cell is a snap point? This is a good point. I did some testing with large 500 cell tables and didn't notice any performance degradation. I think the main reason is that for cases where you are scrolling around like this, we were doing a linear search through the scroll stops any way. In addition, I think there's a lot of performance enhancement opportunities to be had by calling this function less (such as only after an animation completes). > > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:59 > > + // offset than the previously selected snap stop. We always want to stop at the closest snap stop to the original offset. > > Clearer as "We always want to stop at the snap stop closest to the original > offset" Fixed! > > Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp:134 > > + if (*originalOffsetForDirectionalSnapping < scrollDestinationOffset && previous && (*previous).first <= *originalOffsetForDirectionalSnapping) > > + previous.reset(); > > + if (*originalOffsetForDirectionalSnapping > scrollDestinationOffset && next && (*next).first >= *originalOffsetForDirectionalSnapping) > > + next.reset(); > > Maybe put *originalOffsetForDirectionalSnapping in a local variable. Sure. This new version does this.
(In reply to Frédéric Wang (:fredw) from comment #9) > Comment on attachment 432244 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=432244&action=review > > > Source/WebCore/ChangeLog:18 > > + of snap points and performance does not seem to be changed. In addition, a future change should mean > > of of I've fixed this.
Committed r279272 (239151@main): <https://commits.webkit.org/239151@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 432245 [details].