[webkit-reviews] review denied: [Bug 82034] [Forms] Move numeric related methods in HTMLInputElement class to another place : [Attachment 141871] Patch 2 - Nothing changed

[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 141871: Patch 2 - Nothing changed
https://bugs.webkit.org/attachment.cgi?id=141871&action=review

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


> Source/WebCore/html/DateTimeInputType.cpp:79
> +    const String& stepString = element()->fastGetAttribute(stepAttr);

Holding a reference of a temporary object created by conversion from const
AtomicString& looks dangerous. This should be just "String."

> Source/WebCore/html/InputType.h:164
> +    virtual void setupStepRange(AnyStepHandling, StepRange*) const;

I think we had better make this function a factory.
virtual StepRange createStepRange() const;

> Source/WebCore/html/StepRange.cpp:119
> +double StepRange::step() const
> +{
> +    return m_step;
> +}
> +
> +double StepRange::stepBase() const
> +{
> +    return m_stepBase;
> +}
> +
> +unsigned StepRange::stepBaseDecimalPlaces() const
> +{
> +    return m_stepBaseDecimalPlaces;
>  }
>  
> +unsigned StepRange::stepDecimalPlaces() const
> +{
> +    return m_stepDecimalPlaces;
> +}
> +
> +int StepRange::stepScaleFactor() const
> +{
> +    return m_stepDescription.stepScaleFactor;
> +}

Such simple accessor functions should be defined in StepRange.h in order to
make them inline.

> Source/WebCore/html/StepRange.cpp:121
> +void StepRange::setHasStep(bool hasStep)

Can we merge this to the constructor?

> Source/WebCore/html/StepRange.cpp:174
> +void StepRange::setup(AnyStepHandling anyStepHandling, double stepBase,
double minimum, double maximum, const String& stepString, const
StepDescription& stepDescription, unsigned stepBaseDecimalPlaces)
> +{

Can we merge this to the constructor?