[webkit-reviews] review denied: [Bug 183155] [FreeType] Color emojis in WebKitGTK+ for great justice : [Attachment 334693] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 183155: [FreeType] Color emojis in WebKitGTK+ for great justice
https://bugs.webkit.org/show_bug.cgi?id=183155

Attachment 334693: Patch

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




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