Bug 156921

Created attachment 277087 [details] Patch

Created attachment 277088 [details] Patch for EWS testing

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

Created attachment 277231 [details] Patch Update patch to fix conflict after bug 156846 and bug 156906.

Created attachment 277253 [details] Patch

Created attachment 277355 [details] Patch

Created attachment 277473 [details] Patch Minor change: Remove useless MathOperator:: prefix in setGlyphAssembly during the code move.

Created attachment 278390 [details] Patch Updating after https://trac.webkit.org/changeset/200569

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

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

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

Created attachment 280783 [details] Patch

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