[webkit-reviews] review denied: [Bug 88275] [Forms] Introduce InputNumber type as an alias of double for replacing it to Decimal : [Attachment 145711] Patch 1

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied yosin at chromium.org's request for
review:
Bug 88275: [Forms] Introduce InputNumber type as an alias of double for
replacing it to Decimal
https://bugs.webkit.org/show_bug.cgi?id=88275

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>
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(valu
e));

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