Bug 213332 – [Cocoa] Unify "font:" CSS shorthand values between macOS and iOS family

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[Cocoa] Bring text-style fonts to macOS

[Myles C. Maxfield]

Created attachment 402188 [details]
WIP

[Radar WebKit Bug Importer]

<rdar://problem/64479189>

[Myles C. Maxfield]

The fonts exist on all supported versions of macOS (back to at least Mojave). We might as well hook them up.

[Myles C. Maxfield]

Created attachment 402189 [details]
WIP

[Myles C. Maxfield]

Created attachment 402227 [details]
Patch

[Darin Adler]

Comment on attachment 402227 [details]
Patch

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

Various comments about moved code that aren’t new.

r=me assuming all the tests pass

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1278
> +        const auto& request = fontDescription.fontSelectionRequest();

Don’t need the explicit const here, and I would suggest leaving it out. It’s going to be a const& whether we specify that or not.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1280
> +        RetainPtr<CFStringRef> familyNameStr = family.string().createCFString();

I suggest auto

Also not sure why we chose to use a local variable for the family but not for the locale. Maybe best to do it for neither?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1281
> +        RetainPtr<CTFontDescriptorRef> fontDescriptor = adoptCF(CTFontDescriptorCreateWithTextStyle(familyNameStr.get(), RenderThemeCocoa::singleton().contentSizeCategory(), fontDescription.computedLocale().string().createCFString().get()));

I suggest auto.

I think descriptor would be a good name. I don’t think we need the two word name fontDescriptor for something with such a small scope.

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:84
>      static auto strings { makeNeverDestroyed(convertArray<AtomString>(styles)) };

How long would the list need to be for us to get value from sorting this so we can use std::binary_search instead of std::find?

> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:85
> +    auto fontDescriptor = adoptCF(CTFontDescriptorCreateWithTextStyle(parameters.fontName.string().createCFString().get(), RenderThemeCocoa::singleton().contentSizeCategory(), parameters.locale.string().createCFString().get()));

I think having this be just the word descriptor would be better.

Also, this seems identical to the code for "UICTFontTextStyle"; could we find a way to share the code?

> Source/WebCore/rendering/RenderThemeCocoa.h:39
> +    virtual CFStringRef contentSizeCategory() const = 0;

This is used in the class’s implementation, so I think it can and should be private rather than public.

> Source/WebCore/rendering/RenderThemeCocoa.mm:42
> +#define PlatformFont NSFont
> +#define PlatformFontClass NSFont

Do these need to be macros? The word "platform" is so ugly for such things. Maybe CocoaFont and "inline Class cocoaFontClass();"

Also, if we use auto, I think we don’t need PlatformFont/CocoaFont at all.

> Source/WebCore/rendering/RenderThemeCocoa.mm:197
> +    static NeverDestroyed<FontCascadeDescription> systemFont;
> +    static NeverDestroyed<FontCascadeDescription> headlineFont;
> +    static NeverDestroyed<FontCascadeDescription> bodyFont;
> +    static NeverDestroyed<FontCascadeDescription> subheadlineFont;
> +    static NeverDestroyed<FontCascadeDescription> footnoteFont;
> +    static NeverDestroyed<FontCascadeDescription> caption1Font;
> +    static NeverDestroyed<FontCascadeDescription> caption2Font;
> +    static NeverDestroyed<FontCascadeDescription> shortHeadlineFont;
> +    static NeverDestroyed<FontCascadeDescription> shortBodyFont;
> +    static NeverDestroyed<FontCascadeDescription> shortSubheadlineFont;
> +    static NeverDestroyed<FontCascadeDescription> shortFootnoteFont;
> +    static NeverDestroyed<FontCascadeDescription> shortCaption1Font;
> +    static NeverDestroyed<FontCascadeDescription> tallBodyFont;
> +    static NeverDestroyed<FontCascadeDescription> title0Font;
> +    static NeverDestroyed<FontCascadeDescription> title1Font;
> +    static NeverDestroyed<FontCascadeDescription> title2Font;
> +    static NeverDestroyed<FontCascadeDescription> title3Font;
> +    static NeverDestroyed<FontCascadeDescription> title4Font;

