[webkit-reviews] review granted: [Bug 82034] [Forms] Move numeric related methods in HTMLInputElement class to another place : [Attachment 142930] Patch 7

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has granted yosin at chromium.org's request for
review:
Bug 82034: [Forms] Move numeric related methods in HTMLInputElement class to
another place
https://bugs.webkit.org/show_bug.cgi?id=82034

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=142930&action=review


> Source/WebCore/html/DateInputType.cpp:74
> +    DEFINE_STATIC_LOCAL(const StepRange::StepDescription, stepDescription,
(dateDefaultStep, dateDefaultStepBase, true, false, dateStepScaleFactor));

"ture, false' is not good for code readability.  How about the followings?
    const bool ParsedStepValueShouldBeInteger = true;
    const bool ScaledStepValueShouldBeInteger = true;
    DEFINE_STATIC_LOCAL(... , ParseStepValueShoudlBeInteger,
!ScaledStepValeuShouldBeInteger, ...)

We usually use enum in such cases.  But I feel enum is overkill in this case
because the number of callers of the StepDescription constructors is limited.

> Source/WebCore/html/DateTimeInputType.cpp:72
> +    DEFINE_STATIC_LOCAL(const StepRange::StepDescription, stepDescription,
(dateTimeDefaultStep, dateTimeDefaultStepBase, false, true,
dateTimeStepScaleFactor));

ditto.

> Source/WebCore/html/DateTimeLocalInputType.cpp:78
> +    DEFINE_STATIC_LOCAL(const StepRange::StepDescription, stepDescription,
(dateTimeLocalDefaultStep, dateTimeLocalDefaultStepBase, false, true,
dateTimeLocalStepScaleFactor));

ditto.

> Source/WebCore/html/MonthInputType.cpp:102
> +    DEFINE_STATIC_LOCAL(const StepRange::StepDescription, stepDescription,
(monthDefaultStep, monthDefaultStepBase, true, false, monthStepScaleFactor));

ditto.

> Source/WebCore/html/NumberInputType.cpp:113
> +    DEFINE_STATIC_LOCAL(const StepRange::StepDescription, stepDescription,
(numberDefaultStep, numberDefaultStepBase, false, false,
numberStepScaleFactor));

ditto.

> Source/WebCore/html/RangeInputType.cpp:95
> +    DEFINE_STATIC_LOCAL(const StepRange::StepDescription, stepDescription,
(rangeDefaultStep, rangeDefaultStepBase, false, false, rangeStepScaleFactor));

ditto.

> Source/WebCore/html/TimeInputType.cpp:84
> +    DEFINE_STATIC_LOCAL(const StepRange::StepDescription, stepDescription,
(timeDefaultStep, timeDefaultStepBase, false, false, timeStepScaleFactor));

ditto.

> Source/WebCore/html/WeekInputType.cpp:66
> +    DEFINE_STATIC_LOCAL(const StepRange::StepDescription, stepDescription,
(weekDefaultStep, weekDefaultStepBase, false, true, weekStepScaleFactor));

ditto.