Bug 148358 - [Cocoa] Unify FontCache
Summary: [Cocoa] Unify FontCache
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-22 01:47 PDT by Myles C. Maxfield
Modified: 2015-08-24 15:49 PDT (History)
7 users (show)

See Also:


Attachments
Patch (58.93 KB, patch)
2015-08-22 01:48 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (58.85 KB, patch)
2015-08-22 02:17 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (724.35 KB, application/zip)
2015-08-22 02:36 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-mavericks (595.76 KB, application/zip)
2015-08-22 03:06 PDT, Build Bot
no flags Details
Patch (58.93 KB, patch)
2015-08-23 21:32 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (61.65 KB, patch)
2015-08-23 23:46 PDT, Myles C. Maxfield
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2015-08-22 01:47:11 PDT
[Cocoa] Unify FontCache
Comment 1 Myles C. Maxfield 2015-08-22 01:48:25 PDT
Created attachment 259716 [details]
Patch
Comment 2 Myles C. Maxfield 2015-08-22 02:17:27 PDT
Created attachment 259718 [details]
Patch
Comment 3 Build Bot 2015-08-22 02:36:27 PDT
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
Comment 4 Build Bot 2015-08-22 02:36:30 PDT
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
Comment 5 Build Bot 2015-08-22 03:06:21 PDT
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
Comment 6 Build Bot 2015-08-22 03:06:24 PDT
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
Comment 7 Darin Adler 2015-08-23 09:25:57 PDT
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.
Comment 8 Myles C. Maxfield 2015-08-23 21:32:34 PDT
Created attachment 259746 [details]
Patch
Comment 9 Myles C. Maxfield 2015-08-23 23:41:16 PDT
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!
Comment 10 Myles C. Maxfield 2015-08-23 23:46:26 PDT
Created attachment 259749 [details]
Patch
Comment 11 Antti Koivisto 2015-08-24 11:29:15 PDT
Comment on attachment 259749 [details]
Patch

r=me
Comment 12 Myles C. Maxfield 2015-08-24 11:36:31 PDT
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
Comment 13 Myles C. Maxfield 2015-08-24 11:37:52 PDT
Committed r188871: <http://trac.webkit.org/changeset/188871>
Comment 14 Tim Horton 2015-08-24 11:51:33 PDT
Small build fix in http://trac.webkit.org/changeset/188874
Comment 15 Tim Horton 2015-08-24 12:04:16 PDT
And https://trac.webkit.org/changeset/188875, which I'm less sure of. Myles, can you check?
Comment 16 Myles C. Maxfield 2015-08-24 15:49:21 PDT
http://trac.webkit.org/changeset/188877 fixes these up.