Bug 34347 – MathML Support for mrow and Stretchy Operators

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Alex Milowski]

Created attachment 47736 [details]
Patch to add mrow and stretchy operator support.

This patch provides support for row layout and height calculation propagation to allow operators to stretch.  It also provide a basic implementation of stretchy operators.

[Alex Milowski]

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

[Roland Steiner]

Just a few comments from a quick look at your patch (all strictly IMHO, since I'm not a reviewer):

Index: WebCore/mathml/RenderMathMLOperator.cpp
===================================================================
--- WebCore/mathml/RenderMathMLOperator.cpp	(revision 0)
+++ WebCore/mathml/RenderMathMLOperator.cpp	(revision 0)

+// This is a table of stretchy characters.
+// FIXME: Should this be read from the unicode characteristics somehow?
+// table:  stretchy operator, top char, extension char, bottom char, middle char
+static const UChar stretchy[9][5] = {
+{ 0x28  , 0x239b, 0x239c, 0x239d, 0x0    }, // left parenthesis
+{ 0x29  , 0x239e, 0x239f, 0x23a0, 0x0    }, // right parenthesis
+{ 0x5b  , 0x23a1, 0x23a2, 0x23a3, 0x0    }, // left square bracket
+{ 0x5d  , 0x23a4, 0x23a5, 0x23a6, 0x0    }, // right square bracket
+{ 0x7b  , 0x23a7, 0x23aa, 0x23a9, 0x23a8 }, // left curly bracket
+{ 0x7c  , 0x23d0, 0x23d0, 0x23d0, 0x0    }, // vertical bar
+{ 0x7d  , 0x23ab, 0x23aa, 0x23ad, 0x23ac }, // right curly bracket
+{ 0x222b, 0x2320, 0x23ae, 0x2321, 0x0    }, // integral sign
+{ 0x0   , 0x0,    0x0,    0x0,    0x0    }
+};

Could you make the elements of this array into a struct with 5 UChar members? Having explicitly named members would be more readable than an fixed numerical array index. Also, the name 'stretchy' is the same as the attribute that you reference later, which is IMHO a bit confusing:

+    bool noStretch = false;
+    if (mo) {
+        AtomicString stretchyAttr = mo->getAttribute(MathMLNames::stretchyAttr);
+        noStretch = equalIgnoringCase(stretchyAttr, "false");
+    }
+    

Later in the code:

+    bool canStretch = index >= 0 && stretchy[index][0];
+    if (noStretch || !firstChar || m_stretchHeight <= 24 || !canStretch) {

When browsing, the meaning of 'noStretch' vs. 'canStretch' is not immediately clear without reading the assignments. Renaming your character array to, e.g. 'stretchCharacters', and 'noStretch' to 'stretchY' or 'stretchYValue' (i.e., repeating the name of the attribute value it's storing, reversing true and false in the process) would be clearer IMHO.

Lastly, iterating from 0 to sizeof(array)/sizeof(array[0]) rather than having an end sentinel may be more succinct, but this may just be my personal preference. The current usage also causes you to have 2 different values for index that both mean 'N/A': -1 and index-of-sentinel.

[Kenneth Rohde Christiansen]

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


> +    int index = -1;
> +    if (firstChar) {
> +        for (index++; stretchy[index][0]; index++) {
> +            if (stretchy[index][0] == firstChar)
> +                break;
> +        }
> +    }
> +    
> +    bool noStretch = false;
> +    if (mo) {
> +        AtomicString stretchyAttr = mo->getAttribute(MathMLNames::stretchyAttr);
> +        noStretch = equalIgnoringCase(stretchyAttr, "false");
> +    }

So you are checking if we are allowed to stretch? If so, couldn't this be above the other if (firstChar)?

> +    // either we have no characters, the stretch height is small, or we have a non-stretchable character
> +    // If so, then we just set the font size and wrap the text
> +    bool canStretch = index >= 0 && stretchy[index][0];

You only need the above index if you are going to stretch. Couldn't you separate out the code in a private method, isStretchable or so?

The below if cases are huge, so it would be good to have them separated out in methods as well.

> +    if (noStretch || !firstChar || m_stretchHeight <= 24 || !canStretch) {
> +
> +        m_isStacked = false;
> +        RenderBlock* container = new (renderArena()) RenderBlock(node());
> +        
> +        RefPtr<RenderStyle> newStyle = RenderStyle::create();
> +        newStyle->inheritFrom(style());
> +        newStyle->setDisplay(BLOCK);
> +
> +        int currentFontSize = style()->fontSize();
> +        if (!noStretch && m_stretchHeight > 0 && m_stretchHeight <= 24  && m_stretchHeight > currentFontSize && canStretch) {

24?

> +            FontDescription* desc = new FontDescription();
> +            desc->setIsAbsoluteSize(true);
> +            desc->setSpecifiedSize(m_stretchHeight);
> +            desc->setComputedSize(m_stretchHeight);
> +            newStyle->setFontDescription(*desc);
> +            newStyle->font().update(newStyle->font().fontSelector());
> +        }
> +        
> +        newStyle->setVerticalAlign(BASELINE);
> +        container->setStyle(newStyle.release());
> +        addChild(container);
> +     
> +        RenderText* text = new (renderArena()) RenderText(node(), m_operator ? StringImpl::create(&m_operator, 1) : StringImpl::create(mo->textContent()));
> +        text->setStyle(container->style());
> +        container->addChild(text);
> +    
> +    } else {
> +        // build stretchable characters
> +        m_isStacked = true;
> +        
> +        if (stretchy[index][4]) {
> +        
> +            // we have a middle glyph (e.g. a curly bracket)
> +            
> +            int half = (m_stretchHeight - 10) / 2;

You use a lot of constants in the code around here. Where do they come from? I see 10, -2 and -4 in many places.

> +            if (half <= 10) {
> +                // we only have enough space for the top & bottom glyph
> +                makeGlyph(stretchy[index][1], half);
> +                makeGlyph(stretchy[index][4], 10, -2);
> +                makeGlyph(stretchy[index][3], 0, -4);
> +            } else {
> +                // we have to extend both the top and bottom to the middle
> +                makeGlyph(stretchy[index][1], 10);
> +                int remaining = half-10;

Missing spaces around -

> +                while (remaining > 0) {
> +                    if (remaining < 10) {
> +                        makeGlyph(stretchy[index][2], remaining);
> +                        remaining = 0;
> +                    } else {
> +                        makeGlyph(stretchy[index][2], 10);
> +                        remaining -= 10;
> +                    }
> +                }
> +                
> +                // The middle glyph
> +                makeGlyph(stretchy[index][4], 10, -2);
> +                
> +                remaining = half-10;

same thing

> +                // make sure we have the full height in case the height is odd
> +                if (m_stretchHeight % 2 == 1)
> +                    remaining++;
> +                    
> +                while (remaining > 0) {
> +                    if (remaining < 10) {
> +                        makeGlyph(stretchy[index][2], remaining);
> +                        remaining = 0;
> +                    } else {
> +                        makeGlyph(stretchy[index][2], 10);
> +                        remaining -= 10;
> +                    }
> +                }
> +                makeGlyph(stretchy[index][3], 0, -4);

For these indices 2, 3 etc, would it make sense using an enum?

> +            }
> +        } else {
> +        
> +            // we do not have a middle glyph
> +            

Excessive newlining above

> +            int remaining = m_stretchHeight - 20;
> +            makeGlyph(stretchy[index][1], 10);
> +            while (remaining > 0) {
> +                if (remaining < 10) {
> +                    makeGlyph(stretchy[index][2], remaining);
> +                    remaining = 0;
> +                } else {
> +                    makeGlyph(stretchy[index][2], 10);
> +                    remaining -= 10;
> +                }
> +            }
> +            makeGlyph(stretchy[index][3], 0, -4);
> +        }
> +    }
> +       
> +}
> +
> +RefPtr<RenderStyle> RenderMathMLOperator::makeStackableStyle(int size, int topRelative)
> +{
> +    RefPtr<RenderStyle> newStyle = RenderStyle::create();
> +    newStyle->inheritFrom(style());
> +    newStyle->setDisplay(BLOCK);
> +
> +    // 14px is the size we'll use for all characters

ok, why?

> +    FontDescription* desc = new FontDescription();
> +    desc->setIsAbsoluteSize(true);
> +    desc->setSpecifiedSize(14);
> +    desc->setComputedSize(14);
> +    newStyle->setFontDescription(*desc);
> +    newStyle->font().update(newStyle->font().fontSelector());
> +    newStyle->setLineHeight(Length(12, Fixed));

and 12?


> +    virtual void layout();
> +    virtual RefPtr<RenderStyle> makeStackableStyle(int size, int topRelative);

create* is more common for RefPtr in WebKit, I guess.


> +        } else if (current->isBoxModelObject()) {
> +
> +            RenderBoxModelObject* box = toRenderBoxModelObject(current);

excessive newlining

> +
> +            // Check to see if this box has a larger height
> +            if (box->offsetHeight() > maxHeight)
> +                maxHeight = box->offsetHeight();
> +                
> +        }

Why a newline above?

> +        
> +    }
> +    return maxHeight;
> +}
> +
> +
> +
> +void RenderMathMLRow::layout() 

