Base: Glyph-related tweaks #26343
Base: Glyph-related tweaks #26343
Conversation
github-actions
bot
added
the
👀 pr-needs-review
| if (code_point == 0xFFFD) { | ||
| dbgln("FIXME: Font \"{}\" does not contain the replacement character", m_font->name()); | ||
| continue; | ||
| } | ||
| dbgln("Font \"{}\" does not contain code point {:X} (this should've been filtered out earlier)", m_font->name(), code_point); | ||
| VERIFY_NOT_REACHED(); |
There was a problem hiding this comment.
Isn't that reachable with user-provided fonts? If a PDF or a webpage gives us a font and asks to draw text that contains glyphs that are not in the font, we should not crash (and not print a FIXME either).
There was a problem hiding this comment.
This shouldn't really crash in any case, since I think LibWeb is the only user of GlyphRun, and there the glyphs are collected with Gfx::for_each_glyph_position(), which should only call the provided callback (that collects the glyphs to be passed to this constructor) with a glyph that is in the font, or the replacement character if it isn't (which is why this does a continue before the VERIFY_NOT_REACHED() in case that's missing - because we didn't check that that's actually in the font).
But I guess the dbgln would end up being quite spammy if we actually ended up here in a real-world scenario, and this debug code has kind of served it's purpose already, so I've dropped this commit.
The question mark is no longer used as a fallback as of 9e66662.
implicitfield
changed the title
github-actions
bot
removed
the
👀 pr-needs-review
3 participants
When viewing websites in Browser that use the default font (which resolves to "Roman Regular"), nothing was being drawn for missing glyphs because that font didn't contain the replacement character (which is now implemented in this PR, it's visually the same as in the "Katica Regular" font). Previously, those glyphs would've been replaced with the question mark character, but that is no longer the case as of 9e66662 (and I noticed that the documentation also referred to this, so I've also updated that now).