Bug 82889 – [GTK] Honor GTK+ font settings

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Milan Crha]

GTK follows system font settings on its cairo contexts, but WebKitGtk doesn't follow this settings, which makes pages rendered in Gtk look ugly and inconsistent with the rest of application. If I apply the below change, then I get consistent behaviour, with one exception, if I change font setting while webkitview is already created, then the font change is not noticed in it. Note the system settings has both font Hinting and font Antialiasing, and I use Hinting: Slight, Antialiasing: Rgba. You can see a sample screen shot here [1], where the first part from GtkHTML uses system font setting (on a monospaced font), while the webkit does not. The difference is noticeable.

[1] https://bugzilla.gnome.org/show_bug.cgi?id=673108#c1

diff -up webkit-1.8.0/Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp.fonts webkit-1.8.0/Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp
--- webkit-1.8.0/Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp.fonts	2012-04-02 13:58:12.393501948 +0200
+++ webkit-1.8.0/Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp	2012-04-02 13:58:15.363537998 +0200
@@ -94,10 +94,10 @@ void setCairoFontOptionsFromFontConfigPa
             cairo_font_options_set_antialias(options, CAIRO_ANTIALIAS_GRAY);
     }
 
-    if (FcPatternGetInteger(pattern, FC_HINT_STYLE, 0, &integerResult) == FcResultMatch)
+    /*if (FcPatternGetInteger(pattern, FC_HINT_STYLE, 0, &integerResult) == FcResultMatch)
         cairo_font_options_set_hint_style(options, convertFontConfigHintStyle(integerResult));
     if (FcPatternGetBool(pattern, FC_HINTING, 0, &booleanResult) == FcResultMatch && !booleanResult)
-        cairo_font_options_set_hint_style(options, CAIRO_HINT_STYLE_NONE);
+        cairo_font_options_set_hint_style(options, CAIRO_HINT_STYLE_NONE);*/
 }
 
 static cairo_font_options_t* getDefaultFontOptions()
