Bug 83736 – Don't modify shared style objects in RenderMathMLRoot.cpp

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

[Dave Barton]

Don't modify shared style objects in RenderMathMLRoot.cpp

[Dave Barton]

Created attachment 136802 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 136802 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp:92
> +    IntPoint adjustedPaintOffset = roundedIntPoint(paintOffset + location() + computedCSSContentBoxRect().location());

Is it common to call computed* from within paint?  I thought those were just for CSSComputedStyle?

[Dave Barton]

Comment on attachment 136802 [details]
Patch

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

>> Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp:92
>> +    IntPoint adjustedPaintOffset = roundedIntPoint(paintOffset + location() + computedCSSContentBoxRect().location());
> 
> Is it common to call computed* from within paint?  I thought those were just for CSSComputedStyle?

Yes, I copied that line from Julien's change to RenderMathMLSquareRoot.cpp in bug 83380.

[Julien Chaffraix]

Comment on attachment 136802 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp:79
> +    RenderObject* index = firstChild()->nextSibling();
> +    if (!index || !index->isBoxModelObject())
> +        return 0;
> +    return toRenderBoxModelObject(index);

AFAICT you even have stronger assumptions here. index() is likely to be a RenderBox. I wonder if you *expect* it to be one (meaning you could ASSERT here and maybe keep the checks for defensive programming).

> Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp:89
> +    if (!firstChild() || !index())

This can be simplified to if (!index())

>>> Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp:92
>>> +    IntPoint adjustedPaintOffset = roundedIntPoint(paintOffset + location() + computedCSSContentBoxRect().location());
>> 
>> Is it common to call computed* from within paint?  I thought those were just for CSSComputedStyle?
> 
> Yes, I copied that line from Julien's change to RenderMathMLSquareRoot.cpp in bug 83380.

It's a fairly new paradigm as we needed to stop exposing intrinsic padding to the code outside rendering/. This is not uncommon to call this function as it just returns the value of the padding as specified by CSS vs the padding including intrinsic padding from the padding* functions.

> Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp:164
> +    m_overbarLeftPointShift = 0;
> +    m_intrinsicPaddingAfter = 0;

Moving them in the other branch of the following |if| would be better IMHO.

> Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp:170
>          if (shift > 1.)
>              shift = 1.0f;

Nit: max would look better.

> Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp:190
> +    // |index| should be a RenderBlock here, unless the user has overriden its { position: absolute }.

I don't think I understand this comment here. If you override { position: absolute } it will still be in your child.

[Dave Barton]

Comment on attachment 136802 [details]
Patch

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

I'll submit another patch.

>> Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp:79
>> +    return toRenderBoxModelObject(index);
> 
> AFAICT you even have stronger assumptions here. index() is likely to be a RenderBox. I wonder if you *expect* it to be one (meaning you could ASSERT here and maybe keep the checks for defensive programming).

If it's valid MathML, |index| will come from a MathML element, but MathML token elements like <mn> or <mi> usually just produce a RenderInline, which is a RenderBoxModelObject not a RenderBox. If it's not valid MathML, |index| could come from any element currently.

>>>> Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp:92
>>>> +    IntPoint adjustedPaintOffset = roundedIntPoint(paintOffset + location() + computedCSSContentBoxRect().location());
>>> 
>>> Is it common to call computed* from within paint?  I thought those were just for CSSComputedStyle?
>> 
>> Yes, I copied that line from Julien's change to RenderMathMLSquareRoot.cpp in bug 83380.
> 
> It's a fairly new paradigm as we needed to stop exposing intrinsic padding to the code outside rendering/. This is not uncommon to call this function as it just returns the value of the padding as specified by CSS vs the padding including intrinsic padding from the padding* functions.

Also RenderMathMLRoot and RenderMathMLSquareRoot are like TableCell - they actually have intrinsic padding so they need to know about it and deal with it. Classes without intrinsic padding would just call contentBoxRect().

>> Source/WebCore/rendering/mathml/RenderMathMLRoot.cpp:190
>> +    // |index| should be a RenderBlock here, unless the user has overriden its { position: absolute }.
> 
> I don't think I understand this comment here. If you override { position: absolute } it will still be in your child.

It could then be a RenderInline, as in our discussion in index() above.

[Dave Barton]

Created attachment 136928 [details]
Patch

[WebKit Review Bot]

Comment on attachment 136928 [details]
Patch

Clearing flags on attachment: 136928

Committed r114012: <http://trac.webkit.org/changeset/114012>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.