msqrt's implied mrow should do operator stretching

• Component: MathML
• Status: RESOLVED FIXED

[Dave Barton]

msqrt's implied mrow should do operator stretching

[Dave Barton]

Created attachment 134098 [details]
Patch

[WebKit Review Bot]

Attachment 134098 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/math..." exit_code: 1
LayoutTests/platform/mac/mathml/presentation/mo-stretch-expected.png:0:  Have to enable auto props in the subversion config file (/home/ubuntu/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/ubuntu/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dave Barton]

David Carlisle, do you want to review the MathML content of this for the WebKit folks? Click on "Review Patch" or "Formatted Diff" for the above patch, and near the end there's a test case added to mo-stretch.html, and at the very end the output of that case. The parentheses inside the radical are stretched around the 1/y. If you could comment briefly saying whether this agrees with the MathML spec, the WebKit reviewers will doubtless buy you a beer next time they're in England. :) Thanks for your help!

P.S. Don't worry about the "f" overlapping the "(" - that's an existing problem, that will be fixed in 2 bugs/patches from now, I believe.

[Eric Seidel (no email)]

Comment on attachment 134098 [details]
Patch

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()?

[Dave Barton]

(In reply to comment #4)
> (From update of attachment 134098 [details])
> 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.

I have been thinking & reading code & trying out things since our meeting, and I think we can basically use absolute positioning instead of anonymous blocks for a lot of stuff - so the next bug/patch may be less ho-hum!

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

Yes, this patch overrides any user padding-left/padding-top. I don't remember us discussing it, and it isn't ideal, but it's no worse than what's done now. Fixing this is definitely lower priority than getting basic MathML working in Safari/Chrome/etc., IMHO, since users will rarely add extra padding to a <msqrt>, and could do it if necessary with an <mpadded> or other wrapper element.

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

I'll add a comment.

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

> > 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()?

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?)

[Eric Seidel (no email)]

I fixed the (true, true) nonsense in bug 82369.

[Dave Barton]

I'm looking at redoing this using intrinsic padding. Thanks for the suggestions and sorry for any time any of you might consider wasted looking at the current patch.

[Eric Seidel (no email)]

No time wasted.  :)  I could be OK with this patch, but I would like Jullien to comment before we move forward.

[David Carlisle]

I can confirm that the () stretching to the size of its parent implied or real mrow as shown in the patch image is as required by the MathML spec

[Julien Chaffraix]

Comment on attachment 134098 [details]
Patch

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.

[Dave Barton]

Created attachment 136062 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 136062 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:45
> +    , m_intrinsicPaddingBefore(0)
> +    , m_intrinsicPaddingAfter(0)
> +    , m_intrinsicPaddingStart(0)
> +    , m_intrinsicPaddingEnd(0)

