[Webkit-unassigned] [Bug 191976] [FreeType] Color emoji not properly supported

[bugzilla-daemon at webkit.org]

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

--- Comment #18 from Michael Catanzaro <mcatanzaro at igalia.com> ---
(In reply to Carlos Garcia Campos from comment #17)
> I thought about it, but I saw the other bool parameter in the function and
> decided to keep a bool. I can change both, though. 

FWIW I noticed that during my review and would have complained if not for the other bool parameter already right next to it.

> > > Source/WebCore/platform/graphics/cairo/FontCairoHarfbuzzNG.cpp:136
> > > +    if (auto systemFallback = FontCache::singleton().systemFallbackForCharacters(m_fontDescription, baseFont, false, isEmoji, characters, length)) {
> > 
> > There are color fonts that don't support emoji, and it's conceptually
> > possible to have an emoji font that isn't color. It isn't right to pass
> > "isEmoji" to "bool coloredFont"
> 
> I think so, what we want here is to prefer a colored font when rendering
> emojis.

It's too confusing: someone looking at this is likely to think it's a bug upon discovering the parameter is named coloredFont and not isEmoji. Converting the code to use enums or a flags parameter would help make this more explicit, since then you'd need to write out something like isEmoji ? Colored : NotColored. So that tips the balance in favor of doing that conversion in this patch IMO.

-- 
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/20190103/a4c68ecf/attachment.html>