3 newlines? :-)


> +            if (!block->hasBase() && !block->isRenderMathMLOperator()) {
> +
> +                // Check to see if this box has a larger height

Newline

> +                if (block->offsetHeight() > maxHeight)
> +                    maxHeight = block->offsetHeight();
> +            }
> +            if (block->isRenderMathMLOperator())
> +                if (block->offsetHeight() > operatorHeight)
> +                    operatorHeight = block->offsetHeight();
> +        } else if (current->isBoxModelObject()) {
> +
> +            RenderBoxModelObject* box = toRenderBoxModelObject(current);
> +
> +            // Check to see if this box has a larger height

enwlines

[Alex Milowski]

Created attachment 49927 [details]
Updates to operator stacking to address review comments.

[Alex Milowski]

(In reply to comment #2)
> Just a few comments from a quick look at your patch (all strictly IMHO, since
> I'm not a reviewer):

I've address your comments in the latest patch.  Thanks.

[Alex Milowski]

I believe I've addressed all your review comments as well.  Please take a look when you get a chance.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/mathml/RenderMathMLOperator.cpp:82:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/mathml/RenderMathMLOperator.cpp:83:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/mathml/RenderMathMLOperator.cpp:84:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/mathml/RenderMathMLOperator.cpp:85:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/mathml/RenderMathMLOperator.cpp:86:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/mathml/RenderMathMLOperator.cpp:95:  One space before end of line comments  [whitespace/comments] [5]
WebCore/mathml/RenderMathMLOperator.cpp:130:  Missing spaces around ==  [whitespace/operators] [3]
WebCore/mathml/RenderMathMLOperator.cpp:155:  Missing spaces around /  [whitespace/operators] [3]
WebCore/mathml/RenderMathMLOperator.cpp:156:  Missing spaces around <  [whitespace/operators] [3]
Total errors found: 9 in 28 files


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

[Alex Milowski]

Created attachment 49928 [details]
Fixed style errors...

[Kenneth Rohde Christiansen]

Comment on attachment 49928 [details]
Fixed style errors...

Looks better but still some issues :-) r- because of inconsistency with variable/comment

129     Element* mo = 0;
130     if (node()->nodeType() == Node::ELEMENT_NODE) {
131         mo = static_cast<Element*>(node());
132         if (mo) {

You don't seem to use mo outside, so you could do

if (Element* mo = static_cast<Element*>(node())) {
...
}

150     // canStretch indicates whether the character is streatchable via a number of factors.
151     bool isStretchy = false;

Please fix the comment or variable.

195     
196     } else {

unneeded newline

260     }
261        
262 }

same thing

[Alex Milowski]

(In reply to comment #9)
> (From update of attachment 49928 [details])
> Looks better but still some issues :-) r- because of inconsistency with
> variable/comment
> 
> 129     Element* mo = 0;
> 130     if (node()->nodeType() == Node::ELEMENT_NODE) {
> 131         mo = static_cast<Element*>(node());
> 132         if (mo) {
> 
> You don't seem to use mo outside, so you could do

I actually is used later on in one case.  Maybe I'll just get it again because
it seems easy to miss.

Newlines are obviously a habit that is hard to break.

[Alex Milowski]

Created attachment 50012 [details]
Updated patch to address review comments

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/mathml/RenderMathMLOperator.cpp:193:  Missing space after ,  [whitespace/comma] [3]
WebCore/mathml/RenderMathMLOperator.cpp:196:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 2 in 28 files


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

[Alex Milowski]

Created attachment 50016 [details]
Style errors fix

*sigh*

[WebKit Commit Bot]

Comment on attachment 50016 [details]
Style errors fix

Rejecting patch 50016 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 12428 test cases.
http/tests/navigation/postredirect-reload.html -> crashed

Exiting early after 1 failures. 11816 tests run.
487.54s total testing time

11815 test cases (99%) succeeded
1 test case (<1%) crashed
11 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/332754

[Kenneth Rohde Christiansen]

Comment on attachment 50016 [details]
Style errors fix

Resetting cq, as the crash seems unrelated

[Eric Seidel (no email)]

(In reply to comment #15)
> (From update of attachment 50016 [details])
> Resetting cq, as the crash seems unrelated

This was bug 30519.  Please feel encouraged to search for/file bugs about flakey tests.

[WebKit Commit Bot]

Comment on attachment 50016 [details]
Style errors fix

Clearing flags on attachment: 50016

Committed r55607: <http://trac.webkit.org/changeset/55607>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.