[OS X] Make checking if a font is the system font more robust
Created attachment 229925 [details] Patch
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?
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?
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.
Created attachment 229951 [details] Patch
First patch r+'ed by dino. Second patch simply addresses comments. http://trac.webkit.org/changeset/167713
Re-opened since this is blocked by bug 132070
Created attachment 230010 [details] Patch
Re-landed in http://trac.webkit.org/changeset/167724
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.
Yeah - I marked the bug as closed but forgot to clear the r?. Thanks for the review.