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

[bugzilla-daemon at webkit.org]

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





--- Comment #7 from Alex Milowski <alex at milowski.com>  2010-02-26 10:29:27 PST ---
(In reply to comment #6)
> 
> 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


Odd that the style checker doesn't find these things.

> 
> > +                // FIXME: This should be in ems
> 
> Is this hard to fix?

That's just incorrect.  I removed the comment for now.  It may
need to be made proportional to the size of the parts but that
will be worked on later as we work on "squeezing" the
mathematics to make it more readable.

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

It is just a subjective expansion of the msubsup since it needs to
extend above and below the row height.  I added a descriptive
global for that.

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

I made inline functions for this in the RenderMathMLBlock header so I can use
them elsewhere too.

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

There is a parent implementation but it doesn't return zero for this object and
that is why I need this method here.  There are similar requirements for
other MathML rendering objects.

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

I fixed that to use the method parameters rather than fixing it.  That should
be correct and much more clear.

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