Bug 156921 – Move selection and drawing of stretchy operators into a separate MathOperator class

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

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

Yet another step towards bug 152244.

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

Created attachment 277087 [details]
Patch

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

Created attachment 277088 [details]
Patch for EWS testing

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

Created attachment 277144 [details]
Patch for EWS testing

I'm removing the ASSERT for large operator until we know the exact operator type (bug 156950).

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

Created attachment 277231 [details]
Patch

Update patch to fix conflict after bug 156846 and bug 156906.

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

Created attachment 277253 [details]
Patch

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

Created attachment 277355 [details]
Patch

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

Created attachment 277473 [details]
Patch

Minor change: Remove useless MathOperator:: prefix in setGlyphAssembly during the code move.

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

Created attachment 278390 [details]
Patch

Updating after https://trac.webkit.org/changeset/200569

[Martin Robinson]

Comment on attachment 278390 [details]
Patch

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

Most of this is just code movement, so it looks good to me. I have a couple quick questions and nits though:

> Source/WebCore/rendering/mathml/MathOperator.cpp:85
> +    : m_baseCharacter(0)
> +    , m_isVertical(false)
> +    , m_stretchType(StretchType::Unstretched)
> +    , m_width(0)
> +    , m_ascent(0)
> +    , m_descent(0)
> +    , m_italicCorrection(0)

I think we are preferring in-class declaration initializers now. Perhaps that will allow you to remove this constructor.

http://www.stroustrup.com/C++11FAQ.html#member-init

> Source/WebCore/rendering/mathml/MathOperator.cpp:103
> +    ASSERT(sizeVariant.isValid() && sizeVariant.font->mathData());

Make this ASSERT two separate ASSERTs so it is easier to tell from a stack trace which one failed.

> Source/WebCore/rendering/mathml/MathOperator.cpp:128
> +    baseGlyph.font->mathData()->getMathVariants(baseGlyph.glyph, true, sizeVariants, assemblyParts);

Move the declaration of sizeVariants and assemblyParts right before this line.

> Source/WebCore/rendering/mathml/MathOperator.cpp:137
> +        GlyphData glyphData(sizeVariant, baseGlyph.font);
> +        setSizeVariant(glyphData);
> +        m_italicCorrection = glyphData.font->mathData()->getItalicCorrection(*glyphData.font, glyphData.glyph);
> +        if (heightForGlyph(glyphData) >= displayOperatorMinHeight)
> +            break;
> +    }

Why not wait to call setSizeVariant and set m_italicCorrection until heightForGlyph(glyphData) >= displayOperatorMinHeight is true?

> Source/WebCore/rendering/mathml/MathOperator.cpp:140
> +bool MathOperator::calculateGlyphAssemblyFallBack(const RenderStyle& style, const Vector<OpenTypeMathData::AssemblyPart>& assemblyParts, GlyphAssemblyData& assemblyData) const

Nit: FallBack -> Fallback

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

(In reply to comment #9)
> > Source/WebCore/rendering/mathml/MathOperator.cpp:85
> > +    : m_baseCharacter(0)
> > +    , m_isVertical(false)
> > +    , m_stretchType(StretchType::Unstretched)
> > +    , m_width(0)
> > +    , m_ascent(0)
> > +    , m_descent(0)
> > +    , m_italicCorrection(0)
> 
> I think we are preferring in-class declaration initializers now. Perhaps
> that will allow you to remove this constructor.

OK, I think I already did that in my branch after feedback in other patches :-)

> > Source/WebCore/rendering/mathml/MathOperator.cpp:137
> > +        GlyphData glyphData(sizeVariant, baseGlyph.font);
> > +        setSizeVariant(glyphData);
> > +        m_italicCorrection = glyphData.font->mathData()->getItalicCorrection(*glyphData.font, glyphData.glyph);
> > +        if (heightForGlyph(glyphData) >= displayOperatorMinHeight)
> > +            break;
> > +    }
> 
> Why not wait to call setSizeVariant and set m_italicCorrection until
> heightForGlyph(glyphData) >= displayOperatorMinHeight is true?

So the idea is that if we were not able to find a large enough stretchy operator, we use the latest (largest) one in the list as a fallback.

[Martin Robinson]

(In reply to comment #10)

> > > Source/WebCore/rendering/mathml/MathOperator.cpp:137
> > > +        GlyphData glyphData(sizeVariant, baseGlyph.font);
> > > +        setSizeVariant(glyphData);
> > > +        m_italicCorrection = glyphData.font->mathData()->getItalicCorrection(*glyphData.font, glyphData.glyph);
> > > +        if (heightForGlyph(glyphData) >= displayOperatorMinHeight)
> > > +            break;
> > > +    }
> > 
> > Why not wait to call setSizeVariant and set m_italicCorrection until
> > heightForGlyph(glyphData) >= displayOperatorMinHeight is true?
> 
> So the idea is that if we were not able to find a large enough stretchy
> operator, we use the latest (largest) one in the list as a fallback.

Ah, I understand. Thanks!

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

Created attachment 280783 [details]
Patch

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

Committed r201854: <http://trac.webkit.org/changeset/201854>