Maybe use a struct for this instead of lots of separate locals. Would be nice to find a way to get rid of the repetitive calls to setIsAbsoluteSize, which we might be able to do with a struct/tuple hybrid approach. Not sure how much it would improve things.

> Source/WebCore/rendering/RenderThemeCocoa.mm:202
> +    static CFStringRef userTextSize = contentSizeCategory();
> +
> +    if (userTextSize != contentSizeCategory()) {
> +        userTextSize = contentSizeCategory();

I don’t understand this code at all.

> Source/WebCore/rendering/RenderThemeCocoa.mm:262
> +    RetainPtr<CFDictionaryRef> traits = adoptCF(CTFontCopyTraits(font));

I suggest auto here.

> Source/WebCore/rendering/RenderThemeCocoa.mm:267
> +    static float weightThresholds[] = { -0.6, -0.365, -0.115, 0.130, 0.235, 0.350, 0.5, 0.7 };

Should use const or constexpr here.

> Source/WebCore/rendering/RenderThemeIOS.h:53
> +    CFStringRef contentSizeCategory() const override;

final

> Source/WebCore/rendering/RenderThemeMac.h:44
> +    CFStringRef contentSizeCategory() const override;

final

> LayoutTests/TestExpectations:4336
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-body.html [ ImageOnlyFailure ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-caption1.html [ ImageOnlyFailure ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-caption2.html [ ImageOnlyFailure ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-footnote.html [ ImageOnlyFailure ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-headline.html [ ImageOnlyFailure ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-short-body.html [ ImageOnlyFailure ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-short-caption1.html [ ImageOnlyFailure ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-short-footnote.html [ ImageOnlyFailure ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-short-headline.html [ ImageOnlyFailure ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-short-subheadline.html [ ImageOnlyFailure ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-subheadline.html [ ImageOnlyFailure ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-tall-body.html [ ImageOnlyFailure ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-title0.html [ ImageOnlyFailure ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-title1.html [ ImageOnlyFailure ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-title2.html [ ImageOnlyFailure ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-title3.html [ ImageOnlyFailure ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-title4.html [ ImageOnlyFailure ]

Maybe put these into a directory so we don’t have to list all the individual tests?

Why is there a bug for this? Is there some reason that other ports *should* some day support "apple" prefixed keywords?

> LayoutTests/platform/ios/TestExpectations:3512
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-body.html [ Pass ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-caption1.html [ Pass ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-caption2.html [ Pass ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-footnote.html [ Pass ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-headline.html [ Pass ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-short-body.html [ Pass ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-short-caption1.html [ Pass ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-short-footnote.html [ Pass ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-short-headline.html [ Pass ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-short-subheadline.html [ Pass ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-subheadline.html [ Pass ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-tall-body.html [ Pass ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-title0.html [ Pass ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-title1.html [ Pass ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-title2.html [ Pass ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-title3.html [ Pass ]
> +webkit.org/b/213332 fast/text/text-styles/-apple-system-title4.html [ Pass ]

Why is there a bug listed here? I think that Pass expectations don’t need a bug number.

[Myles C. Maxfield]

Created attachment 402256 [details]
Benchmark results

[Myles C. Maxfield]

(In reply to Darin Adler from comment #6)
> Comment on attachment 402227 [details]
> > Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:84
> >      static auto strings { makeNeverDestroyed(convertArray<AtomString>(styles)) };
> 
> How long would the list need to be for us to get value from sorting this so
> we can use std::binary_search instead of std::find?

I made a microbenchmark, it's attached as an image to this bug "Benchmark results." It says that std::find is better here (which makes sense, because these are AtomStrings, where == is basically free, but < isn't)

[Myles C. Maxfield]

(In reply to Darin Adler from comment #6)
> Comment on attachment 402227 [details]
> > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1278
> > +        const auto& request = fontDescription.fontSelectionRequest();
> 
> Don’t need the explicit const here, and I would suggest leaving it out. It’s
> going to be a const& whether we specify that or not.

There's a compiler error if it's not there.

> > Source/WebCore/rendering/RenderThemeCocoa.h:39
> > +    virtual CFStringRef contentSizeCategory() const = 0;
> 
> This is used in the class’s implementation, so I think it can and should be
> private rather than public.

It's used in more places than the class's implementation.

> 
> > Source/WebCore/rendering/RenderThemeCocoa.mm:42
> > +#define PlatformFont NSFont
> > +#define PlatformFontClass NSFont
> 
> Do these need to be macros? The word "platform" is so ugly for such things.
> Maybe CocoaFont and "inline Class cocoaFontClass();"
> 
> Also, if we use auto, I think we don’t need PlatformFont/CocoaFont at all.

I can use a lambda to have the return type of the function actually be the right type (auto lambdas are awesome!!).

> 
> > Source/WebCore/rendering/RenderThemeCocoa.mm:197
> > +    static NeverDestroyed<FontCascadeDescription> systemFont;
> > +    static NeverDestroyed<FontCascadeDescription> headlineFont;
> > +    static NeverDestroyed<FontCascadeDescription> bodyFont;
> > +    static NeverDestroyed<FontCascadeDescription> subheadlineFont;
> > +    static NeverDestroyed<FontCascadeDescription> footnoteFont;
> > +    static NeverDestroyed<FontCascadeDescription> caption1Font;
> > +    static NeverDestroyed<FontCascadeDescription> caption2Font;
> > +    static NeverDestroyed<FontCascadeDescription> shortHeadlineFont;
> > +    static NeverDestroyed<FontCascadeDescription> shortBodyFont;
> > +    static NeverDestroyed<FontCascadeDescription> shortSubheadlineFont;
> > +    static NeverDestroyed<FontCascadeDescription> shortFootnoteFont;
> > +    static NeverDestroyed<FontCascadeDescription> shortCaption1Font;
> > +    static NeverDestroyed<FontCascadeDescription> tallBodyFont;
> > +    static NeverDestroyed<FontCascadeDescription> title0Font;
> > +    static NeverDestroyed<FontCascadeDescription> title1Font;
> > +    static NeverDestroyed<FontCascadeDescription> title2Font;
> > +    static NeverDestroyed<FontCascadeDescription> title3Font;
> > +    static NeverDestroyed<FontCascadeDescription> title4Font;
> 
> Maybe use a struct for this instead of lots of separate locals. Would be
> nice to find a way to get rid of the repetitive calls to setIsAbsoluteSize,
> which we might be able to do with a struct/tuple hybrid approach. Not sure
> how much it would improve things.

I think it would not improve things. There would be extra machinery to read and understand, and I'm not sure it would actually make the code smaller or more readable. This existing code is verbose, but simpler. Also, inserting a "fontCascadeDescription." everywhere below where these variables are used would negate the benefit of collapsing the setIsAbsoluteSize calls.

> 
> > Source/WebCore/rendering/RenderThemeCocoa.mm:202
> > +    static CFStringRef userTextSize = contentSizeCategory();
> > +
> > +    if (userTextSize != contentSizeCategory()) {
> > +        userTextSize = contentSizeCategory();
> 
> I don’t understand this code at all.

userTextSize is static, so this is checking if the return of contentSizeCategory() has changed since the last time this was called. If it has, it resets all the cached values, which flags that they need to be updated.

[Myles C. Maxfield]

Created attachment 402259 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 402260 [details]
Patch for committing

[EWS]

Committed r263255: <https://trac.webkit.org/changeset/263255>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402260 [details].

[Darin Adler]

Comment on attachment 402260 [details]
Patch for committing

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

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1278
> +        const auto& request = fontDescription.fontSelectionRequest();

Probably should just be "auto". Saying "const auto&" here implies that this is a reference to something that already exists, but I think this is really just a reference to a temporary. Code generated would be identical with plain "auto".

> Source/WebCore/rendering/RenderThemeCocoa.mm:176
> +    static NeverDestroyed<FontCascadeDescription> systemFont;

Looks like this is an unused global.

> Source/WebCore/rendering/RenderThemeCocoa.mm:177
> +    static NeverDestroyed<FontCascadeDescription> headlineFont;

Still convinced that this repetitive style is leading to mistakes.

> Source/WebCore/rendering/RenderThemeCocoa.mm:197
> +    if (userTextSize != contentSizeCategory()) {

Peculiar that we compare two CFStringRef with == rather than string equality. Presumably OK here because the contentSizeCategory always returns special constant CFStringRef values, but not obvious to the reader. I would have commented.

> Source/WebCore/rendering/RenderThemeCocoa.mm:200
> +        headlineFont.get().setIsAbsoluteSize(false);

Still don’t understand why setIsAbsoluteSize(false) is what we need to to to correctly "reset" when the content size category changes. Unclear and oblique.

Code seems to say: "If the size category changes, clear this seemingly-unrelated flag on these font descriptions."

> Source/WebCore/rendering/RenderThemeCocoa.mm:211
> +        tallBodyFont.get().setIsAbsoluteSize(false);

Why is it correct/OK that title[0-4]Font do *not* get reset here? I think it’s probably an error. If it’s correct, is the reason obvious without a comment?

> Source/WebCore/rendering/RenderThemeCocoa.mm:263
> +    static constexpr const float weightThresholds[] = { -0.6, -0.365, -0.115, 0.130, 0.235, 0.350, 0.5, 0.7 };

No need for "const" here. "constexpr" always implies const.

> Source/WebCore/rendering/RenderThemeCocoa.mm:407
> +    RetainPtr<CTFontRef> font = adoptCF(CTFontCreateWithFontDescriptor(fontDescriptor.get(), 0, nullptr));

Should use auto.

> Source/WebCore/rendering/RenderThemeIOS.mm:1393
> +    RetainPtr<CTFontDescriptorRef> fontDescriptor = adoptCF(CTFontDescriptorCreateWithTextStyle(kCTUIFontTextStyleShortFootnote, RenderThemeIOS::singleton().contentSizeCategory(), 0));

auto would make this better

> Source/WebCore/rendering/RenderThemeIOS.mm:1408
> +    RetainPtr<CTFontDescriptorRef> fontDescriptor = adoptCF(CTFontDescriptorCreateWithTextStyle(kCTUIFontTextStyleShortCaption1, RenderThemeIOS::singleton().contentSizeCategory(), 0));

auto would make this better

[Darin Adler]

(In reply to Myles C. Maxfield from comment #8)
> (In reply to Darin Adler from comment #6)
> > How long would the list need to be for us to get value from sorting this so
> > we can use std::binary_search instead of std::find?
> 
> I made a microbenchmark, it's attached as an image to this bug "Benchmark
> results." It says that std::find is better here (which makes sense, because
> these are AtomStrings, where == is basically free, but < isn't)

I realize now that the most straightforward way of writing a sort/binary_search ends up sorting the *strings* and binary searching based on the *strings*. I had intended a sort of the *pointers*, and a binary search based on the *pointers*. Which I think might be faster. When I wrote my comment I didn’t realize that would be a bit tricky to write; need to pass in a functor for "<" that compares the pointer values.

So what you benchmarked wasn’t actually what I was suggesting.

[Myles C. Maxfield]

(In reply to Darin Adler from comment #14)
> (In reply to Myles C. Maxfield from comment #8)
> > (In reply to Darin Adler from comment #6)
> > > How long would the list need to be for us to get value from sorting this so
> > > we can use std::binary_search instead of std::find?
> > 
> > I made a microbenchmark, it's attached as an image to this bug "Benchmark
> > results." It says that std::find is better here (which makes sense, because
> > these are AtomStrings, where == is basically free, but < isn't)
> 
> I realize now that the most straightforward way of writing a
> sort/binary_search ends up sorting the *strings* and binary searching based
> on the *strings*. I had intended a sort of the *pointers*, and a binary
> search based on the *pointers*. Which I think might be faster. When I wrote
> my comment I didn’t realize that would be a bit tricky to write; need to
> pass in a functor for "<" that compares the pointer values.
> 
> So what you benchmarked wasn’t actually what I was suggesting.

I measured again using the comparison function lhs.impl() < rhs.impl() and got new numbers. Looks like there is no difference between them (at least for the data sizes present in this corpus).

[Myles C. Maxfield]

Created attachment 408263 [details]
New benchmark results

[Darin Adler]

(In reply to Myles C. Maxfield from comment #15)
> > > (In reply to Darin Adler from comment #6)
> > > > How long would the list need to be for us to get value from sorting this so
> > > > we can use std::binary_search instead of std::find?
>
> I measured again using the comparison function lhs.impl() < rhs.impl() and
> got new numbers. Looks like there is no difference between them (at least
> for the data sizes present in this corpus).

There’s a reason I asked "how long would the list need to be". I’m surprised that there’s no benefit for binary search even when there are >600.

[Myles C. Maxfield]

Comment on attachment 402260 [details]
Patch for committing

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

>> Source/WebCore/rendering/RenderThemeCocoa.mm:177
>> +    static NeverDestroyed<FontCascadeDescription> headlineFont;
> 
> Still convinced that this repetitive style is leading to mistakes.

Indeed; you found a mistake below on line 211. This has convinced me to change my mind about this code.

>> Source/WebCore/rendering/RenderThemeCocoa.mm:200
>> +        headlineFont.get().setIsAbsoluteSize(false);
> 
> Still don’t understand why setIsAbsoluteSize(false) is what we need to to to correctly "reset" when the content size category changes. Unclear and oblique.
> 
> Code seems to say: "If the size category changes, clear this seemingly-unrelated flag on these font descriptions."

I looked through the code; I can't find where this field could ever get set to true. There's only one caller of this function, and it immediately copies the results out and discards the returned value. I don't think this is actually necessary.

[Myles C. Maxfield]

(In reply to Myles C. Maxfield from comment #18)
> Comment on attachment 402260 [details]
> Patch for committing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402260&action=review
> 
> >> Source/WebCore/rendering/RenderThemeCocoa.mm:177
> >> +    static NeverDestroyed<FontCascadeDescription> headlineFont;
> > 
> > Still convinced that this repetitive style is leading to mistakes.
> 
> Indeed; you found a mistake below on line 211. This has convinced me to
> change my mind about this code.
> 
> >> Source/WebCore/rendering/RenderThemeCocoa.mm:200
> >> +        headlineFont.get().setIsAbsoluteSize(false);
> > 
> > Still don’t understand why setIsAbsoluteSize(false) is what we need to to to correctly "reset" when the content size category changes. Unclear and oblique.
> > 
> > Code seems to say: "If the size category changes, clear this seemingly-unrelated flag on these font descriptions."
> 
> I looked through the code; I can't find where this field could ever get set
> to true. There's only one caller of this function, and it immediately copies
> the results out and discards the returned value. I don't think this is
> actually necessary.

I did all this stuff in https://bugs.webkit.org/show_bug.cgi?id=216293

[Myles C. Maxfield]

Created attachment 408298 [details]
Debugged benchmark results

After investigation, it turns out the benchmark results are bogus - apparently TimingScope has some static member variable that it uses for coalescing, which messed up all the data gathering. I've attached new (debugged!) results. The new results say that std::lower_bound() is pretty much always faster than std::find().

The "slow" lines on the graph measure doing code point comparison, and the other lines on the graph measure doing pointer comparison. The left graphs show array sizes of 1-100 while the right graphs show array sizes of 1-10,000. The Y-scales for all the graphs are different (the operations are too fast for Numbers to print useful Y values).

[Myles C. Maxfield]

I guess in order for it to be fair, we'd have to incorporate the cost of sorting the array, which has to be done at runtime because it would be sorted according to pointer value. Given the size of this array is only 17, sorting may actually affect the results.

[Darin Adler]

Sorting the array one time and keeping the results around, not sorting it every time.

[Darin Adler]

I suggest using std::binary_search instead of std::lower_bound.