Bug 223880 – Stop using a hash as key in the FontCascadeCache

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebCore Misc.

[Chris Dumez]

Stop using a hash as key in the FontCascadeCache. Instead, use HashTraits and use FontCascadeCacheKey as key.

[Chris Dumez]

Created attachment 424540 [details]
Patch

[Darin Adler]

Comment on attachment 424540 [details]
Patch

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

> Source/WebCore/platform/graphics/FontCache.cpp:462
> +    auto addResult = m_fontCascadeCache.add(key, nullptr);

This is going to require a change to Chris Lord’s patch to make the font cache work from non-main threads. We’ll have to isolate the strings in the key after adding it.

> Source/WebCore/platform/graphics/FontCache.cpp:584
> +    // FIXME: Should hash the key and the family name characters rather than making a hash out of other hashes.

Also should use Hasher instead of IntegerHasher. And, yes, I know that’s how we got into this mess in the first place.

[Chris Dumez]

Created attachment 424559 [details]
Patch

[Chris Dumez]

Created attachment 424565 [details]
Patch

[Darin Adler]

Comment on attachment 424565 [details]
Patch

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

> Source/WebCore/platform/graphics/FontCache.cpp:406
> +bool FontCascadeCacheKey::operator==(const FontCascadeCacheKey& other) const

This does’t need to be a member function. Could just rename from keysMatch to operator== and leave it as a non-member function.

> Source/WebCore/platform/graphics/FontCache.cpp:590
> +    for (unsigned i = 0; i < key.families.size(); ++i) {
> +        auto& family = key.families[i];

How about a modern for loop?

    for (auto& family : key.families) {

[Chris Dumez]

(In reply to Darin Adler from comment #5)
> Comment on attachment 424565 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424565&action=review
> 
> > Source/WebCore/platform/graphics/FontCache.cpp:406
> > +bool FontCascadeCacheKey::operator==(const FontCascadeCacheKey& other) const
> 
> This does’t need to be a member function. Could just rename from keysMatch
> to operator== and leave it as a non-member function.
> 
> > Source/WebCore/platform/graphics/FontCache.cpp:590
> > +    for (unsigned i = 0; i < key.families.size(); ++i) {
> > +        auto& family = key.families[i];
> 
> How about a modern for loop?
> 
>     for (auto& family : key.families) {

But having the index is convenient to look up the family in the "other" object.

[Chris Dumez]

Created attachment 424579 [details]
Patch

[Darin Adler]

Comment on attachment 424565 [details]
Patch

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

>>> Source/WebCore/platform/graphics/FontCache.cpp:406
>>> +bool FontCascadeCacheKey::operator==(const FontCascadeCacheKey& other) const
>> 
>> This does’t need to be a member function. Could just rename from keysMatch to operator== and leave it as a non-member function.
> 
> But having the index is convenient to look up the family in the "other" object.

In the == function, but I was talking about the hash function.

[Darin Adler]

Comment on attachment 424579 [details]
Patch

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

> Source/WebCore/platform/graphics/FontCache.cpp:590
> +    for (unsigned i = 0; i < key.families.size(); ++i) {
> +        auto& family = key.families[i];

Unlike the == function, this can use the modern for loop.

> Source/WebCore/platform/graphics/FontCache.h:199
> +namespace WTF {

We don’t need to put this in the WTF namespace. It doesn’t have to the the default hash for FontCascadeCacheKey. It can just be the hash we pass to the HashMap template.

> Source/WebCore/platform/graphics/FontCache.h:219
> +typedef HashMap<FontCascadeCacheKey, std::unique_ptr<FontCascadeCacheEntry>> FontCascadeCache;

How about using instead of typedef since we have to touch this line?

[Chris Dumez]

Created attachment 424593 [details]
Patch

[Chris Dumez]

*** This bug has been marked as a duplicate of bug 223858 ***