[webkit-reviews] review granted: [Bug 200241] Use CTFontCreateForCharactersWithLanguageAndOption if available instead of CTFontCreateForCharactersWithLanguage : [Attachment 375105] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted youenn fablet
<youennf at gmail.com>'s request for review:
Bug 200241: Use CTFontCreateForCharactersWithLanguageAndOption if available
instead of CTFontCreateForCharactersWithLanguage
https://bugs.webkit.org/show_bug.cgi?id=200241

Attachment 375105: Patch

https://bugs.webkit.org/attachment.cgi?id=375105&action=review




--- Comment #7 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 375105
  --> https://bugs.webkit.org/attachment.cgi?id=375105
Patch

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

I'm surprised we need to use new SPI to avoid this regression. I wonder if this
new SPI is fundamentally, philosophically required, or if we're just trying to
fix-up/work-around the patch in https://bugs.webkit.org/show_bug.cgi?id=199769.
At any rate, I don't want to stop progress, so I'll r+ this, but it is likely
that we can avoid the use of this new SPI by fixing
https://bugs.webkit.org/show_bug.cgi?id=199769 a different way. I've filed
https://bugs.webkit.org/show_bug.cgi?id=200273 for it.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1410
>  #else
> +#if HAVE(CTFONTCREATEFORCHARACTERSWITHLANGUAGE_AND_OPTION)

All the preprocessor macros are making this function super ugly :(

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1660
> +#if HAVE(CTFONTCREATEFORCHARACTERSWITHLANGUAGE_AND_OPTION)
> +		   auto fallbackWarmingFont =
adoptCF(CTFontCreateForCharactersWithLanguageAndOption(warmingFont.get(),
&character, 1, nullptr, kCTFontFallbackOptionSystem, &coveredLength));
> +#else

Is this actually necessary? We just throw the font away here. If we don't do
this, would our future use of CoreText's internal caches cause misses rather
than hits?