Bug 148358 – [Cocoa] Unify FontCache

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[Cocoa] Unify FontCache

[Myles C. Maxfield]

Created attachment 259716 [details]
Patch

[Myles C. Maxfield]

Created attachment 259718 [details]
Patch

[Build Bot]

Comment on attachment 259718 [details]
Patch

Attachment 259718 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/89434

New failing tests:
fast/text/weighted-italicized-system-font.html

[Build Bot]

Created attachment 259720 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 259718 [details]
Patch

Attachment 259718 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/89525

New failing tests:
fast/text/weighted-italicized-system-font.html

[Build Bot]

Created attachment 259721 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Darin Adler]

Comment on attachment 259718 [details]
Patch

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

> Source/WebCore/platform/graphics/FontCache.h:222
> +FontWeight fontWeightfromCT(CGFloat weight);

Strange name. Strong to have “from” all lowercase in a name that’s mostly using intercaps. Also strange to use CT in this name and then use CoreText in the name of the function just below!

> Source/WebCore/platform/graphics/FontCache.h:226
> +typedef std::pair<bool, bool> SynthesisPair; // synthetic bold, synthetic italic

We normally put typedefs first, rather than mixed in with functions.

> Source/WebCore/platform/graphics/FontCache.h:231
> +bool requiresCustomFallbackFont(const UInt32 character);

The const here has no value so it should be omitted; we don’t mark all our int arguments const, although we could. I think the type here should be UChar32, not UInt32, even though they are roughly the same thing, but I could be wrong about the types used in CoreText functions.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:187
>      if (weight < -0.6)
> -        weightMask = FontWeight100Mask;
> +        return FontWeight100;
>      else if (weight < -0.365)
> -        weightMask = FontWeight200Mask;
> +        return FontWeight200;
>      else if (weight < -0.115)
> -        weightMask = FontWeight300Mask;
> +        return FontWeight300;
>      else if (weight <  0.130)
> -        weightMask = FontWeight400Mask;
> +        return FontWeight400;
>      else if (weight <  0.235)
> -        weightMask = FontWeight500Mask;
> +        return FontWeight500;
>      else if (weight <  0.350)
> -        weightMask = FontWeight600Mask;
> +        return FontWeight600;
>      else if (weight <  0.500)
> -        weightMask = FontWeight700Mask;
> +        return FontWeight700;
>      else if (weight <  0.700)
> -        weightMask = FontWeight800Mask;
> +        return FontWeight800;
>      else
> +        return FontWeight900;

Can delete all those “elses”.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:354
> +        return fontFamilies;

return { };

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:358
> +        return fontFamilies;

return { };

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:368
> +    copyToVector(visited, fontFamilies);
> +    return fontFamilies;

Should declare fontFamilies right down here, not at the top of the function. Also, would be nice to have a version of copyToVector that makes a new vector so we don’t need a local variable. Came up in that other patch, I believe.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:381
> +SynthesisPair computeNecessarySynthesis(CTFontRef font, const FontDescription& fontDescription, bool isPlatformFont)

The use of pair here is really unclear. A struct with two bool would probably be better.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:385
> +        return std::make_pair(false, false);

Could just say:

    return { false, false};

or event:

    return { };

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:389
> +        return std::make_pair(false, false);

Ditto.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:398
> +    bool syntheticBold = (fontDescription.fontSynthesis() & FontSynthesisWeight) && (desiredTraits & kCTFontTraitBold) && !(actualTraits & kCTFontTraitBold);
> +    bool syntheticOblique = (fontDescription.fontSynthesis() & FontSynthesisStyle) && (desiredTraits & kCTFontTraitItalic) && !(actualTraits & kCTFontTraitItalic);

I would suggest considering different names here. The variable itself does not contain “synthetic bold”, instead it is a flag that says, “should synthesize bold” or “need synthetic bold” or something like that.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:400
> +    return std::make_pair(syntheticBold, syntheticOblique);

Can do:

    return { syntheticBold, syntheticOblique };

Or:

    return {
        (fontDescription.fontSynthesis() & FontSynthesisWeight) && (desiredTraits & kCTFontTraitBold) && !(actualTraits & kCTFontTraitBold),
        (fontDescription.fontSynthesis() & FontSynthesisStyle) && (desiredTraits & kCTFontTraitItalic) && !(actualTraits & kCTFontTraitItalic)
    };

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:422
> +    if (whitelist.size() && !whitelist.contains(family.lower()))

No need for lower() here since the HashSet uses CaseFoldingHash.

It seems really oblique to use emptiness to indicate whether there is a white list or not. An empty list doesn’t seem like the same thing as no list at all.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:425
> +    RetainPtr<CTFontRef> foundFont = adoptCF(CTFontCreateForCSS(family.string().createCFString().get(), toCoreTextFontWeight(weight), requestedTraits, size));

I suggest using auto here.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:478
> +        RetainPtr<CTFontRef> autoActivatedFont = adoptCF(CTFontCreateWithName(family.string().createCFString().get(), size, nullptr));

Not sure exactly why we put this into a local variable if we want to ignore it. Is there some better idiom for this?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:512
> +RefPtr<Font> FontCache::systemFallbackForCharacters(const FontDescription& description, const Font* originalFontData, bool isPlatformFont, const UChar* characters, unsigned length)

I think this cannot return null and thus is should return Ref, not RefPtr.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:532
> +    CTFontRef substituteFont = fallbackDedupSet().add(result).iterator->get();

