Bug 157521 – Implement RenderMathMLOperator::layoutBlock

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

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

Created attachment 278502 [details]
WIP Patch

This is another piece from bug 155018, where we implement layoutBlock for operators with useMathOperator(). This improves a bit the metrics of stretchy operators and so will require some rebasing some tests.

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

Created attachment 278622 [details]
Patch for EWS testing

[Build Bot]

Comment on attachment 278622 [details]
Patch for EWS testing

Attachment 278622 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1303846

New failing tests:
mathml/opentype/opentype-stretchy.html
mathml/opentype/horizontal-munderover.html

[Build Bot]

Created attachment 278623 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

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

Created attachment 278624 [details]
Patch

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

Created attachment 281719 [details]
Patch

[Brent Fulgham]

Comment on attachment 281719 [details]
Patch

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

I'm trusting your judgement on the changes to the layout tests (they look good to my untrained eye). Otherwise, this change seems like a good direction, and I hope it reduces some layout churn by not triggering unnecessary style changes.

r=me.

> Source/WebCore/ChangeLog:10
> +        We start implement RenderMathMLOperator::layoutBlock to do special layout when the

This wording is weird. I think it might be better to say:

"Add an initial implementation of RenderMathMLOperator::layoutBlock, which will perform special layout when the..."

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:214
> +    setLogicalHeight(m_mathOperator.ascent() + m_mathOperator.descent());

This seems like a better approach. I assume 'stretchTo' was sometimes getting called during layout, and we ended up triggering additional layouts because of the style change?

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:271
> +        for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox())

for (auto child = ...

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

(In reply to comment #6)
> > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:214
> > +    setLogicalHeight(m_mathOperator.ascent() + m_mathOperator.descent());
> 
> This seems like a better approach. I assume 'stretchTo' was sometimes
> getting called during layout, and we ended up triggering additional layouts
> because of the style change?

Thanks for the review. Yes, stretchTo is called during the layout of mrow and munderover for sure. Then it's not clear what happens when RenderMathMLOperator::updateStyle() is called but we definitely want to avoid this kind of style change during layout.

More improvements and removal of RenderMathMLOperator::updateStyle will happen in bug 155018, this is just one more step.

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

Committed r202284: <http://trac.webkit.org/changeset/202284>