[webkit-reviews] review denied: [Bug 88383] [Forms] Introduce Decimal behind the InputNumber type : [Attachment 145929] Patch 1

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied yosin at chromium.org's request for
review:
Bug 88383: [Forms] Introduce Decimal behind the InputNumber type
https://bugs.webkit.org/show_bug.cgi?id=88383

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

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


> Source/WebCore/html/MonthInputType.cpp:97
> -    return months;
> +    return convertDoubleToInputNumber(months);

nit: we had better add this function before this patch.

> Source/WebCore/html/MonthInputType.cpp:118
> -    return months;
> +    return convertDoubleToInputNumber(months);

nit: we had better add this function before this patch.

> Source/WebCore/html/NumberInputType.cpp:96
> -    const double floatMax =
convertDoubleToInputNumber(numeric_limits<float>::max());
> +    const double floatMax = numeric_limits<float>::max();
>      if (newValue < -floatMax) {

nit: we had better add this function before this patch.

> Source/WebCore/html/NumberInputType.cpp:161
>  
> -    unsigned minValueDecimalPlaces;
> -    String minValue = element()->fastGetAttribute(minAttr);
> -    double minValueDouble =
parseToDoubleForNumberTypeWithDecimalPlaces(minValue, &minValueDecimalPlaces);
> -    if (!isfinite(minValueDouble))
> -	   return false;
> -
> -    unsigned maxValueDecimalPlaces;
> -    String maxValue = element()->fastGetAttribute(maxAttr);
> -    double maxValueDouble =
parseToDoubleForNumberTypeWithDecimalPlaces(maxValue, &maxValueDecimalPlaces);
> -    if (!isfinite(maxValueDouble))
> +    String stepString = element()->fastGetAttribute(stepAttr);
> +    if (equalIgnoringCase(stepString, "any"))
>	   return false;
>  
> -    if (maxValueDouble < minValueDouble) {
> -	   maxValueDouble = minValueDouble;
> -	   maxValueDecimalPlaces = minValueDecimalPlaces;
> +    const Decimal minimum =
parseToDecimalForNumberType(element()->fastGetAttribute(minAttr));
> +    const Decimal maximum =
parseToDecimalForNumberType(element()->fastGetAttribute(maxAttr));
> +    const Decimal step = parseToDecimalForNumberType(stepString, 1);
> +    if (minimum.isFinite() && maximum.isFinite() && step.isFinite()) {
> +	   preferredSize = max(max(calculateRenderSize(minimum),
calculateRenderSize(maximum)), calculateRenderSize(step));
> +	   return true;
>      }
>  
> -    String stepValue = element()->fastGetAttribute(stepAttr);
> -    if (equalIgnoringCase(stepValue, "any"))
> -	   return false;
> -    unsigned stepValueDecimalPlaces;
> -    double stepValueDouble =
parseToDoubleForNumberTypeWithDecimalPlaces(stepValue,
&stepValueDecimalPlaces);
> -    if (!isfinite(stepValueDouble)) {
> -	   stepValueDouble = 1;
> -	   stepValueDecimalPlaces = 0;
> -    }
> -
> -    unsigned length = lengthBeforeDecimalPoint(minValueDouble);
> -    length = max(length, lengthBeforeDecimalPoint(maxValueDouble));
> -    length = max(length, lengthBeforeDecimalPoint(stepValueDouble));
> -
> -    unsigned lengthAfterDecimalPoint = minValueDecimalPlaces;
> -    lengthAfterDecimalPoint = max(lengthAfterDecimalPoint,
maxValueDecimalPlaces);
> -    lengthAfterDecimalPoint = max(lengthAfterDecimalPoint,
stepValueDecimalPlaces);
> -
> -    // '.' should be counted if the value has decimal places.
> -    if (lengthAfterDecimalPoint > 0)
> -	   length += lengthAfterDecimalPoint + 1;
> -
> -    preferredSize = length;
> -    return true;
> +    return false;

I think this causes a behavior change.
Would you add a test case for the following to
fast/forms/number/input-number-size.html please?
    shouldBe('numberWidth(123456, 123456, 0.0000005)',
'textWidthPlusSpinButtonWidth(14)');

> Source/WebCore/html/StepRange.cpp:169
> +    if (!isfinite(doubleValue))
> +	   return Decimal::nan();
> +
> +    NumberToStringBuffer buffer;
> +    return Decimal::fromString(numberToString(doubleValue, buffer));

Decimal class should have this implementation.
Do we need to handle infinity? If not, we should have a comment and an
assertion.

> Source/WebCore/html/StepRange.cpp:179
> +    if (!decimalValue.isFinite())
> +	   return std::numeric_limits<double>::quiet_NaN();
> +
> +    bool valid;
> +    const double doubleValue = decimalValue.toString().toDouble(&valid);
> +    return valid ? doubleValue : std::numeric_limits<double>::quiet_NaN();

ditto.

> Source/WebCore/html/StepRange.h:38
> +// FIXME: We should rename InputNumber to Decimal in all places.
> +typedef Decimal InputNumber;
> +InputNumber convertDoubleToInputNumber(double);
> +double convertInputNumberToDouble(const InputNumber&);
>  

Please introduce isnan(const Decimal&) and isfinite(const Decimal&) to reduce
the patch size.

> Source/WebCore/html/StepRange.h:129
> +    InputNumber roundByStep(const InputNumber& value, const InputNumber&
base) const
> +    {
> +	   return base + ((value - base) / m_step).round() * m_step;
> +    }
> +

nit: we had better add this function before this patch.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:94
> +    const uint64_t leadingDigitsOf2Power1024 = UINT64_C(17976931348623159);
> +    DEFINE_STATIC_LOCAL(const Decimal, htmlMax, (Decimal::Positive, 292,
leadingDigitsOf2Power1024));
> +    DEFINE_STATIC_LOCAL(const Decimal, htmlMin, (Decimal::Negative, 292,
leadingDigitsOf2Power1024));

Could you explain why 17976931348623159E+292 ? Is it compatible with
parseToDoubelForNumberType() below?