Bug 147751 – Make FontDescriptionKey sensitive to FontFeatureSettings

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

[Myles C. Maxfield]

Make FontDescriptionKey sensitive to FontFeatureSettings

[Myles C. Maxfield]

Created attachment 258407 [details]
Patch

[Myles C. Maxfield]

Created attachment 258408 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 258408 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +

I should probably also say that this patch is in preparation for implementing font-feature-settings.

[Sam Weinig]

Comment on attachment 258407 [details]
Patch

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

> Source/WebCore/platform/graphics/FontCache.h:75
> +        , m_featureSettings(0)

Is this needed? m_featureSettings will be default contracted to null.

> Source/WebCore/platform/graphics/FontCache.h:132
> +    unsigned m_size;
> +    unsigned m_weight;
> +    unsigned m_flags;

Can we default these to 0 here?

> Source/WebCore/platform/graphics/FontFeatureSettings.cpp:72
> +    unsigned result = 0x5E787623U; // Chosen randomly

Why do we need a random number?

[Sam Weinig]

Comment on attachment 258408 [details]
Patch

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

> Source/WebCore/platform/graphics/FontCache.h:110
> +        unsigned result = WTF::intHash(m_size);
> +        result = WTF::pairIntHash(result, WTF::intHash(m_weight));
> +        result = WTF::pairIntHash(result, WTF::intHash(m_flags));
> +        result = WTF::pairIntHash(result, WTF::intHash(m_flags));
> +        result = WTF::PairHash<AtomicString, unsigned>::hash(std::make_pair(m_locale, result));
> +        result = WTF::pairIntHash(result, m_featureSettings ? m_featureSettings->hash() : 0);

The way we usually do this to create an array of unsigned and pass it to StringHasher::hashMemory.  See SecurityOriginHash.h for an example.  We really need to add a hash_combine like thing (http://www.boost.org/doc/libs/1_55_0/doc/html/hash/combine.html).

[Blaze Burg]

Comment on attachment 258408 [details]
Patch

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

> Source/WebCore/platform/graphics/FontCache.h:72
> +        : m_size(0)

What about

> Source/WebCore/platform/graphics/FontCache.h:131
> +    unsigned m_weight;

You could initialize these like so:

unsigned m_size {0};

and remove the default constructor entirely.

[Myles C. Maxfield]

Comment on attachment 258408 [details]
Patch

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

>> Source/WebCore/platform/graphics/FontCache.h:110
>> +        result = WTF::pairIntHash(result, m_featureSettings ? m_featureSettings->hash() : 0);
> 
> The way we usually do this to create an array of unsigned and pass it to StringHasher::hashMemory.  See SecurityOriginHash.h for an example.  We really need to add a hash_combine like thing (http://www.boost.org/doc/libs/1_55_0/doc/html/hash/combine.html).

Ah, yes, I've seen that pattern in PlatformFontData. I'll use that.

[Myles C. Maxfield]

Comment on attachment 258407 [details]
Patch

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

>> Source/WebCore/platform/graphics/FontCache.h:75
>> +        , m_featureSettings(0)
> 
> Is this needed? m_featureSettings will be default contracted to null.

Nope!

>> Source/WebCore/platform/graphics/FontCache.h:132
>> +    unsigned m_flags;
> 
> Can we default these to 0 here?

Yep!

>> Source/WebCore/platform/graphics/FontFeatureSettings.cpp:72
>> +    unsigned result = 0x5E787623U; // Chosen randomly
> 
> Why do we need a random number?

Because this is the input to a hash function, it seems like a better idea than just 0 for increasing entropy.

[Myles C. Maxfield]

Comment on attachment 258408 [details]
Patch

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

> Source/WebCore/platform/graphics/FontCache.h:109
> +        result = WTF::pairIntHash(result, WTF::intHash(m_flags));
> +        result = WTF::pairIntHash(result, WTF::intHash(m_flags));
> +        result = WTF::PairHash<AtomicString, unsigned>::hash(std::make_pair(m_locale, result));

Wow, I completely messed this up

[Myles C. Maxfield]

Created attachment 258409 [details]
Patch

[Myles C. Maxfield]

Created attachment 258410 [details]
Patch

[Myles C. Maxfield]

Created attachment 258412 [details]
Patch

[Myles C. Maxfield]

Committed r188088: <http://trac.webkit.org/changeset/188088>

[Darin Adler]

Comment on attachment 258412 [details]
Patch

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

> Source/WebCore/platform/graphics/FontFeatureSettings.cpp:75
> +    unsigned result = 0;
> +    for (size_t i = 0; i < size(); ++i)
> +        result = WTF::pairIntHash(result, at(i).hash());
> +    return result;

Is this really a good way to hash a vector? I think there’s a better approach where we accumulate with a single hash rather than hashing the old hash with a new hash value every time. Likely more efficient as well. There must be some good example elsewhere where we do this.

[Darin Adler]

Comment on attachment 258412 [details]
Patch

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

>> Source/WebCore/platform/graphics/FontFeatureSettings.cpp:75
>> +    return result;
> 
> Is this really a good way to hash a vector? I think there’s a better approach where we accumulate with a single hash rather than hashing the old hash with a new hash value every time. Likely more efficient as well. There must be some good example elsewhere where we do this.

Hmm, I see you discussing this with Sam earlier, but I am not really sure about where you ended up here. I’d like to understand so I can do it right in the future.

[Myles C. Maxfield]

(In reply to comment #15)
> Comment on attachment 258412 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258412&action=review
> 
> >> Source/WebCore/platform/graphics/FontFeatureSettings.cpp:75
> >> +    return result;
> > 
> > Is this really a good way to hash a vector? I think there’s a better approach where we accumulate with a single hash rather than hashing the old hash with a new hash value every time. Likely more efficient as well. There must be some good example elsewhere where we do this.
> 
> Hmm, I see you discussing this with Sam earlier, but I am not really sure
> about where you ended up here. I’d like to understand so I can do it right
> in the future.

It looks like StringHasher has an interface where you can push data into it willy-nilly. I'll use that.

[Myles C. Maxfield]

(In reply to comment #16)
> (In reply to comment #15)
> > Comment on attachment 258412 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=258412&action=review
> > 
> > >> Source/WebCore/platform/graphics/FontFeatureSettings.cpp:75
> > >> +    return result;
> > > 
> > > Is this really a good way to hash a vector? I think there’s a better approach where we accumulate with a single hash rather than hashing the old hash with a new hash value every time. Likely more efficient as well. There must be some good example elsewhere where we do this.
> > 
> > Hmm, I see you discussing this with Sam earlier, but I am not really sure
> > about where you ended up here. I’d like to understand so I can do it right
> > in the future.
> 
> It looks like StringHasher has an interface where you can push data into it
> willy-nilly. I'll use that.

https://bugs.webkit.org/show_bug.cgi?id=147866