Bug 80437

Summary: Update usage of LayoutUnits in RenderBlock*
Product: WebKit Reporter: Levi Weintraub <leviw>
Component: Layout and RenderingAssignee: Levi Weintraub <leviw>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, eae, eric, jchaffraix, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 60318    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Levi Weintraub 2012-03-06 12:34:12 PST
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.
Comment 1 Levi Weintraub 2012-03-06 15:37:25 PST
Created attachment 130454 [details]
Patch
Comment 2 Levi Weintraub 2012-03-08 17:25:31 PST
Pinging reviewers.
Comment 3 Julien Chaffraix 2012-03-14 11:36:13 PDT
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 4 Levi Weintraub 2012-03-14 15:06:51 PDT
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?
Comment 5 Levi Weintraub 2012-03-14 16:31:46 PDT
Created attachment 131955 [details]
Patch
Comment 6 Julien Chaffraix 2012-03-14 19:05:51 PDT
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!
Comment 7 Levi Weintraub 2012-03-15 17:43:10 PDT
Created attachment 132163 [details]
Patch for landing
Comment 8 WebKit Review Bot 2012-03-16 03:53:15 PDT
Comment on attachment 132163 [details]
Patch for landing

Clearing flags on attachment: 132163

Committed r110983: <http://trac.webkit.org/changeset/110983>
Comment 9 WebKit Review Bot 2012-03-16 03:53:19 PDT
All reviewed patches have been landed.  Closing bug.