We need a type for these, sadly.  (Not your responsiblity to add one.  Just generally lamenting about a lack of a good Quad type for this.)

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:77
> +    if (!firstChild()) {
> +        RefPtr<RenderStyle> newStyle = RenderStyle::createAnonymousStyle(style());
> +        newStyle->setDisplay(INLINE_BLOCK);

Are we going to need this "wrap me in an anonymous row" call in a bunch of places?

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:79
> +        RenderMathMLRow* newMRow = new (renderArena()) RenderMathMLRow(document() /* anonymous block */);

We should add a RenderMathMLRow::createAnonymous(document) function at some point.  This is fine, but always forcing return of PassOwnPtr is a safer long-term design.  (Again, not a comment on this pathc, more a communication about my future design thoughts for the rendering tree.)

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:94
> +    if (oldChild == firstChild() || !firstChild())
> +        RenderMathMLBlock::removeChild(oldChild);
> +    else
> +        firstChild()->removeChild(oldChild);

Is this how things like RenderBlock work (which will create anonymous blocks to wrap inline kids?)  This seems more complicated than I woudl think.

[Eric Seidel (no email)]

Comment on attachment 136062 [details]
Patch

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

>> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:45
>> +    , m_intrinsicPaddingBefore(0)
>> +    , m_intrinsicPaddingAfter(0)
>> +    , m_intrinsicPaddingStart(0)
>> +    , m_intrinsicPaddingEnd(0)
> 
> We need a type for these, sadly.  (Not your responsiblity to add one.  Just generally lamenting about a lack of a good Quad type for this.)

I had assumed these were top/bottom/left/right... Is that not the case?

[Dave Barton]

> View in context: https://bugs.webkit.org/attachment.cgi?id=136062&action=review
> 
> > Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:77
> > +    if (!firstChild()) {
> > +        RefPtr<RenderStyle> newStyle = RenderStyle::createAnonymousStyle(style());
> > +        newStyle->setDisplay(INLINE_BLOCK);
> 
> Are we going to need this "wrap me in an anonymous row" call in a bunch of places?

See http://www.w3.org/TR/MathML3/chapter3.html#presm.inferredmrow - inferred <mrow>s occur for msqrt, mstyle, merror, mpadded, mphantom, menclose, mtd, mscarry, and math. <math>'s renderer RenderMathMLMath is about the only one implemented so far, and it derives from RenderMathMLRow using inheritance. We could factor this code out when we start implementing the others, if that's what you're suggesting.

> > Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:79
> > +        RenderMathMLRow* newMRow = new (renderArena()) RenderMathMLRow(document() /* anonymous block */);
> 
> We should add a RenderMathMLRow::createAnonymous(document) function at some point.  This is fine, but always forcing return of PassOwnPtr is a safer long-term design.  (Again, not a comment on this pathc, more a communication about my future design thoughts for the rendering tree.)

Sounds good to me. I'm just staying consistent for now with existing code elsewhere.
 
> > Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:94
> > +    if (oldChild == firstChild() || !firstChild())
> > +        RenderMathMLBlock::removeChild(oldChild);
> > +    else
> > +        firstChild()->removeChild(oldChild);
> 
> Is this how things like RenderBlock work (which will create anonymous blocks to wrap inline kids?)  This seems more complicated than I woudl think.

RenderButton::removeChild is like this, to deal with its anonymous block child. The Ruby classes are more complex to deal with theirs, and RenderBlock::removeChild is even more complex.

===

> View in context: https://bugs.webkit.org/attachment.cgi?id=136062&action=review
> 
> >> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:45
> >> +    , m_intrinsicPaddingBefore(0)
> >> +    , m_intrinsicPaddingAfter(0)
> >> +    , m_intrinsicPaddingStart(0)
> >> +    , m_intrinsicPaddingEnd(0)
> > 
> > We need a type for these, sadly.  (Not your responsiblity to add one.  Just generally lamenting about a lack of a good Quad type for this.)
> 
> I had assumed these were top/bottom/left/right... Is that not the case?

That's correct in horizontal left-to-right writing mode, but not other modes. See http://www.w3.org/TR/css3-writing-modes/#logical-to-physical and the code in RenderMathMLBlock.cpp in this patch.

[Dave Barton]

P.S. We should definitely let Julien comment on this before any review+, because of the intrinsic padding issue. I'm working on commenting on his bug 83380 about that. We could also add FIXME comments at least that mention Eric's issues.

[Julien Chaffraix]

Dave, when you reply to comments, could you use the 'Review Patch' link. As it is, your comment are not shown up when reviewing and it makes it more difficult to comment and review at the same time.

(In reply to comment #14)
> > View in context: https://bugs.webkit.org/attachment.cgi?id=136062&action=review
> > 
> > > Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:77
> > > +    if (!firstChild()) {
> > > +        RefPtr<RenderStyle> newStyle = RenderStyle::createAnonymousStyle(style());
> > > +        newStyle->setDisplay(INLINE_BLOCK);
> > 
> > Are we going to need this "wrap me in an anonymous row" call in a bunch of places?
> 
> See http://www.w3.org/TR/MathML3/chapter3.html#presm.inferredmrow - inferred <mrow>s occur for msqrt, mstyle, merror, mpadded, mphantom, menclose, mtd, mscarry, and math. <math>'s renderer RenderMathMLMath is about the only one implemented so far, and it derives from RenderMathMLRow using inheritance. We could factor this code out when we start implementing the others, if that's what you're suggesting.
> 
> > > Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:79
> > > +        RenderMathMLRow* newMRow = new (renderArena()) RenderMathMLRow(document() /* anonymous block */);
> > 
> > We should add a RenderMathMLRow::createAnonymous(document) function at some point.  This is fine, but always forcing return of PassOwnPtr is a safer long-term design.  (Again, not a comment on this pathc, more a communication about my future design thoughts for the rendering tree.)
> Sounds good to me. I'm just staying consistent for now with existing code elsewhere.

Actually I would prefer if we stay consistent with the pattern we have started deploying with Abhishek, basically you expose a

static RenderMathMLRow* RenderMathMLRow::createAnonymousWithParentRenderer(const RenderObject*);

> > > Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:94
> > > +    if (oldChild == firstChild() || !firstChild())
> > > +        RenderMathMLBlock::removeChild(oldChild);
> > > +    else
> > > +        firstChild()->removeChild(oldChild);
> > 
> > Is this how things like RenderBlock work (which will create anonymous blocks to wrap inline kids?)  This seems more complicated than I woudl think.
> 
> RenderButton::removeChild is like this, to deal with its anonymous block child. The Ruby classes are more complex to deal with theirs, and RenderBlock::removeChild is even more complex.

I don't really think this code is right to be frank. Your logic is simple and shouldn't need any special casing here (I wouldn't expect the function to be called with |oldChild| belonging to firstChild() for example). Also if you don't have a firstChild, I don't think you would be called anyway as you have nothing to remove. All in all, IMHO this code could be removed and let the base class handle that just fine (this is what the table code is doing and it's much more close to RenderMathMLSquareRoot than RenderButton).

> ===
> 
> > View in context: https://bugs.webkit.org/attachment.cgi?id=136062&action=review
> > 
> > >> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:45
> > >> +    , m_intrinsicPaddingBefore(0)
> > >> +    , m_intrinsicPaddingAfter(0)
> > >> +    , m_intrinsicPaddingStart(0)
> > >> +    , m_intrinsicPaddingEnd(0)
> > > 
> > > We need a type for these, sadly.  (Not your responsiblity to add one.  Just generally lamenting about a lack of a good Quad type for this.)
> > 
> > I had assumed these were top/bottom/left/right... Is that not the case?
> 
> That's correct in horizontal left-to-right writing mode, but not other modes. See http://www.w3.org/TR/css3-writing-modes/#logical-to-physical and the code in RenderMathMLBlock.cpp in this patch.

I agree with your naming, Dave. We want a writing-mode aware naming which is the case here. Also it matches what is in RenderTableCell. Now intrinsic padding is not a super good name as it can be easily confused with some CSS padding. Discussing this a bit with other people, the name 'alignment padding' or 'alignment space' was mentioned (it's used for flex-boxes for example). Not sure how MathML wants to use that but if you have a more explicit naming that would be nice.

[Julien Chaffraix]

Comment on attachment 136062 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:34
> +#include "PaintInfo.h" // for ENABLE(DEBUG_MATH_LAYOUT)

It would be nice to wrap that in #if ENABLE(DEBUG_MATH_LAYOUT) ... #endif.

We haven't done a great job at that so it's more a nit than anything.

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:64
> +    WritingMode writingMode = style()->writingMode();
> +    if (writingMode == TopToBottomWritingMode)
> +        return result + m_intrinsicPaddingBefore;
> +    if (writingMode == BottomToTopWritingMode)
> +        return result + m_intrinsicPaddingAfter;
> +    return result + (style()->isLeftToRightDirection() ? m_intrinsicPaddingStart : m_intrinsicPaddingEnd);

I think a switch statement would be way better as it would ensure you don't miss any case while making the code more readable.

> Source/WebCore/rendering/mathml/RenderMathMLBlock.h:69
> +    virtual void paint(PaintInfo&, const LayoutPoint&) OVERRIDE;

Usually we only mark the functions that we touched to avoid unnecessary churn when blaming.

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:106
> +    m_intrinsicPaddingStart = static_cast<int>(roundf(gFrontWidthEms * style()->fontSize()));

I wonder why you need to reset this twice: once in layout() and once in computePreferredLogicalWidth(). It sounds like once should be enough (and I would bet on computePreferredLogicalWidths as you need to keep your existing padding in the old width).

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:108
> +    m_intrinsicPaddingAfter = 0;

m_intrinsicPaddingEnd is never reset...

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:116
> +    int baseHeight = roundToInt(getBoxModelObjectHeight(firstChild()));
> +    if (baseHeight > static_cast<int>(gThresholdBaseHeightEms * style()->fontSize())) {
> +        m_intrinsicPaddingAfter = static_cast<int>(roundf(gBigRootBottomPaddingEms * style()->fontSize()));
> +        setLogicalHeight(logicalHeight() + m_intrinsicPaddingAfter);
> +    }

It really looks like this should be moved to computeLogicalHeight. If we overflow, this will make us compute the wrong overflow box as this is done as part of layout (this was an existing bug so it can be postponed to another bug).

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.h:43
> +    virtual bool createsAnonymousWrapper() const { return true; }
> +    virtual void removeLeftoverAnonymousBlock(RenderBlock*) { }

I really wonder if this is needed, especially since you don't care about cleaning up the tree.

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.h:45
> +    virtual void computePreferredLogicalWidths();

OVERRIDE?

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.h:48
> +    virtual void paint(PaintInfo&, const LayoutPoint&);

Ditto.

[Dave Barton]

Comment on attachment 136062 [details]
Patch

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

Thanks as always for the very helpful criticisms and education. I am preparing a new patch responding to your & Eric's comments.

>> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:106
>> +    m_intrinsicPaddingStart = static_cast<int>(roundf(gFrontWidthEms * style()->fontSize()));
> 
> I wonder why you need to reset this twice: once in layout() and once in computePreferredLogicalWidth(). It sounds like once should be enough (and I would bet on computePreferredLogicalWidths as you need to keep your existing padding in the old width).

Isn't it conceivable that someone could call layout() without having called computePreferredLogicalWidths() first? Maybe even as a bug, or some weird code path where they're trying some tricky optimization? This seemed to me like a quick bit of defensive programming to me. RenderBlock::layoutBlock starts by calling recomputeLogicalWidth(), for instance. I'd defer to you guys, but I'd appreciate an explanation from a rendering code expert, for my understanding.

>> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:108
>> +    m_intrinsicPaddingAfter = 0;
> 
> m_intrinsicPaddingEnd is never reset...

It's initialized to 0 by our base class RenderMathMLBlock, and never changed from that.

>> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:116
>> +    }
> 
> It really looks like this should be moved to computeLogicalHeight. If we overflow, this will make us compute the wrong overflow box as this is done as part of layout (this was an existing bug so it can be postponed to another bug).

There are things I need to learn about here. Can you give me a pointer to code for or an explanation of an overflow box?

>> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.h:43
>> +    virtual void removeLeftoverAnonymousBlock(RenderBlock*) { }
> 
> I really wonder if this is needed, especially since you don't care about cleaning up the tree.

Looking at the 2 calls to removeLeftoverAnonymousBlock in RenderBlock.cpp, don't I need this to make sure the anonymous RenderMathMLRow block won't get removed if its children are made non-inline for some reason?

[Julien Chaffraix]

Comment on attachment 136062 [details]
Patch

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

>>> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:106
>>> +    m_intrinsicPaddingStart = static_cast<int>(roundf(gFrontWidthEms * style()->fontSize()));
>> 
>> I wonder why you need to reset this twice: once in layout() and once in computePreferredLogicalWidth(). It sounds like once should be enough (and I would bet on computePreferredLogicalWidths as you need to keep your existing padding in the old width).
> 
> Isn't it conceivable that someone could call layout() without having called computePreferredLogicalWidths() first? Maybe even as a bug, or some weird code path where they're trying some tricky optimization? This seemed to me like a quick bit of defensive programming to me. RenderBlock::layoutBlock starts by calling recomputeLogicalWidth(), for instance. I'd defer to you guys, but I'd appreciate an explanation from a rendering code expert, for my understanding.

computePreferredLogicalWidths is lazily called whenever you request your preferred logical widths. I would expect layout() to query those information (or explicitly call computePreferredLogicalWidths() as needed). Apart from bugs, I wouldn't expect the mentioned case to happen. As far as defensive programming goes, I wonder if this would help you here: if you have not dirtied / recomputed your preferred logical widths, I would expect other parts of the code to break anyway. I don't know the logic good enough to give precise examples here though.

>>> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:108
>>> +    m_intrinsicPaddingAfter = 0;
>> 
>> m_intrinsicPaddingEnd is never reset...
> 
> It's initialized to 0 by our base class RenderMathMLBlock, and never changed from that.

To some extends, so is m_intrinsicPaddingAfter as you never set it to anything that its default value 0 :)

Let's pick one convention here: I would rather resetting all of them at the same time to avoid forgetting any of them.

>>> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:116
>>> +    }
>> 
>> It really looks like this should be moved to computeLogicalHeight. If we overflow, this will make us compute the wrong overflow box as this is done as part of layout (this was an existing bug so it can be postponed to another bug).
> 
> There are things I need to learn about here. Can you give me a pointer to code for or an explanation of an overflow box?

As usual, we don't have much explanations on that. We model overflow as 2 extra boxes on RenderBox (look at the class RenderOverflow and its usage). The layout overflow represents the overflowing content itself (as derived from our layout computations), the visual overflow is the layout overflow clipped by anything overflow clip we may have. A good function to look for that is RenderBlock::computeOverflow that is called towards the end of layoutBlock.

>>> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.h:43
>>> +    virtual void removeLeftoverAnonymousBlock(RenderBlock*) { }
>> 
>> I really wonder if this is needed, especially since you don't care about cleaning up the tree.
> 
> Looking at the 2 calls to removeLeftoverAnonymousBlock in RenderBlock.cpp, don't I need this to make sure the anonymous RenderMathMLRow block won't get removed if its children are made non-inline for some reason?

I double check and I think you are right here.

[Dave Barton]

Comment on attachment 136062 [details]
Patch

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

>>>> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:106
>>>> +    m_intrinsicPaddingStart = static_cast<int>(roundf(gFrontWidthEms * style()->fontSize()));
>>> 
>>> I wonder why you need to reset this twice: once in layout() and once in computePreferredLogicalWidth(). It sounds like once should be enough (and I would bet on computePreferredLogicalWidths as you need to keep your existing padding in the old width).
>> 
>> Isn't it conceivable that someone could call layout() without having called computePreferredLogicalWidths() first? Maybe even as a bug, or some weird code path where they're trying some tricky optimization? This seemed to me like a quick bit of defensive programming to me. RenderBlock::layoutBlock starts by calling recomputeLogicalWidth(), for instance. I'd defer to you guys, but I'd appreciate an explanation from a rendering code expert, for my understanding.
> 
> computePreferredLogicalWidths is lazily called whenever you request your preferred logical widths. I would expect layout() to query those information (or explicitly call computePreferredLogicalWidths() as needed). Apart from bugs, I wouldn't expect the mentioned case to happen. As far as defensive programming goes, I wonder if this would help you here: if you have not dirtied / recomputed your preferred logical widths, I would expect other parts of the code to break anyway. I don't know the logic good enough to give precise examples here though.

You are right of course. As I even pointed out, RenderBlock::layoutBlock starts by calling recomputeLogicalWidth(), and it will ask for min & max logical widths.

>>>> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:108
>>>> +    m_intrinsicPaddingAfter = 0;
>>> 
>>> m_intrinsicPaddingEnd is never reset...
>> 
>> It's initialized to 0 by our base class RenderMathMLBlock, and never changed from that.
> 
> To some extends, so is m_intrinsicPaddingAfter as you never set it to anything that its default value 0 :)
> 
> Let's pick one convention here: I would rather resetting all of them at the same time to avoid forgetting any of them.

But I do (sometimes) set intrinsicPaddingAfter on line 114 in this patch.

>>>> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:116
>>>> +    }
>>> 
>>> It really looks like this should be moved to computeLogicalHeight. If we overflow, this will make us compute the wrong overflow box as this is done as part of layout (this was an existing bug so it can be postponed to another bug).
>> 
>> There are things I need to learn about here. Can you give me a pointer to code for or an explanation of an overflow box?
> 
> As usual, we don't have much explanations on that. We model overflow as 2 extra boxes on RenderBox (look at the class RenderOverflow and its usage). The layout overflow represents the overflowing content itself (as derived from our layout computations), the visual overflow is the layout overflow clipped by anything overflow clip we may have. A good function to look for that is RenderBlock::computeOverflow that is called towards the end of layoutBlock.

