[webkit-reviews] review canceled: [Bug 52444] After appending MATHM with jquery the table renders with overlaps : [Attachment 137622] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> 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 137622: Patch
https://bugs.webkit.org/attachment.cgi?id=137622&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=137622&action=review


> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:49
> +    , m_preferredLogicalHeight(-1)

We usually avoid magic constants:

static LayoutUnit preferredLogicalHeightUnset = -1;

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:164
> +    // Ensure a full repaint will happen eventually.
> +    setNeedsLayout(true, MarkOnlyThis);

It really looks like you are abusing the existing logic to need to force a
layout to properly repaint (our repaint logic is far from being obvious or
unified unfortunately). I guess this is due to the fact that your order of
operations is different from the normal case. I don't have an alternative
solution though and would need to think about it so don't feel blocked by that.


> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:167
> +    setLogicalWidth(15000); // an arbitrary large value, like
RenderBlock.cpp BLOCK_MAX_WIDTH

Again, we prefer to avoid magic constants. Should it be kept in sync with the
value of BLOCK_MAX_WIDTH as FixedTableLayout.cpp's TABLE_MAX_WIDTH?

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:186
> +LayoutUnit
RenderMathMLBlock::preferredLogicalHeightAfterSizing(RenderObject* child)

I am not sure I understand the "after sizing" part. What do we size here? The
children?

> Source/WebCore/rendering/mathml/RenderMathMLBlock.h:70
> +    LayoutUnit preferredLogicalHeightIfOK() const { return
preferredLogicalWidthsDirty() ? -1 : m_preferredLogicalHeight; }

I am not a huge fan of that function. Your use case seems to be either to check
that it is set (basically != -1) or to get the value. This means you could just
have 2 new functions:
* bool isPreferredLogicalHeightSet() const { return
!preferredLogicalWidthsDirty() && m_preferredLogicalHeight != -1; }
* LayoutUnit preferredLogicalHeight() const {
ASSERT(!preferredLogicalWidthsDirty()); return m_preferredLogicalHeight; }

> Source/WebCore/rendering/mathml/RenderMathMLBlock.h:108
> +    void sizeChildrenInWide();

The naming is not 100% to me. What does "in wide" means? Does it have a special
meaning in MathML?

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:45
> +    virtual void updateFromElement();
> +    
> +    virtual RenderMathMLOperator* unembellishedOperator() { return this; }

OVERRIDE?

> Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:67
> +	   if (child->isRenderMathMLBlock()) {
> +	       RenderMathMLOperator* renderMo =
toRenderMathMLBlock(child)->unembellishedOperator();
> +	       // FIXME: Only skip renderMo if it is stretchy.
> +	       if (renderMo)
> +		   continue;
>	   }

I am wondering if this shouldn't be part of your
preferredLogicalHeightAfterSizing. That would likely make the code easier to
read and you are already checking for some of those conditions anyway.