Bug 153987 – Refactor RenderMathMLRoot layout function to avoid using flexbox

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

[Alejandro G. Castro]

Next patch in the series refactoring the MathML code to remove flexbox dependency.

[Alejandro G. Castro]

Created attachment 270857 [details]
Patch

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

Created attachment 273193 [details]
Patch

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

Created attachment 273283 [details]
Patch

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

Created attachment 273705 [details]
Patch

I'm uploading updated version of the MathML refactoring patches to solve conflicts after bug 154388 and bug 155005 ; and to do other minor changes.

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

Created attachment 275288 [details]
Patch

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

Created attachment 275400 [details]
Patch

[Brent Fulgham]

Comment on attachment 275400 [details]
Patch

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

This looks like a very nice cleanup of a lot of complicated code. I can't approve it until I see passing tests, so please address the handful of minor comments I made, and upload a rebaselined patch. Thanks! r- due to the need to rebaseline.

> Source/WebCore/ChangeLog:12
> +        msqrt (row of children under a square root) is now implemented directly in RenderMathMLRoot, so RenderMathMLSquareRoot is removed and RenderMathMLRoot now inherits from RenderMathMLRow.

Could you please wrap these on ~100 character boundaries?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3784
> +    return children[0].get();

Nice! Much simpler!

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3796
> +    return children[1].get();

Ditto.

> Source/WebCore/mathml/MathMLInlineContainerElement.cpp:60
> +        if (is<RenderMathMLRow>(*renderer()) && !hasTagName(mrootTag))

This might be better as:

if (renderer() && is<RenderMathMLRow>(*renderer()) && !hasTagName(mrootTag))

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:105
> +    // FIXME: The layout of RenderMathMLMenclose should probably be rewritten from stratch to remove anonymous nodes and style change.

"rewritten from scratch"

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:114
> +    for (size_t i = 0; i < notationalValueSize; i++) {

This would be better as:

for (auto& notionalValue : notionalValues) {
    if (notionalValue == "circle")

> Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:46
> +    virtual void layoutBlock(bool relayoutChildren, LayoutUnit pageLogicalHeight = 0) override;

Don't write virtual with override.

> Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp:-82
> -    ASSERT(!isEmpty());

Are you sure we want to lose this check? isValid() only checks isEmpty if it's not a square root. Is it perhaps impossible to call getBase on square roots?

Maybe is should be ASSERT(m_kind != SquareRoot)?

> Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:140
> +    // See http://wkb.ug/130326 and http://wkb.ug/155019

We usually use "http://webkit.org/b/130326". I'm not sure "wkb.ug" is a WebKit domain.

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

Comment on attachment 275400 [details]
Patch

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

>> Source/WebCore/rendering/mathml/RenderMathMLMenclose.cpp:114
>> +    for (size_t i = 0; i < notationalValueSize; i++) {
> 
> This would be better as:
> 
> for (auto& notionalValue : notionalValues) {
>     if (notionalValue == "circle")

Yes, this is part of the old code and will go away later. This syntax should be used for bug 155019, though.

>> Source/WebCore/rendering/mathml/RenderMathMLMenclose.h:46
>> +    virtual void layoutBlock(bool relayoutChildren, LayoutUnit pageLogicalHeight = 0) override;
> 
> Don't write virtual with override.

Oops, I forgot to update this after the style change.

>> Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp:-82
>> -    ASSERT(!isEmpty());
> 
> Are you sure we want to lose this check? isValid() only checks isEmpty if it's not a square root. Is it perhaps impossible to call getBase on square roots?
> 
> Maybe is should be ASSERT(m_kind != SquareRoot)?

ASSERT(m_kind == RootWithIndex) is stronger than ASSERT(m_kind != SquareRoot) and so ensures that getBase is not called for m_kind == SquareRoot.

The syntax of m_kind == SquareRoot is <msqrt> child1 child2 child3 ... childN </msqrt> so getBase does not make sense in that case (despite what the a11y code does). The layout functions just relies on RenderMathMLRow's implementation and adjusts the metrics and position to add the radical symbol and overbar.

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

Created attachment 275997 [details]
Patch

This patch addressed Brent's comments and do other minor fixes (like adding link to FIXME comment, using logical height/width etc).

However, it still needs to be applied over the 4 other patches.

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

Created attachment 276632 [details]
Patch

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

Created attachment 277375 [details]
Patch

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

Created attachment 278395 [details]
Patch

Updating after https://trac.webkit.org/changeset/200569

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

Created attachment 278615 [details]
Patch for EWS testing

[Build Bot]

Comment on attachment 278615 [details]
Patch for EWS testing

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

New failing tests:
mathml/radical-fallback.html
mathml/presentation/roots.xhtml

[Build Bot]

Created attachment 278616 [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 278617 [details]
Patch

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

Created attachment 281553 [details]
Patch

[Build Bot]

Comment on attachment 281553 [details]
Patch

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

New failing tests:
mathml/radical-fallback.html

[Build Bot]

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

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

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

Created attachment 281563 [details]
Patch

Adding the ios-simulator references for the radical fallback test.

[Brent Fulgham]

Comment on attachment 281563 [details]
Patch

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

So much deleted code! :-) Looks good. r=me.

> LayoutTests/TestExpectations:-120
> -

Yay!

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

Probably could have been 'auto'

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

Committed r202168: <http://trac.webkit.org/changeset/202168>