Bug 88275 – [Forms] Introduce InputNumber type as an alias of double for replacing it to Decimal

• Component: Forms
• Status: RESOLVED FIXED

To reduce size of patch, we would like to introduce new type "InputNumber" type as an alias of double which will be replaced with "Decimal".

This bug is a part of bug 80009.

Created attachment 145711 [details]
Patch 1

Comment on attachment 145711 [details]
Patch 1

* Build locally with enabling all input types.
* EWS bots are too busy.

Could you review this patch?
Thanks in advance.

P.S.
It seems this patch is too big, however, replacing InputType to Decimal patch will be smaller. I expect to touch following files only:

1. Decimal.cpp - for convertDoubleToInputType, convertInputTypeToDouble
2. Decimal.h - for convertDoubleToInputType, convertInputTypeToDouble
3. InputType.cpp - for applyStep and stepFromRenderer
4. InputType.h - for removing parseNumberWithDecimalPlaces
2. NumberInputType.cpp - for removing parseNumberWithDecimalPlaces and changing sizeShouldIncludeDecoration.
3. StepRange.cpp - for alignValueForStep, parseStep and stepMismatch

[Kent Tamura]

Comment on attachment 145711 [details]
Patch 1

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

> Source/WebCore/html/BaseDateAndTimeInputType.cpp:60
> -    element()->setValue(serializeWithMilliseconds(value));
> +    element()->setValue(serializeWithMilliseconds(convertDoubleToInputNumber(value)));

serializeWithMilliseconds() should take a double argument because it has strong relationship with DateComponents, and the number type and the range type don't have it.

> Source/WebCore/html/BaseDateAndTimeInputType.cpp:115
> +        return numeric_limits<InputNumber>::quiet_NaN();

This should be DateComponents::invalidMilliseconds().

> Source/WebCore/html/BaseDateAndTimeInputType.cpp:140
> -String BaseDateAndTimeInputType::serialize(double value) const
> +String BaseDateAndTimeInputType::serialize(const InputNumber& value) const
>  {
>      if (!isfinite(value))
>          return String();

should add convertInputNumberToDouble() for setMillisecondToDateComponents() below.

> Source/WebCore/html/BaseDateAndTimeInputType.cpp:159
> -String BaseDateAndTimeInputType::serializeWithMilliseconds(double value) const
> +String BaseDateAndTimeInputType::serializeWithMilliseconds(const InputNumber& value) const

should revert.

> Source/WebCore/html/BaseDateAndTimeInputType.cpp:188
> -    return serializeWithMilliseconds(parsedValue);
> +    return serializeWithMilliseconds(convertDoubleToInputNumber(parsedValue));

ditto.

> Source/WebCore/html/BaseDateAndTimeInputType.h:51
> -    virtual bool setMillisecondToDateComponents(double, DateComponents*) const = 0;
> +    virtual bool setMillisecondToDateComponents(const InputNumber&, DateComponents*) const = 0;

Same comment as serializeWithMilliseconds().  setMillisecondToDateComponents() should take a double argument.

> Source/WebCore/html/DateInputType.cpp:93
> -bool DateInputType::setMillisecondToDateComponents(double value, DateComponents* date) const
> +bool DateInputType::setMillisecondToDateComponents(const InputNumber& value, DateComponents* date) const
>  {
>      ASSERT(date);
> -    return date->setMillisecondsSinceEpochForDate(value);
> +    return date->setMillisecondsSinceEpochForDate(convertInputNumberToDouble(value));

should revert

> Source/WebCore/html/DateInputType.h:53
> -    virtual bool setMillisecondToDateComponents(double, DateComponents*) const OVERRIDE;
> +    virtual bool setMillisecondToDateComponents(const InputNumber&, DateComponents*) const OVERRIDE;

should revert.

> Source/WebCore/html/DateTimeInputType.cpp:91
> -bool DateTimeInputType::setMillisecondToDateComponents(double value, DateComponents* date) const
> +bool DateTimeInputType::setMillisecondToDateComponents(const InputNumber& value, DateComponents* date) const
>  {
>      ASSERT(date);
> -    return date->setMillisecondsSinceEpochForDateTime(value);
> +    return date->setMillisecondsSinceEpochForDateTime(convertInputNumberToDouble(value));

should revert

> Source/WebCore/html/DateTimeInputType.h:51
> -    virtual bool setMillisecondToDateComponents(double, DateComponents*) const OVERRIDE;
> +    virtual bool setMillisecondToDateComponents(const InputNumber&, DateComponents*) const OVERRIDE;

should revert

> Source/WebCore/html/DateTimeLocalInputType.cpp:97
> -bool DateTimeLocalInputType::setMillisecondToDateComponents(double value, DateComponents* date) const
> +bool DateTimeLocalInputType::setMillisecondToDateComponents(const InputNumber& value, DateComponents* date) const
>  {
>      ASSERT(date);
> -    return date->setMillisecondsSinceEpochForDateTimeLocal(value);
> +    return date->setMillisecondsSinceEpochForDateTimeLocal(convertInputNumberToDouble(value));

should revert

> Source/WebCore/html/DateTimeLocalInputType.h:52
> -    virtual bool setMillisecondToDateComponents(double, DateComponents*) const OVERRIDE;
> +    virtual bool setMillisecondToDateComponents(const InputNumber&, DateComponents*) const OVERRIDE;

should revert

> Source/WebCore/html/InputType.cpp:924
>  
> -    element()->setValueAsNumber(newValue, ec, eventBehavior);
> +    element()->setValueAsInputNumber(newValue, ec, eventBehavior);
>  

We can call InputType::setValueAsInputNumber() directly, and can remove HTMLInputElement::setValueAsInputNumber().

> Source/WebCore/html/NumberInputType.cpp:102
> +    const InputNumber floatMax = convertDoubleToInputNumber(numeric_limits<float>::max());

Please add a FIXME comment that we need to change this doubleMax in the future.

> Source/WebCore/html/StepRange.h:33
> +// FIXME: type InputNumber will be replaced with Decimal.

A sentence should be started with a capital letter. type -> Type

Created attachment 145744 [details]
Patch 2

Comment on attachment 145744 [details]
Patch 2

* Build with all input types.
* Revert serializeWithMilliseconds and setMillisecondToDateComponents
** size of patch is decreased by 8K.
* Changed as review comments
* EWS bots are too busy...

Could you review again?
Thanks in advance.

[Kent Tamura]

Comment on attachment 145744 [details]
Patch 2

ok

[WebKit Review Bot]

Comment on attachment 145744 [details]
Patch 2

Clearing flags on attachment: 145744

Committed r119540: <http://trac.webkit.org/changeset/119540>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

It appears that this patch increased the binary size significantly:
http://build.chromium.org/f/chromium/perf/linux-release-64/sizes/report.html?history=150&rev=140809

Blame list: http://trac.webkit.org/log/?verbose=on&rev=119550&stop_rev=119536

[Ryosuke Niwa]

Actually, this could be due to the libjingle change: http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=/trunk/src&mode=html&range=140704:140708

[Kent Tamura]

(In reply to comment #9)
> It appears that this patch increased the binary size significantly:
> http://build.chromium.org/f/chromium/perf/linux-release-64/sizes/report.html?history=150&rev=140809
> 
> Blame list: http://trac.webkit.org/log/?verbose=on&rev=119550&stop_rev=119536

(In reply to comment #10)
> Actually, this could be due to the libjingle change: http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=/trunk/src&mode=html&range=140704:140708

I don't think the patch in this bug caused the significant size change.