Bug 82034 – [Forms] Move numeric related methods in HTMLInputElement class to another place

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

For ease of maintenance and reduce dependency, we should move following numeric value related methods to another place for number, range, date, and local datetime  input type:
 * rangeUnderflow
 * rangeOverflow
 * minimum
 * maximum
 * getAllowedValueStep
 * stepMismatch
 * minimumString
 * maximumString
 * stepBBaseString
 * stepString
 * stepUp - should call m_inputType->stepUp
 * stepDown - should call m_inputType->stepUp
 * isSteppable - should be removed
 
This list is based on the HTML5 specification[1] having max/min/step content attributes and valuesAsDate/valueAsNumber/stepDown/stepUp methods, may not be exhaustive and contain extra methods.
 
[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element.html#the-input-element

*** Bug 85959 has been marked as a duplicate of this bug. ***

Following methods are used in renderer and/or validator. So, we keep them.
 * rangeUnderflow
 * rangeOverflow
 * minimum
 * maximum
 * getAllowedValueStep
 * stepMismatch
 * minimumString
 * maximumString
 * stepBBaseString
 * stepString

Created attachment 141855 [details]
Patch 1

Comment on attachment 141855 [details]
Patch 1

Build with CR-Linux and CR-Mac.
Could you review my patch?
Thanks in advance.

P.S. decimal places related code will be removed when we introduce decimal arithmetic.

[Build Bot]

Comment on attachment 141855 [details]
Patch 1

Attachment 141855 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12689619

[WebKit Review Bot]

Comment on attachment 141855 [details]
Patch 1

Attachment 141855 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12685657

New failing tests:
fast/css/pseudo-in-range-invalid-value.html

[WebKit Review Bot]

Created attachment 141861 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 141871 [details]
Patch 2 - Nothing changed

Comment on attachment 141871 [details]
Patch 2 - Nothing changed

Noting changed since Patch1.
I'm not sure why Mac-EWS failed.
Anyway, could you review this patch?
Thanks in advance.

[Build Bot]

Comment on attachment 141871 [details]
Patch 2 - Nothing changed

Attachment 141871 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12703373

[Kent Tamura]

Comment on attachment 141871 [details]
Patch 2 - Nothing changed

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?

Created attachment 141892 [details]
Patch 3

Comment on attachment 141892 [details]
Patch 3

* Fix Mac build
* Changes by review comments: use createStepRange and StepRange ctor instead of setupStepRange and step method.
* CR-Linux failure tests aren't related to this patch.

Please review again.
Thanks in advance.

[Kent Tamura]

Comment on attachment 141892 [details]
Patch 3

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

Please make sure this patch is compilable with enabling all of ENABLE_INPUT_TYPE_*.

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

should be String.

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

ditto.

> Source/WebCore/html/HTMLInputElement.h:69
> +    StepRange createStepRange(AnyStepHandling) const;

Do we need this?

> Source/WebCore/html/InputType.cpp:258
> +    StepRange stepRange(createStepRange(RejectAny));
> +    return doubleValue < stepRange.minimum();

You don't need the "stepRange" object.
  return doubleValue < createStepRange(RejectAny).minimum();
is enough.

> Source/WebCore/html/InputType.cpp:271
> +    StepRange stepRange(createStepRange(RejectAny));
> +    return doubleValue > stepRange.maximum();

ditto.

> Source/WebCore/html/InputType.cpp:282
> +    StepRange stepRange(createStepRange(RejectAny));
> +    return stepRange.minimum();

ditto.

> Source/WebCore/html/InputType.cpp:288
> +    StepRange stepRange(createStepRange(RejectAny));
> +    return stepRange.maximum();

ditto.

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

should be String.

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

should be String.

> Source/WebCore/html/NumberInputType.h:51
> +    virtual StepRange createStepRange(AnyStepHandling) const;

should add OVERRIDE

> Source/WebCore/html/RangeInputType.cpp:273
> +    StepRange stepRange(createStepRange(RejectAny));
> +    return serializeForNumberType(stepRange.defaultValue());

return serializeForNumberType(createStepRange(RejectAny).defaultValue());
is enough.

> Source/WebCore/html/RangeInputType.h:51
> +    virtual StepRange createStepRange(AnyStepHandling) const;

should add OVERRIDE

> Source/WebCore/html/StepRange.cpp:35
> +StepRange::StepRange()

Is this used?

> Source/WebCore/html/StepRange.cpp:54
> +StepRange::StepRange(const StepRange& stepRange)
>  {
> -    double clampedValue = max(minimum, min(value, maximum));
> -    if (!hasStep)
> -        return clampedValue;
> -    // Rounds clampedValue to minimum + N * step.
> -    clampedValue = minimum + round((clampedValue - minimum) / step) * step;
> -    if (clampedValue > maximum)
> -       clampedValue -= step;
> -    ASSERT(clampedValue >= minimum);
> -    ASSERT(clampedValue <= maximum);
> -    return clampedValue;
> +    m_hasStep = stepRange.m_hasStep;
> +    m_maximum = stepRange.m_maximum;
> +    m_minimum = stepRange.m_minimum;
> +    m_step = stepRange.m_step;
> +    m_stepBase = stepRange.m_stepBase;
> +    m_stepBaseDecimalPlaces = stepRange.m_stepBaseDecimalPlaces;
> +    m_stepDecimalPlaces = stepRange.m_stepDecimalPlaces;

we should use member initializers, and we can make some members const.

> Source/WebCore/html/StepRange.cpp:66
> +StepRange::StepRange(AnyStepHandling anyStepHandling, double stepBase, double minimum, double maximum, const String& stepString, const StepDescription& stepDescription, unsigned stepBaseDecimalPlaces)
> +{
> +    ASSERT(isfinite(maximum));
> +    ASSERT(isfinite(minimum));
> +    ASSERT(isfinite(stepBase));
> +    m_maximum = maximum;
> +    m_minimum = minimum;
> +    m_stepBase = stepBase;
> +    m_stepBaseDecimalPlaces = stepBaseDecimalPlaces;
> +    m_stepDescription = stepDescription;

ditto.

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

should be String.

> Source/WebCore/html/TimeInputType.h:49
> +    virtual StepRange createStepRange(AnyStepHandling) const;

should add OVERRIDE.

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

should be String.

> Source/WebCore/html/WeekInputType.h:48
> +    virtual StepRange createStepRange(AnyStepHandling) const;

should add OVERRIDE

(In reply to comment #14)
> (From update of attachment 141892 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141892&action=review
> 
> > Source/WebCore/html/HTMLInputElement.h:69
> > +    StepRange createStepRange(AnyStepHandling) const;
> 
> Do we need this?

Static inline function sliderPosition in SliderThumbElement.cpp needs this.

> > Source/WebCore/html/StepRange.cpp:35
> > +StepRange::StepRange()
> 
> Is this used?

Default method of createStepRange in InputType uses this.

Created attachment 141906 [details]
Patch 4

Comment on attachment 141906 [details]
Patch 4

Please review again.
Thanks in advance.

I did:
* Build with ENABLE_INPUT_TYPE_*
* Build with Chromium-Mac
* Changes as review comments
** Use StepRange ctor instead of setup method.
** Fix const String& issue for fastGetAttribute
* HTMLInputElement::createStepRange is used by sliderPosition in SliderThumbElement.cpp
* Default ctor of StepRange is used by default method of InputType::createStepRange

Test failures of CR-Linux build aren't related to this patch.

[WebKit Review Bot]

Comment on attachment 141906 [details]
Patch 4

Attachment 141906 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12706310

New failing tests:
media/media-document-audio-repaint.html
media/audio-repaint.html
media/controls-drag-timebar.html

[WebKit Review Bot]

Created attachment 141969 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Kent Tamura]

(In reply to comment #18)
> Test failures of CR-Linux build aren't related to this patch.

How did you confirm it?

Comment on attachment 141906 [details]
Patch 4

I'm changing StepRange now...

Created attachment 142143 [details]
Patch 5

(In reply to comment #21)
> (In reply to comment #18)
> > Test failures of CR-Linux build aren't related to this patch.
> 
> How did you confirm it?

I examined HTML files of failed tests and check they contains "input" element or not.

[Kent Tamura]

(In reply to comment #24)
> (In reply to comment #21)
> > (In reply to comment #18)
> > > Test failures of CR-Linux build aren't related to this patch.
> > 
> > How did you confirm it?
> 
> I examined HTML files of failed tests and check they contains "input" element or not.

<audio> and <video> contain some <input type=range> in shadow trees.

Failure of media test was caused by wrong handling of "precision" attribute with "float" value. Patch 4 marked it had step value instead of no step value.

Comment on attachment 142143 [details]
Patch 5

* Introducing DoubleWithDecimalPlaces and DoubleWithDecimalPlacesOrMissing for making StepRange immutable.
* Local build with ENABLE_INPUT_TYPE_*
* Build with clang on CR-Mac
* Failure of CR-Linux on media tests was fixed.

Please review again.
Thanks in advance.

Comment on attachment 142143 [details]
Patch 5

This patch succeeded on CR-Linux EWS. Failed tests aren't related to this patch.

[Kent Tamura]

Comment on attachment 142143 [details]
Patch 5

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?

Created attachment 142683 [details]
Patch 6

[Early Warning System Bot]

Comment on attachment 142683 [details]
Patch 6

Attachment 142683 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12727329

[Gyuyoung Kim]

Comment on attachment 142683 [details]
Patch 6

Attachment 142683 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12723628

[WebKit Review Bot]

Comment on attachment 142683 [details]
Patch 6

Attachment 142683 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12715864

[Build Bot]

Comment on attachment 142683 [details]
Patch 6

Attachment 142683 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12718841

[Early Warning System Bot]

Comment on attachment 142683 [details]
Patch 6

Attachment 142683 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12725476

[Gustavo Noronha (kov)]

Comment on attachment 142683 [details]
Patch 6

Attachment 142683 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12719827

[Kent Tamura]

Comment on attachment 142683 [details]
Patch 6

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

> Source/WebCore/html/HTMLInputElement.h:161
>  
> -    void setEditingValue(const String&);
> -
>      double valueAsDate() const;

Do not edit unrelated part.

> Source/WebCore/html/HTMLInputElement.h:294
>  
> -    virtual void copyNonAttributePropertiesFromElement(const Element&);
> +    virtual void copyNonAttributeProperties(const Element* source);
>  

ditto.

[Build Bot]

Comment on attachment 142683 [details]
Patch 6

Attachment 142683 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12716881

[Build Bot]

Comment on attachment 142683 [details]
Patch 6

Attachment 142683 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12726446

Created attachment 142930 [details]
Patch 7

Comment on attachment 142930 [details]
Patch 7

* Make StepRange immutable
* CR-Linux test failures aren't related to this patch
** fast/text/international/thai-line-breaks.html
** platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html

Thanks in advance.
-yosi

[Kent Tamura]

Comment on attachment 142930 [details]
Patch 7

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.

Created attachment 142942 [details]
Patch 8

Comment on attachment 142942 [details]
Patch 8

* Build with ENABLE_INPUT_TYPE_*=1
* Build with CR-Linux w/ clang
* Introduce StepValueShouldBe enum to combine two Boolean values. We need three value rather than four value.
** There is no combination parsed and scaled should be integer.

Could you review again?
Thanks in advance.

[WebKit Review Bot]

Comment on attachment 142942 [details]
Patch 8

Clearing flags on attachment: 142942

Committed r117738: <http://trac.webkit.org/changeset/117738>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.