Bug 34277 – MathML Support for munder, mover, and munderover

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Alex Milowski]

Created attachment 47632 [details]
munder/over support patch

This patch adds basic rendering of munder, mover, and munderover.

[WebKit Review Bot]

Attachment 47632 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/mathml/RenderMathMLUnderOver.cpp:123:  Missing spaces around !=  [whitespace/operators] [3]
WebCore/mathml/RenderMathMLUnderOver.cpp:125:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/mathml/RenderMathMLUnderOver.cpp:162:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/mathml/RenderMathMLUnderOver.cpp:164:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/mathml/RenderMathMLUnderOver.cpp:189:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/mathml/RenderMathMLUnderOver.cpp:211:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/mathml/RenderMathMLUnderOver.cpp:226:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 7


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Milowski]

Created attachment 47636 [details]
Updated patch to fix style errors

[Alex Milowski]

Created attachment 47729 [details]
Updated patch to include updates from the change in CSS from bug 34275

[Alex Milowski]

Created attachment 48389 [details]
Updated patch to latest trunk with anonymous blocks removed.

[Kenneth Rohde Christiansen]

Comment on attachment 48389 [details]
Updated patch to latest trunk with anonymous blocks removed.

  
> +static const double underNormalAdjust = -0.4;
> +static const double fontAdjust = 0.75;

Other places in WebKit we have globals prefixed with g. I think Darin Adler suggested that to me once.

Also it would be nice with a comment explaining where these numbers come from.

> +    
> +RenderMathMLUnderOver::RenderMathMLUnderOver(Node* expression) 
> +    : RenderMathMLBlock(expression) 
> +{
> +    Element* element = static_cast<Element*>(expression);
> +    // Determine what kind of under/over expression we have by element name
> +    m_kind = Under;
> +    if (element->hasLocalName(MathMLNames::moverTag))
> +        m_kind = Over;
> +    else if (element->hasLocalName(MathMLNames::munderoverTag))
> +        m_kind = UnderOver;
> +}

So in the above, if it is an "under" you get to call hasLocalName 2 times. So I guess you didn't add an if for 'under' as some kind of optimization? Is that call expensive? Is the 'over' tag the most common?

