[webkit-reviews] review canceled: [Bug 82353] msqrt's implied mrow should do operator stretching : [Attachment 136062] Patch

[bugzilla-daemon at webkit.org]

Dave Barton <dbarton at mathscribe.com> has canceled Dave Barton
<dbarton at mathscribe.com>'s request for review:
Bug 82353: msqrt's implied mrow should do operator stretching
https://bugs.webkit.org/show_bug.cgi?id=82353

Attachment 136062: Patch
https://bugs.webkit.org/attachment.cgi?id=136062&action=review

------- Additional Comments from Dave Barton <dbarton at mathscribe.com>
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?