Update usage of LayoutUnits in RenderBlock*

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Levi Weintraub]

Bringing RenderBlock and RenderBlockLineLayout up to date with our subpixellayout branch. There are a couple virtual methods to be changed that are being left out to be landed separately so the function signatures don't differ.

[Levi Weintraub]

Created attachment 130454 [details]
Patch

[Levi Weintraub]

Pinging reviewers.

[Julien Chaffraix]

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?

[Levi Weintraub]

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?

[Levi Weintraub]

Created attachment 131955 [details]
Patch

[Julien Chaffraix]

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!

[Levi Weintraub]

Created attachment 132163 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 132163 [details]
Patch for landing

Clearing flags on attachment: 132163

Committed r110983: <http://trac.webkit.org/changeset/110983>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.