Bug 132030 – [OS X] Make checking if a font is the system font more robust

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

[Myles C. Maxfield]

[OS X] Make checking if a font is the system font more robust

[Myles C. Maxfield]

Created attachment 229925 [details]
Patch

[Dean Jackson]

Comment on attachment 229925 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/FontMac.mm:40
> +extern "C" bool CTFontDescriptorIsSystemUIFont(CTFontDescriptorRef);

Do we want to wrap this in a #if __MAC_MIN thing as well, to make it clear we only use it on 1090+?

> Source/WebCore/platform/graphics/mac/FontMac.mm:545
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED < 1090

So this existed on 10.9 as well?

[Tim Horton]

Comment on attachment 229925 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/FontMac.mm:37
> +#if defined(__has_include) && __has_include(<CoreText/CTFontDescriptorPriv.h>)

we don't need the defined() part

> Source/WebCore/platform/graphics/mac/FontMac.mm:548
> +    return firstFamily().startsWith('.');

does this actually make sense on any platform? were there dot-prefixed fonts in 10.8?

[Myles C. Maxfield]

Comment on attachment 229925 [details]
Patch

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

>> Source/WebCore/platform/graphics/mac/FontMac.mm:37
>> +#if defined(__has_include) && __has_include(<CoreText/CTFontDescriptorPriv.h>)
> 
> we don't need the defined() part

Done.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:40
>> +extern "C" bool CTFontDescriptorIsSystemUIFont(CTFontDescriptorRef);
> 
> Do we want to wrap this in a #if __MAC_MIN thing as well, to make it clear we only use it on 1090+?

I actually feel like this would do more harm than good. It would add more preprocessor bloat, and wouldn't actually be very valuable. There are many places in WebKit where functions are declared, yet never used or defined. Therefore, seeing a function declaration already doesn't necessarily mean that we ever use it.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:545
>> +#if __MAC_OS_X_VERSION_MIN_REQUIRED < 1090
> 
> So this existed on 10.9 as well?

Yep!

>> Source/WebCore/platform/graphics/mac/FontMac.mm:548
>> +    return firstFamily().startsWith('.');
> 
> does this actually make sense on any platform? were there dot-prefixed fonts in 10.8?

Conveniently, the only platforms with the dot prefix are also platforms which have the CTFontDescriptorIsSystemUIFont() call :) Done.

[Myles C. Maxfield]

Created attachment 229951 [details]
Patch

[Myles C. Maxfield]

First patch r+'ed by dino. Second patch simply addresses comments.

http://trac.webkit.org/changeset/167713

[WebKit Commit Bot]

Re-opened since this is blocked by bug 132070

[Myles C. Maxfield]

Created attachment 230010 [details]
Patch

[Myles C. Maxfield]

Re-landed in http://trac.webkit.org/changeset/167724

[Darin Adler]

Comment on attachment 230010 [details]
Patch

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

I think you already landed this, but it was still marked review?

> Source/WebCore/platform/graphics/mac/FontMac.mm:546
> +    RetainPtr<CTFontDescriptorRef> descriptor = CTFontCopyFontDescriptor(primaryFont()->platformData().ctFont());

This needs an adoptCF, otherwise it will be a storage leak.

[Myles C. Maxfield]

Yeah - I marked the bug as closed but forgot to clear the r?. Thanks for the review.