[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


Kenneth Rohde Christiansen <kenneth at webkit.org> changed:

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




--- Comment #5 from Kenneth Rohde Christiansen <kenneth at webkit.org>  2010-02-25 17:57:30 PST ---
(From update of attachment 48389)

> +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.

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