Bug 159783

Summary: Check whether font is nonnull for GlyphData instead of calling GlyphData::isValid()
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: MathMLAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: ba8342776, bfulgham, cdumez, commit-queue, dbarton, esprehn+autocc, glenn, kondapallykalyan, pvollan
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 159702    
Attachments:
Description Flags
Patch none

Frédéric Wang (:fredw)
Reported 2016-07-14 14:33:16 PDT
See bug 155018 comment 36. Also, probably the reason for bug 159702. fred@debian:~/webkit/WebKit/Source/WebCore/rendering/mathml$ grep '\.isValid' * MathOperator.cpp: return data.isValid() ? data.font->boundsForGlyph(data.glyph) : FloatRect(); MathOperator.cpp: return data.isValid() ? data.font->widthForGlyph(data.glyph) : 0; MathOperator.cpp: return baseGlyph.isValid() && baseGlyph.font == &style.fontCascade().primaryFont(); MathOperator.cpp: ASSERT(sizeVariant.isValid()); MathOperator.cpp: ASSERT(m_assembly.extension.isValid()); MathOperator.cpp: ASSERT(m_assembly.extension.isValid()); MathOperator.cpp: ASSERT(m_assembly.topOrRight.isValid()); MathOperator.cpp: ASSERT(m_assembly.bottomOrLeft.isValid()); MathOperator.cpp: if (m_assembly.middle.isValid()) { MathOperator.cpp: ASSERT(m_assembly.bottomOrLeft.isValid()); MathOperator.cpp: ASSERT(m_assembly.topOrRight.isValid()); MathOperator.cpp: if (m_assembly.middle.isValid()) { MathOperator.cpp: ASSERT(m_variant.isValid()); RenderMathMLOperator.cpp: float glyphWidth = data.isValid() ? data.font->widthForGlyph(data.glyph) : 0; RenderMathMLToken.cpp: if (m_mathVariantGlyph.isValid()) { RenderMathMLToken.cpp: if (m_mathVariantGlyph.isValid()) RenderMathMLToken.cpp: if (!m_mathVariantGlyph.isValid()) { RenderMathMLToken.cpp: if (info.context().paintingDisabled() || info.phase != PaintPhaseForeground || style().visibility() != VISIBLE || !m_mathVariantGlyph.isValid()) RenderMathMLToken.cpp: if (m_mathVariantGlyph.isValid())
Attachments
Patch (10.99 KB, patch)
2016-07-14 23:51 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Frédéric Wang (:fredw)
Comment 1 2016-07-14 23:51:51 PDT
Created attachment 283740 [details] Patch
Brent Fulgham
Comment 2 2016-07-15 09:36:18 PDT
Comment on attachment 283740 [details] Patch I wonder if there is other code that really means "font is non-null and also isValid"? r=me
WebKit Commit Bot
Comment 3 2016-07-15 10:04:34 PDT
Comment on attachment 283740 [details] Patch Clearing flags on attachment: 283740 Committed r203280: <http://trac.webkit.org/changeset/203280>
WebKit Commit Bot
Comment 4 2016-07-15 10:04:39 PDT
All reviewed patches have been landed. Closing bug.
Frédéric Wang (:fredw)
Comment 5 2016-07-15 10:24:33 PDT
(In reply to comment #2) > Comment on attachment 283740 [details] > Patch > > I wonder if there is other code that really means "font is non-null and also > isValid"? r=me Oh, I just saw this comment. As defined in GlyphData, isValid means "font || glyph" which sounds weird for me. So just checking 'font' is stronger...
Leroy Wilder
Comment 6 2025-03-23 20:10:51 PDT
Hey, that’s a solid catch! Checking if the font is nonnull for GlyphData instead of relying on `GlyphData::isValid()` makes a lot of sense—cuts out an extra call and keeps it straightforward. I’ve run into similar situations where `isValid()` can be a bit of a black box, and just confirming the font’s there feels more reliable. Did you notice any funky edge cases that prompted this, or just optimizing for cleaner code? Either way, I’m stealing this approach for my next tweak—thanks for the heads-up! https://ragdollhit.io/
Note You need to log in before you can comment on or make changes to this bug.