[Webkit-unassigned] [Bug 79274] Fix <msubsup> formatting

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=79274


Julien Chaffraix <jchaffraix at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #128903|review?, commit-queue?      |
               Flag|                            |




--- Comment #23 from Julien Chaffraix <jchaffraix at webkit.org>  2012-02-26 10:10:17 PST ---
(From update of attachment 128903)
View in context: https://bugs.webkit.org/attachment.cgi?id=128903&action=review

We are getting closer. One more iteration and it should be fine I would say.

> Source/WebCore/ChangeLog:3
> +        Fix <msubsup> formatting

BTW usually we prefer bugzilla bug titles to be more precise. Here it's difficult to understand what cases you want to fix (all of them? certain ones? in this case which ones?)

> Source/WebCore/ChangeLog:12
> +        Test: 6 existing test files in mathml/presentation now produce improved results,

Saying how improved they are would help us, people who don't know what MathML should look like. From the traces, it looks like they are more densely laid out but I don't know if it's good or not.

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

First, the name should be proper camelCase ie superScriptWrapper! (unless those are supposed to be some keywords and I missed it in the spec)

Please add 2 ASSERTs here:
ASSERT(superScriptWrapper->isRenderMathMLBlock());
ASSERT(subScriptWrapper->isRenderMathMLBlock());

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

Ditto for the camelCase here too.

I really would like an ASSERT that those are *really* what you want. I doesn't look like you implement some isMathMLSubSup() function so it may not be good for this bug but should be either a FIXME or filed as a potential bug.

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:151
> +    LayoutUnit baseAscent = base->baselinePosition(AlphabeticBaseline, true, lineDirection);
> +    LayoutUnit baseDescent = getBoxModelObjectHeight(base) - baseAscent;

You have to be super careful in your naming. Ascent and descent have a meaning with fonts which does not totally match what you are doing here: baselinePosition may take into account CSS margins for example.

The first variable is the baseBaseline. The second one is not something I know a standard name for: how about baseExtendUnderBaseline?

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

This should be setNeedsLayout(true, false).

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

Ditto.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.