Bug 156542 - Ensure that RenderMathMLOperator::stretchTo functions are called with stretchy operators that have the correct direction
Summary: Ensure that RenderMathMLOperator::stretchTo functions are called with stretch...
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:
Blocks: 152244
  Show dependency treegraph
 
Reported: 2016-04-13 09:26 PDT by Frédéric Wang (:fredw)
Modified: 2016-04-14 11:05 PDT (History)
10 users (show)

See Also:


Attachments
Patch (13.83 KB, patch)
2016-04-13 09:53 PDT, Frédéric Wang (:fredw)
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-yosemite (897.29 KB, application/zip)
2016-04-13 10:57 PDT, Build Bot
no flags Details
Patch (12.93 KB, patch)
2016-04-14 00:33 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (13.81 KB, patch)
2016-04-14 04:37 PDT, Frédéric Wang (:fredw)
darin: 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-04-13 09:26:00 PDT
I'm extracting this change from bug 152244.

This has already been done for horizontal stretchy operators in the RenderMathMLUnderOver refactoring (http://trac.webkit.org/changeset/199293)
Let's do that for RenderMathMLRow and vertical stretchy operators too.
We can then add appropriate ASSERT in the RenderMathMLOperator::stretchTo functions.

Note that this change causes test failures for the following dynamic DOM/style tests, probably because some relayout that used to be forced by useless stretchTo called is not done anymore.

mathml/presentation/mo-lspace-rspace-dynamic.html
mathml/presentation/style-changed.html

So the solution from bug 152244 was to be a bit more aggressive with updates, until we better manage such things in the MathML code (bug 156536).
Comment 1 Frédéric Wang (:fredw) 2016-04-13 09:53:57 PDT
Created attachment 276329 [details]
Patch
Comment 2 Build Bot 2016-04-13 10:57:12 PDT
Comment on attachment 276329 [details]
Patch

Attachment 276329 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1149342

New failing tests:
fast/regions/mathml-crash.html
Comment 3 Build Bot 2016-04-13 10:57:15 PDT
Created attachment 276334 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 4 Frédéric Wang (:fredw) 2016-04-14 00:33:40 PDT
Created attachment 276376 [details]
Patch
Comment 5 Manuel Rego Casasnovas 2016-04-14 03:47:06 PDT
Comment on attachment 276376 [details]
Patch

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

LGTM. Just a small nit comment in the tests.

> LayoutTests/mathml/presentation/row-nonstretchy-or-horizontal.html:2
> +<!-- Horizontal operators or non-stretchy operators inside an mrow element should not stretch. -->

Probably this is common in MathML tests, but its' not that common in other layout tests in WebKit.
This kind of test description is usually in a <p> or something like that. So you can read them if you just run the test in browser.

> LayoutTests/mathml/presentation/underover-nonstretchy-or-vertical.html:2
> +<!-- Vertical or non-stretchy operators inside an munderover element should not stretch. -->

Ditto.
Comment 6 Frédéric Wang (:fredw) 2016-04-14 04:37:27 PDT
Created attachment 276387 [details]
Patch
Comment 7 Darin Adler 2016-04-14 09:44:21 PDT
Comment on attachment 276387 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:208
> +    ASSERT(hasOperatorFlag(MathMLOperatorDictionary::Stretchy) && m_isVertical);

One of our rules for assertions is that we don’t use && at the top level of an assert expression. Instead we separately assert the two expressions on either side of the &&. That way if the assertion fails we can quickly see which failed.
Comment 8 Frédéric Wang (:fredw) 2016-04-14 11:04:41 PDT
Committed r199544: <http://trac.webkit.org/changeset/199544>
Comment 9 Frédéric Wang (:fredw) 2016-04-14 11:05:55 PDT
(In reply to comment #7)
> an assert expression. Instead we separately assert the two expressions on
> either side of the &&. That way if the assertion fails we can quickly see
> which failed.

OK, I split them. Thanks.