[macOS] Migrate off of CTFontCreateForCSS

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[Cocoa] Migrate off of CTFontCreateForCSS

[Myles C. Maxfield]

Created attachment 302313 [details]
Test

[Myles C. Maxfield]

Created attachment 302315 [details]
Test

[Myles C. Maxfield]

Created attachment 302352 [details]
Test

[Myles C. Maxfield]

Created attachment 302447 [details]
First draft

[Jon Lee]

rdar://problem/29653799

[Myles C. Maxfield]

Created attachment 302568 [details]
First draft

[Myles C. Maxfield]

Created attachment 302716 [details]
First draft

[Myles C. Maxfield]

Created attachment 302770 [details]
Passes family name tests

[Myles C. Maxfield]

Created attachment 302776 [details]
Needs perf testing

[Myles C. Maxfield]

Created attachment 302777 [details]
Needs perf testing

[Myles C. Maxfield]

  fast/text/font-weights-zh.html [ Failure ]
  fast/text/font-weights.html [ Failure ]
  fast/text/trak-optimizeLegibility.html [ Crash ]

[Myles C. Maxfield]

Created attachment 302792 [details]
Patch

[Build Bot]

Comment on attachment 302792 [details]
Patch

Attachment 302792 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3198442

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 302794 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Myles C. Maxfield]

Created attachment 302806 [details]
Patch

[Myles C. Maxfield]

Created attachment 302808 [details]
Patch

[Darin Adler]

Comment on attachment 302808 [details]
Patch

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

Not done yet reviewing, here are my comments about the first part of the patch.

> Source/WebCore/platform/graphics/FontCache.h:232
> +    std::unique_ptr<FontPlatformData> createFontPlatformDataForTesting(const FontDescription& fontDescription, const AtomicString& family)
> +    {
> +        return createFontPlatformData(fontDescription, family, nullptr, nullptr);
> +    }

To keep class definitions readable, it’s often nice to have an inline function like this one declared here, but then defined after the class definition. Would you consider that in this case?

> Source/WebCore/platform/graphics/FontCache.h:247
> +    WEBCORE_EXPORT std::unique_ptr<FontPlatformData> createFontPlatformData(const FontDescription&, const AtomicString& family, const FontFeatureSettings* fontFaceFeatures, const FontVariantSettings* fontFaceVariantSettings);

Are the last two argument names needed here? Maybe fontFaceFeatures and fontFaceVariantSettings don’t add anything beyond what the type names already clarify.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:38
> +#define PLATFORM_FONT_LOOKUP ((PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101200) || PLATFORM(IOS))

I think this needs a longer more specific name. Like SHOULD_USE_CORE_TEXT_FONT_LOOKUP or something like that.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:798
>      return family.length() >= 1 && family[0] == '.';

Given how AtomicString's operator[] works, there is no need for the length check in this function. It’s redundant.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:814
> +    FontDatabase(const FontDatabase&) = delete;
> +    FontDatabase(FontDatabase&&) = delete;
> +    FontDatabase& operator=(const FontDatabase&) = delete;
> +    FontDatabase& operator=(FontDatabase&&) = delete;

We don’t need to delete all four of these. If we delete the copy constructor then the move constructor is automatically deleted.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:820
> +            : minimum(1)
> +            , maximum(0)

I think you should initialize these where they are defined rather than here.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:861
> +        InstalledFont()
> +        {
> +        }

I think it is more elegant to write:

    InstalledFont() = default;

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:866
> +            auto traits = adoptCF(static_cast<CFDictionaryRef>(CTFontDescriptorCopyAttribute(fontDescriptor, kCTFontTraitsAttribute)));

What guarantees fontDescriptor is non-null?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:873
> +            this->stretch = Range(width, width); // FIXME: Normalize this.

What exactly does "normalize" mean here?

No need for "this->".

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:876
> +            uint32_t symbolicTraits;

The correct type to use with kCFNumberSInt32Type is SInt32 rather than uint32_t. Since the code doesn’t depend on the use of the unsigned type, I suggest using the type that more directly matches the CFNumberGetValue contract.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:877
> +            auto success = CFNumberGetValue(symbolicTraitsNumber, kCFNumberSInt32Type, &symbolicTraits);

What guarantees symbolicTraitsNumber is non-null?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:884
> +            success = CFNumberGetValue(weightNumber.get(), kCFNumberFloatType, &weight);

What guarantees weightNumber is non-null?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:898
> +        InstalledFontCollection()
> +        {
> +        }

