[Webkit-unassigned] [Bug 183155] [FreeType] Color emojis in WebKitGTK+ for great justice

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=183155

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #334693|review?                     |review-
              Flags|                            |

--- Comment #7 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 334693
  --> https://bugs.webkit.org/attachment.cgi?id=334693
Patch

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

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:73
> +    bool italic = fontDescription.italic();
> +    if (!FcPatternAddInteger(pattern, FC_SLANT, italic ? FC_SLANT_ITALIC : FC_SLANT_ROMAN))

Since you're moving this code, might as well take the opportunity to get rid of the unnecessary local variable and do fontDescription.italic() ? FC_SLANT_ITALIC : FC_SLANT_ROMAN

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:98
> +    if (!configurePatternForFontDescription(pattern.get(), fontDescription))
> +        return nullptr;

Nit: to make this more readable, add one blank line above and below. It's unrelated to forcing scalable fonts and unrelated to the FcConfigSubstitute/cairo_ft_font_options_substitute/FcDefaultSubstitute dance.

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:112
> -    return FcFontSetMatch(nullptr, &fallbacks, 1, pattern, &fontConfigResult);
> +    return adoptRef(FcFontSetMatch(nullptr, &fallbacks, 1, pattern, &fontConfigResult));

Good catch.

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:126
> +    // Try first a direct match.
> +    FcResult fontConfigResult;
> +    if (RefPtr<FcPattern> resultPattern = adoptRef(FcFontMatch(nullptr, pattern.get(), &fontConfigResult))) {
> +        FontPlatformData alternateFontData(resultPattern.get(), description);
> +        return fontForPlatformData(alternateFontData);
> +    }

I'm pretty sure this is wrong. I think Fontconfig will always return a pattern here, whatever works best. Now you'll never consider the fallback case. In short: I suspect this disables CSS font fallback. E.g. if you have, say:

font: Cantarell; Liberation Sans; sans

I suspect that, with this change, fontconfig will always return some best-effort result for Cantarell, whatever its closest match is, without ever considering the fallbacks Liberation Sans or sans.

If I'm wrong, please explain why.

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:310
> -    cairo_matrix_init(&fontMatrix, fontConfigMatrix.xx, -fontConfigMatrix.yx,
> -        -fontConfigMatrix.xy, fontConfigMatrix.yy, 0, 0);
> +
> +    cairo_matrix_init(&fontMatrix, 1, -fontConfigMatrix.yx, -fontConfigMatrix.xy, 1, 0, 0);

Is ignoring the scale really the right thing to do? This seems quite doubtful....

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180227/2eeaf849/attachment.html>