Bug 132159

Summary: [OS X] Use CTFontCreateForCSS instead of doing font search ourselves
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cmarcelo, commit-queue, darin, dino, enrica, jonlee, mitz, sam, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=145665
Attachments:
Description Flags
Patch
none
Patch
none
Without frame-and-rules.html
none
Patch
none
Patch
none
Patch
none
Patch
darin: review+
Patch
none
Patch
none
Patch for committing
none
Patch for landing
none
Patch for landing none

Description Myles C. Maxfield 2014-04-24 17:49:26 PDT
[OS X] Use CTFontCreateForCSS instead of doing font search ourselves
Comment 1 Myles C. Maxfield 2014-04-24 18:03:36 PDT
Created attachment 230124 [details]
Patch
Comment 2 Myles C. Maxfield 2014-04-25 19:37:45 PDT
Created attachment 230231 [details]
Patch
Comment 3 Myles C. Maxfield 2014-04-25 19:40:29 PDT
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"
Comment 4 Myles C. Maxfield 2014-04-25 19:45:17 PDT
Created attachment 230232 [details]
Without frame-and-rules.html
Comment 5 Enrica Casucci 2014-04-28 16:09:23 PDT
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.
Comment 6 Myles C. Maxfield 2014-04-28 16:32:55 PDT
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.
Comment 7 Simon Fraser (smfr) 2014-04-28 17:09:37 PDT
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.
Comment 8 Myles C. Maxfield 2014-04-28 18:26:40 PDT
<rdar://problem/16718861>
Comment 9 Myles C. Maxfield 2014-04-28 18:31:04 PDT
Putting this on the back burner for now, as the current NSFontManager path works (it's just a maintenance burden)
Comment 10 Myles C. Maxfield 2014-08-12 20:32:54 PDT
Created attachment 236497 [details]
Patch
Comment 11 Myles C. Maxfield 2014-10-09 20:58:21 PDT
Created attachment 239597 [details]
Patch
Comment 12 Myles C. Maxfield 2014-11-17 19:29:36 PST
Created attachment 241757 [details]
Patch
Comment 13 Myles C. Maxfield 2014-12-19 10:42:51 PST
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.
Comment 14 Myles C. Maxfield 2015-04-10 17:56:42 PDT
Created attachment 250546 [details]
Patch
Comment 15 Darin Adler 2015-04-10 19:45:34 PDT
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.
Comment 16 Darin Adler 2015-04-10 19:46:57 PDT
(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?
Comment 17 Myles C. Maxfield 2015-04-11 07:45:09 PDT
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.
Comment 18 Sam Weinig 2015-04-12 13:44:12 PDT
(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.
Comment 19 Darin Adler 2015-04-13 08:39:11 PDT
(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.
Comment 20 Myles C. Maxfield 2015-04-13 08:50:54 PDT
Looks like there is a misunderstanding. Screen fonts should never be used anywhere. I believe this is currently the case.
Comment 21 Myles C. Maxfield 2015-04-13 15:55:37 PDT
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
Comment 22 Myles C. Maxfield 2015-04-13 16:16:34 PDT
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.
Comment 23 Myles C. Maxfield 2015-04-14 14:08:32 PDT
Created attachment 250738 [details]
Patch
Comment 24 Myles C. Maxfield 2015-04-14 14:10:16 PDT
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
Comment 25 Myles C. Maxfield 2015-04-14 18:28:31 PDT
Created attachment 250772 [details]
Patch
Comment 26 Darin Adler 2015-04-15 09:41:55 PDT
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?
Comment 27 Myles C. Maxfield 2015-04-15 19:21:35 PDT
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
Comment 28 Myles C. Maxfield 2015-04-15 19:35:50 PDT
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.
Comment 29 Myles C. Maxfield 2015-04-15 19:42:47 PDT
Created attachment 250891 [details]
Patch for committing
Comment 30 Darin Adler 2015-04-16 09:37:45 PDT
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));
Comment 31 Myles C. Maxfield 2015-04-16 10:37:27 PDT
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!
Comment 32 Myles C. Maxfield 2015-04-16 10:58:19 PDT
Created attachment 250930 [details]
Patch for landing
Comment 33 Myles C. Maxfield 2015-04-28 18:59:27 PDT
Created attachment 251915 [details]
Patch for landing
Comment 34 Myles C. Maxfield 2015-04-29 11:32:53 PDT
Committed r183562: <http://trac.webkit.org/changeset/183562>