[webkit-reviews] review canceled: [Bug 52444] After appending MATHM with jquery the table renders with overlaps : [Attachment 137750] 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 52444: After appending MATHM with jquery the table renders with overlaps
https://bugs.webkit.org/show_bug.cgi?id=52444

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

------- Additional Comments from Dave Barton <dbarton at mathscribe.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=137750&action=review


This is new, calling layout() inside computePreferredLogicalWidths(). I
appreciate you thinking it through with me!

I'll submit another patch.

>> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:160
>> +static const LayoutUnit largeLogicalWidth = 15000;
> 
> There is not really a convention on how to name static variable or constant.
However it's usually preferred to somehow prefix it so that it's clear that it
is a constant. Some people would use cLargeLogicalWidth (for constant) or
sLargeLogicalWidth (for static). I would go for the c-prefixed but that's your
call.

Will do.

>> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:169
>> +	LayoutUnit oldAvailableLogicalWidth = everHadLayout() ?
availableLogicalWidth() : 0;
> 
> Shouldn't it just be oldAvailableLogicalWidth = availableLogicalWidth()?

You're right. I was worried about uninitialized values, but m_frameRect/etc.
are initialized to zeros.

>> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:183
>> +	    child->layoutIfNeeded();
> 
> This would definitely need a comment as IIRC that's needed because you
stretch your operators during layout. Is it stupid to wonder if we couldn't do
that at computeLogicalWidths time? (maybe just as a FIXME here explaining why
and what is preventing us for now)

There's a comment in RenderMathMLBlock.h explaining the need for preferred
logical heights at all, because of e.g. operator stretching. The whole purpose
of this function is to compute each child's preferred logical height if
necessary. I will add a comment saying that this layoutIfNeeded() is to do
that.

The RenderMathMLBlock.h comment also explains that this must happen at
computePreferredLogicalWidths time, before computeLogicalWidths in
this->layout().

>> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:194
>> +	    ASSERT(!child->needsLayout());
> 
> Maybe worth moving it at the beginning of the function?

Actually if the child is a RenderMathMLBlock which already had its
preferredLogicalHeight set, then it wouldn't have been laid out again in
sizeChildrenInWide() so it conceivably could need layout now. For instance, its
parent's width may have just changed. If the child is just a RenderInline then
I think it could need layout also. We just need its fontSize() here in that
case.

>> Source/WebCore/rendering/mathml/RenderMathMLBlock.h:111
>> +	void sizeChildrenInWide();
> 
> I really don't understand the InWide part of the name. IMHO it can be just
dropped and you could just call it prepareChildrenForLogicalWidthsComputation.

I kind of like your name, but it's at least part of the contract (comment) for
this function that it sets this->logicalWidth() to a large value. This is used
for instance at the end of RenderMathMLRow::computePreferredLogicalWidths(),
where we do setLogicalWidth(maxPreferredLogicalWidth()); without marking our
children as needing layout again. I'll add an ASSERT() there to make this
clearer.

>> Source/WebCore/rendering/mathml/RenderMathMLBlock.h:113
>> +	static LayoutUnit preferredLogicalHeightAfterSizing(RenderObject*
child);
> 
> I don't think I understand the AfterSizing part, you don't enforce the
constraint and AFAICT it would work at any time. I would just drop the
AfterSizing part and call it preferredLogicalHeightForObject.

If you haven't called sizeChildrenInWide() first, then needed ASSERT()s may
fail. If child->isRenderMathMLBlock(), then
ASSERT(!isPreferredLogicalHeightDirty()) may fail in
toRenderMathMLBlock(child)->preferredLogicalHeight(). Also, if just
child->isBox(), then ASSERT(!child->needsLayout()) could fail.

>> Source/WebCore/rendering/mathml/RenderMathMLRow.h:47
>> +	
> 
> Trailing spaces.

Will fix.