Bug 228927

Summary: [FreeType] Avoid yucky strong alias computations in font fallback code
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKitGTKAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, bugs-noreply, ews-watchlist, gyuyoung.kim, mcatanzaro, mmaxfield, ryuan.choi, sergio, webkit-bug-importer
Priority: P2    
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
See Also: https://bugs.webkit.org/show_bug.cgi?id=147057
https://bugs.freedesktop.org/show_bug.cgi?id=19375
Attachments:
Description Flags
Patch
none
Patch
ews-feeder: commit-queue-
Patch none

Description Michael Catanzaro 2021-08-09 15:35:47 PDT
In bug #147057 I added a bunch of copypasted code from Skia that performs black magic to sort font patterns in order to guess whether one font is strongly-aliased to another. In https://bugs.freedesktop.org/show_bug.cgi?id=19375 Fontconfig added a new FcPatternGetWithBinding() API to do this directly. It has been four years now, so in theory we can now remove all the yucky code and use FcPatternGetWithBinding() instead. I've attached a patch that does exactly that.

Problem is, my patch doesn't work at all. For some reason, FcPatternGetWithBinding() reports that *every* binding is a strong binding, so now every match performed by Fontconfig is accepted, which is not what we want. I will ask the Fontconfig developers if I'm doing something obviously wrong.
Comment 1 Michael Catanzaro 2021-08-09 15:38:03 PDT
Created attachment 435217 [details]
Patch
Comment 2 Michael Catanzaro 2021-08-10 06:45:25 PDT
FontConfig issue: https://gitlab.freedesktop.org/fontconfig/fontconfig/-/issues/294
Comment 3 Michael Catanzaro 2021-08-26 06:48:48 PDT
Created attachment 436504 [details]
Patch
Comment 4 Michael Catanzaro 2021-08-26 06:53:04 PDT
Created attachment 436505 [details]
Patch
Comment 5 Myles C. Maxfield 2021-08-26 11:56:02 PDT
Comment on attachment 436505 [details]
Patch

Nice!
Comment 6 Myles C. Maxfield 2021-08-26 11:57:38 PDT
Comment on attachment 436505 [details]
Patch

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

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:358
> +#if FC_VERSION < 21395

What happens if the version of FontConfig that the build uses is not the same version used at runtime? (We have a similar problem for using ICU and have to call functions instead of using macros.)
Comment 7 Michael Catanzaro 2021-08-26 12:25:54 PDT
(In reply to Myles C. Maxfield from comment #6)
> What happens if the version of FontConfig that the build uses is not the
> same version used at runtime? (We have a similar problem for using ICU and
> have to call functions instead of using macros.)

If it's built using a newer version of Fontconfig than is used at runtime, then fonts are going to get really messed up, because every font will be considered a valid alias for every other font. To avoid this, I could use FcGetVersion() to check the Fontconfig version at runtime instead of using the build-time check.

That said, with Linux software, it's well-understood that the runtime environment has to be newer than the build environment, or random breakage like this will result. I know this is different from macOS, because I remember I landed a change once that broke Safari nightlies because I used only a build-time check and not a runtime version check, see r189526 and r189990. I was very surprised that your build environment included a newer version of SQLite than your users were running. With software written for Linux, everyone just assumes that never happens. We would say the build environment is broken if it contains anything newer than the oldest-supported version that might be used at runtime.

The benefit of using build-time checks instead of runtime checks is it makes it easier to see where the legacy code is, so it can be removed more easily in the future. Future me might like this....
Comment 8 EWS 2021-08-26 12:30:39 PDT
Committed r281644 (241000@main): <https://commits.webkit.org/241000@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436505 [details].