[webkit-reviews] review granted: [Bug 71500] Correct usage of LayoutUnits and integers in Table rendering classes : [Attachment 113552] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Levi Weintraub <leviw at chromium.org>'s
request for review:
Bug 71500: Correct usage of LayoutUnits and integers in Table rendering classes
https://bugs.webkit.org/show_bug.cgi?id=71500

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=113552&action=review


Are all those max<int> needed? If both arguments are already int, I would
expect you could leave out the <int>.

Lets not leave things behind that show the churn from when we converted to
LayoutUnit and back. We should only have things like <int> if they are actually
needed.

> Source/WebCore/rendering/AutoTableLayout.cpp:255
> +	   maxWidth = max<int>(maxWidth, static_cast<int>(min(maxNonPercent,
numeric_limits<LayoutUnit>::max() / 2.0f)));
> +	   maxWidth = max<int>(maxWidth, static_cast<int>(min(maxPercent,
numeric_limits<LayoutUnit>::max() / 2.0f)));

It seems unfortunate to convert from LayoutUnit to int using static_cast. Isn’t
there a better way? For example, could you just leave out the cast entirely,
since the argument to max<int> is an int?

> Source/WebCore/rendering/AutoTableLayout.cpp:392
> +		       int cellLogicalWidth =
max<int>(m_layoutStruct[pos].effectiveMinLogicalWidth,
static_cast<int>(cellMinLogicalWidth * m_layoutStruct[pos].logicalWidth.value()
/ fixedWidth));

Same comment here about static_cast<int>. I think it can be omitted.

> Source/WebCore/rendering/AutoTableLayout.cpp:428
> +		       int colMaxLogicalWidth =
max<int>(m_layoutStruct[pos].effectiveMaxLogicalWidth,
static_cast<int>(spanMaxLogicalWidth ? cellMaxLogicalWidth *
static_cast<float>(m_layoutStruct[pos].effectiveMaxLogicalWidth) /
spanMaxLogicalWidth : cellMaxLogicalWidth));

And here.