[webkit-reviews] review granted: [Bug 133040] Migrate layout ascents and descents to LayoutUnits instead of ints : [Attachment 434530] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Rob Buis <rbuis at igalia.com>'s request
for review:
Bug 133040: Migrate layout ascents and descents to LayoutUnits instead of ints
https://bugs.webkit.org/show_bug.cgi?id=133040

Attachment 434530: Patch

https://bugs.webkit.org/attachment.cgi?id=434530&action=review




--- Comment #17 from Darin Adler <darin at apple.com> ---
Comment on attachment 434530
  --> https://bugs.webkit.org/attachment.cgi?id=434530
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434530&action=review

> Source/WebCore/ChangeLog:8
> +	   Migrate GlyphOverflow to LayoutUnits instead of ints.

This is missing rationale.

Why are we doing this if it has no detectable effect? Does it make some future
change possible? Does it make something more efficient? Does it make code more
elegant? If it does have a good effect, then why no tests?

> Source/WebCore/rendering/LegacyInlineFlowBox.cpp:936
> +    LayoutUnit topGlyphEdge = glyphOverflow ? (isFlippedLine ?
glyphOverflow->bottom : glyphOverflow->top) : 0_lu;

Is it critical to use LayoutUnit rather than auto in these places? I ask
because "staying as a LayoutUnit" is one kind of thing. Converting to
LayoutUnit is another. Maybe this is conversion to LayoutUnit?