[webkit-reviews] review granted: [Bug 87360] [Platform] Introduce Decimal class for Number/Range input type. : [Attachment 144775] Patch 6

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has granted yosin at chromium.org's request for
review:
Bug 87360: [Platform] Introduce Decimal class for Number/Range input type.
https://bugs.webkit.org/show_bug.cgi?id=87360

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=144775&action=review


> Source/WebCore/ChangeLog:25
> +	   (WebCore):
> +	   (DecimalPrivate):
> +	   (SpecialValueHandler):
> +	   (UInt128):
> +	   (WebCore::DecimalPrivate::UInt128::high):
> +	   (WebCore::DecimalPrivate::UInt128::low):

This function list is not synchronized with the real one.

> Source/WebCore/platform/Decimal.cpp:522
> +    if (result.isNaN())
> +	 return false;

wrong indentation

> Source/WebCore/platform/Decimal.cpp:530
> +    if (result.isNaN())
> +	 return false;

ditto.

> Source/WebCore/platform/Decimal.cpp:540
> +    if (result.isNaN())
> +	 return false;

ditto.

> Source/WebCore/platform/Decimal.cpp:558
> +    if (result.isNaN())
> +	 return false;

ditto.

> Source/WebCore/platform/Decimal.h:30
> + */
> +#ifndef Decimal_h

Need a blank line between the copyright header and the first #ifndef.

> Source/WebCore/platform/Decimal.h:130
> +    // Note: toString method supports infinity and nan but formString not.

formString -> fromString

> Source/WebKit/chromium/tests/DecimalTest.cpp:612
> +TEST_F(DecimalTest, FromStringLikeNumber)
> +{
> +    EXPECT_EQ(Decimal::nan(), fromString(" 123 "));
> +    EXPECT_EQ(Decimal::nan(), fromString("1,234"));
> +    EXPECT_EQ(Decimal::nan(), fromString("INF"));
> +}

Needs tests for "Infinity" and "NaN"