[webkit-reviews] review granted: [Bug 96843] Convert MathML to use flexboxes : [Attachment 164258] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Dave Barton
<dbarton at mathscribe.com>'s request for review:
Bug 96843: Convert MathML to use flexboxes
https://bugs.webkit.org/show_bug.cgi?id=96843

Attachment 164258: Patch
https://bugs.webkit.org/attachment.cgi?id=164258&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=164258&action=review


This looks technically fine to me.  I trust that Ojan/Tony gave this at least a
cursory flex-box review, and the MathML experts seem happy with your pixel
results!  Thank you again for all your hard work on this.  Please consider the
nits below before landing.   I look forward to seeing this on in Chrome!

> Source/WebCore/rendering/mathml/RenderMathMLBlock.h:123
> +class RenderMathMLTable : public RenderTable {

So this whole override will not be necessary once the flexbox baseline bug is
fixed?	If so, we should note that in a comment (so others can know when to
remove this.)

> Source/WebCore/rendering/mathml/RenderMathMLFraction.cpp:58
> +    child->style()->setFlexDirection(FlowColumn);

I'm not familiar with what this does.  Presumably Ojan/Tony are and thought it
was OK. :)

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:137
> +    LayoutUnit baseBaseline = base->firstLineBoxBaseline();
> +    if (baseBaseline == -1)
> +	   baseBaseline = baseHeight;

It seems awkward that firstLineBoxBaseline uses this in-band signaling/sentinal
value. :(  It also seems like this pattern of "lookup or return the height"
could be abstracted into a static inline where you pass the baseHeight and the
base memeber and get back a single baseBaseline result.  It would be used in at
least 3 places in this function alone. :)