This isn’t needed. If we omit it, it will be generated.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:900
> +        void insertInstalledFont(InstalledFont&& installedFont)

The word “insert” here is a bit unusual. Maybe it should be “add” instead?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:924
> +    const InstalledFontCollection& lookupFamilyName(const String& familyName)

The verb "look up" is made of two separate words, not a single word "lookup".

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:927
> +        auto iterator = m_familyNameToFontDescriptors.find(folded);

A cleaner way to write a function like this is with the ensure function, like this:

    return m_familyNameToFontDescriptors.ensure(folded, [&] {
        auto familyNameString = folded.createCFString();
        ...
        InstalledFontCollection collection;
        ...
        return collection;
    }).iterator->value;

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:935
> +        auto matches = adoptCF(CTFontDescriptorCreateMatchingFontDescriptors(fontDescriptorToMatch.get(), nullptr));

Slightly nicer to define this inside the if ().

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:940
> +            auto count = CFArrayGetCount(matches.get());
> +            for (CFIndex i = 0; i < count; ++i)
> +                addResult.iterator->value.insertInstalledFont(InstalledFont(static_cast<CTFontDescriptorRef>(CFArrayGetValueAtIndex(matches.get(), i))));

There’s a missed opportunity here to correctly size the installedFonts vector using reserveInitialCapacity and uncheckedAppend. We should consider changing InstalledFontCollection to work a different way. We don’t need to modify an InstalledFontCollection after creating it, so I suggest a design where it has a constructor that takes a Vector<InstalledFont>&&, moves the vector in, and iterates it to compute the three bounds ranges. Then this function would build the Vector<InstalledFont> directly rather than calling a function. That’s nice because we don’t need an insertInstalledFont function at all; it’s just the constructor.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:948
> +        auto iterator = m_postScriptNameToFontDescriptors.find(folded);

Same comment as above about using "ensure".

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:972
> +    FontDatabase()
> +    {
> +    }

More elegant to write = default here.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:974
> +    HashMap<String, InstalledFontCollection> m_familyNameToFontDescriptors;

Is it correct to do full Unicode case folding here rather than ASCII case folding? I think we probably only want ASCII case folding. I suggest using ASCIICaseInsensitiveHash instead of calling foldCase.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:975
> +    HashMap<String, InstalledFont> m_postScriptNameToFontDescriptors;

Ditto.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:990
> +    return { };

I think we prefer to write std::nullopt rather than { }, despite how often I prefer { } in other contexts.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:998
> +        return { };

Ditto.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1002
> +static inline std::optional<float> findClosestStretch(float targetStretch, const FontDatabase::InstalledFontCollection&, const Vector<bool>&)

I don’t understand why this function exists with no body.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1004
> +    UNUSED_PARAM(targetStretch);

I suggest omitting the argument name rather than using UNUSED_PARAM.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1011
> +    UNUSED_PARAM(target);
> +    UNUSED_PARAM(filter);

I suggest omitting the argument names rather than using UNUSED_PARAM.

[Myles C. Maxfield]

Created attachment 302886 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 302808 [details]
Patch

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

>> Source/WebCore/platform/graphics/FontCache.h:247
>> +    WEBCORE_EXPORT std::unique_ptr<FontPlatformData> createFontPlatformData(const FontDescription&, const AtomicString& family, const FontFeatureSettings* fontFaceFeatures, const FontVariantSettings* fontFaceVariantSettings);
> 
> Are the last two argument names needed here? Maybe fontFaceFeatures and fontFaceVariantSettings don’t add anything beyond what the type names already clarify.

I think yes, because features and variants can be supplied in the FontDescription but also in the @font-face block. These variables are specifically for the ones that come from @font-face.

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:866
>> +            auto traits = adoptCF(static_cast<CFDictionaryRef>(CTFontDescriptorCopyAttribute(fontDescriptor, kCTFontTraitsAttribute)));
> 
> What guarantees fontDescriptor is non-null?

The API contract for CTFontDescriptorCreateMatchingFontDescriptors() means that fontDescriptor shouldn't ever be null. I've added an ASSERT().

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:873
>> +            this->stretch = Range(width, width); // FIXME: Normalize this.
> 
> What exactly does "normalize" mean here?
> 
> No need for "this->".