Thanks!

>>>> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.h:43
>>>> +    virtual void removeLeftoverAnonymousBlock(RenderBlock*) { }
>>> 
>>> I really wonder if this is needed, especially since you don't care about cleaning up the tree.
>> 
>> Looking at the 2 calls to removeLeftoverAnonymousBlock in RenderBlock.cpp, don't I need this to make sure the anonymous RenderMathMLRow block won't get removed if its children are made non-inline for some reason?
> 
> I double check and I think you are right here.

Actually you are right. The anonymous RenderMathMLRow is INLINE_BLOCK, so it doesn't count as an anonymous block. I've added a comment and an assert to the next patch about this.

[Dave Barton]

Created attachment 136284 [details]
Patch

[Julien Chaffraix]

Comment on attachment 136284 [details]
Patch

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

Some more comments but it looks good!

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:67
> +    ASSERT(writingMode == LeftToRightWritingMode || writingMode == RightToLeftWritingMode);

Which is why I mentioned a switch statement. It would transform a run-time check into a compile time check (if you leave the 'default' label out). IMHO it would make the code more readable too. Feel free to disagree though.

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:76
> +        // FIXME: Factor out a PassOwnPtr<RenderMathMLRow> RenderMathMLBlock::createAnonymousMRow() method.

To be consistent with the others, it should be RenderMathMLBlock::createAnonymousWithParentRenderer().

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:95
> +void RenderMathMLSquareRoot::computePreferredLogicalWidths()

