[webkit-reviews] review canceled: [Bug 79274] Fix <msubsup> formatting : [Attachment 128627] 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 79274: Fix <msubsup> formatting
https://bugs.webkit.org/show_bug.cgi?id=79274

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

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


There are several logical issues with the underlying code. It looks like MathML
is abusing the CSS logic pretty badly.

Now the patch is making the situation slightly worse by re-creating 2
RenderStyle every time we layout (:-(). I would love to understand why.

As Eric mentioned, the padding computation should be moved to a new function
for clarity. Thanks for reviving this code.

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:52
>  PassRefPtr<RenderStyle> RenderMathMLBlock::createBlockStyle()

The naming is bad. If you are returning an anonymous style, please say so.
Looking at the call site, you don't create anonymous renderer but they will get
anonymous style which is wrong (sic!). I can't think of any nasty consequences
but you are definitely abusing the rendering logic.

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:66
> +    RefPtr<RenderStyle> newStyle =
RenderStyle::createAnonymousStyle(child->style());
> +    newStyle->setDisplay(BLOCK);
> +    wrapper->setStyle(newStyle.release());

This should use the function above.

> Source/WebCore/rendering/mathml/RenderMathMLBlock.h:84
> +    static void checkBlockWrapper(RenderObject* wrapper);

The naming is bad. It doesn't really "check" the block.

I have no idea why you would need to recreate your style *every time* you do a
layout. This is likely a hidden bug in your implementation as you really
shouldn't need to.

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:139
> +    RenderObject* superscriptWrapper = m_scripts->firstChild();
> +    RenderObject* subscriptWrapper = m_scripts->lastChild();

Could we get some ASSERT here that you are actually getting the right objects?
I fear this could break very badly if you allow CSS generated content (:before
/ :after) on MathML elements...

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:143
> +    RenderObject* superscript = superscriptWrapper->firstChild();
> +    RenderObject* subscript = subscriptWrapper->firstChild();

Ditto.

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:161
> +	   superPaddingBottom += - basePaddingTop;

s/+= -/-=/

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:167
> +    baseWrapper->setNeedsLayout(true);

This is super dangerous (I see it was already present not strictly your fault).
Here you will end up trying to mark your parents for layout and because you are
in the middle of your layout that could confuse the delayed-layout code.

The good pattern is to mark *only* your children if you need to (see
RenderTable::layout() for example which marks only its <caption> children if
our logical width changes) and *before* actually laying them.

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:171
> +    superscriptWrapper->setNeedsLayout(true);

Ditto.

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.h:-42
> -    virtual LayoutUnit baselinePosition(FontBaseline, bool firstLine,
LineDirectionMode, LinePositionMode = PositionOnContainingLine) const;

IMHO this is a good change as our baseline is not impacted by subscript /
superscript.