[Webkit-unassigned] [Bug 34278] MathML Support for msubsup

[bugzilla-daemon at webkit.org]

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


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

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #48390|review?                     |review-
               Flag|                            |




--- Comment #6 from Kenneth Rohde Christiansen <kenneth at webkit.org>  2010-02-26 09:30:24 PST ---
(From update of attachment 48390)

> +
> +    RenderObject* object = 0;

Newline at beginning of method, please remove.

> +static const int topAdjustDivisor = 3;
> +static const int subsupScriptMargin = 1;

Mark as global.

> +
> +    
> +RenderMathMLSubSup::RenderMathMLSubSup(Node *subsup) 

Two new lines are not needed. * aligned to the wrong side

> +    m_kind = SubSup;
> +    if (element->hasLocalName(MathMLNames::msubTag))
> +        m_kind = Sub;
> +    else if (element->hasLocalName(MathMLNames::msupTag))
> +        m_kind = Sup;
> +}

Same comment as in last patch

> +                // FIXME: This should be in ems

Is this hard to fix?

> +void  RenderMathMLSubSup::stretchToHeight(int height)
> +{
> +    RenderObject* base = firstChild();
> +    if (!base)
> +        return;
> +    
> +    if (base->isRenderMathMLBlock()) {
> +        RenderMathMLBlock* block = toRenderMathMLBlock(base);
> +        block->stretchToHeight(static_cast<int>(1.2 * height));

where does the constant come from

> +    }
> +    if (height > 0 && m_kind == SubSup && m_scripts) {
> +
> +        RenderObject* script = m_scripts->firstChild();

No need for the newline above

> +        if (script) {
> +            // calculate the script height without the container margins
> +            int topHeight = 0;
> +            RenderObject* top = script;
> +            RenderObject* child = top->firstChild();
> +            if (child && child->isBoxModelObject()) {
> +                RenderBoxModelObject* box = toRenderBoxModelObject(child);
> +                topHeight = box->offsetHeight();
> +            }
> +            int bottomHeight = 0;
> +            RenderObject* bottom = script;
> +            child = bottom->firstChild();
> +            if (child && child->isBoxModelObject()) {
> +                RenderBoxModelObject* box = toRenderBoxModelObject(child);
> +                bottomHeight = box->offsetHeight();
> +            }

There is some duplicated code, would it make sense to use an inline method
instead?


> +
> +int RenderMathMLSubSup::nonOperatorHeight() const 
> +{
> +    return 0;
> +}

You have this code in other classes as well, would it make sense using a parent
implementation? or is that one different from 0?

> +
> +void RenderMathMLSubSup::layout() 
> +{
> +
> +    RenderBlock::layout();

Non-needed newline

> +    if (m_kind == SubSup) {
> +        int width = 0;
> +        RenderObject* current = firstChild();
> +        while (current) {
> +            if (current->isBoxModelObject()) {
> +
> +                RenderBoxModelObject* box = toRenderBoxModelObject(current);

again.

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

remove space before ,
> +            return topAdjust + (base ? base->baselinePosition(true) : 0) + 4;

What doest the true stand for here? maybe add a /* comment */ in front of it?

like baselinePosition( /* recalc */ true)...

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