Bug 147866 – Implement IntegerHasher

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[font-features] Addressing post-commit review of r188088

[Myles C. Maxfield]

Created attachment 258698 [details]
Patch

[Darin Adler]

Comment on attachment 258698 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258698&action=review

Side questions:

1: Why isn’t m_tag in FontFeature marked const?

2: Generally speaking, FontFeature seems to be a custom-written pair; is there a way to use more of std::pair in its implementation?

3: Why so many includes in this header? I don’t think we need to include anything other than Vector.h and AtomicString.h.

> Source/WebCore/platform/graphics/FontFeatureSettings.cpp:79
> +    static_assert(sizeof(unsigned) == sizeof(UChar) * 2, "Must be able to use StringHasher::addCharacters()");
> +    for (size_t i = 0; i < size(); ++i) {
> +        unsigned featureHash = at(i).hash();
> +        UChar lower = featureHash;
> +        UChar upper = featureHash >> (sizeof(UChar) * 8);
> +        hasher.addCharacters(upper, lower);
> +    }

I’d write this:

    for (auto& feature : m_list) {
        unsigned hash = feature.hash();
        hasher.addCharactersAssumingAligned(hash, hash >> 16);
    }

I don’t think we should try to be abstract about how many bits there are in a UChar. And there's no reason to compile in the code to handle unaligned hashing. If you want to make it even cleaner, we could write something called IntegerHasher, like this:

    class IntegerHasher {
    public:
        void add(unsigned integer) { m_underlyingHasher.addCharactersAssumingAligned(integer, integer >> 16); }
        unsigned hash() const { return m_underlyingHasher.hash(); }
    private:
        StringHasher m_underlyingHasher;
    }

Then:

    for (auto& feature : m_list)
        hasher.add(feature.hash());

Finally, I’m not really sure it’s good to be hashing hashes so excessively. Maybe use the hashes of the tag strings, but for the rest it seems excessive to hash just so we can hash again. Maybe like this:

    for (auto& feature : m_list) {
        hasher.add(feature.tag().hash());
        hasher.add(feature.value());
    }

[Myles C. Maxfield]

Created attachment 259196 [details]
Patch

[Anders Carlsson]

Comment on attachment 259196 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259196&action=review

> Source/WTF/wtf/Hasher.h:295
> +

No need for a newline here.

[Myles C. Maxfield]

Committed r188557: <http://trac.webkit.org/changeset/188557>

[Frédéric Wang (:fredw)]

Comment on attachment 259196 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259196&action=review

> Source/WebCore/platform/graphics/FontFeatureSettings.cpp:67
> +    IntegerHasher hasher;

This bug is very old but probably we needed to include <wtf/Hasher.h> in order to use IntegerHash here. I just landed a commit adding this header.