Bug 132159 – [OS X] Use CTFontCreateForCSS instead of doing font search ourselves

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[OS X] Use CTFontCreateForCSS instead of doing font search ourselves

[Myles C. Maxfield]

Created attachment 230124 [details]
Patch

[Myles C. Maxfield]

Created attachment 230231 [details]
Patch

[Myles C. Maxfield]

Diff wasn't able to properly inspect LayoutTests/fast/table/frame-and-rules.html. the difference there is that I updated the test to not try to search for a font named "italic" but instead to search for "times"

[Myles C. Maxfield]

Created attachment 230232 [details]
Without frame-and-rules.html

[Enrica Casucci]

Comment on attachment 230232 [details]
Without frame-and-rules.html

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

I'm not fully sure about the validity of your solution. The code you removed in DRT was forcing the tests to use fonts for the list of supported appkit fonts, therefore making sure WebKit was using fallback for anything other than the fonts that were in the installed list. This guarantees consistent results regardless of the fonts that are installed on a particular system. I think you are losing this piece of functionality with your patch.

> Source/WebCore/ChangeLog:8
> +        This tests migrates to using a CoreText function to perform font lookup

Test? You probably meant patch.

[Myles C. Maxfield]

Yep. That's by design. Turns out that almost none of our tests rely on this behavior, and this patch fixes up those tests. 

Is that a deal breaker?

(In reply to comment #5)
> (From update of attachment 230232 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230232&action=review
> 
> I'm not fully sure about the validity of your solution. The code you removed in DRT was forcing the tests to use fonts for the list of supported appkit fonts, therefore making sure WebKit was using fallback for anything other than the fonts that were in the installed list. This guarantees consistent results regardless of the fonts that are installed on a particular system. I think you are losing this piece of functionality with your patch.
> 
> > Source/WebCore/ChangeLog:8
> > +        This tests migrates to using a CoreText function to perform font lookup
> 
> Test? You probably meant patch.

[Simon Fraser (smfr)]

Comment on attachment 230232 [details]
Without frame-and-rules.html

The "limit the available" fonts code in DRT was added for a reason: tests had different results on different machines because they were sensitive to the set of installed fonts. We should not regress this behavior.

[Myles C. Maxfield]

<rdar://problem/16718861>

[Myles C. Maxfield]

Putting this on the back burner for now, as the current NSFontManager path works (it's just a maintenance burden)

[Myles C. Maxfield]

Created attachment 236497 [details]
Patch

[Myles C. Maxfield]

Created attachment 239597 [details]
Patch

[Myles C. Maxfield]

Created attachment 241757 [details]
Patch

[Myles C. Maxfield]

CTFontCreateForCSS will never return a screen font. However, all our testing assumes that ::only:: screen fonts are used. I am going to migrate all our tests to using printer fonts. Once I do that, this patch can go in.

[Myles C. Maxfield]

Created attachment 250546 [details]
Patch

[Darin Adler]

Comment on attachment 250546 [details]
Patch

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

I see quite a few small mistakes; this also overuses sets. But generally looks fine, so r=me assuming you fix it.

> Source/WebCore/platform/graphics/FontCache.h:123
> +    WEBCORE_EXPORT static void setFontWhitelist(const HashSet<String>&);

Should take a Vector, not a HashSet.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:44
> +#import <wtf/HashSet.h>

Should not be needed since we are including this in FontCache.h. Except that we probably shouldn’t have it in FontCache.h any more.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:114
> +#endif

Should add a blank line before this to match above formatting.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:161
> +static HashSet<String>& fontWhitelist()
> +{
> +    static NeverDestroyed<HashSet<String>> whitelist;
> +    return whitelist;
> +}

We want this to be case insensitive, so we should do that, and use HashSet<String, CaseFoldingHash>, and not call lower() all the time before using the hash table. Avoids unnecessary memory allocation.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:163
> +void FontCache::setFontWhitelist(const HashSet<String>& whitelist)

This should take a vector of strings, not a HashSet. Or if it does take a HashSet then it should just do assignment, not a loop, but it would need to be HashSet<String, CaseFoldingHash>.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:166
> +    HashSet<String>& gWhitelist = fontWhitelist();

Should not use the "g" prefix here.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:178
> +    if (equalIgnoringCase(family, "-webkit-system-font")
> +        || equalIgnoringCase(family, "-apple-system-font")) {

Should merge this into a single line. Also would be good to be "ignoring ASCII case", rather than the general "ignoring Unicode case", if possible without ruining performance.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:199
> +#if ENABLE(PLATFORM_FONT_LOOKUP)

Should have a blank line after this to match the other formatting.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:200
> +    if (family.startsWith("."))

More efficient to just check the first character rather than calling this helper.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:202
> +    const HashSet<String>& whitelist = fontWhitelist();

Should use auto& for the type here.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:210
> +    font = (NSFont*)CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size);

Looks like this CTFont will leak. Need to write something more like this:

    font = (NSFont *)CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size));

But that won’t quite work so you need to either use RetainPtr<NSFont> and write it like that or do this:

    font = (NSFont *)[[CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size)) retain] autorelease];

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:224
>      NSFontManager *fontManager = [NSFontManager sharedFontManager];
>      NSString *availableFamily = cachedAvailableFamily;

