Bug 37044

Summary: MathML Support for mroot & msqrt
Product: WebKit Reporter: François Sausset <sausset>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, cjerdonek, commit-queue, eric
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Macintosh Intel   
OS: OS X 10.6   
Bug Depends on: 37765    
Bug Blocks: 3251    
Attachments:
Description Flags
Based on the Alex Milowski MathML "monster bug". Improved paddings, baseline alignments & root drawing.
none
Updated patch
none
Updated patch
kenneth: review+, commit-queue: commit-queue-
Up-to-date version of the patch
kenneth: review+, commit-queue: commit-queue-
Small style fix
none
Up-to-date version of the patch none

Description François Sausset 2010-04-02 14:24:03 PDT
Created attachment 52450 [details]
Based on the Alex Milowski MathML "monster bug". Improved paddings, baseline alignments & root drawing.

First attempt for an implementation of mroot & msqrt elements.
Comment 1 Alex Milowski 2010-04-13 02:32:05 PDT
> Index: WebCore/mathml/RenderMathMLRoot.cpp
> ===================================================================
> --- WebCore/mathml/RenderMathMLRoot.cpp	(revision 0)
> +++ WebCore/mathml/RenderMathMLRoot.cpp	(revision 0)

> +
> +const int radicalBasePad = 3;
> +const float thresholdBaseHeight = 1.5;

This should probable be prefixed with a 'g':

   gRadicalBasePad
   gThresholdBaseHeight

Any comment describing these constants would be good.

