Bug 37044

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

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.

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

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 on attachment 53668 [details] Updated patch Looks good to me, and it seems that Alex is fine with it as well

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)

Sorry you were bitten by bug 37765. I hope to have that fixed today!

Comment on attachment 53668 [details] Updated patch Sorry again for the trouble.

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 on attachment 53668 [details] Updated patch Fixed again.

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

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

(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

Created attachment 54179 [details] Up-to-date version of the patch Regenerated patch from trunk r58178. It resolves the svn-apply problems.

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

Created attachment 54195 [details] Small style fix Tabs remained in the xhtml test file.

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 on attachment 54424 [details] Up-to-date version of the patch Clearing flags on attachment: 54424 Committed r58349: <http://trac.webkit.org/changeset/58349>

All reviewed patches have been landed. Closing bug.