diff -up webkit-1.8.0/Source/WebCore/platform/graphics/pango/FontPlatformDataPango.cpp.fonts webkit-1.8.0/Source/WebCore/platform/graphics/pango/FontPlatformDataPango.cpp
--- webkit-1.8.0/Source/WebCore/platform/graphics/pango/FontPlatformDataPango.cpp.fonts	2012-04-02 13:59:00.823090496 +0200
+++ webkit-1.8.0/Source/WebCore/platform/graphics/pango/FontPlatformDataPango.cpp	2012-04-02 13:59:38.091543457 +0200
@@ -152,8 +152,8 @@ FontPlatformData::FontPlatformData(cairo
 
     // We force antialiasing and disable hinting to provide consistent
     // typographic qualities for custom fonts on all platforms.
-    cairo_font_options_set_hint_style(options, CAIRO_HINT_STYLE_NONE);
-    cairo_font_options_set_antialias(options, CAIRO_ANTIALIAS_GRAY);
+    /*cairo_font_options_set_hint_style(options, CAIRO_HINT_STYLE_NONE);
+    cairo_font_options_set_antialias(options, CAIRO_ANTIALIAS_GRAY);*/
 
     m_scaledFont = cairo_scaled_font_create(fontFace, &fontMatrix, &ctm, options);
     cairo_font_options_destroy(options);

[Martin Robinson]

WebKitGTK+ should follow your Fontconfig settings over the Xsettings ones. The reason is that XSettings switches are not per-font, while Fontconfig settings are. You simply need to modify your Fontconfig setup to disable hinting. For all of the gory details take a look here: http://neugierig.org/software/chromium/fonts/

Now, if for some reason WebKitGTK+ isn't reading your Fontconfig settings properly and you don't see this issue in Chromium, this is a valid bug.

[Milan Crha]

My idea was that if this is WebKitGTK, then it should follow gtk's font settings, thus also WebKitGTK will follow look & feel of other gtk+ applications. Hinting makes a large difference, noticeable on the first look.

[Martin Robinson]

(In reply to comment #2)
> My idea was that if this is WebKitGTK, then it should follow gtk's font settings, thus also WebKitGTK will follow look & feel of other gtk+ applications. Hinting makes a large difference, noticeable on the first look.

In the end, the choice to follow the Fontconfig settings or the XSettings settings is somewhat arbitrary. In this case, we've chosen to take the route that gives the user more control. XSettings does not allow you to hint some fonts and not others. Not only that, we try to do the same thing as Chromium here.

If you simply update your Fontconfig settings to disable hinting everywhere, I think you'll find fonts are to your liking. You'll also fix all applications that obey Fontconfig over XSettings.

[Milan Crha]

I still believe that in webkitgtk you should follow gtk settings, it's WebKit *GTK*, after all. Nonetheless, I'm not going to fight here.

I would like to ask you for one thing, though. Could you tell me where and how I can change font settings in fontconfig, please? The best some kind of UI tool, like the gtk has. I know about gtk, but not about fontconfig, and I expect users will ask for it, as they might not like tiny fonts, which are hard to read, same as I do not like them for monospace fonts.

[Martin Robinson]

(In reply to comment #4)
> I still believe that in webkitgtk you should follow gtk settings, it's WebKit *GTK*, after all. Nonetheless, I'm not going to fight here.
> 
> I would like to ask you for one thing, though. Could you tell me where and how I can change font settings in fontconfig, please? The best some kind of UI tool, like the gtk has. I know about gtk, but not about fontconfig, and I expect users will ask for it, as they might not like tiny fonts, which are hard to read, same as I do not like them for monospace fonts.

I don't know of a tool to modify fontconfig font settings, but if you look around you should be able to find some examples of how to modify the configuration of all fonts.

The size of the font isn't determined by the fontconfig or XSettings configuration. It's up to the client (Epiphany, for instance) to determine the the default font size. One thing to ensure is that, if you are using Epiphany, you should uncheck "Use system font settings" and choose a more appropriate default size.

[Milan Crha]

It's not about font size, like how tall it's supposed to be, it's about hinting and antialiasing. If I turn than on, then the font is weighter, easier to read for me. If you open font settings for gtk/gnome, then you can tweak these options there and see what I mean. Different settings fit for different monitors (CRT/LCD/Laptop/...).

I looked around, I only found /etc/fonts/..., but that is quite user unfriendly. My intention for the setting is also to verify that your advice actually works.

[Martin Robinson]

(In reply to comment #6)
> It's not about font size, like how tall it's supposed to be, it's about hinting and antialiasing. If I turn than on, then the font is weighter, easier to read for me. If you open font settings for gtk/gnome, then you can tweak these options there and see what I mean. Different settings fit for different monitors (CRT/LCD/Laptop/...).

So the font.conf settings are specifying that the fonts are not hinted nor anti-aliased? Typically those are on by default, unless fontconfig disables them for a particular font.

Here's a guide to configuring fontconfig via the ~/fonts.conf file: https://wiki.archlinux.org/index.php/Font_Configuration#Fontconfig_configuration

[Milan Crha]

Nice, thanks. I also made this system-wide, when I copied 10-sub-pixel-rgb.conf and 10-autohint.conf from /etc/fonts/conf.avail/ to /etc/fonts/conf.d/ and after I restart Evolution (the application using webkitgtk now), the fonts look readable again.

[Michael Catanzaro]

After much discussion with Behdad and Martin (who is still not completely convinced I think :) I want to merge cairo font options into the Fontconfig pattern used for rendering using cairo_ft_font_options_substitute(). This is how the API was designed to be used anyway. Fontconfig will still have final say over whether to actually respect the desktop settings or not, so it can still choose to ignore the desktop's settings, but I don't think it makes sense to have desktop-wide font settings and not tell Fontconfig about them, especially when the whole point of WebKitGTK+ is desktop integration. This should also reduce complaints that we're not following desktop settings and that we're drawing fonts differently than Firefox.

[Michael Catanzaro]

Here's an initial patch. The code changes should all be fine, but it will probably break a huge amount of layout tests. To be able to easily rebase the layout tests, we need accurate test expectations for all currently-failing tests, so it's probably a bad idea to commit this patch in the meantime.

[Michael Catanzaro]

Note that our font rendering still looks completely different from Firefox, but I am hoping that is bug #164689, knock on wood.

[Michael Catanzaro]

Created attachment 294635 [details]
Patch

[Michael Catanzaro]

Created attachment 294636 [details]
Patch

[Michael Catanzaro]

(In reply to comment #11)
> Note that our font rendering still looks completely different from Firefox,
> but I am hoping that is bug #164689, knock on wood.

Nope, even with both of these patches, our font rendering looks nothing at all like Firefox. Alas.

[Carlos Garcia Campos]

Comment on attachment 294636 [details]
Patch

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

> Source/WebCore/PlatformEfl.cmake:160
> +    platform/graphics/freetype/FreeTypeFontUtilities.cpp

This files uses GTK+, it should be added to WebCorePlatformGTK_SOURCES

> Source/WebCore/PlatformGTK.cmake:212
> +    platform/graphics/freetype/FontCacheFreeType.cpp
>      platform/graphics/freetype/FontPlatformDataFreeType.cpp
> +    platform/graphics/freetype/FreeTypeFontUtilities.cpp

It's the other way around, FontPlatformDataFreeType.cpp no longer uses GTK+, so all theses should be moved to WebCore_SOURCES, and FreeTypeFontUtilities.cpp should be moved to WebCorePlatformGTK_SOURCES, but you are adding it to both.

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:-119
> -static CairoUniquePtr<cairo_font_options_t> getDefaultCairoFontOptions()
> -{
> -#if PLATFORM(GTK)
> -    if (GdkScreen* screen = gdk_screen_get_default()) {
> -        const cairo_font_options_t* screenOptions = gdk_screen_get_font_options(screen);
> -        if (screenOptions)
> -            return CairoUniquePtr<cairo_font_options_t>(cairo_font_options_copy(screenOptions));
> -    }
> -#endif
> -    return CairoUniquePtr<cairo_font_options_t>(cairo_font_options_create());
> -}

Can we also remove the #include <gdk/gdk.h> now?

> Source/WebCore/platform/graphics/freetype/FreeTypeFontUtilities.cpp:27
> +#include "FreeTypeFontUtilities.h"

I'm not sure FreeTypeFontUtilities is the best name for this, nor Source/WebCore/platform/graphics/freetype/ the best place. This has nothing to do with freetype actually, it's more a cairo implementation. I would add it to Source/WebCore/platform/graphics/cairo instead, named as CairoFontUtilities or CairoDefaultFontOptions. Or you could add it to CairoUtilities that already has some FreeType stuff (although that would make CairoUtilities a GTK+ file).

> Source/WebCore/platform/graphics/freetype/FreeTypeFontUtilities.cpp:29
> +#include <wtf/Platform.h>

I'm pretty sure you don't need this, I think all config.h files already include Platform.h

> Source/WebCore/platform/graphics/freetype/FreeTypeFontUtilities.cpp:37
> +CairoUniquePtr<cairo_font_options_t> getDefaultCairoFontOptions()

We should take advantage of this patch to properly name this, it should be defaultCairoFonrOptions()

> Source/WebCore/platform/graphics/freetype/FreeTypeFontUtilities.cpp:43
> +            return CairoUniquePtr<cairo_font_options_t>(cairo_font_options_copy(screenOptions));

So, in all the new places where you are now using this, the font options are not modified, they are used to set up a FcPattern. I wonder if we could return the options without copying them, and let the caller decide whether to copy them or not. For the fallback case we could use a static cairo_font_options_t

> Source/WebCore/platform/graphics/freetype/FreeTypeFontUtilities.h:29
> +#include "CairoUniquePtr.h"
> +#include <cairo.h>

CairoUniquePtr.h already includes cairo.h

[Michael Catanzaro]

(In reply to comment #14)
> Nope, even with both of these patches, our font rendering looks nothing at
> all like Firefox. Alas.

Actually this seems to get us about halfway there. Our character positioning is completely different (arguably worse) but it makes the hinting match. That's good.

Will update this patch when I have sufficient time to attend to the layout tests.

[Michael Catanzaro]

(In reply to comment #3)
> In the end, the choice to follow the Fontconfig settings or the XSettings
> settings is somewhat arbitrary. In this case, we've chosen to take the route
> that gives the user more control. XSettings does not allow you to hint some
> fonts and not others. Not only that, we try to do the same thing as Chromium
> here.

By the way, I am only merging *defaults* from GTK+ settings. Users still have full control of the final hinting on a per-font basis with fontconfig, just the default will be GTK+'s default and not fontconfig's default (which is currently only used by WebKitGTK+ and not anything else).

[Michael Catanzaro]

So the big change since I originally wrote this patch is that Fontconfig switched from medium to light hinting by default, which now matches GTK+'s default. So this should make no difference to users who have not touched those hidden font settings. But if you have tweaked your settings, you probably want them to work properly... hence we still need this.

[Michael Catanzaro]

Next patch attempt is mainly for EWS; I'm still building it locally. Big thanks to Carlos Garcia, Carlos Lopez, and Javi Fernandez for doing the hard part to make this possible, which was cleaning up our test expectations. This has to land when the bot is green or almost green, as otherwise it's way too hard to assess impact on layout tests.

I tried running tests locally and am at something like 200 failures and 400 timeouts, so it's just not practical now. I'll watch the bots after this lands to see what the impact is. In theory, any breakage should be fixable without altering test expectations by just modifying our test Fontconfig configuration, but I want to assess the impact before making any changes there.

[Michael Catanzaro]

Created attachment 303711 [details]
Patch

[Michael Catanzaro]

Created attachment 303719 [details]
Patch

[Michael Catanzaro]

Created attachment 303730 [details]
Patch

[Michael Catanzaro]

(In reply to comment #15)
> It's the other way around, FontPlatformDataFreeType.cpp no longer uses GTK+,
> so all theses should be moved to WebCore_SOURCES, and
> FreeTypeFontUtilities.cpp should be moved to WebCorePlatformGTK_SOURCES, but
> you are adding it to both.

I'm sure I did it because I couldn't get it to link otherwise.

This time around, it doesn't seem to work no matter what I try:

FAILED: lib/libwebkit2gtk-4.0.so.37.20.0 
: && /usr/lib64/ccache/c++  -fPIC -fno-exceptions -fno-strict-aliasing -fno-rtti -std=c++1y -O2 -DNDEBUG  -Wl,--no-undefined  -L/home/mcatanzaro/Projects/GNOME/install/lib  -fuse-ld=gold -Wl,--disable-new-dtags -fuse-ld=gold -Wl,--disable-new-dtags -shared -Wl,-soname,libwebkit2gtk-4.0.so.37 -o lib/libwebkit2gtk-4.0.so.37.20.0 @CMakeFiles/WebKit2.rsp  && :
lib/libWebCoreGTK.a(lib/../Source/WebCore/CMakeFiles/WebCore.dir/platform/graphics/freetype/FontCacheFreeType.cpp.o):FontCacheFreeType.cpp:function WebCore::FontCache::systemFallbackForCharacters(WebCore::FontDescription const&, WebCore::Font const*, bool, unsigned short const*, unsigned int): error: undefined reference to 'WebCore::getDefaultCairoFontOptions()'
lib/libWebCoreGTK.a(lib/../Source/WebCore/CMakeFiles/WebCore.dir/platform/graphics/freetype/FontCacheFreeType.cpp.o):FontCacheFreeType.cpp:function WebCore::FontCache::createFontPlatformData(WebCore::FontDescription const&, WTF::AtomicString const&, WebCore::FontTaggedSettings<int> const*, WebCore::FontVariantSettings const*): error: undefined reference to 'WebCore::getDefaultCairoFontOptions()'
lib/libWebCoreGTK.a(lib/../Source/WebCore/CMakeFiles/WebCore.dir/platform/graphics/freetype/FontCacheFreeType.cpp.o):FontCacheFreeType.cpp:function WebCore::FontCache::createFontPlatformData(WebCore::FontDescription const&, WTF::AtomicString const&, WebCore::FontTaggedSettings<int> const*, WebCore::FontVariantSettings const*): error: undefined reference to 'WebCore::getDefaultCairoFontOptions()'
lib/libWebCoreGTK.a(lib/../Source/WebCore/CMakeFiles/WebCore.dir/platform/graphics/freetype/FontPlatformDataFreeType.cpp.o):FontPlatformDataFreeType.cpp:function void std::call_once<WebCore::getDefaultFontconfigOptions()::{lambda(_FcPattern*)#1}, _FcPattern*&>(std::once_flag&, WebCore::getDefaultFontconfigOptions()::{lambda(_FcPattern*)#1}&&, _FcPattern*&)::{lambda()#2}::_FUN(): error: undefined reference to 'WebCore::getDefaultCairoFontOptions()'
collect2: error: ld returned 1 exit status

Linkers!

[Michael Catanzaro]

(In reply to comment #23)
> I'm sure I did it because I couldn't get it to link otherwise.
> 
> This time around, it doesn't seem to work no matter what I try:

Ah, it was just a missing namespace

[Michael Catanzaro]

Created attachment 303733 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 303733 [details]
Patch

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

I don't think I can review this.

> Source/WebCore/PlatformGTK.cmake:-190
> -    platform/graphics/freetype/FontPlatformDataFreeType.cpp

Why move it?

> Source/WebCore/platform/graphics/cairo/CairoUtilities.h:31
> +#include "CairoUniquePtr.h"

Can't you forward-declare the type instead of including it in the header? Header includes make our builds slower.

[Michael Catanzaro]

(In reply to comment #26)
> Comment on attachment 303733 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303733&action=review
> 
> I don't think I can review this.

I wouldn't expect you to... I'll bug Carlos.

> > Source/WebCore/PlatformGTK.cmake:-190
> > -    platform/graphics/freetype/FontPlatformDataFreeType.cpp
> 
> Why move it?

I can't get it to link otherwise.

> > Source/WebCore/platform/graphics/cairo/CairoUtilities.h:31
> > +#include "CairoUniquePtr.h"
> 
> Can't you forward-declare the type instead of including it in the header?
> Header includes make our builds slower.

I doubt it, since it's needed for template expansion? Anyway, if we remove it we have to include cairo.h directly instead, which is way bigger.

[Carlos Garcia Campos]

Comment on attachment 303733 [details]
Patch

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

>>> Source/WebCore/PlatformGTK.cmake:-190
>>> -    platform/graphics/freetype/FontPlatformDataFreeType.cpp
>> 
>> Why move it?
> 
> I can't get it to link otherwise.

Before this patch FontPlatformDataFreeType used GTK+ so we had to add it to WebCorePlatformGTK_SOURCES. Norw that it doesn't it should be moved to WebCore_SOURCES that doesn't depend on GTK+.

> Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:54
> +CairoUniquePtr<cairo_font_options_t> getDefaultCairoFontOptions()
> +{
> +    return CairoUniquePtr<cairo_font_options_t>(cairo_font_options_create());
> +}

I know this is the existing name, but this patch is a good moment to improve it. The get sounds like a global static object is returned, even when it returns a unique_ptr. So, maybe we can rename this to defaultCairoFontOptions(). Or wait, I think we can stop returning a copy. Before this patch, we always wanted a copy because FontPlatformData::buildScaledFont was the only user and it changes the options, but now we are using this only to pass it to cairo_ft_font_options_substitute that expects a const cairo_font_options_t*. So, I think this should use NeverDestroyed to create a global default font options, and let the user decide whether to copy it or not.

>>> Source/WebCore/platform/graphics/cairo/CairoUtilities.h:31
>>> +#include "CairoUniquePtr.h"
>> 
>> Can't you forward-declare the type instead of including it in the header? Header includes make our builds slower.
> 
> I doubt it, since it's needed for template expansion? Anyway, if we remove it we have to include cairo.h directly instead, which is way bigger.

And then we don't need this here because we will return a pointer

> Source/WebCore/platform/graphics/gtk/GdkCairoUtilities.cpp:41
> +        const cairo_font_options_t* screenOptions = gdk_screen_get_font_options(screen);
> +        if (screenOptions)

if (const auto* options = gdk_screen_get_font_options(screen))
    return options;

> Source/WebCore/platform/graphics/gtk/GdkCairoUtilities.cpp:44
> +    return CairoUniquePtr<cairo_font_options_t>(cairo_font_options_create());

And here we would use a lazy initialized never destroyed

[Michael Catanzaro]

(In reply to comment #28)
> Before this patch FontPlatformDataFreeType used GTK+ so we had to add it to
> WebCorePlatformGTK_SOURCES. Norw that it doesn't it should be moved to
> WebCore_SOURCES that doesn't depend on GTK+.

I know that's right, but I really don't know how to make that work. See comment #23. I know it's not cool to ask you to debug my problem, but I'm stumped.

> > Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:54
> > +CairoUniquePtr<cairo_font_options_t> getDefaultCairoFontOptions()
> > +{
> > +    return CairoUniquePtr<cairo_font_options_t>(cairo_font_options_create());
> > +}
> 
> I know this is the existing name, but this patch is a good moment to improve
> it. The get sounds like a global static object is returned, even when it
> returns a unique_ptr. So, maybe we can rename this to
> defaultCairoFontOptions(). Or wait, I think we can stop returning a copy.
> Before this patch, we always wanted a copy because
> FontPlatformData::buildScaledFont was the only user and it changes the
> options, but now we are using this only to pass it to
> cairo_ft_font_options_substitute that expects a const cairo_font_options_t*.
> So, I think this should use NeverDestroyed to create a global default font
> options, and let the user decide whether to copy it or not.

OK.

> >>> Source/WebCore/platform/graphics/cairo/CairoUtilities.h:31
> >>> +#include "CairoUniquePtr.h"
> >> 
> >> Can't you forward-declare the type instead of including it in the header? Header includes make our builds slower.
> > 
> > I doubt it, since it's needed for template expansion? Anyway, if we remove it we have to include cairo.h directly instead, which is way bigger.
> 
> And then we don't need this here because we will return a pointer

OK.

> > Source/WebCore/platform/graphics/gtk/GdkCairoUtilities.cpp:41
> > +        const cairo_font_options_t* screenOptions = gdk_screen_get_font_options(screen);
> > +        if (screenOptions)
> 
> if (const auto* options = gdk_screen_get_font_options(screen))
>     return options;

Yes that's nicer, though I think I would omit the const there...? It's not useful.

> > Source/WebCore/platform/graphics/gtk/GdkCairoUtilities.cpp:44
> > +    return CairoUniquePtr<cairo_font_options_t>(cairo_font_options_create());
> 
> And here we would use a lazy initialized never destroyed

OK.

[Carlos Garcia Campos]

(In reply to comment #29)
> (In reply to comment #28)
> > Before this patch FontPlatformDataFreeType used GTK+ so we had to add it to
> > WebCorePlatformGTK_SOURCES. Norw that it doesn't it should be moved to
> > WebCore_SOURCES that doesn't depend on GTK+.
> 
> I know that's right, but I really don't know how to make that work. See
> comment #23. I know it's not cool to ask you to debug my problem, but I'm
> stumped.

This patch is doing the right thing now, FontPlatformDataFreeType no longer uses GTK+ so it's moved to the common sources.

[Michael Catanzaro]

Ummm... OK great, I got it working and forgot! See comment #24, my linker error was due to a missing namespace. :)

[Michael Catanzaro]

Created attachment 304781 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 304781 [details]
Patch

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

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:296
> -    CairoUniquePtr<cairo_font_options_t> options = getDefaultCairoFontOptions();
> +    cairo_font_options_t* options = getDefaultCairoFontOptions();

Default options should never be modified, because then they are no the default ones anymore. Here you have to explicitly copy them.

CairoUniquePtr<cairo_font_options_t> options(cairo_font_options_copy(getDefaultCairoFontOptions()));

> Source/WebCore/platform/graphics/gtk/GdkCairoUtilities.cpp:52
> +    static LazyNeverDestroyed<cairo_font_options_t*> options;
> +    static std::once_flag flag;
> +    std::call_once(flag, [] {
> +        if (auto* screen = gdk_screen_get_default()) {
> +            if (auto* screenOptions = gdk_screen_get_font_options(screen)) {
> +                options.construct(cairo_font_options_copy(screenOptions));
> +                return;
> +            }
> +        }
> +        options.construct(cairo_font_options_create());
> +    });
> +    return options;

This is not what I meant. In the case of the GTK+ default options can change at any time, so we don't want to init once, we can simply return always the result of gdk_screen_get_font_options(screen). Only in case that's nullptr, we want to use our global options like in the non gtk case, but in this case we want to lazy initialize because in most of the cases it won't be used.

[Michael Catanzaro]

Comment on attachment 304781 [details]
Patch

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

>> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:296
>> +    cairo_font_options_t* options = getDefaultCairoFontOptions();
> 
> Default options should never be modified, because then they are no the default ones anymore. Here you have to explicitly copy them.
> 
> CairoUniquePtr<cairo_font_options_t> options(cairo_font_options_copy(getDefaultCairoFontOptions()));

Oh no! Good catch....

>> Source/WebCore/platform/graphics/gtk/GdkCairoUtilities.cpp:52
>> +    return options;
> 
> This is not what I meant. In the case of the GTK+ default options can change at any time, so we don't want to init once, we can simply return always the result of gdk_screen_get_font_options(screen). Only in case that's nullptr, we want to use our global options like in the non gtk case, but in this case we want to lazy initialize because in most of the cases it won't be used.

Ah, right.

[Michael Catanzaro]

Created attachment 304889 [details]
Patch

[Michael Catanzaro]

OK, let's do a test commit to see how layout tests react. I'm expecting to need to roll this out.

[Michael Catanzaro]

Committed r214283: <http://trac.webkit.org/changeset/214283>

[Michael Catanzaro]

Bots are happy.

[Adrian Perez]

(In reply to Michael Catanzaro from comment #38)
> Bots are happy.

/me is happy, too! :-)