Summary: | Move selection and drawing of stretchy operators into a separate MathOperator class | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Frédéric Wang (:fredw) <fred.wang> | ||||||||||||||||||||
Component: | MathML | Assignee: | Frédéric Wang (:fredw) <fred.wang> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | alex, mrobinson, rego | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Bug Depends on: | 156906, 156910, 156913 | ||||||||||||||||||||||
Bug Blocks: | 152244, 156950, 157071 | ||||||||||||||||||||||
Attachments: |
|
Description
Frédéric Wang (:fredw)
2016-04-22 12:50:07 PDT
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> |