[webkit-reviews] review granted: [Bug 239389] RenderDeprecatedFlexibleBox::applyLineClamp should use size_t : [Attachment 457704] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted zalan <zalan at apple.com>'s request for
review:
Bug 239389: RenderDeprecatedFlexibleBox::applyLineClamp should use size_t
https://bugs.webkit.org/show_bug.cgi?id=239389

Attachment 457704: Patch

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




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

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

> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:988
> +static std::optional<LayoutUnit> getHeightForLineCount(const
RenderBlockFlow& block, size_t lineCount, bool includeBottom, size_t& count)

If we are changing the return type, it would be better to return a structure
with both the count and the height, instead of using a return value plus an out
argument.

But maybe count is an in/out argument?

> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:1003
> +		       return *height + obj->y() + (includeBottom ?
(block.borderBottom() + block.paddingBottom()) : 0_lu);

I am not familiar with how we avoid overflow in math like this; do we stay far
from the limits of the LayoutUnit type to avoid it? Is there a guarantee that
this expression won’t overflow?

> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:1066
> +    size_t numVisibleLines = lineClamp.isPercentage() ? std::max<size_t>(1,
(maxLineCount + 1) * lineClamp.value() / 100) : lineClamp.value();

What prevents overflow in the expression "(maxLineCount + 1) *
lineClamp.value() / 100"?