Bug 228630 – Add a HashTraits implementation for LayoutUnit

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Martin Robinson]

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.

[Martin Robinson]

Created attachment 434746 [details]
Patch

[Fujii Hironori]

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

[Martin Robinson]

Created attachment 434823 [details]
Patch

[Martin Robinson]

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

[Fujii Hironori]

Comment on attachment 434823 [details]
Patch

Nice. LGTM.

[Darin Adler]

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?

[Martin Robinson]

Created attachment 434894 [details]
Patch

[Martin Robinson]

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

[EWS]

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

[Radar WebKit Bug Importer]

<rdar://problem/81511360>