Bug 157521 - Implement RenderMathMLOperator::layoutBlock
Summary: Implement RenderMathMLOperator::layoutBlock
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords:
Depends on: 157519
Blocks: 155018 157568
  Show dependency treegraph
 
Reported: 2016-05-10 09:31 PDT by Frédéric Wang (:fredw)
Modified: 2016-06-21 11:34 PDT (History)
12 users (show)

See Also:


Attachments
WIP Patch (3.21 KB, patch)
2016-05-10 09:31 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch for EWS testing (822.52 KB, patch)
2016-05-11 06:15 PDT, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (666.68 KB, application/zip)
2016-05-11 07:13 PDT, Build Bot
no flags Details
Patch (292.40 KB, patch)
2016-05-11 07:18 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (292.41 KB, patch)
2016-06-20 23:52 PDT, Frédéric Wang (:fredw)
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>