Bug 88383 - [Forms] Introduce Decimal behind the InputNumber type
: [Forms] Introduce Decimal behind the InputNumber type
Status: RESOLVED FIXED
: WebKit
Forms
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 88394 88480 88489 88621
: 80009
  Show dependency treegraph
 
Reported: 2012-06-05 18:18 PST by
Modified: 2012-06-10 19:31 PST (History)


Attachments
Patch 1 (71.40 KB, patch)
2012-06-05 21:25 PST, yosin@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Patch 2 (68.99 KB, patch)
2012-06-07 02:54 PST, yosin@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Patch 3 (68.86 KB, patch)
2012-06-07 03:03 PST, yosin@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Patch 4 (68.92 KB, patch)
2012-06-07 03:18 PST, yosin@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Patch 5 (68.38 KB, patch)
2012-06-08 01:57 PST, yosin@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Patch 6 - Update ChnageLog (68.31 KB, patch)
2012-06-08 02:37 PST, yosin@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff
Patch 7 (67.49 KB, patch)
2012-06-08 05:17 PST, yosin@chromium.org
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-06-05 18:18:27 PST
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.
------- Comment #1 From 2012-06-05 21:25:06 PST -------
Created an attachment (id=145929) [details]
Patch 1
------- Comment #2 From 2012-06-05 21:52:15 PST -------
(From update of attachment 145929 [details])
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.
------- Comment #3 From 2012-06-05 23:16:40 PST -------
(From update of attachment 145929 [details])
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?
------- Comment #4 From 2012-06-05 23:17:45 PST -------
(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.
------- Comment #5 From 2012-06-05 23:35:43 PST -------
(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.
------- Comment #6 From 2012-06-06 00:03:07 PST -------
(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.
------- Comment #7 From 2012-06-07 02:54:20 PST -------
Created an attachment (id=146240) [details]
Patch 2
------- Comment #8 From 2012-06-07 03:03:48 PST -------
Created an attachment (id=146241) [details]
Patch 3
------- Comment #9 From 2012-06-07 03:18:07 PST -------
Created an attachment (id=146242) [details]
Patch 4
------- Comment #10 From 2012-06-07 03:33:17 PST -------
(From update of attachment 146242 [details])
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.
------- Comment #11 From 2012-06-07 21:33:56 PST -------
(From update of attachment 146242 [details])
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
------- Comment #12 From 2012-06-08 01:57:27 PST -------
Created an attachment (id=146508) [details]
Patch 5
------- Comment #13 From 2012-06-08 02:37:24 PST -------
Created an attachment (id=146522) [details]
Patch 6 - Update ChnageLog
------- Comment #14 From 2012-06-08 02:42:36 PST -------
(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
------- Comment #15 From 2012-06-08 02:44:41 PST -------
(In reply to comment #14)
> (From update of attachment 146522 [details] [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.
------- Comment #16 From 2012-06-08 03:06:16 PST -------
(From update of attachment 146522 [details])
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?
------- Comment #17 From 2012-06-08 05:17:26 PST -------
Created an attachment (id=146542) [details]
Patch 7
------- Comment #18 From 2012-06-08 05:35:54 PST -------
(From update of attachment 146542 [details])
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
------- Comment #19 From 2012-06-10 18:03:56 PST -------
(From update of attachment 146542 [details])
ok
------- Comment #20 From 2012-06-10 19:30:58 PST -------
(From update of attachment 146542 [details])
Clearing flags on attachment: 146542

Committed r119948: <http://trac.webkit.org/changeset/119948>
------- Comment #21 From 2012-06-10 19:31:03 PST -------
All reviewed patches have been landed.  Closing bug.