Might save a little reference count churn here by using WTF::move(result); not sure.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:171
> +static inline FontWeight appkitWeightToFontWeight(NSInteger appKitWeight)
> +{
> +    return appKitWeight == 1 ? FontWeight100 :
> +        appKitWeight == 2 ? FontWeight200 :
> +        appKitWeight <= 4 ? FontWeight300 :
> +        appKitWeight == 5 ? FontWeight400 :
> +        appKitWeight == 6 ? FontWeight500 :
> +        appKitWeight <= 8 ? FontWeight600 :
> +        appKitWeight == 9 ? FontWeight700 :
> +        appKitWeight <= 11 ? FontWeight800 :
> +        FontWeight900;
> +}

Why is the coding style of this function so different from fontWeightfromCT; they are both doing the same kind of job.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:228
> +        RetainPtr<CFDictionaryRef> featureIdentifier = adoptCF(CFDictionaryCreate(kCFAllocatorDefault, featureKeys, featureValues, WTF_ARRAY_LENGTH(featureKeys), &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
> +        RetainPtr<CFDictionaryRef> featureIdentifierPtr = featureIdentifier.get();
> +        RetainPtr<CFArrayRef> featureArray = adoptCF(CFArrayCreate(kCFAllocatorDefault, (const void**)&featureIdentifierPtr, 1, &kCFTypeArrayCallBacks));

Why is the type of featureIdentifierPtr RetainPtr<CFDictionaryRef>? I think it should be CFTypeRef and then I think we would not need any type casting and we would not do extra reference count churn.

As the code currently is, there is no reason to use featureIdentifierPtr instead of featureIdentifier in the CFArrayCreate call.!

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:410
> +    static NeverDestroyed<AtomicString> timesStr("Times", AtomicString::ConstructFromLiteral);

I’m not sure this optimization is helpful and needed.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:422
> +    static NeverDestroyed<AtomicString> lucidaGrandeStr("Lucida Grande", AtomicString::ConstructFromLiteral);

I’m not sure this optimization is helpful and needed.

[Myles C. Maxfield]

Created attachment 259746 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 259718 [details]
Patch

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

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:368
>> +    return fontFamilies;
> 
> Should declare fontFamilies right down here, not at the top of the function. Also, would be nice to have a version of copyToVector that makes a new vector so we don’t need a local variable. Came up in that other patch, I believe.

I think this is a good idea to do in a follow-up patch.

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:422
>> +    if (whitelist.size() && !whitelist.contains(family.lower()))
> 
> No need for lower() here since the HashSet uses CaseFoldingHash.
> 
> It seems really oblique to use emptiness to indicate whether there is a white list or not. An empty list doesn’t seem like the same thing as no list at all.

How would you feel if I made the Whitelist an Optional<HashSet>?

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:478
>> +        RetainPtr<CTFontRef> autoActivatedFont = adoptCF(CTFontCreateWithName(family.string().createCFString().get(), size, nullptr));
> 
> Not sure exactly why we put this into a local variable if we want to ignore it. Is there some better idiom for this?

The variable is necessary because of the WARN_UNUSED_RESULT. A better solution is to forgo the RetainPtr entirely and just use CFRelease(CTFontCreateWithName()).

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:512
>> +RefPtr<Font> FontCache::systemFallbackForCharacters(const FontDescription& description, const Font* originalFontData, bool isPlatformFont, const UChar* characters, unsigned length)
> 
> I think this cannot return null and thus is should return Ref, not RefPtr.

It can return nullptr on other ports (see FontCacheFreeType.cpp).

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:532
>> +    CTFontRef substituteFont = fallbackDedupSet().add(result).iterator->get();
> 
> Might save a little reference count churn here by using WTF::move(result); not sure.

I don't want to move from the dedupSet; that's the whole point of it (unless I am misunderstanding you).

>> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:228
>> +        RetainPtr<CFArrayRef> featureArray = adoptCF(CFArrayCreate(kCFAllocatorDefault, (const void**)&featureIdentifierPtr, 1, &kCFTypeArrayCallBacks));
> 
> Why is the type of featureIdentifierPtr RetainPtr<CFDictionaryRef>? I think it should be CFTypeRef and then I think we would not need any type casting and we would not do extra reference count churn.
> 
> As the code currently is, there is no reason to use featureIdentifierPtr instead of featureIdentifier in the CFArrayCreate call.!

Good catch!

[Myles C. Maxfield]

Created attachment 259749 [details]
Patch

[Antti Koivisto]

Comment on attachment 259749 [details]
Patch

r=me

[Myles C. Maxfield]

Comment on attachment 259718 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:368
>>> +    return fontFamilies;
>> 
>> Should declare fontFamilies right down here, not at the top of the function. Also, would be nice to have a version of copyToVector that makes a new vector so we don’t need a local variable. Came up in that other patch, I believe.
> 
> I think this is a good idea to do in a follow-up patch.

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

>>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:422
>>> +    if (whitelist.size() && !whitelist.contains(family.lower()))
>> 
>> No need for lower() here since the HashSet uses CaseFoldingHash.
>> 
>> It seems really oblique to use emptiness to indicate whether there is a white list or not. An empty list doesn’t seem like the same thing as no list at all.
> 
> How would you feel if I made the Whitelist an Optional<HashSet>?

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

[Myles C. Maxfield]

Committed r188871: <http://trac.webkit.org/changeset/188871>

[Tim Horton]

Small build fix in http://trac.webkit.org/changeset/188874

[Tim Horton]

And https://trac.webkit.org/changeset/188875, which I'm less sure of. Myles, can you check?

[Myles C. Maxfield]

http://trac.webkit.org/changeset/188877 fixes these up.