[webkit-reviews] review granted: [Bug 80437] Update usage of LayoutUnits in RenderBlock* : [Attachment 131955] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Levi Weintraub
<leviw at chromium.org>'s request for review:
Bug 80437: Update usage of LayoutUnits in RenderBlock*
https://bugs.webkit.org/show_bug.cgi?id=80437

Attachment 131955: Patch
https://bugs.webkit.org/attachment.cgi?id=131955&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
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().descen
t() : 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!