[webkit-reviews] review denied: [Bug 82034] [Forms] Move numeric related methods in HTMLInputElement class to another place : [Attachment 142143] Patch 5

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied 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 142143: Patch 5
https://bugs.webkit.org/attachment.cgi?id=142143&action=review

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


> Source/WebCore/html/DateInputType.cpp:76
> +    StepRange::StepDescription stepDescription;
> +    stepDescription.defaultStep = dateDefaultStep;
> +    stepDescription.parsedStepValueShouldBeInteger = true;
> +    stepDescription.stepScaleFactor = dateStepScaleFactor;

StepDescription is constant.  We don't need to create it every time when
createStepRange() is called. We had better make a static object like the
following, and pass it to the StepRange constructor. StepRange should hold a
reference/pointer to a static StepDescription object.

  DEFINE_STATIC_LOCAL(const StepRange::StepDescription, stepDescription,
(dataDefaultStep, 0, true, false, dateStepScaleFactor));

> Source/WebCore/html/StepRange.cpp:43
> +StepRange::StepRange()
> +    : m_hasStep(false)
> +    , m_maximum(100)
> +    , m_minimum(0)
> +    , m_step(1)
> +    , m_stepBase(0)
> +    , m_stepBaseDecimalPlaces(0)
> +    , m_stepDecimalPlaces(0)
>  {

Should add ASSERT_NOT_REACHED()?

> Source/WebCore/html/StepRange.h:63
> +	   int defaultStep;
> +	   int defaultStepBase;
> +	   bool parsedStepValueShouldBeInteger;
> +	   bool scaledStepValueShouldBeInteger;
> +	   int stepScaleFactor;
> +

We had better reorder them like int,int,int,bool,bool in order to minimize the
object size.

> Source/WebCore/html/StepRange.h:127
> +    const bool m_hasStep;
> +    const double m_maximum; // maximum must be >= minimum.
> +    const double m_minimum;
> +    const double m_step;
> +    const double m_stepBase;
> +    const unsigned m_stepBaseDecimalPlaces;
> +    const unsigned m_stepDecimalPlaces;
> +    const StepDescription m_stepDescription;

ditto.

> Source/WebCore/html/WeekInputType.cpp:47
> -static const double weekDefaultStepBase = -259200000.0; // The first day of
1970-W01.
> -static const double weekDefaultStep = 1.0;
> -static const double weekStepScaleFactor = 604800000.0;
> +static const int weekDefaultStepBase = -259200000; // The first day of
1970-W01.
> +static const int weekDefaultStep = 1;
> +static const int weekStepScaleFactor = 604800000;

Why did you change them to int though you didn't change such values for other
types?