I missed that but shouldn't it be computeLogicalWidth here? I bet your would prefer this to be recomputed for each layout, not when our preferred widths are dirty.

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.h:40
> +    virtual void addChild(RenderObject* newChild, RenderObject *beforeChild = 0) OVERRIDE;

Mhh, style violation, the * should be next to RenderObject.

> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.h:45
> +    virtual bool createsAnonymousWrapper() const OVERRIDE { return true; }
> +    
> +    virtual void computePreferredLogicalWidths() OVERRIDE;
> +    virtual void computeLogicalHeight() OVERRIDE;
> +    virtual void layout() OVERRIDE;

I don't think we want to expose those functions to every objects.

[Dave Barton]

Comment on attachment 136284 [details]
Patch

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

I am learning a lot, thanks. I think I've gone past editing Alex's existing code, and now writing new code exposes more of what I don't know, being new to WebKit.

>> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:67
>> +    ASSERT(writingMode == LeftToRightWritingMode || writingMode == RightToLeftWritingMode);
> 
> Which is why I mentioned a switch statement. It would transform a run-time check into a compile time check (if you leave the 'default' label out). IMHO it would make the code more readable too. Feel free to disagree though.

When I learned C (K&R First Ed.) compilers couldn't check that. :) Seriously, I've always avoided switch/case since it started out like a goto statement, but this is cool.