Need to remove these two lines of code.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:296
> +#endif

Should have a blank line before this to match the other formatting.

> Source/WebKit/mac/WebView/WebView.mm:8820
> +    HashSet<String> wtfWhitelist;

I’d like to see:

    Vector<String> vector;

> Source/WebKit/mac/WebView/WebView.mm:8821
> +    for (NSString *nsString in whitelist)

Should just name this "string".

> Source/WebKit/mac/WebView/WebViewPrivate.h:1043
> ++ (void)_setFontWhitelist:(NSSet *)whitelist;

This should just take an NSArray of NSString, not an NSSet. It’s not helpful to have this be a set when it’s case insensitive strings anyway; the set won’t detect enough duplicates to be the correct data structure.

> Source/WebKit2/Shared/WebProcessCreationParameters.h:114
> +    HashSet<String> fontWhitelist;

Vector<String>

> Source/WebKit2/UIProcess/WebProcessPool.h:473
> +    HashSet<String> m_fontWhitelist;

Vector<String>

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:480
> +static NSSet *getFontWhitelist()

Should not have get in its name.

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:487
> +    NSMutableSet *availableFontList = [[NSMutableSet alloc] initWithCapacity:[allowedFamilies count] * 2];

Why bother with this capacity estimate?

> Tools/WebKitTestRunner/mac/TestControllerMac.mm:262
> +static WKArrayRef generateWhitelist()

Should return a smart pointer, not a raw pointer.

> Tools/WebKitTestRunner/mac/TestControllerMac.mm:266
> +        NSArray* fontsForFamily = [[NSFontManager sharedFontManager] availableMembersOfFontFamily:fontFamily];

Incorrect formatting for NSArray *

> Tools/WebKitTestRunner/mac/TestControllerMac.mm:268
> +        for (NSArray* fontInfo in fontsForFamily) {

Incorrect formatting for NSArray *

> Tools/WebKitTestRunner/mac/TestControllerMac.mm:293
> +    auto whitelist = adoptWK(generateWhitelist());

No need for this local variable; would read better in-line.

[Darin Adler]

(In reply to comment #13)
> CTFontCreateForCSS will never return a screen font. However, all our testing
> assumes that ::only:: screen fonts are used. I am going to migrate all our
> tests to using printer fonts. Once I do that, this patch can go in.

Why is that change OK? Don’t we have screen fonts for a reason? Do we really want to remove the use of them? If so, why not make that change first, independently, before switching to CTFontCreateForCSS?

[Myles C. Maxfield]

Wow, I accidentally marked this as r?. It is definitely NOT ready.

Sorry for wasting your time, Darin.

Regarding screen fonts, I did a considerable amount of work removing our use of them already (starting during winter break last year). We currently now never use screen fonts (which is good). This ChangeLog is from before that work; I haven't updated it.

[Sam Weinig]

(In reply to comment #16)
> (In reply to comment #13)
> > CTFontCreateForCSS will never return a screen font. However, all our testing
> > assumes that ::only:: screen fonts are used. I am going to migrate all our
> > tests to using printer fonts. Once I do that, this patch can go in.
> 
> Why is that change OK? Don’t we have screen fonts for a reason? 

We removed support for screen fonts in http://trac.webkit.org/changeset/179368.

[Darin Adler]

(In reply to comment #18)
> (In reply to comment #16)
> > (In reply to comment #13)
> > > CTFontCreateForCSS will never return a screen font. However, all our testing
> > > assumes that ::only:: screen fonts are used. I am going to migrate all our
> > > tests to using printer fonts. Once I do that, this patch can go in.
> > 
> > Why is that change OK? Don’t we have screen fonts for a reason? 
> 
> We removed support for screen fonts in
> http://trac.webkit.org/changeset/179368.

Can we fix the tests to always use printer fonts, then, as a separate step before landing this patch? Or is there some misunderstanding here.

[Myles C. Maxfield]

Looks like there is a misunderstanding. Screen fonts should never be used anywhere. I believe this is currently the case.

[Myles C. Maxfield]

Comment on attachment 250546 [details]
Patch

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

>> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:210
>> +    font = (NSFont*)CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size);
> 
> Looks like this CTFont will leak. Need to write something more like this:
> 
>     font = (NSFont *)CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size));
> 
> But that won’t quite work so you need to either use RetainPtr<NSFont> and write it like that or do this:
> 
>     font = (NSFont *)[[CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size)) retain] autorelease];

desiredWeight needs to be multiplied by 100 due to API contract

[Myles C. Maxfield]

Comment on attachment 250546 [details]
Patch

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

>>> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:210
>>> +    font = (NSFont*)CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size);
>> 
>> Looks like this CTFont will leak. Need to write something more like this:
>> 
>>     font = (NSFont *)CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size));
>> 
>> But that won’t quite work so you need to either use RetainPtr<NSFont> and write it like that or do this:
>> 
>>     font = (NSFont *)[[CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size)) retain] autorelease];
> 
> desiredWeight needs to be multiplied by 100 due to API contract

