[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 #10 from Julien Chaffraix <jchaffraix at webkit.org>  2012-03-27 14:07:09 PST ---
(From update of attachment 134098)
View in context: https://bugs.webkit.org/attachment.cgi?id=134098&action=review

Some comments.

>>> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:83
>>> +    setNeedsLayout(true, false);
>> 
>> I wish these were enums. :)  Maybe I'll fix that today!
> 
> I'll add a comment.

This will be switched to enums so no need for that.

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:92
>      

Above, there is a call to

RenderMathMLBlock::paint(info, paintOffset);

It should be updated to the new base class.

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:96
> +    int baseHeight = contentLogicalHeight();
> +    int overbarWidth = contentLogicalWidth();

There should be a conversion here as you are taking a LayoutUnit and moving it to |int| (now they are the same but it should change soon). You have 2 choices here:
* roundToInt
* floorToInt

For consistency, maybe flooring is better but I will let you judge.

>> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:166
>> +    ASSERT(style()->refCount() == 1);
> 
> You should explain why this is with a brief comment.

I agree, I was going to say that this was wrong due to style sharing but it works because of the cloning...

>>> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:168
>>> +    setNeedsLayout(true, false);
>> 
>> Is this needed to cause our children to layout?
> 
> I may be confused, but I added it for the opposite reason. If our children are perhaps marked as needing layout, but we aren't, then some really smart code might not pick up our style()->setPaddingBottom(...). I thought to be pedantic, we should ensure selfNeedsLayout() here.

I think this is needed indeed as you could hit some of the dedicated layout phases. Please add a huge FIXME as we are modifying our style prior layout which should not happen.

>>> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:177
>>> +        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()?
> 
> Huge subtrees shouldn't be re-layed-out. Our children will be marked as not needing layout after our first layout() call, so they won't be laid out again by our second layout() call. (Right?)

Calling setNeedsLayout(true, false) is not uncommon. There are different uses:
* usually it's for ensuring that a child is laid out to respond to some changes (the example is to respond to intrinsic padding change on table cells). In this case, the call is made *before* calling layout though.
* sometimes it is to implement a 2 pass layout like here. This is usually a sign that something is bad (see RenderTextControlSingleLine::layout for example).

I don't see any potential issues here apart that it should be RenderMathMLRow::layout. We should move to a dedicated intrinsic padding to handle that which would remove most of this complexity.

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.h:40
> +    virtual void styleDidChange(StyleDifference, const RenderStyle* oldStyle);

Let annotate the function with OVERRIDE.

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