Bug 80437

Created attachment 130454 [details] Patch

Pinging reviewers.

Comment on attachment 130454 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130454&action=review > Source/WebCore/ChangeLog:26 > + (WebCore::RenderBlock::newLine): Use a LayoutUnit the y position. +for ? > Source/WebCore/rendering/InlineTextBox.cpp:482 > + LayoutUnit logicalStart = logicalLeftSide + (isHorizontal() ? adjustedPaintOffset.x() : adjustedPaintOffset.y()); Shouldn't we snap / round the result of the addition and not round first then do the addition? > Source/WebCore/rendering/RenderBlock.cpp:5397 > - LayoutUnit snappedResult = ceiledLayoutUnit(result); > + LayoutUnit snappedResult = ceilf(result); I don't understand why ceilf is better as snappedResult is still a LayoutUnit and that's what ceiledLayoutUnit returns. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:137 > m_right = m_block->pixelSnappedLogicalLeftForFloat(newFloat); > if (m_isFirstLine && !m_block->style()->isLeftToRightDirection()) > - m_right -= m_block->textIndentOffset(); > + m_right -= roundToInt(m_block->textIndentOffset()); I wonder if we don't have some problem here as we round before adding. Same for m_left. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2694 > LayoutUnit firstLineEllipsisWidth = firstLineFont.width(constructTextRun(this, firstLineFont, &horizontalEllipsis, 1, firstLineStyle())); > - LayoutUnit ellipsisWidth = (font == firstLineFont) ? firstLineEllipsisWidth : font.width(constructTextRun(this, font, &horizontalEllipsis, 1, style())); > + LayoutUnit ellipsisWidth = (font == firstLineFont) ? firstLineEllipsisWidth : static_cast<LayoutUnit>(font.width(constructTextRun(this, font, &horizontalEllipsis, 1, style()))); Shouldn't the ellipsisWidth be an int here (== device pixels) as it maps to the font's width?

Comment on attachment 130454 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130454&action=review I'm going to address a couple of the concerns and re-upload. >> Source/WebCore/rendering/RenderBlock.cpp:5397 >> + LayoutUnit snappedResult = ceilf(result); > > I don't understand why ceilf is better as snappedResult is still a LayoutUnit and that's what ceiledLayoutUnit returns. The intention with ceiledLayoutUnit was to ceil now, and preserve the precision in a LayoutUnit later. Unfortunately, here we actually need to ceil because blocks continue to paint on pixel boundaries, so we need to expand our width to ensure we contain the line. I'll write a better comment in the changelog. >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2694 >> + LayoutUnit ellipsisWidth = (font == firstLineFont) ? firstLineEllipsisWidth : static_cast<LayoutUnit>(font.width(constructTextRun(this, font, &horizontalEllipsis, 1, style()))); > > Shouldn't the ellipsisWidth be an int here (== device pixels) as it maps to the font's width? font.width returns a float. Since we currently truncate to ints seemingly without a problem, maybe this is okay?

Created attachment 131955 [details] Patch

Comment on attachment 131955 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131955&action=review > Source/WebCore/rendering/RenderBlockLineLayout.cpp:652 > + LayoutUnit rootDescent = includeRootLine ? lineBox->renderer()->style(lineInfo.isFirstLine())->font().fontMetrics().descent() : 0; > + LayoutUnit rootAscent = includeRootLine ? lineBox->renderer()->style(lineInfo.isFirstLine())->font().fontMetrics().ascent() : 0; > + LayoutUnit boxAscent = renderer->style(lineInfo.isFirstLine())->font().fontMetrics().ascent() - baselineShift; > + LayoutUnit boxDescent = renderer->style(lineInfo.isFirstLine())->font().fontMetrics().descent() + baselineShift; Shouldn't it stay |int| to be coherent with the other change to |ellipsisWidth|? > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2694 > + int firstLineEllipsisWidth = firstLineFont.width(constructTextRun(this, firstLineFont, &horizontalEllipsis, 1, firstLineStyle())); > + int ellipsisWidth = (font == firstLineFont) ? firstLineEllipsisWidth : font.width(constructTextRun(this, font, &horizontalEllipsis, 1, style())); >> Shouldn't the ellipsisWidth be an int here (== device pixels) as it maps to the font's width? >font.width returns a float. Since we currently truncate to ints seemingly without a problem, maybe this is okay? I missed the fact that we did convert from float. The font code does a lot of truncating / rounding so I think it can go both ways. Just pick one between here and the above spot and let's document it and stick to it!

Created attachment 132163 [details] Patch for landing

Comment on attachment 132163 [details] Patch for landing Clearing flags on attachment: 132163 Committed r110983: <http://trac.webkit.org/changeset/110983>

All reviewed patches have been landed. Closing bug.