>> Source/WebCore/rendering/mathml/RenderMathMLSquareRoot.cpp:95
>> +void RenderMathMLSquareRoot::computePreferredLogicalWidths()
> 
> I missed that but shouldn't it be computeLogicalWidth here? I bet your would prefer this to be recomputed for each layout, not when our preferred widths are dirty.

I think it needs to be computePreferredLogicalWidths() in any case. Then it only needs to be called again when style()->fontSize() changes, not every layout().

[Dave Barton]

Created attachment 136393 [details]
Patch

[Julien Chaffraix]

Comment on attachment 136393 [details]
Patch

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

r=me.

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:69
> +        return result + (style()->isLeftToRightDirection() ? m_intrinsicPaddingStart : m_intrinsicPaddingEnd);

AFAICT the parentheses around the ternary '?' operator are unneeded here as it has lower priority and we usually omit them.

[Dave Barton]

Comment on attachment 136393 [details]
Patch

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

Thanks for all the review & the commit!

>> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:69
>> +        return result + (style()->isLeftToRightDirection() ? m_intrinsicPaddingStart : m_intrinsicPaddingEnd);
> 
> AFAICT the parentheses around the ternary '?' operator are unneeded here as it has lower priority and we usually omit them.

It has lower precedence than + so they are necessary, I believe.

[WebKit Review Bot]

Comment on attachment 136393 [details]
Patch

Clearing flags on attachment: 136393

Committed r113749: <http://trac.webkit.org/changeset/113749>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.