We need to not run toAppKitFontWeight() in this case.

[Myles C. Maxfield]

Created attachment 250738 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 250738 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:241
> +    font = (NSFont*)CTFontCreateForCSS((CFStringRef)desiredFamily, toCoreTextFontWeight(weight), requestedTraits, size);

Darin's comment about leaking

[Myles C. Maxfield]

Created attachment 250772 [details]
Patch

[Darin Adler]

Comment on attachment 250772 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:243
> +    font = (NSFont*)CTFontCreateForCSS((CFStringRef)desiredFamily, toCoreTextFontWeight(weight), requestedTraits, size);

Yes, as you said, this still contains a leak.

> Source/WebKit/mac/WebView/WebView.mm:8823
> +    WebCore::FontCache::setFontWhitelist(vector);

iOS compilation error: WebKit/mac/WebView/WebView.mm:8822:25: error: no type named 'setFontWhitelist' in 'WebCore::FontCache'

> Source/WebKit/mac/WebView/WebViewPrivate.h:1044
> +@interface WebView (WebViewFontSelection)
> ++ (void)_setFontWhitelist:(NSArray *)whitelist;
> +@end

The underlying feature is Mac-only, so maybe you want this to be Mac-only? Or maybe just let it do nothing on iOS?

[Myles C. Maxfield]

Comment on attachment 250772 [details]
Patch

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

>> Source/WebKit/mac/WebView/WebViewPrivate.h:1044
>> +@end
> 
> The underlying feature is Mac-only, so maybe you want this to be Mac-only? Or maybe just let it do nothing on iOS?

I intend for it to just do nothing on iOS

[Myles C. Maxfield]

Comment on attachment 250546 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:210
>>>> +    font = (NSFont*)CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size);
>>> 
>>> Looks like this CTFont will leak. Need to write something more like this:
>>> 
>>>     font = (NSFont *)CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size));
>>> 
>>> But that won’t quite work so you need to either use RetainPtr<NSFont> and write it like that or do this:
>>> 
>>>     font = (NSFont *)[[CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, desiredWeight, requestedTraits, size)) retain] autorelease];
>> 
>> desiredWeight needs to be multiplied by 100 due to API contract
> 
> We need to not run toAppKitFontWeight() in this case.

I feel like the name of the function should have been named CFBridgingAutorelease(), since by inspection it appears that the object gets released before it gets retained :(

Anyway, done.

[Myles C. Maxfield]

Created attachment 250891 [details]
Patch for committing

[Darin Adler]

Comment on attachment 250891 [details]
Patch for committing

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

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:243
> +    font = (NSFont*)[[CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, toCoreTextFontWeight(weight), requestedTraits, size)) retain] autorelease];

There is no need for the retain and autorelease here nor for the cast to NSFont* (which is formatted incorrectly, missing the space before the *). Look at all the other uses of CFBridgingRelease in WebCore and you will see the pattern. Should just be:

    font = CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, toCoreTextFontWeight(weight), requestedTraits, size));

[Myles C. Maxfield]

Comment on attachment 250891 [details]
Patch for committing

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

>> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:243
>> +    font = (NSFont*)[[CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, toCoreTextFontWeight(weight), requestedTraits, size)) retain] autorelease];
> 
> There is no need for the retain and autorelease here nor for the cast to NSFont* (which is formatted incorrectly, missing the space before the *). Look at all the other uses of CFBridgingRelease in WebCore and you will see the pattern. Should just be:
> 
>     font = CFBridgingRelease(CTFontCreateForCSS((CFStringRef)desiredFamily, toCoreTextFontWeight(weight), requestedTraits, size));

That makes more sense!

[Myles C. Maxfield]

Created attachment 250930 [details]
Patch for landing

[Myles C. Maxfield]

Created attachment 251915 [details]
Patch for landing

[Myles C. Maxfield]

Committed r183562: <http://trac.webkit.org/changeset/183562>