Bug 34278 – MathML Support for msubsup

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alex Milowski]

Created attachment 47633 [details]
msubsup patch

This patch adds basic rendering support for msubsup.

[Alex Milowski]

Created attachment 47635 [details]
Correct patch with missing rendering object

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/mathml/RenderMathMLSubSup.cpp:222:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1


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

[Alex Milowski]

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

[Alex Milowski]

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

[Alex Milowski]

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

[Kenneth Rohde Christiansen]

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


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

[Alex Milowski]

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

[Kenneth Rohde Christiansen]

It sounds like you fixed the issues; did you forget to upload a new patch?

[Alex Milowski]

(In reply to comment #8)
> It sounds like you fixed the issues; did you forget to upload a new patch?

I'm waiting for the other patch to commit because I expect merge problems with this patch on MathMLInlineContainer.cpp

[Alex Milowski]

Created attachment 49656 [details]
Updated patch to address reviewer comments and merged with latest trunk

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/mathml/RenderMathMLBlock.h:55:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/mathml/RenderMathMLBlock.h:63:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/mathml/RenderMathMLBlock.h:71:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/mathml/RenderMathMLBlock.h:79:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/mathml/RenderMathMLSubSup.cpp:184:  Missing space after ,  [whitespace/comma] [3]
WebCore/mathml/RenderMathMLSubSup.cpp:189:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 6 in 15 files


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

[Alex Milowski]

Created attachment 49658 [details]
Updated patch to address style errors

[WebKit Commit Bot]

Comment on attachment 49658 [details]
Updated patch to address style errors

Clearing flags on attachment: 49658

Committed r55386: <http://trac.webkit.org/changeset/55386>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.