Bug 88383 - [Forms] Introduce Decimal behind the InputNumber type
: [Forms] Introduce Decimal behind the InputNumber type
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Forms
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: yosin
:
Depends on: 88394 88480 88489 88621
Blocks: 80009
  Show dependency treegraph
 
Reported: 2012-06-05 18:18 PDT by yosin
Modified: 2012-06-10 19:31 PDT (History)
2 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description yosin 2012-06-05 18:18:27 PDT
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 yosin 2012-06-05 21:25:06 PDT
Created attachment 145929 [details]
Patch 1
Comment 2 yosin 2012-06-05 21:52:15 PDT
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.
Comment 3 Kent Tamura 2012-06-05 23:16:40 PDT
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?
Comment 4 Kent Tamura 2012-06-05 23:17:45 PDT
(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 yosin 2012-06-05 23:35:43 PDT
(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 yosin 2012-06-06 00:03:07 PDT
(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 yosin 2012-06-07 02:54:20 PDT
Created attachment 146240 [details]
Patch 2
Comment 8 yosin 2012-06-07 03:03:48 PDT
Created attachment 146241 [details]
Patch 3
Comment 9 yosin 2012-06-07 03:18:07 PDT
Created attachment 146242 [details]
Patch 4
Comment 10 yosin 2012-06-07 03:33:17 PDT
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.
Comment 11 Kent Tamura 2012-06-07 21:33:56 PDT
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
Comment 12 yosin 2012-06-08 01:57:27 PDT
Created attachment 146508 [details]
Patch 5
Comment 13 yosin 2012-06-08 02:37:24 PDT
Created attachment 146522 [details]
Patch 6 - Update ChnageLog
Comment 14 yosin 2012-06-08 02:42:36 PDT
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
Comment 15 yosin 2012-06-08 02:44:41 PDT
(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.
Comment 16 Kent Tamura 2012-06-08 03:06:16 PDT
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?
Comment 17 yosin 2012-06-08 05:17:26 PDT
Created attachment 146542 [details]
Patch 7
Comment 18 yosin 2012-06-08 05:35:54 PDT
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
Comment 19 Kent Tamura 2012-06-10 18:03:56 PDT
Comment on attachment 146542 [details]
Patch 7

ok
Comment 20 WebKit Review Bot 2012-06-10 19:30:58 PDT
Comment on attachment 146542 [details]
Patch 7

Clearing flags on attachment: 146542

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