Bug 34278

Summary: MathML Support for msubsup
Product: WebKit Reporter: Alex Milowski <alex>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fred.wang, kenneth, michael.vm, webkit.review.bot, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Macintosh Intel   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 3251    
Attachments:
Description Flags
msubsup patch
none
Correct patch with missing rendering object
none
Updated patch to fix style errors
none
Updated patch to include updates from the change in CSS from bug 34275
none
Updated patch to latest trunk with anonymous blocks removed.
kenneth: review-
Updated patch to address reviewer comments and merged with latest trunk
none
Updated patch to address style errors none

Description Alex Milowski 2010-01-28 10:55:49 PST
Created attachment 47633 [details]
msubsup patch

This patch adds basic rendering support for msubsup.
Comment 1 Alex Milowski 2010-01-28 10:59:18 PST
Created attachment 47635 [details]
Correct patch with missing rendering object
Comment 2 WebKit Review Bot 2010-01-28 11:03:56 PST
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.
Comment 3 Alex Milowski 2010-01-28 11:09:21 PST
Created attachment 47637 [details]
Updated patch to fix style errors
Comment 4 Alex Milowski 2010-01-29 14:21:51 PST
Created attachment 47735 [details]
Updated patch to include updates from the change in CSS from bug 34275
Comment 5 Alex Milowski 2010-02-08 20:58:43 PST
Created attachment 48390 [details]
Updated patch to latest trunk with anonymous blocks removed.
Comment 6 Kenneth Rohde Christiansen 2010-02-26 09:30:24 PST
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)...
Comment 7 Alex Milowski 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.
Comment 8 Kenneth Rohde Christiansen 2010-02-26 11:43:24 PST
It sounds like you fixed the issues; did you forget to upload a new patch?
Comment 9 Alex Milowski 2010-02-26 12:03:26 PST
(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
Comment 10 Alex Milowski 2010-02-26 15:38:57 PST
Created attachment 49656 [details]
Updated patch to address reviewer comments and merged with latest trunk
Comment 11 WebKit Review Bot 2010-02-26 15:45:41 PST
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.
Comment 12 Alex Milowski 2010-02-26 15:54:57 PST
Created attachment 49658 [details]
Updated patch to address style errors
Comment 13 WebKit Commit Bot 2010-03-01 16:22:21 PST
Comment on attachment 49658 [details]
Updated patch to address style errors

Clearing flags on attachment: 49658

Committed r55386: <http://trac.webkit.org/changeset/55386>
Comment 14 WebKit Commit Bot 2010-03-01 16:22:26 PST
All reviewed patches have been landed.  Closing bug.