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+

Frédéric Wang (:fredw)
Reported 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.
Attachments
WIP Patch (3.21 KB, patch)
2016-05-10 09:31 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
Patch for EWS testing (822.52 KB, patch)
2016-05-11 06:15 PDT, Frédéric Wang (:fredw) 		buildbot: commit-queue-
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
Patch (292.41 KB, patch)
2016-06-20 23:52 PDT, Frédéric Wang (:fredw) 		bfulgham: review+
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Frédéric Wang (:fredw)
Comment 1 2016-05-11 06:15:50 PDT
Created attachment 278622 [details] Patch for EWS testing
Build Bot
Comment 2 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
Build Bot
Comment 3 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
Frédéric Wang (:fredw)
Comment 4 2016-05-11 07:18:36 PDT
Created attachment 278624 [details] Patch
Frédéric Wang (:fredw)
Comment 5 2016-06-20 23:52:28 PDT
Created attachment 281719 [details] Patch
Brent Fulgham
Comment 6 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 = ...
Frédéric Wang (:fredw)
Comment 7 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.
Frédéric Wang (:fredw)
Comment 8 2016-06-21 11:34:53 PDT
Committed r202284: <http://trac.webkit.org/changeset/202284>
Note You need to log in before you can comment on or make changes to this bug.