Bug 34347

Summary: MathML Support for mrow and Stretchy Operators
Product: WebKit Reporter: Alex Milowski <alex>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric, fred.wang, michael.vm, rolandsteiner, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac (Intel)   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 3251    
Attachments:
Description Flags
Patch to add mrow and stretchy operator support.
none
Updated patch to latest trunk with anonymous blocks removed.
kenneth: review-
Updates to operator stacking to address review comments.
none
Fixed style errors...
kenneth: review-
Updated patch to address review comments
none
Style errors fix none

Description Alex Milowski 2010-01-29 14:34:06 PST
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.
Comment 1 Alex Milowski 2010-02-08 21:00:18 PST
Created attachment 48392 [details]
Updated patch to latest trunk with anonymous blocks removed.
Comment 2 Roland Steiner 2010-03-01 01:06:31 PST
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.
Comment 3 Kenneth Rohde Christiansen 2010-03-02 05:09:46 PST
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
Comment 4 Alex Milowski 2010-03-03 11:31:21 PST
Created attachment 49927 [details]
Updates to operator stacking to address review comments.
Comment 5 Alex Milowski 2010-03-03 11:31:57 PST
(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.
Comment 6 Alex Milowski 2010-03-03 11:32:47 PST
I believe I've addressed all your review comments as well.  Please take a look when you get a chance.
Comment 7 WebKit Review Bot 2010-03-03 11:35:35 PST
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.
Comment 8 Alex Milowski 2010-03-03 11:41:57 PST
Created attachment 49928 [details]
Fixed style errors...
Comment 9 Kenneth Rohde Christiansen 2010-03-04 04:40:36 PST
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
Comment 10 Alex Milowski 2010-03-04 06:41:51 PST
(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.
Comment 11 Alex Milowski 2010-03-04 07:11:10 PST
Created attachment 50012 [details]
Updated patch to address review comments
Comment 12 WebKit Review Bot 2010-03-04 07:13:59 PST
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.
Comment 13 Alex Milowski 2010-03-04 07:19:59 PST
Created attachment 50016 [details]
Style errors fix

*sigh*
Comment 14 WebKit Commit Bot 2010-03-04 11:57:39 PST
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
Comment 15 Kenneth Rohde Christiansen 2010-03-05 04:38:25 PST
Comment on attachment 50016 [details]
Style errors fix

Resetting cq, as the crash seems unrelated
Comment 16 Eric Seidel (no email) 2010-03-05 11:28:25 PST
(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.
Comment 17 WebKit Commit Bot 2010-03-05 18:52:56 PST
Comment on attachment 50016 [details]
Style errors fix

Clearing flags on attachment: 50016

Committed r55607: <http://trac.webkit.org/changeset/55607>
Comment 18 WebKit Commit Bot 2010-03-05 18:53:03 PST
All reviewed patches have been landed.  Closing bug.