Bug 200241

Created attachment 375102 [details] Patch

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.

Created attachment 375105 [details] Patch

<rdar://problem/53495386>

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

(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

(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 :)

Created attachment 375198 [details] Patch for landing

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

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

Created attachment 375230 [details] Patch for landing

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.

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

Created attachment 375235 [details] Patch for landing

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 on attachment 375235 [details] Patch for landing Clearing flags on attachment: 375235 Committed r248071: <https://trac.webkit.org/changeset/248071>

All reviewed patches have been landed. Closing bug.

Reopening to attach new patch.

Created attachment 375261 [details] Patch

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

Comment on attachment 375261 [details] Patch Clearing flags on attachment: 375261 Committed r248081: <https://trac.webkit.org/changeset/248081>

All reviewed patches have been landed. Closing bug.

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

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

(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.