Bug 83736

Created attachment 136802 [details] Patch

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 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 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 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.

Created attachment 136928 [details] Patch

Comment on attachment 136928 [details] Patch Clearing flags on attachment: 136928 Committed r114012: <http://trac.webkit.org/changeset/114012>

All reviewed patches have been landed. Closing bug.