Bug 159783 – Check whether font is nonnull for GlyphData instead of calling GlyphData::isValid()

• Status: RESOLVED
• Resolution: FIXED
• Component: MathML

[Frédéric Wang (:fredw)]

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())

[Frédéric Wang (:fredw)]

Created attachment 283740 [details]
Patch

[Brent Fulgham]

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 on attachment 283740 [details]
Patch

Clearing flags on attachment: 283740

Committed r203280: <http://trac.webkit.org/changeset/203280>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Frédéric Wang (:fredw)]

(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]

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/