Bug 200241 – Use CTFontCreateForCharactersWithLanguageAndOption if available instead of CTFontCreateForCharactersWithLanguage

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[youenn fablet]

Use CTFontCreateForCharactersWithLanguageAndOption if available instead of CTFontCreateForCharactersWithLanguage.
This will make things a bit faster.

[youenn fablet]

Created attachment 375102 [details]
Patch

[EWS Watchlist]

Attachment 375102 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/pal/spi/cocoa/CoreTextSPI.h:135:  The parameter name "option" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 375105 [details]
Patch

[youenn fablet]

<rdar://problem/53495386>

[EWS Watchlist]

Attachment 375105 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/pal/spi/cocoa/CoreTextSPI.h:135:  The parameter name "option" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

https://perf-safari.apple.com/v3/#/analysis/task/6663

[Myles C. Maxfield]

Comment on attachment 375105 [details]
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?

[Geoffrey Garen]

Comment on attachment 375105 [details]
Patch

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

>> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1410
>> +#if HAVE(CTFONTCREATEFORCHARACTERSWITHLANGUAGE_AND_OPTION)
> 
> All the preprocessor macros are making this function super ugly :(

One way to help with preprocessor goo is to move the conditional behavior into a helper function, and define multiple versions of that helper function, one per preprocessor configuration.

In this case, I think I would create a helper function named "createCTFont", with three different implementations.

[Per Arne Vollan]

Comment on attachment 375105 [details]
Patch

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

Is it possible to avoid the copy of the font descriptor in the InstalledFont constructor? Should we stop using kCTFontFallbackOptionAttribute when using CTFontCreateForCharactersWithLanguageAndOption?

> Source/WTF/wtf/Platform.h:1544
> +#define HAVE_CTFONTCREATEFORCHARACTERSWITHLANGUAGE_AND_OPTION 1

Nit: maybe avoid using underscores in the define since the function does not have them?

[youenn fablet]

(In reply to Myles C. Maxfield from comment #7)
> Comment on attachment 375105 [details]
> 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.

I envisioned various alternative approaches, the most promising one being to only wrap a font if needed before calling CTFontCreateForCharactersWithLanguage.

All in all, the current approach seems like the simplest to me.
When we can only rely on CTFontCreateForCharactersWithLanguageAndOption, we will be able to clean-up everything and no longer care about setting the font fallback attribute anywhere.

> > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1410
> >  #else
> > +#if HAVE(CTFONTCREATEFORCHARACTERSWITHLANGUAGE_AND_OPTION)
> 
> All the preprocessor macros are making this function super ugly :(

Will fix it.

> > 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?

It seems more appropriate to do that in case we are trying to prewarm a font whose space character is fallbacking to another font.

> In this case, I think I would create a helper function named "createCTFont",
> with three different implementations.

Will do.

> Is it possible to avoid the copy of the font descriptor in the InstalledFont
> constructor? Should we stop using kCTFontFallbackOptionAttribute when using
> CTFontCreateForCharactersWithLanguageAndOption?

I think this is doable in case HAVE_CTFONTCREATEFORCHARACTERSWITHLANGUAGE_AND_OPTION is 1.
But I do not believe this is costly and is needed whenever HAVE_CTFONTCREATEFORCHARACTERSWITHLANGUAGE_AND_OPTION is 0.

> > Source/WTF/wtf/Platform.h:1544
> > +#define HAVE_CTFONTCREATEFORCHARACTERSWITHLANGUAGE_AND_OPTION 1
> 
> Nit: maybe avoid using underscores in the define since the function does not
> have them?

OK

[Per Arne Vollan]

(In reply to youenn fablet from comment #10)
> > Is it possible to avoid the copy of the font descriptor in the InstalledFont
> > constructor? Should we stop using kCTFontFallbackOptionAttribute when using
> > CTFontCreateForCharactersWithLanguageAndOption?
> 
> I think this is doable in case
> HAVE_CTFONTCREATEFORCHARACTERSWITHLANGUAGE_AND_OPTION is 1.
> But I do not believe this is costly and is needed whenever
> HAVE_CTFONTCREATEFORCHARACTERSWITHLANGUAGE_AND_OPTION is 0.
> 

It would be great if this is possible when HAVE_CTFONTCREATEFORCHARACTERSWITHLANGUAGE_AND_OPTION is 1 :)

[youenn fablet]

Created attachment 375198 [details]
Patch for landing

[EWS Watchlist]

Attachment 375198 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/pal/spi/cocoa/CoreTextSPI.h:135:  The parameter name "option" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 375198 [details]
Patch for landing

Attachment 375198 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12838872

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 375201 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[youenn fablet]

Created attachment 375230 [details]
Patch for landing

[EWS Watchlist]

Attachment 375230 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/pal/spi/cocoa/CoreTextSPI.h:135:  The parameter name "option" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

(In reply to Build Bot from comment #17)
> Attachment 375230 [details] did not pass style-queue:

I had to change a bit two of the tests to flush the font cache to not hit a debug assertion.
I also had to compile out another debug assertion in case user installed font attribute is not supported.

[WebKit Commit Bot]

Comment on attachment 375230 [details]
Patch for landing

Rejecting attachment 375230 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 375230, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/12842257

[youenn fablet]

Created attachment 375235 [details]
Patch for landing

[EWS Watchlist]

Attachment 375235 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/pal/spi/cocoa/CoreTextSPI.h:135:  The parameter name "option" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 375235 [details]
Patch for landing

Clearing flags on attachment: 375235

Committed r248071: <https://trac.webkit.org/changeset/248071>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 375261 [details]
Patch

[youenn fablet]

(In reply to youenn fablet from comment #25)
> Created attachment 375261 [details]
> Patch

Patch to fix old MacOS build

[WebKit Commit Bot]

Comment on attachment 375261 [details]
Patch

Clearing flags on attachment: 375261

Committed r248081: <https://trac.webkit.org/changeset/248081>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ali Juma]

(In reply to WebKit Commit Bot from comment #22)
> Comment on attachment 375235 [details]
> Patch for landing
> 
> Clearing flags on attachment: 375235
> 
> Committed r248071: <https://trac.webkit.org/changeset/248071>

This change seems to break the public iOS SDK build when using Xcode 11 beta 5:

Undefined symbols for architecture x86_64:
  "_CTFontCreateForCharactersWithLanguageAndOption", referenced from:
      WebCore::createFontForCharacters(__CTFont const*, __CFString const*, WebCore::AllowUserInstalledFonts, unsigned short const*, unsigned int) in UnifiedSource367.o
      WebCore::FontCache::prewarm(WebCore::FontCache::PrewarmInformation const&)::$_4::operator()() const in UnifiedSource367.o


Does the new logic need to be guarded by something like "#if USE(APPLE_INTERNAL_SDK)" or is this something that will be fixed by waiting for a new Xcode 11 beta?

[youenn fablet]

(In reply to Ali Juma from comment #29)
> (In reply to WebKit Commit Bot from comment #22)
> > Comment on attachment 375235 [details]
> > Patch for landing
> > 
> > Clearing flags on attachment: 375235
> > 
> > Committed r248071: <https://trac.webkit.org/changeset/248071>
> 
> This change seems to break the public iOS SDK build when using Xcode 11 beta
> 5:
> 
> Undefined symbols for architecture x86_64:
>   "_CTFontCreateForCharactersWithLanguageAndOption", referenced from:
>       WebCore::createFontForCharacters(__CTFont const*, __CFString const*,
> WebCore::AllowUserInstalledFonts, unsigned short const*, unsigned int) in
> UnifiedSource367.o
>       WebCore::FontCache::prewarm(WebCore::FontCache::PrewarmInformation
> const&)::$_4::operator()() const in UnifiedSource367.o
> 
> 
> Does the new logic need to be guarded by something like "#if
> USE(APPLE_INTERNAL_SDK)" or is this something that will be fixed by waiting
> for a new Xcode 11 beta?

I think it will solve itself but in the meantime, it might be best to guard it with APPLE_INTERNAL_SDK.
Do you want to submit a patch to update Platform.h to take that into account in HAVE_CTFONTCREATEFORCHARACTERSWITHLANGUAGEANDOPTION definition?

[Ali Juma]

(In reply to youenn fablet from comment #30)
> (In reply to Ali Juma from comment #29)
> > (In reply to WebKit Commit Bot from comment #22)
> > > Comment on attachment 375235 [details]
> > > Patch for landing
> > > 
> > > Clearing flags on attachment: 375235
> > > 
> > > Committed r248071: <https://trac.webkit.org/changeset/248071>
> > 
> > This change seems to break the public iOS SDK build when using Xcode 11 beta
> > 5:
> > 
> > Undefined symbols for architecture x86_64:
> >   "_CTFontCreateForCharactersWithLanguageAndOption", referenced from:
> >       WebCore::createFontForCharacters(__CTFont const*, __CFString const*,
> > WebCore::AllowUserInstalledFonts, unsigned short const*, unsigned int) in
> > UnifiedSource367.o
> >       WebCore::FontCache::prewarm(WebCore::FontCache::PrewarmInformation
> > const&)::$_4::operator()() const in UnifiedSource367.o
> > 
> > 
> > Does the new logic need to be guarded by something like "#if
> > USE(APPLE_INTERNAL_SDK)" or is this something that will be fixed by waiting
> > for a new Xcode 11 beta?
> 
> I think it will solve itself but in the meantime, it might be best to guard
> it with APPLE_INTERNAL_SDK.
> Do you want to submit a patch to update Platform.h to take that into account
> in HAVE_CTFONTCREATEFORCHARACTERSWITHLANGUAGEANDOPTION definition?

Ok, opened bug 200477 with a patch.