Bug 156913

Created attachment 277066 [details] Patch (for EWS testing)

Created attachment 277067 [details] Patch

Comment on attachment 277067 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277067&action=review > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:552 > if (size >= stretchSize()) this should be targetSize

Created attachment 277230 [details] Patch Update patch after http://trac.webkit.org/changeset/199978

Created attachment 277471 [details] Patch Minor change: the removal of the character parameter of findStretchyData is done in bug 156910.

Comment on attachment 277471 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277471&action=review The refactor looks good, just some minor points. > Source/WebCore/ChangeLog:10 > + We use union to simplify the structure used to manage the glyph assembly and the type of stretching. I would say the union does not simplify, it saves memory and explains we can not use both members at the same time. Is it like this? I do not see them in WebKit that frequently but in this case it makes sense to add meaning about just one of the options is valid. > Source/WebCore/ChangeLog:11 > + We also modify the signature of some functions to retrieve display operator and stretchy data. Can you add the reason to modify the signature of these functions? > Source/WebCore/ChangeLog:21 > + (WebCore::RenderMathMLOperator::findDisplayStyleLargeOperator): We rename the function and > + change its signature to remove the return value. > + We now also set the italic correction when a display operator is found. Join the lines that are about the same function. > Source/WebCore/ChangeLog:37 > + (WebCore::RenderMathMLOperator::italicCorrection): Deleted. The function is not deleted but moved to the header. > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:308 > + findDisplayStyleLargeOperator(); If I understood correctly this function I'd rather use a name such as: calculateLargeOperatorDisplayStyle. > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:385 > + ASSERT(sizeVariant.isValid() && sizeVariant.font->mathData()); Is the mathData assertion checked every time we call this function? > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:396 > +bool RenderMathMLOperator::getGlyphAssemblyFallBack(Vector<OpenTypeMathData::AssemblyPart> assemblyParts, GlyphAssemblyData& assemblyData) const I would use again calculate in the name of the function. > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:498 > +void RenderMathMLOperator::findDisplayStyleLargeOperator() See previous comment about the name of the function. > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:504 > if (!getBaseGlyph(style(), baseGlyph) || !baseGlyph.font->mathData()) > - return data; > + return; Are we sure the values are the same we got when returning data? > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:524 > +void RenderMathMLOperator::findStretchyData(float* maximumGlyphWidth, LayoutUnit targetSize) I would use calculate again here. > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:89 > + struct GlyphAssemblyData { > + union { > + GlyphData top; > + GlyphData right; > + }; > + GlyphData extension; > + union { > + GlyphData bottom; > + GlyphData left; > + }; > + GlyphData middle; > + GlyphAssemblyData() > + : top(), extension(), bottom(), middle() { } > }; We usually put the constructor as the first member in the structs, also use one line per field initialized. Check other definitions for reference.

Comment on attachment 277471 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277471&action=review >> Source/WebCore/ChangeLog:10 >> + We use union to simplify the structure used to manage the glyph assembly and the type of stretching. > > I would say the union does not simplify, it saves memory and explains we can not use both members at the same time. Is it like this? I do not see them in WebKit that frequently but in this case it makes sense to add meaning about just one of the options is valid. The old code already uses a shared m_data array and some member functions of different names. So the memory management & presentation of members do not really change: it's just becomes a simple struct instead of a more complex class. And yes, there are already ASSERT checking that the we correctly call the members depending of direction / kind of structure. >> Source/WebCore/ChangeLog:11 >> + We also modify the signature of some functions to retrieve display operator and stretchy data. > > Can you add the reason to modify the signature of these functions? Yes I'll do that. It's just because in the MathOperator class we will directly set the members instead of returning them. >> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:385 >> + ASSERT(sizeVariant.isValid() && sizeVariant.font->mathData()); > > Is the mathData assertion checked every time we call this function? Currently this function is called in two places where sizeVariant.font is set to the font that was used to retrieve sizeVariant.glyph value... from the MATH table. So font->mathData() is checked before the use. >> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:396 >> +bool RenderMathMLOperator::getGlyphAssemblyFallBack(Vector<OpenTypeMathData::AssemblyPart> assemblyParts, GlyphAssemblyData& assemblyData) const > > I would use again calculate in the name of the function. I'm not sure I understand the subtility... At the end the goal is to set the variant/assembly in the MathOperator class so I see that set/calculate is better for findDisplayOperator & findStretchyData. However, here the use is different: We only convert the MATH table's GlyphAssembly structure into our fallback GlyphAssemblyData structure (the assembly is only set in findStretchyData). >> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:504 >> + return; > > Are we sure the values are the same we got when returning data? This would return an unstretched data which is how it is initialized in RenderMathMLOperator::updateStyle.

Created attachment 278250 [details] Patch

Comment on attachment 278250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278250&action=review > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:78 > + GlyphAssemblyData() > + : top(), extension(), bottom(), middle() { } This formatting is not so good. Either put it all one one line, or indent the second line please. I don’t think the explicit extension and middle initialization is needed. What happens if you omit this constructor? Do we get a compile time error? Is there some way to get the compiler to generate this without us. I suggest considering omitting these unions since the types are the same. Just: GlyphData topOrRight; > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:94 > + enum StretchType { > + Unstretched, > + SizeVariant, > + GlyphAssembly > + }; I think this usually looks better when all on a single line. Also worth considering using an enum class rather than old-style enum.

Comment on attachment 278250 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278250&action=review > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:118 > + bool calculateGlyphAssemblyFallBack(Vector<OpenTypeMathData::AssemblyPart>, GlyphAssemblyData&) const; Peculiar to take a Vector as an argument. Usually it would be const Vector& or Vector&&.

Committed r200569: <http://trac.webkit.org/changeset/200569>

OK, I've removed the union for GlyphData and used the enum class. The comment about Vector was actually already handled in bug 156921, apparently I forgot it when I split the patch.