In the scroll snapping code we are converting LayoutUnits to float in order to hash them. This runs the risk of introducing floating point errors.
Created attachment 434746 [details] Patch
Comment on attachment 434746 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434746&action=review > Source/WebCore/platform/LayoutUnit.h:847 > + static unsigned hash(const WebCore::LayoutUnit& p) { return intHash(bitwise_cast<Bits>(p.rawValue())); } I prefer DefaultHash<int>::hash(p.rawValue()) rather than bitwise_cast. > Source/WebCore/platform/LayoutUnit.h:865 > +template<> struct DefaultHash<WebCore::LayoutUnit> : LayoutUnitHash { }; I prefer not having unnecessary LayoutUnitHash class. Simply merge the members to DefaultHash<WebCore::LayoutUnit>.
Created attachment 434823 [details] Patch
(In reply to Fujii Hironori from comment #2) > Comment on attachment 434746 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=434746&action=review > > > Source/WebCore/platform/LayoutUnit.h:847 > > + static unsigned hash(const WebCore::LayoutUnit& p) { return intHash(bitwise_cast<Bits>(p.rawValue())); } > > I prefer DefaultHash<int>::hash(p.rawValue()) rather than bitwise_cast. > > > Source/WebCore/platform/LayoutUnit.h:865 > > +template<> struct DefaultHash<WebCore::LayoutUnit> : LayoutUnitHash { }; > > I prefer not having unnecessary LayoutUnitHash class. Simply merge the > members to DefaultHash<WebCore::LayoutUnit>. Thanks for the review! I've uploaded a new version of the change addressing these concerns.
Comment on attachment 434823 [details] Patch Nice. LGTM.
Comment on attachment 434823 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434823&action=review This is great. Note that this could have been done entirely within ScrollSnapOffsetsInfo.cpp, but I think it’s OK as is. > Source/WebCore/platform/LayoutUnit.h:851 > +// The empty value is INT_MIN, the deleted value is INT_MAX; Comments should say *why* not just *what*. What guarantees these values aren’t needed in hash tables? Why are these good choices?
Created attachment 434894 [details] Patch
(In reply to Darin Adler from comment #6) > Comment on attachment 434823 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=434823&action=review > > This is great. Note that this could have been done entirely within > ScrollSnapOffsetsInfo.cpp, but I think it’s OK as is. > > > Source/WebCore/platform/LayoutUnit.h:851 > > +// The empty value is INT_MIN, the deleted value is INT_MAX; > > Comments should say *why* not just *what*. What guarantees these values > aren’t needed in hash tables? Why are these good choices? I've done my best to explain the reasoning for these choices.
Committed r280632 (240245@main): <https://commits.webkit.org/240245@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 434894 [details].
<rdar://problem/81511360>