[webkit-reviews] review granted: [Bug 104955] Clamp values in LayoutUnit::setRawValue when SATURATED_LAYOUT_ARITHMETIC is enabled : [Attachment 179509] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Emil A Eklund
<eae at chromium.org>'s request for review:
Bug 104955: Clamp values in LayoutUnit::setRawValue when
SATURATED_LAYOUT_ARITHMETIC is enabled
https://bugs.webkit.org/show_bug.cgi?id=104955

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=179509&action=review


Looks fine, some comment. Please, leave some time before landing to get a green
EWS and in case some of people more knowledgeable in the different hardware we
support can double check.

> Source/WebCore/platform/LayoutUnit.h:182
> +	   m_value = clampTo(value, std::numeric_limits<int>::min(),
std::numeric_limits<int>::max());

We should probably mention that this may lead to a loss of precision if long
long is 64 bits but as we are casting the information to an int (32 bits on all
64 bits architecture that we know about), it shouldn't matter.

Also, unless I am mistaking, this looks like it could be written: m_value =
clampTo<int>(value);
(clampToInteger would be ambiguous AFAICT so it would require an unfortunate
cast)

> Tools/TestWebKitAPI/ForwardingHeaders/WebCore/KURL.h:4
> +#endif

Nit: We should probably wait until we enable the test to land that

> Tools/TestWebKitAPI/TestWebKitAPI.gyp/TestWebKitAPI.gyp:57
> +		   '<(source_dir)/WebCore/WebCore.gyp/WebCore.gyp:webcore',

For archive's sake, we discussed that over IM and it makes more sense to link
against WebCore if we are to enable the test for KURL but also to test any
class that isn't fully implemented in its header file.

> Tools/TestWebKitAPI/Tests/WebCore/LayoutUnit.cpp:79
> +    ASSERT_NEAR(LayoutUnit(1.0f).toFloat(), 1.0f, tolerance);
> +    ASSERT_NEAR(LayoutUnit(1.1f).toFloat(), 1.1f, tolerance);
> +    ASSERT_NEAR(LayoutUnit(1.25f).toFloat(), 1.25f, tolerance);
> +    ASSERT_NEAR(LayoutUnit(1.33f).toFloat(), 1.33f, tolerance);
> +    ASSERT_NEAR(LayoutUnit(1.3333f).toFloat(), 1.3333f, tolerance);
> +    ASSERT_NEAR(LayoutUnit(1.53434f).toFloat(), 1.53434f, tolerance);
> +    ASSERT_NEAR(LayoutUnit(345634).toFloat(), 345634.0f, tolerance);
> +    ASSERT_NEAR(LayoutUnit(345634.12335f).toFloat(), 345634.12335f,
tolerance);
> +    ASSERT_NEAR(LayoutUnit(-345634.12335f).toFloat(), -345634.12335f,
tolerance);
> +    ASSERT_NEAR(LayoutUnit(-345634).toFloat(), -345634.0f, tolerance);    

Are these ASSERT_NEAR really needed? Below you use ASSERT_FLOAT_EQ for the
division and I wouldn't expect much loss in precisions for most of numbers here
as they don't require much mantissa bits to represent.