CoreText reports font widths as a floating point number between -1 and 1. However, CSS operates on width percentages between 50% and 200%. When I implement font-stretch, I'll have to convert between one and the other.

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:974
>> +    HashMap<String, InstalledFontCollection> m_familyNameToFontDescriptors;
> 
> Is it correct to do full Unicode case folding here rather than ASCII case folding? I think we probably only want ASCII case folding. I suggest using ASCIICaseInsensitiveHash instead of calling foldCase.

Unfortunately, the CSS fonts spec explicitly mentions full unicode case folding. https://drafts.csswg.org/css-fonts-4/#font-family-casing

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1002
>> +static inline std::optional<float> findClosestStretch(float targetStretch, const FontDatabase::InstalledFontCollection&, const Vector<bool>&)
> 
> I don’t understand why this function exists with no body.

This is the stub which I will fill in when I implement font-stretch (which I will do very soon).

[Myles C. Maxfield]

This patch represents a 27% performance progression.

[Myles C. Maxfield]

Created attachment 302924 [details]
Patch

[Jon Lee]

Comment on attachment 302924 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:869
> +                    slant = symbolicTraits & kCTFontTraitItalic ? 20 : 0;

Where does the "20" come from?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1015
> +    if (targetStyle >= 20) {

Can we define constants for these?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1025
> +            return targetStyle - font.style.maximum + thresholdDistance;

Is there a reason for unneeded math? |thresholdDistance| contains a "- targetStyle" that cancels out this |targetStyle|.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1038
> +                return font.style.minimum - targetStyle + thresholdDistance;

Ditto. This is just font.style.minimum.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1040
> +            return 0 - font.style.maximum + thresholdDistance2;

0?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1053
> +                return targetStyle - font.style.minimum + thresholdDistance;

Ditto. This is just -font.style.minimum.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1068
> +            return font.style.minimum - targetStyle + thresholdDistance;

Ditto.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1138
> +            return font.weight.minimum - targetWeight + thresholdDistance;

Ditto.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1150
> +            return targetWeight - font.weight.maximum + thresholdDistance;

Ditto.

[Simon Fraser (smfr)]

(In reply to comment #20)
> This patch represents a 27% performance progression.

In what test?

[Myles C. Maxfield]

Created attachment 302950 [details]
Patch

[Myles C. Maxfield]

(In reply to comment #23)
> (In reply to comment #20)
> > This patch represents a 27% performance progression.
> 
> In what test?

It's in the ChangeLog.

[Simon Fraser (smfr)]

Comment on attachment 302924 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:843
> +    struct InstalledFont {

"installed" is a bit vague. Installed in what?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:859
> +                    // FIXME: Normalize this from Core Text's [-1, 1] range to CSS's [50%, 200%] range.

Do you need to fix this FIXME? Seems pretty simple.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:890
> +    struct InstalledFontCollection {

Is this always a set of fonts sharing a name? Maybe the name can reflect that more strongly?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1091
> +    

Blank line.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1287
> +    if (!isMainThread()) {
> +        callOnMainThread([] {

Does this do the right thing in iOS WK1 (with the web thread)?

[Myles C. Maxfield]

Comment on attachment 302924 [details]
Patch

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

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:843
>> +    struct InstalledFont {
> 
> "installed" is a bit vague. Installed in what?

Installed on the machine. This is a term from the spec.

This term is used as a distinction from "system font" which would be San Francisco on macOS/iOS.

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:859
>> +                    // FIXME: Normalize this from Core Text's [-1, 1] range to CSS's [50%, 200%] range.
> 
> Do you need to fix this FIXME? Seems pretty simple.

The mapping is nontrivial, and we probably need CoreText SPI to do it correctly.

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:869
>> +                    slant = symbolicTraits & kCTFontTraitItalic ? 20 : 0;
> 
> Where does the "20" come from?

From the spec. Fonts with slant angles >= 20 are considered to be italic/oblique.

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:890
>> +    struct InstalledFontCollection {
> 
> Is this always a set of fonts sharing a name? Maybe the name can reflect that more strongly?

How about "InstalledFontFamily"?

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1287
>> +        callOnMainThread([] {
> 
> Does this do the right thing in iOS WK1 (with the web thread)?

There is no modification to this piece - this function just moved down in the file so it could call FontDatabase::singleton().clear().

[Myles C. Maxfield]

Created attachment 302962 [details]
Patch

[Dave Hyatt]

Comment on attachment 302962 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 302962 [details]
Patch

Clearing flags on attachment: 302962

Committed r213163: <http://trac.webkit.org/changeset/213163>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

This change appears to have caused two API test timeouts on Sierra bots:

Tests that timed out:
  FontCacheTest.FontLookupFromFamilyName
  FontCacheTest.FontLookupFromPostScriptName

https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK1%20%28Tests%29/builds/4052

[Ryan Haddad]

It also seems to have caused the Sierra 32-bit build to fail:

https://build.webkit.org/builders/Apple%20Sierra%20Release%20%2832-bit%20Build%29/builds/4102

[Darin Adler]

Comment on attachment 302886 [details]
Patch

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

You already landed this, but I have some comments.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:906
> +            for (auto& font : this->installedFonts)

No need for the "this->" here. I believe the code would work fine without it.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:933
> +    const InstalledFontCollection& lookUpFamilyName(const String& familyName)

Seems to me this should have a different name. Maybe collectionForFamily.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:937
> +            auto familyNameString = folded.createCFString();

Why is it better to use "folded" here rather than familyName?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:956
> +    const InstalledFont& lookupPostScriptName(const AtomicString& postScriptName)

Same problem with "lookup" here as with the function above.

This should probably be named fontForPostScriptName.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:960
> +            auto postScriptNameString = folded.createCFString();

Why is it better to use "folded" here rather than postScriptName?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1025
> +    if (targetStyle >= 20) {

This magic number of 20 seems a bit mysterious.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1052
> +    } else if (targetStyle > -20) {

Magic number of -20 also mysterious.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1093
> +        return { };

std::nullopt

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1114
> +    {

Need comments explaining why 400 and 500 are special, explaining the algorithm here.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1118
> +            return { };

std::nullopt

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1123
> +            return { };

std::nullopt

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1127
> +                return result.value();

Should just do "return result", no reason to convert to a non-optional and back to an optional.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1129
> +                return result.value();

Ditto.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1132
> +                return result.value();

Ditto.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1134
> +                return result.value();

Ditto.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1175
> +        return { };

std::nullopt

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1225
> +    Vector<bool> filter(familyFonts.size(), true);

If this never needs to be resized, then it should probably be a std::unique_ptr containing an array instead:

    std::unique_ptr<bool[]> filter { new bool[familyFonts.size()] };

I don't see any good reason to use a Vector, since the main features of Vector as resizability and ability to allocate up to a fixed size on the stack.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1227
> +    float targetStretch = 0;

This code seems a bit strange. Why a local variable that just holds zero? Maybe a FIXME missing?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1233
> +    float targetStyle = requestedTraits & kCTFontTraitItalic ? 20 : 0;

Magic number of 20 here is non-obvious as above.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1250
> +    auto findFont = [&](const FontDatabase::InstalledFont& font, size_t) -> std::optional<std::reference_wrapper<const FontDatabase::InstalledFont>> {
> +        return std::reference_wrapper<const FontDatabase::InstalledFont>(font);
> +    };
> +    if (const auto& result = iterateActiveFontsWithReturn<std::reference_wrapper<const FontDatabase::InstalledFont>>(familyFonts, filter, findFont))
> +        return &result.value().get();
> +    return nullptr;

Seems like this should use a pointer rather than a reference. Also no need to capture anything:

    auto findFont = [](const FontDatabase::InstalledFont& font, size_t) -> std::optional<const FontDatabase::InstalledFont*> {
        return &font;
    };
    return iterateActiveFontsWithReturn<const FontDatabase::InstalledFont*>(familyFonts, filter, findFont).value_or(nullptr);

Might also try to see if it compiles without declaring the lambda result type. Might just all work automatically.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1269
> +        if (((requestedTraits & kCTFontTraitItalic) && postScriptFont.style.maximum < 20) || (weight >= FontWeight600 && postScriptFont.weight.maximum < 600)) {

More magic numbers without an explanation of what the concept of the algorithm is.

[Myles C. Maxfield]

Committed r213204: <http://trac.webkit.org/changeset/213204>

[Myles C. Maxfield]

Comment on attachment 302886 [details]
Patch

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

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:906
>> +            for (auto& font : this->installedFonts)
> 
> No need for the "this->" here. I believe the code would work fine without it.

Really? installedFonts will refer to the argument, not the member variable, and the initializer will move from the argument before this is run. installedFonts will be empty if referred to here.

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:937
>> +            auto familyNameString = folded.createCFString();
> 
> Why is it better to use "folded" here rather than familyName?

There's no difference. The CoreText call accepting this string is case insensitive.

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1025
>> +    if (targetStyle >= 20) {
> 
> This magic number of 20 seems a bit mysterious.

Jon had the same comment, and I pulled the literal out into a constant.