Bug 200241 - Use CTFontCreateForCharactersWithLanguageAndOption if available instead of CTFontCreateForCharactersWithLanguage
Summary: Use CTFontCreateForCharactersWithLanguageAndOption if available instead of CT...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-29 14:05 PDT by youenn fablet
Modified: 2019-08-06 13:43 PDT (History)
14 users (show)

See Also:


Attachments
Patch (8.66 KB, patch)
2019-07-29 14:18 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (8.71 KB, patch)
2019-07-29 14:33 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (11.02 KB, patch)
2019-07-30 17:42 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-highsierra (2.53 MB, application/zip)
2019-07-30 19:16 PDT, Build Bot
no flags Details
Patch for landing (13.45 KB, patch)
2019-07-31 11:06 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (13.46 KB, patch)
2019-07-31 12:33 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (1.92 KB, patch)
2019-07-31 15:53 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2019-07-29 14:05:49 PDT
Use CTFontCreateForCharactersWithLanguageAndOption if available instead of CTFontCreateForCharactersWithLanguage.
This will make things a bit faster.
Comment 1 youenn fablet 2019-07-29 14:18:34 PDT
Created attachment 375102 [details]
Patch
Comment 2 Build Bot 2019-07-29 14:20:39 PDT
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.
Comment 3 youenn fablet 2019-07-29 14:33:09 PDT
Created attachment 375105 [details]
Patch
Comment 4 youenn fablet 2019-07-29 14:33:54 PDT
<rdar://problem/53495386>
Comment 5 Build Bot 2019-07-29 14:35:06 PDT
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.
Comment 6 youenn fablet 2019-07-29 15:17:26 PDT Comment hidden (obsolete)
Comment 7 Myles C. Maxfield 2019-07-30 11:12:38 PDT
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?
Comment 8 Geoffrey Garen 2019-07-30 12:59:00 PDT
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.
Comment 9 Per Arne Vollan 2019-07-30 13:55:02 PDT
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?
Comment 10 youenn fablet 2019-07-30 14:26:56 PDT
(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
Comment 11 Per Arne Vollan 2019-07-30 15:27:06 PDT
(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 :)
Comment 12 youenn fablet 2019-07-30 17:42:31 PDT
Created attachment 375198 [details]
Patch for landing
Comment 13 Build Bot 2019-07-30 17:44:01 PDT
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.
Comment 14 Build Bot 2019-07-30 19:16:00 PDT
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.
Comment 15 Build Bot 2019-07-30 19:16:02 PDT
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
Comment 16 youenn fablet 2019-07-31 11:06:03 PDT
Created attachment 375230 [details]
Patch for landing
Comment 17 Build Bot 2019-07-31 11:06:56 PDT
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.
Comment 18 youenn fablet 2019-07-31 11:08:29 PDT
(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.
Comment 19 WebKit Commit Bot 2019-07-31 12:30:48 PDT
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
Comment 20 youenn fablet 2019-07-31 12:33:13 PDT
Created attachment 375235 [details]
Patch for landing
Comment 21 Build Bot 2019-07-31 12:35:25 PDT
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.
Comment 22 WebKit Commit Bot 2019-07-31 13:59:06 PDT
Comment on attachment 375235 [details]
Patch for landing

Clearing flags on attachment: 375235

Committed r248071: <https://trac.webkit.org/changeset/248071>
Comment 23 WebKit Commit Bot 2019-07-31 13:59:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 youenn fablet 2019-07-31 15:53:46 PDT
Reopening to attach new patch.
Comment 25 youenn fablet 2019-07-31 15:53:47 PDT
Created attachment 375261 [details]
Patch
Comment 26 youenn fablet 2019-07-31 15:54:22 PDT
(In reply to youenn fablet from comment #25)
> Created attachment 375261 [details]
> Patch

Patch to fix old MacOS build
Comment 27 WebKit Commit Bot 2019-07-31 16:24:15 PDT
Comment on attachment 375261 [details]
Patch

Clearing flags on attachment: 375261

Committed r248081: <https://trac.webkit.org/changeset/248081>
Comment 28 WebKit Commit Bot 2019-07-31 16:24:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Ali Juma 2019-08-06 12:54:30 PDT
(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?
Comment 30 youenn fablet 2019-08-06 13:28:53 PDT
(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?
Comment 31 Ali Juma 2019-08-06 13:42:43 PDT
(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.