> +
> +RenderMathMLRoot::RenderMathMLRoot(Node *expression) 
> +: RenderMathMLBlock(expression) 
> +{
> +}
> +
> +void RenderMathMLRoot::addChild(RenderObject* child, RenderObject* )
> +{
> +    if (isEmpty()) {
> +        // Add a block for the index
> +        RenderBlock* block = new (renderArena()) RenderBlock(node());
> +        RefPtr<RenderStyle> indexStyle = makeBlockStyle();
> +        indexStyle->setDisplay(INLINE_BLOCK);
> +        block->setStyle(indexStyle.release());
> +        RenderBlock::addChild(block);
> +        
> +        // this is the base, so wrap it so we can pad it
> +        block = new (renderArena()) RenderBlock(node());
> +        RefPtr<RenderStyle> baseStyle = makeBlockStyle();
> +        baseStyle->setDisplay(INLINE_BLOCK);
> +        baseStyle->setPaddingLeft(Length(0.5 * 0.75 , Percent));

You should make these numbers constants.



> +void RenderMathMLRoot::paint(PaintInfo& info, int tx, int ty)
> +{
> +    RenderMathMLBlock::paint(info , tx , ty);
> +    
> +    tx += x();
> +    ty += y();
> +    
> +    RenderBoxModelObject* indexBox = toRenderBoxModelObject(lastChild());
> +    
> +    int maxHeight = indexBox->offsetHeight();
> +    if (!maxHeight) {
> +        // default to the font size in pixels if we're empty
> +        maxHeight = style()->fontSize();
> +    }
> +    int width = indexBox->offsetWidth();
> +    
> +    int indexWidth = 0;
> +    RenderObject* current = firstChild();
> +    while (current != lastChild()) {
> +        if (current->isBoxModelObject()) {
> +            RenderBoxModelObject* box = toRenderBoxModelObject(current);
> +            indexWidth += box->offsetWidth();
> +        }
> +        current = current->nextSibling();
> +    }
> +    
> +    int frontWidth = static_cast<int>(style()->fontSize() * 0.75);

Make the number a constant as well.

> +    int topStartShift = 0;
> +    // Base height above which the shape of the root changes
> +    int thresholdHeight = static_cast<int>(thresholdBaseHeight * style()->fontSize());
> +    
> +    if (maxHeight > thresholdHeight && thresholdHeight) {
> +        float shift = (maxHeight - thresholdHeight) / static_cast<float>(thresholdHeight);
> +        if (shift > 1.)
> +            shift = 1.;
> +        topStartShift = static_cast<int>(0.5 * frontWidth * shift);
> +    }

Same here with the 0.5

> +    
> +    width += topStartShift;
> +    
> +    int start = tx + indexWidth + radicalBasePad + style()->paddingLeft().value() - static_cast<int>(0.2 * style()->fontSize());
> +    ty += style()->paddingTop().value() - static_cast<int>(0.2 * style()->fontSize());
> +    
> +    FloatPoint topStart(start - topStartShift, ty);
> +    FloatPoint bottomLeft(start - 0.5 * frontWidth , ty + maxHeight + radicalBasePad);
> +    FloatPoint topLeft(start - 0.8 * frontWidth , ty + 0.625 * maxHeight);
> +    FloatPoint leftEnd(start - frontWidth , topLeft.y() + 0.05 * style()->fontSize());

Similarly, make the numbers named constants.

> +    
> +    info.context->save();
> +    
> +    info.context->setStrokeThickness(0.02 * style()->fontSize());

Same here.

> +    info.context->setStrokeStyle(SolidStroke);
> +    info.context->setStrokeColor(style()->color(), sRGBColorSpace);
> +    info.context->setLineJoin(MiterJoin);
> +    info.context->setMiterLimit(style()->fontSize());
> +    
> +    Path root;
> +    
> +    root.moveTo(FloatPoint(topStart.x() + width , ty));

No space before comma.

> +    // draw top
> +    root.addLineTo(topStart);
> +    // draw from top left corner to bottom point of radical
> +    root.addLineTo(bottomLeft);
> +    // draw from bottom point to top of left part of radical base "pocket"
> +    root.addLineTo(topLeft);
> +    // draw to end
> +    root.addLineTo(leftEnd);
> +    
> +    info.context->beginPath();
> +    info.context->addPath(root);
> +    info.context->strokePath();
> +    
> +    info.context->save();
> +    
> +    // Build a mask to draw the thick part of the root.
> +    Path mask;
> +    
> +    mask.moveTo(topStart);
> +    mask.addLineTo(bottomLeft);
> +    mask.addLineTo(topLeft);
> +    mask.addLineTo(FloatPoint(2 * topLeft.x() - leftEnd.x(), 2 * topLeft.y() - leftEnd.y()));

Make the '2' a constant.

> +    
> +    info.context->beginPath();
> +    info.context->addPath(mask);
> +    info.context->clip(mask);
> +    
> +    // Draw the thick part of the root.
> +    info.context->setStrokeThickness(0.1 * style()->fontSize());

Same here.

> +    info.context->setLineCap(SquareCap);
> +    
> +    Path line;
> +    
> +    line = line.createLine(bottomLeft, topLeft);
> +    
> +    info.context->beginPath();
> +    info.context->addPath(line);
> +    info.context->strokePath();
> +    
> +    info.context->restore();
> +    
> +    info.context->restore();
> +
> +}
> +
> +void RenderMathMLRoot::layout()
> +{
> +    RenderBlock::layout();
> +    
> +    int maxHeight = toRenderBoxModelObject(lastChild())->offsetHeight();
> +    
> +    RenderObject* current = lastChild()->firstChild();
> +    
> +    toRenderMathMLBlock(current)->style()->setVerticalAlign(BASELINE);
> +    
> +    if (!maxHeight)
> +        maxHeight = style()->fontSize();
> +    
> +    // Base height above which the shape of the root changes
> +    int thresholdHeight = static_cast<int>(thresholdBaseHeight * style()->fontSize());
> +    int topStartShift = 0;
> +    
> +    if (maxHeight > thresholdHeight && thresholdHeight) {
> +        float shift = (maxHeight - thresholdHeight) / static_cast<float>(thresholdHeight);
> +        if (shift > 1.)
> +            shift = 1.;
> +        topStartShift = static_cast<int>(0.5 * static_cast<int>(style()->fontSize() * 0.75) * shift);
> +        
> +        style()->setPaddingBottom(Length(static_cast<int>(0.2 * style()->fontSize()), Fixed));

More named constants here.

> +    }
> +    
> +    // Positioning of the index
> +    RenderBoxModelObject* indexBox = toRenderBoxModelObject(firstChild()->firstChild());
> +    
> +    int indexShift = indexBox->offsetWidth() + topStartShift;
> +    int rootMarginTop = static_cast<int>(0.375 * maxHeight) + style()->paddingBottom().value() + indexBox->offsetHeight() - (maxHeight + static_cast<int>(0.2 * style()->fontSize()));
> +    
> +    style()->setPaddingLeft(Length(indexShift, Fixed));
> +    if (rootMarginTop > 0)
> +        style()->setPaddingTop(Length(rootMarginTop + static_cast<int>(0.2 * style()->fontSize()), Fixed));

Same here.

> +    
> +    setNeedsLayoutAndPrefWidthsRecalc();
> +    markContainingBlocksForLayout();
> +    RenderBlock::layout();
> +    
> +    indexBox->style()->setBottom(Length(static_cast<int>(0.375 * (maxHeight)) + style()->paddingBottom().value(), Fixed));

Same here.

> +    
> +    indexBox->layout();
> +}
> +    
> +}
> +
> +#endif // ENABLE(MATHML)
> +
> +
> Index: WebCore/mathml/RenderMathMLSquareRoot.cpp
> ===================================================================
> --- WebCore/mathml/RenderMathMLSquareRoot.cpp	(revision 0)
> +++ WebCore/mathml/RenderMathMLSquareRoot.cpp	(revision 0)

> +
> +const int radicalBasePad = 3;
> +const float thresholdBaseHeight = 1.5;

Change the names to be prefixed with a 'g' as before.

> +    
> +RenderMathMLSquareRoot::RenderMathMLSquareRoot(Node *expression) 
> +    : RenderMathMLBlock(expression) 
> +{
> +}
> +
> +void RenderMathMLSquareRoot::paint(PaintInfo& info, int tx, int ty)
> +{
> +    RenderMathMLBlock::paint(info, tx, ty);
> +   
> +    tx += x();
> +    ty += y();
> +
> +    int maxHeight = 0;
> +    int width = 0;
> +    RenderObject* current = firstChild();
> +    while (current) {
> +        if (current->isBoxModelObject()) {
> +            
> +            RenderBoxModelObject* box = toRenderBoxModelObject(current);
> +            
> +            // Check to see if this box has a larger height
> +            if (box->offsetHeight() > maxHeight)
> +                maxHeight = box->offsetHeight();
> +            width += box->offsetWidth();
> +        }
> +        current = current->nextSibling();
> +    }
> +    // default to the font size in pixels if we're empty
> +    if (!maxHeight)
> +        maxHeight = style()->fontSize();
> +    
> +    int frontWidth = static_cast<int>(style()->fontSize() * 0.75);

use named constants.

> +    int topStartShift = 0;
> +    // Base height above which the shape of the root changes
> +    int thresholdHeight = static_cast<int>(thresholdBaseHeight * style()->fontSize());
> +    
> +    if (maxHeight > thresholdHeight && thresholdHeight) {
> +        float shift = (maxHeight - thresholdHeight) / static_cast<float>(thresholdHeight);
> +        if (shift > 1.)
> +            shift = 1.;
> +        topStartShift = static_cast<int>(0.5 * frontWidth * shift);
> +    }
> +    
> +    width += topStartShift;
> +    
> +    FloatPoint topStart(tx + frontWidth - topStartShift, ty);
> +    FloatPoint bottomLeft(tx + frontWidth * 0.5 , ty + maxHeight + radicalBasePad);
> +    FloatPoint topLeft(tx + frontWidth * 0.2 , ty + 0.5 * maxHeight);
> +    FloatPoint leftEnd(tx , topLeft.y() + 0.05 * style()->fontSize());
> +    
> +    info.context->save();
> +    
> +    info.context->setStrokeThickness(0.02 * style()->fontSize());

Same in all of the above.


> +    mask.addLineTo(FloatPoint(2 * topLeft.x() - leftEnd.x(), 2 * topLeft.y() - leftEnd.y()));

Same above.

> +    
> +    info.context->beginPath();
> +    info.context->addPath(mask);
> +    info.context->clip(mask);
> +    
> +    // Draw the thick part of the root.
> +    info.context->setStrokeThickness(0.1 * style()->fontSize());

Same above.


> +void RenderMathMLSquareRoot::layout()
> +{
> +    int maxHeight = 0;
> +    
> +    RenderObject* current = firstChild();
> +    while (current) {
> +        if (current->isBoxModelObject()) {
> +            RenderBoxModelObject* box = toRenderBoxModelObject(current);
> +            
> +            if (box->offsetHeight() > maxHeight)
> +                maxHeight = box->offsetHeight();
> +            
> +            box->style()->setVerticalAlign(BASELINE);
> +        }
> +        current = current->nextSibling();
> +    }
> +    
> +    if (!maxHeight)
> +        maxHeight = style()->fontSize();
> +
> +    
> +    if (maxHeight > static_cast<int>(thresholdBaseHeight * style()->fontSize()))
> +        style()->setPaddingBottom(Length(static_cast<int>(0.2 * style()->fontSize()), Fixed));

Same above.


In general, you should try to use global constants for all your calculations.  I went
back through that in the rest of the MathML code and tried to make that change.  Try
to use names like "gPaddingBottomFactor" rather than something more specific like
"gDoubleHeight".

Looks good otherwise.
Comment 2 François Sausset 2010-04-17 11:47:48 PDT
Created attachment 53604 [details]
Updated patch

Numeric values have been moved to constants as suggested.
Only the value "2" has not been changed because it is not an adjustable parameter for the root drawing.
Comment 3 Alex Milowski 2010-04-19 03:50:32 PDT
> ===================================================================
> --- WebCore/mathml/RenderMathMLRoot.cpp	(revision 0)
> +++ WebCore/mathml/RenderMathMLRoot.cpp	(revision 0)

[snip] 

> +
> +void RenderMathMLRoot::addChild(RenderObject* child, RenderObject* )
> +{
> +    if (isEmpty()) {
> +        // Add a block for the index
> +        RenderBlock* block = new (renderArena()) RenderBlock(node());
> +        RefPtr<RenderStyle> indexStyle = makeBlockStyle();
> +        indexStyle->setDisplay(INLINE_BLOCK);
> +        block->setStyle(indexStyle.release());
> +        RenderBlock::addChild(block);
> +        
> +        // FIXME: the wrapping does not seem to be needed anymore.
> +        // this is the base, so wrap it so we can pad it
> +        block = new (renderArena()) RenderBlock(node());
> +        RefPtr<RenderStyle> baseStyle = makeBlockStyle();
> +        baseStyle->setDisplay(INLINE_BLOCK);
> +//        baseStyle->setPaddingLeft(Length(5 * gRadicalWidth , Percent));

You shouldn't have commented out code in the patch.

> +        block->setStyle(baseStyle.release());
> +        RenderBlock::addChild(block);
> +        block->addChild(child);
> +    } else {
> +        // always add to the index
> +        firstChild()->addChild(child);
> +    }
> +}
> +    
> +void RenderMathMLRoot::paint(PaintInfo& info, int tx, int ty)
> +{
> +    RenderMathMLBlock::paint(info , tx , ty);
> +    
> +    tx += x();
> +    ty += y();
> +    
> +    RenderBoxModelObject* indexBox = toRenderBoxModelObject(lastChild());
> +    
> +    int maxHeight = indexBox->offsetHeight();
> +    if (!maxHeight) {
> +        // default to the font size in pixels if we're empty
> +        maxHeight = style()->fontSize();
> +    }

You really shouldn't have the braces around this just to have
the commented for the assignment.  

> +    int width = indexBox->offsetWidth();
> +    
> +    int indexWidth = 0;
> +    RenderObject* current = firstChild();
> +    while (current != lastChild()) {
> +        if (current->isBoxModelObject()) {
> +            RenderBoxModelObject* box = toRenderBoxModelObject(current);
> +            indexWidth += box->offsetWidth();
> +        }
> +        current = current->nextSibling();
> +    }
> +    
> +    int frontWidth = static_cast<int>(style()->fontSize() * gRadicalWidth);
> +    int topStartShift = 0;
> +    // Base height above which the shape of the root changes
> +    int thresholdHeight = static_cast<int>(gThresholdBaseHeight * style()->fontSize());
> +    
> +    if (maxHeight > thresholdHeight && thresholdHeight) {
> +        float shift = (maxHeight - thresholdHeight) / static_cast<float>(thresholdHeight);
> +        if (shift > 1.)
> +            shift = 1.;

I think you want 1.0 because you want to force a float comparison.  I don't see
anything in the style guide that says 1. is bad so it is up to you.
Comment 4 François Sausset 2010-04-19 04:03:06 PDT
Created attachment 53668 [details]
Updated patch

I removed the comment, but let the FIXME, as it seems that part of the code is not needed anymore.
I also corrected the small style mistake.
About 1., it is indeed to force a float comparison.
Comment 5 Kenneth Rohde Christiansen 2010-04-19 07:30:01 PDT
Comment on attachment 53668 [details]
Updated patch

Looks good to me, and it seems that Alex is fine with it as well
Comment 6 WebKit Commit Bot 2010-04-19 09:04:15 PDT
Comment on attachment 53668 [details]
Updated patch

Rejecting patch 53668 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--test', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', '53668', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
', 'commit', '--all', '-F', '-'], input=message)
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/system/executive.py", line 85, in run_command
    return Executive().run_command(*args, **kwargs)
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/common/system/executive.py", line 186, in run_command
    string_to_communicate = str(input)
UnicodeEncodeError: 'ascii' codec can't encode character u'\xe7' in position 16: ordinal not in range(128)
Comment 7 Eric Seidel (no email) 2010-04-19 12:26:59 PDT
Sorry you were bitten by bug 37765.  I hope to have that fixed today!
Comment 8 Eric Seidel (no email) 2010-04-20 12:58:12 PDT
Comment on attachment 53668 [details]
Updated patch

Sorry again for the trouble.
Comment 9 Eric Seidel (no email) 2010-04-21 03:48:43 PDT
Comment on attachment 53668 [details]
Updated patch

The fix for bug 37765 was rolled out.  So this will fail to land.  Hopefully I'll reland the fix tomorrow at which point I'll cq+ this again.
Comment 10 Eric Seidel (no email) 2010-04-21 15:12:55 PDT
Comment on attachment 53668 [details]
Updated patch

Fixed again.
Comment 11 WebKit Commit Bot 2010-04-21 22:30:01 PDT
Comment on attachment 53668 [details]
Updated patch

Rejecting patch 53668 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Kenneth Rohde Christiansen', u'--force']" exit_code: 1
Last 500 characters of output:
cpp
patching file WebCore/mathml/RenderMathMLSquareRoot.h
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/mathml/presentation/roots.xhtml
patching file LayoutTests/platform/mac/mathml/presentation/roots-expected.checksum
patching file LayoutTests/platform/mac/mathml/presentation/roots-expected.txt
error: pathspec 'LayoutTests/platform/mac/mathml/presentation/roots-expected.png' did not match any file(s) known to git.
Did you forget to 'git add'?

Full output: http://webkit-commit-queue.appspot.com/results/1831024
Comment 12 Chris Jerdonek 2010-04-23 09:13:09 PDT
(In reply to comment #11)
> (From update of attachment 53668 [details])
> Rejecting patch 53668 from commit-queue.
> 
> Failed to run
> "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply',
> u'--reviewer', u'Kenneth Rohde Christiansen', u'--force']" exit_code: 1
> Last 500 characters of output:
> <snip>
> LayoutTests/platform/mac/mathml/presentation/roots-expected.checksum
> patching file LayoutTests/platform/mac/mathml/presentation/roots-expected.txt
> error: pathspec
> 'LayoutTests/platform/mac/mathml/presentation/roots-expected.png' did not match
> any file(s) known to git.
> Did you forget to 'git add'?
> 
> Full output: http://webkit-commit-queue.appspot.com/results/1831024

The last 500 characters of output is deceptive in that the final "pathspec" error message has nothing to do with the commit-queue failing.  That output is actually normal (but should probably be corrected to be suppressed).

This was the only problem file (perhaps the patch needs to be brought up to date?):

patching file WebCore/WebCore.xcodeproj/project.pbxproj
Hunk #2 succeeded at 5573 (offset 1 line).
Hunk #3 succeeded at 16346 (offset 3 lines).
Hunk #4 FAILED at 18880.
Hunk #5 FAILED at 21118.
2 out of 5 hunks FAILED -- saving rejects to file WebCore/WebCore.xcodeproj/project.pbxproj.rej

That's the file that caused exit code 1.
Comment 13 Chris Jerdonek 2010-04-23 09:26:57 PDT
(In reply to comment #12)
> The last 500 characters of output is deceptive in that the final "pathspec"
> error message has nothing to do with the commit-queue failing.  That output is
> actually normal (but should probably be corrected to be suppressed).

I filed a bug for this issue here:

https://bugs.webkit.org/show_bug.cgi?id=38047
Comment 14 François Sausset 2010-04-23 11:52:30 PDT
Created attachment 54179 [details]
Up-to-date version of the patch

Regenerated patch from trunk r58178.
It resolves the svn-apply problems.
Comment 15 WebKit Commit Bot 2010-04-23 14:53:01 PDT
Comment on attachment 54179 [details]
Up-to-date version of the patch

Rejecting patch 54179 from commit-queue.

Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1
Last 500 characters of output:
ed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe
svnlook: Can't write to stream: Broken pipe

    The following files contain tab characters:

        trunk/LayoutTests/mathml/presentation/roots.xhtml

    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
 at /usr/local/git/libexec/git-core/git-svn line 570


Full output: http://webkit-commit-queue.appspot.com/results/1796067
Comment 16 François Sausset 2010-04-23 15:02:14 PDT
Created attachment 54195 [details]
Small style fix

Tabs remained in the xhtml test file.
Comment 17 François Sausset 2010-04-27 10:05:32 PDT
Created attachment 54424 [details]
Up-to-date version of the patch

Regenerated patch from trunk r58312 to avoid problems with svn-apply on .xcodeproj files.
Comment 18 WebKit Commit Bot 2010-04-27 16:16:09 PDT
Comment on attachment 54424 [details]
Up-to-date version of the patch

Clearing flags on attachment: 54424

Committed r58349: <http://trac.webkit.org/changeset/58349>
Comment 19 WebKit Commit Bot 2010-04-27 16:16:17 PDT
All reviewed patches have been landed.  Closing bug.