[Webkit-unassigned] [Bug 82353] msqrt's implied mrow should do operator stretching

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=82353





--- Comment #4 from Eric Seidel <eric at webkit.org>  2012-03-27 11:04:55 PST ---
(From update of attachment 134098)
View in context: https://bugs.webkit.org/attachment.cgi?id=134098&action=review

The result looks OK. The code looks tolerable.  (Nothing against your coding skills, simply ho-hum on this design we hashed out at our meeting.)  Would be curious of Julien's opinion.

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:81
> +    newStyle->setPaddingLeft(Length(gFrontWidthEms * style()->fontSize(), Fixed));
> +    newStyle->setPaddingTop(Length(gPaddingTopEms * style()->fontSize(), Fixed));

So this overrides any padding-left/padding-top set by CSS (as we discussed), just confirming?

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:83
> +    setNeedsLayout(true, false);

I wish these were enums. :)  Maybe I'll fix that today!

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:166
> +    ASSERT(style()->refCount() == 1);

You should explain why this is with a brief comment.

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:168
> +    setNeedsLayout(true, false);

Is this needed to cause our children to layout?

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:177
> +        setNeedsLayout(true, false);
> +        
> +        RenderBlock::layout();

This style of calling layout from within layout concerns me.  These could be huge-subtrees we're re-laying-out.   It may be OK for now, but do we have a plan for the "right" way?  Do we know if other parts of the code call setNeedsLayout() and layout() from within layout()?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.