[webkit-reviews] review granted: [Bug 183155] [FreeType] Color emojis in WebKitGTK+ for great justice : [Attachment 334735] Updated patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted 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 334735: Updated patch

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




--- Comment #12 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 334735
  --> https://bugs.webkit.org/attachment.cgi?id=334735
Updated patch

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

I still don't understand why ignoring the scale is correct, but since this
doesn't break anything, let's do it.

How hard is it to add a test for this? You could just have an HTML page that
displays a single character and save the expected result as an image, right?
Then we could at least see that it is colored.

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

I don't think this change is unrelated. It used to be live code, and now you've
turned it into dead code, right? But very subtle dead code that most developers
would not notice. So I would address bug #183210 in this same patch. Up to you,
as long as you follow up promptly so that we don't forget about this.