[webkit-reviews] review denied: [Bug 80009] Introduce Decimal arithmetic to fix rounding errors in number/range input types : [Attachment 139585] Patch 5

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied  review:
Bug 80009: Introduce Decimal arithmetic to fix rounding errors in number/range
input types
https://bugs.webkit.org/show_bug.cgi?id=80009

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

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


Don't you apply Decimal to NumberInputType?

> Source/WebCore/ChangeLog:62
> +	   * html/HTMLInputElement.cpp:
> +	   (WebCore::HTMLInputElement::getAllowedValueStep):
> +	   (WebCore):
> +	   * html/HTMLInputElement.h:
> +	   (WebCore):
> +	   (HTMLInputElement):
> +	   * html/InputType.cpp:
> +	   (WebCore::InputType::getAllowedValueStep):
> +	   (WebCore):
> +	   * html/InputType.h:
> +	   (WebCore):
> +	   (InputType):
> +	   * html/RangeInputType.cpp:
> +	   (WebCore::RangeInputType::getAllowedValueStep):
> +	   (WebCore):
> +	   (WebCore::RangeInputType::handleKeydownEvent):
> +	   (WebCore::RangeInputType::fallbackValue):
> +	   (WebCore::RangeInputType::maximum):
> +	   (WebCore::RangeInputType::minimum):
> +	   (WebCore::RangeInputType::sanitizeValue):
> +	   * html/RangeInputType.h:
> +	   (RangeInputType):
> +	   * html/StepRange.cpp:
> +	   (WebCore::StepRange::StepRange):
> +	   (WebCore::StepRange::clampValue):
> +	   (WebCore::StepRange::valueFromElement):
> +	   * html/StepRange.h:
> +	   (StepRange):
> +	   (WebCore::StepRange::defaultValue):
> +	   (WebCore::StepRange::proportionFromValue):
> +	   (WebCore::StepRange::valueFromProportion):
> +	   * html/shadow/SliderThumbElement.cpp:
> +	   (WebCore::sliderPosition):
> +	   (WebCore::RenderSliderThumb::layout):
> +	   (WebCore::SliderThumbElement::setPositionFromPoint):

Please write what you changed / why you changed, especially for existing
functions/files.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:20206
> +				458A2810154AB5FB00004037 /* Decimal.h */,
> +				458A280E154AB5E600004037 /* Decimal.cpp */,
>				49E912A40EFAC8E6009D0CAF /* animation */,
>				FD31604012B026A300C1A359 /* audio */,

I recommend inserting new files to sorted positions in order to avoid patch
conflict.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:27862
>				78D02BC5154A18DF00B62D05 /*
CSSPropertyAnimation.cpp in Sources */,
> +				458A280F154AB5E600004037 /* Decimal.cpp in
Sources */,

ditto.

> Source/WebCore/html/RangeInputType.cpp:114
>  double RangeInputType::minimum() const
>  {
> -    return parseToDouble(element()->fastGetAttribute(minAttr),
rangeDefaultMinimum);
> +    return parseToDouble(StepRange(element()).minimum.toString(),
numeric_limits<double>::quiet_NaN());
>  }
>  
>  double RangeInputType::maximum() const

Do we need these functions?

> Source/WebCore/html/RangeInputType.cpp:202
> +    const Decimal bigStep = ::std::max((stepRange.maximum -
stepRange.minimum) / 10, step);

::std::max() should be max().

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:76
> +    if (!parseToDoubleForNumberType(string, &doubleResult))
> +	   return false;
> +    *result = Decimal::fromString(string);

Parsing twice is not reasonable.

> Source/WebCore/platform/Decimal.cpp:353
> +Decimal::Decimal(uint64_t coefficient, int exponent, int sign)
> +    : m_data(coefficient, exponent, sign)

Passing sign as int is unreasonable.  It should be an enum.

> Source/WebCore/platform/Decimal.h:120
> +    static Decimal quietNaN(int);

Do we need to support negative NaN?

> Source/WebKit/chromium/tests/DecimalTest.cpp:113
> +TEST_F(DecimalTest, AppNumberStepUpStepDownFromRenderer)

What's 'App'?

> Source/WebKit/chromium/tests/DecimalTest.cpp:195
> +}

Please add more fromString tests.  e.g.
fromString(".5")
fromString(" 123 ")
fromString("1,234")
fromString("+3")
fromString("INF")
...

> Source/WebKit/chromium/tests/DecimalTest.cpp:216
> +    const Decimal inf(Decimal::infinity(0));
> +    const Decimal minf(Decimal::infinity(1));
> +    const Decimal nan(Decimal::quietNaN(0));
> +    const Decimal mnan(Decimal::quietNaN(1));
> +    const Decimal ten(10);

They should be:
Infinity
MinusInfinity / NegativeInfinity
NaN
MinusNaN / NegativeNaN
Ten

> Source/WebKit/chromium/tests/DecimalTest.cpp:255
> +}

We should add tests for values with larger/smaller exponent.

> Source/WebKit/chromium/tests/DecimalTest.cpp:329
> +}

We should add tests for values with larger/smaller exponent.