Bug 88383 – [Forms] Introduce Decimal behind the InputNumber type

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

This is a part of bug 80009, introducing decimal arithmetic.

To reduce size of patch for easy reviewing and roll out, we introduced InputNumber(bug 88275) type as alias of "double" type, although size of patch was 68KB, without behavior change.

In this bug, we'll change InputNumber as Decimal class and fix rounding issue.

Created attachment 145929 [details]
Patch 1

Comment on attachment 145929 [details]
Patch 1

Could you review this patch?
Thanks in advance.

* Local build with enabling all input type.
* Test files increase size of this patch. They are 22KB.

[Kent Tamura]

Comment on attachment 145929 [details]
Patch 1

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?

[Kent Tamura]

(In reply to comment #3)
> > 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.

correction: nit: we had better change it before this patch.

(In reply to comment #3)
> > 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.

Before using Decimal, one has scale and another is above. So, we can share code after Decimal.

(In reply to comment #5)
> (In reply to comment #3)
> > > 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.
> 
> Before using Decimal, one has scale and another is above. So, we can share code after Decimal.

It seems passing "scale" as parameter can reduce size of patch. I'll try it.

Created attachment 146240 [details]
Patch 2

Created attachment 146241 [details]
Patch 3

Created attachment 146242 [details]
Patch 4

Comment on attachment 146242 [details]
Patch 4

Could you review this patch?
Thanks in advance.

* Changes are
** NumericInputType::sizeShouldIncludeDecoration
* convertDecimalToDouble
* convertDoubleToDecimal
* Local build with enabling all INPUT_TYPE
* GTK build failure is caused by previous build.

[Kent Tamura]

Comment on attachment 146242 [details]
Patch 4

View in context: https://bugs.webkit.org/attachment.cgi?id=146242&action=review

Almost ok

> Source/WebCore/html/NumberInputType.cpp:215
> +    return value.isFinite() ? serializeForNumberType(value) : emptyString();

emptyString() should be String().  Do not change the function behavior though it might have no side-effect.

> Source/WebCore/html/RangeInputType.cpp:223
> +    return value.isFinite() ? serializeForNumberType(value) : emptyString();

ditto.

> Source/WebCore/html/StepRange.h:49
> +    struct DecimalOrMissing {

Can you remove DecimalOrMissing by representing the missing state with Decimal::nan()?

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:65
> +        // Decimal::toString append exponent, e.g. "0e-18"

append -> appends

Created attachment 146508 [details]
Patch 5

Created attachment 146522 [details]
Patch 6 - Update ChnageLog

Comment on attachment 146522 [details]
Patch 6 - Update ChnageLog

Could you review this patch?
Thanks in advance.

* Patch 6 changes emptyString() => String() and NumberWithDecimalPlaces => InputNumber
* Patch 6 changes ChangeLog only from Patch 5.
* Local build on CR-Linux with enabling all input type

(In reply to comment #14)
> (From update of attachment 146522 [details])
> Could you review this patch?
> Thanks in advance.
> 
> * Patch 6 changes emptyString() => String() and NumberWithDecimalPlaces => InputNumber
> * Patch 6 changes ChangeLog only from Patch 5.
> * Local build on CR-Linux with enabling all input type

Patch 6 also changes comment in Source/WebCore/html/parser/HTMLParserIdioms.cpp as pointed.

[Kent Tamura]

Comment on attachment 146522 [details]
Patch 6 - Update ChnageLog

View in context: https://bugs.webkit.org/attachment.cgi?id=146522&action=review

> Source/WebCore/ChangeLog:73
> +        (WebCore::StepRange::roundByStep): Added for sharing code.

Looks false.

> Source/WebCore/html/NumberInputType.cpp:215
> -    if (!isfinite(value))
> -        return String();
> -    return serializeForNumberType(value);
> +    return value.isFinite() ? serializeForNumberType(value) : String();

You could reduce the patch size if you didn't change 'if' to a ternary operator.

> Source/WebCore/html/RangeInputType.cpp:223
> -    if (!isfinite(value))
> -        return String();
> -    return serializeForNumberType(value);
> +    return value.isFinite() ? serializeForNumberType(value) : String();

You could reduce the patch size if you didn't change 'if' to a ternary operator.

> Source/WebCore/html/StepRange.cpp:27
> +#include <wtf/dtoa.h>

Is it needed?

> Source/WebCore/html/StepRange.cpp:-30
> -using namespace std;
> -

Is this change needed now?

Created attachment 146542 [details]
Patch 7

Comment on attachment 146542 [details]
Patch 7

Could you review this patch?
Thanks in advance.

* Changes according to review comments
* Local build on CR-Linux with enable all input types
* Patch size is decreased 1KB then 67KB

[Kent Tamura]

Comment on attachment 146542 [details]
Patch 7

ok

[WebKit Review Bot]

Comment on attachment 146542 [details]
Patch 7

Clearing flags on attachment: 146542

Committed r119948: <http://trac.webkit.org/changeset/119948>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.