Bug 153742 – Refactor RenderMathMLUnderOver layout functions to avoid using flexbox

• Component: MathML
• Status: RESOLVED FIXED

[Alejandro G. Castro]

Following patch in the set of patches trying to refactor MathML code without flexbox, this time refactor UnderOver renderer.

[Alejandro G. Castro]

Created attachment 270392 [details]
Patch

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

Comment on attachment 270392 [details]
Patch

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

> Source/WebCore/css/mathml.css:47
>  

I think it makes sense to remove the following rules too (if that does not break tests):

munder, mover, munderover {
    align-items: center;
}

mover > :last-child, munderover > :last-child {
    order: -1;
}

[Alejandro G. Castro]

(In reply to comment #2)
> Comment on attachment 270392 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270392&action=review
> 
> > Source/WebCore/css/mathml.css:47
> >  
> 
> I think it makes sense to remove the following rules too (if that does not
> break tests):
> 
> munder, mover, munderover {
>     align-items: center;
> }
> 
> mover > :last-child, munderover > :last-child {
>     order: -1;
> }

I tested them and it works for me, thanks for the proposal, I've upload a modified version.

[Alejandro G. Castro]

Created attachment 271163 [details]
Patch

[Martin Robinson]

Comment on attachment 271163 [details]
Patch

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

Looks good. I have a few minor nits. I really like the way this code is moving.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:98
> -            stretchWidth = std::max<LayoutUnit>(stretchWidth, downcast<RenderBox>(*child).logicalWidth());
> +            stretchWidth = std::max<LayoutUnit>(stretchWidth, downcast<RenderBox>(*child).maxPreferredLogicalWidth());

Do you mind double-checking why this is necessary and updating the changelog?

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:147
> +    RenderBox* base = nullptr;
> +    RenderBox* underScript = nullptr;
> +    RenderBox* overScript = nullptr;
> +    if (!getBaseAndUnderAndOverScripts(base, underScript, overScript))
> +        return;

I would split this into three methods:
- One that returns the base getBase that returns a reference.
- One that returns a pointer getOver(RenderBox&)
- One that returns a pointer getUnder(RenderBox&)

I think that will make the code quite a bit easier to read and help prevent inadvertently returning NULL pointers.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:153
> +    m_minPreferredLogicalWidth = m_maxPreferredLogicalWidth = std::max<LayoutUnit>(basePreferredWidth, std::max<LayoutUnit>(underPreferredWidth, overPreferredWidth));

You might need to call setPreferredLogicalWidthsDirty(false); here.

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

Comment on attachment 271163 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:182
> +        underScript->layoutIfNeeded();

Can you please also set the logical width based on the logical widths of children after layout (instead of the preferred widths used by recomputeLogicalWidth)? That will give more accurate value in the case where the expression contains vertical stretchy operators.

See https://github.com/alexgcastro/webkit/commit/906b30791b88996ef1327de8b34a75165a561e4a#diff-3c2dcb037c1593b2df71c13e8e72f618R184

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

Created attachment 272755 [details]
Patch

> Do you mind double-checking why this is necessary and updating the changelog?

This does not seem necessary. At that point the child is already laid out so the logical width should already be caculated and using it will give more accurate value (again because of https://bugs.webkit.org/show_bug.cgi?id=130326).

> I would split this into three methods:

I'm not sure that's really what you asked for, but I've used three functions returning a RenderBox& and use a isValid function + m_kind checks + ASSERT to ensure all the calls are correct.

> You might need to call setPreferredLogicalWidthsDirty(false); here.

Done, and also added the ASSERT.

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

Comment on attachment 272755 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:160
> +        return;

I forgot removing the dirty flag here.

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

Comment on attachment 272755 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:110
> +    // <munderover> base under over <munderover>

should be </munder>, </mover>, </munderover>

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

Created attachment 273190 [details]
Patch

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

Created attachment 273701 [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 275284 [details]
Patch

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

Created attachment 275397 [details]
Patch

[Javier Fernandez]

Comment on attachment 275397 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:69
> +

The idea behind original code was that if Base (firstChild) is a complex object which defines its own baseline we will use it as part of the containers first-line baseline. 
The new code seems to assume that base element of the under/over script has always maxAscentForChild as firstLine baseline. 

I don't the change is wrong at all, but other layout models follow a more general approach by relying on the first child's baseline implementation.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:120
> +        break;

No need to break here because we already return.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:123
> +        break;

Ditto.

[Javier Fernandez]

I implemented most of that code, so it'd be better if someone else could review it as well and give the review result.

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

Comment on attachment 275397 [details]
Patch

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

>> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:69
>> +
> 
> The idea behind original code was that if Base (firstChild) is a complex object which defines its own baseline we will use it as part of the containers first-line baseline. 
> The new code seems to assume that base element of the under/over script has always maxAscentForChild as firstLine baseline. 
> 
> I don't the change is wrong at all, but other layout models follow a more general approach by relying on the first child's baseline implementation.

In the old MathML code, when firstBaseLine was called on a child and that child does not have a baseline, then we fallback to the child height. This operation is now always done by the helper function ascentForChild introduced in bug 153742, also provides better accuracy in some cases (when the ascent is computed from LayoutUnit's rather than only a firstLineBaseLine int, see bug 155879).

I'm not sure what happens when firstBaseLine is called outside the MathML code, but I thought it was easier to just reuse ascentForChild here and it seems that we get better result when rounding the sum of LayoutUnit's instead of summing up an int and a LayoutUnit.

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

Created attachment 275879 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 275879 [details]
Patch

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

r=me. Check my comments before landing.

> Source/WebCore/ChangeLog:26
> +        (WebCore::RenderMathMLUnderOver::getOver): Added. Helper function.

Fix these 3 names so they don't include the get prefix

> Source/WebCore/ChangeLog:29
> +        (WebCore::RenderMathMLUnderOver::getHorizontalOffset): Added, helper to calculate the

Ditto.

> Source/WebCore/ChangeLog:31
> +        (WebCore::RenderMathMLUnderOver::layoutBlock): Added, it layouts the base, underscript and

Nit: lays out

> Source/WebCore/ChangeLog:33
> +        one child contains stretchy operators. It later sets the locations of children accordingly

something is wrong/missing in this sentence "from the preferred width one child..."

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:82
> +                    if (renderOperator->hasOperatorFlag(MathMLOperatorDictionary::Stretchy) && !renderOperator->isVertical()) {

I am not qualified to review the test results but we should definitely have a test for this change.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:116
> +    switch (m_kind) {

I know this is a refactoring, but we should definitely replace m_kind by a more descriptive name like m_underOverType, a bit longer but much more understandable. We can do that in a follow up patch though.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:202
> +    // We set the logical width.

Remove this useless comment.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:208
> +    setLogicalWidth(logicalWidth);

So if I understood this correctly, first we compute the width of the container, then we compute the height of base and/or under/over, and then we stretch the value of the container to the max of under/over. Is that how it should work?

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

Comment on attachment 275879 [details]
Patch

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

>> Source/WebCore/ChangeLog:33
>> +        one child contains stretchy operators. It later sets the locations of children accordingly
> 
> something is wrong/missing in this sentence "from the preferred width one child..."

I think it's "when one child"

>> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:82
>> +                    if (renderOperator->hasOperatorFlag(MathMLOperatorDictionary::Stretchy) && !renderOperator->isVertical()) {
> 
> I am not qualified to review the test results but we should definitely have a test for this change.

I'll try adding a test for that. However, this is an optimization to avoid calling stretchTo when an operator is not stretchy. Such call is likely to fail for non-stretchy operator so I don't think there will be a visible change. In RenderMathMLOperator::stretchTo we add some ASSERT to prevent such call so a test would at least work in debug mode.

>> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:116
>> +    switch (m_kind) {
> 
> I know this is a refactoring, but we should definitely replace m_kind by a more descriptive name like m_underOverType, a bit longer but much more understandable. We can do that in a follow up patch though.

In bug 155542, we will make RenderMathMLUnderOver inherit from RenderMathMLScripts and share the same member for that. So probably m_scriptType would be better?

>> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:208
>> +    setLogicalWidth(logicalWidth);
> 
> So if I understood this correctly, first we compute the width of the container, then we compute the height of base and/or under/over, and then we stretch the value of the container to the max of under/over. Is that how it should work?

I'm not sure I understand the question but see http://www.mathml-association.org/MathMLinHTML5/S3.html#F23

The height of the container is essentially the sum of the heights of the base, under, over.
The width of the container is the maximum width of the base, under, over.

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

Created attachment 276136 [details]
Final Patch

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

Committed r199293: <http://trac.webkit.org/changeset/199293>