Bug 80437 - Update usage of LayoutUnits in RenderBlock*
Summary: Update usage of LayoutUnits in RenderBlock*
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Levi Weintraub
URL:
Keywords:
Depends on:
Blocks: 60318
  Show dependency treegraph
 
Reported: 2012-03-06 12:34 PST by Levi Weintraub
Modified: 2012-03-16 03:53 PDT (History)
6 users (show)

See Also:


Attachments
Patch (22.60 KB, patch)
2012-03-06 15:37 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (22.70 KB, patch)
2012-03-14 16:31 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch for landing (21.07 KB, patch)
2012-03-15 17:43 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.