Bug 83736

Summary: Don't modify shared style objects in RenderMathMLRoot.cpp
Product: WebKit Reporter: Dave Barton <dbarton>
Component: MathMLAssignee: Dave Barton <dbarton>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, jchaffraix, macpherson, menard, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Dave Barton 2012-04-11 17:57:13 PDT
Don't modify shared style objects in RenderMathMLRoot.cpp
Comment 1 Dave Barton 2012-04-11 18:14:55 PDT
Created attachment 136802 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-04-11 18:36:30 PDT
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?
Comment 3 Dave Barton 2012-04-11 18:51:50 PDT
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.
Comment 4 Julien Chaffraix 2012-04-12 09:08:21 PDT
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.
Comment 5 Dave Barton 2012-04-12 10:11:55 PDT
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.
Comment 6 Dave Barton 2012-04-12 10:33:46 PDT
Created attachment 136928 [details]
Patch
Comment 7 WebKit Review Bot 2012-04-12 12:07:25 PDT
Comment on attachment 136928 [details]
Patch

Clearing flags on attachment: 136928

Committed r114012: <http://trac.webkit.org/changeset/114012>
Comment 8 WebKit Review Bot 2012-04-12 12:07:30 PDT
All reviewed patches have been landed.  Closing bug.