Bug 157521

Summary: Implement RenderMathMLOperator::layoutBlock
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: MathMLAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, bfulgham, cfleizach, commit-queue, darin, dbarton, esprehn+autocc, glenn, kondapallykalyan, mrobinson, rego, svillar
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 157519    
Bug Blocks: 155018, 157568    
Attachments:
Description Flags
WIP Patch
none
Patch for EWS testing
buildbot: commit-queue-
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Patch
none
Patch bfulgham: review+

Description Frédéric Wang (:fredw) 2016-05-10 09:31:39 PDT
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.
Comment 1 Frédéric Wang (:fredw) 2016-05-11 06:15:50 PDT
Created attachment 278622 [details]
Patch for EWS testing
Comment 2 Build Bot 2016-05-11 07:13:17 PDT
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
Comment 3 Build Bot 2016-05-11 07:13:20 PDT
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
Comment 4 Frédéric Wang (:fredw) 2016-05-11 07:18:36 PDT
Created attachment 278624 [details]
Patch
Comment 5 Frédéric Wang (:fredw) 2016-06-20 23:52:28 PDT
Created attachment 281719 [details]
Patch
Comment 6 Brent Fulgham 2016-06-21 11:05:03 PDT
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 = ...
Comment 7 Frédéric Wang (:fredw) 2016-06-21 11:15:49 PDT
(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.
Comment 8 Frédéric Wang (:fredw) 2016-06-21 11:34:53 PDT
Committed r202284: <http://trac.webkit.org/changeset/202284>