> +
> +void RenderMathMLUnderOver::addChild(RenderObject* child, RenderObject* beforeChild)
> +{
> +    
> +    RenderMathMLBlock* row = new (renderArena()) RenderMathMLBlock(node());

I have notices that you have one extra newline after {. This is different from current WebKit code. Please avoid that.

> +    RefPtr<RenderStyle> rowStyle = makeBlockStyle();
> +    row->setStyle(rowStyle.release());
> +    
> +    // look through the children for rendered elements
> +    int blocks = 0;
> +    RenderObject* current = this->firstChild();
> +    while (current) {
> +        blocks++;
> +        current = current->nextSibling();
> +    }

If I didn't miss anything in the above you don't use current for anything, but to count?

> +void  RenderMathMLUnderOver::stretchToHeight(int height)
> +{
> +
> +    RenderObject* base = firstChild();
> +    if (!base)
> +        return;
> +    if (m_kind != Under) 
> +        base = base->nextSibling();
> +    if (!base)
> +        return;
> +    // use the child of the row which is the actual base
> +    base = base->firstChild();

So if it is not under you have to get the first child of the next sibling, or else the first child will do. It is not exactly obvious to me, maybe add a comment on the logic?

> +
> +
> +void RenderMathMLUnderOver::layout() 

You don't need two newlines in the above

> +{
> +
> +    RenderBlock::layout();
> +    RenderObject* over = 0;
> +    RenderObject* base = 0;
> +    switch (m_kind) {
> +    case Over:
> +        // We need to calculate the baseline over the over versus the start of the base and 
> +        // adjust the placement of the base.
> +        over = firstChild();
> +        if (over) {
> +            // FIXME: descending glyphs intrude into base (e.g. lowercase y over base)
> +            // FIXME: bases that ascend higher than the line box intrude into the over
> +            int overSpacing = static_cast<int>(0.667 * (getOffsetHeight(over) - over->firstChild()->baselinePosition(true)));

Shouldn't you use a descriptive constant instead of 0.667?

> +            
> +            // base row wrapper
> +            base = over->nextSibling();
> +            if (base) {
> +                if (overSpacing>0) 

Coding style, missing spaces around >

> +
> +int RenderMathMLUnderOver::baselinePosition(bool , bool) const

Remove that space before the ,

In the future I would put the tests as a separate patch, especially because of the images.

Generally looks good, but please fix the above before landing.

[Alex Milowski]

(In reply to comment #5)
> (From update of attachment 48389 [details])
> 
> > +static const double underNormalAdjust = -0.4;
> > +static const double fontAdjust = 0.75;
> 
> Other places in WebKit we have globals prefixed with g. I think Darin Adler
> suggested that to me once.
> 
> Also it would be nice with a comment explaining where these numbers come from.

That's actually unused code.  I will make a constant for the other factor.

> 
> > +    
> > +RenderMathMLUnderOver::RenderMathMLUnderOver(Node* expression) 
> > +    : RenderMathMLBlock(expression) 
> > +{
> > +    Element* element = static_cast<Element*>(expression);
> > +    // Determine what kind of under/over expression we have by element name
> > +    m_kind = Under;
> > +    if (element->hasLocalName(MathMLNames::moverTag))
> > +        m_kind = Over;
> > +    else if (element->hasLocalName(MathMLNames::munderoverTag))
> > +        m_kind = UnderOver;
> > +}
> 
> So in the above, if it is an "under" you get to call hasLocalName 2 times. So I
> guess you didn't add an if for 'under' as some kind of optimization? Is that
> call expensive? Is the 'over' tag the most common?

Nope.  I've changed it to check for the localname for munder and then default to
Under  as a last resort.  I don't really think it is any faster but it is probably
easier to understand.

> > +    RefPtr<RenderStyle> rowStyle = makeBlockStyle();
> > +    row->setStyle(rowStyle.release());
> > +    
> > +    // look through the children for rendered elements
> > +    int blocks = 0;
> > +    RenderObject* current = this->firstChild();
> > +    while (current) {
> > +        blocks++;
> > +        current = current->nextSibling();
> > +    }
> 
> If I didn't miss anything in the above you don't use current for anything, but
> to count?

Just counting.  I don't see a clean way to get the number of children
from the RenderObject API.

> 
> > +void  RenderMathMLUnderOver::stretchToHeight(int height)
> > +{
> > +
> > +    RenderObject* base = firstChild();
> > +    if (!base)
> > +        return;
> > +    if (m_kind != Under) 
> > +        base = base->nextSibling();
> > +    if (!base)
> > +        return;
> > +    // use the child of the row which is the actual base
> > +    base = base->firstChild();
> 
> So if it is not under you have to get the first child of the next sibling, or
> else the first child will do. It is not exactly obvious to me, maybe add a
> comment on the logic?

Sure.  I add a comment but the base (what things are under/over) is
either the first or second child depending on what kind of element
you have.

> > +{
> > +
> > +    RenderBlock::layout();
> > +    RenderObject* over = 0;
> > +    RenderObject* base = 0;
> > +    switch (m_kind) {
> > +    case Over:
> > +        // We need to calculate the baseline over the over versus the start of the base and 
> > +        // adjust the placement of the base.
> > +        over = firstChild();
> > +        if (over) {
> > +            // FIXME: descending glyphs intrude into base (e.g. lowercase y over base)
> > +            // FIXME: bases that ascend higher than the line box intrude into the over
> > +            int overSpacing = static_cast<int>(0.667 * (getOffsetHeight(over) - over->firstChild()->baselinePosition(true)));
> 
> Shouldn't you use a descriptive constant instead of 0.667?

Just a scaling factor.  I'm removing two thirds of the space.  I've made
that a global constant.

> In the future I would put the tests as a separate patch, especially because of
> the images.

I was told differently and so I've included the pixel tests in all my
patches.

> 
> Generally looks good, but please fix the above before landing.

There is an updated patch on the way.  Just testing it now.

[Alex Milowski]

Created attachment 49585 [details]
Updated patch to address reviewer comments

[Kenneth Rohde Christiansen]

Great!

> I was told differently and so I've included the pixel tests in all my
> patches.

Fine, I don't feel strongly about it.

[WebKit Commit Bot]

Comment on attachment 49585 [details]
Updated patch to address reviewer comments

Clearing flags on attachment: 49585

Committed r55310: <http://trac.webkit.org/changeset/55310>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.