[Webkit-unassigned] [Bug 34277] MathML Support for munder, mover, and munderover

[bugzilla-daemon at webkit.org]

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





--- Comment #6 from Alex Milowski <alex at milowski.com>  2010-02-26 07:19:48 PST ---
(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.

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