Bug 30847 – step attribute support for date&time types

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

[Kent Tamura]

Add support for step attribute of INPUT element with type=date, time, datetime, datetime-local, month, and week.

[Kent Tamura]

Created attachment 48483 [details]
Proposed patch

[Darin Adler]

Comment on attachment 48483 [details]
Proposed patch

> +static const double monthDefaultMaximum = (INT_MAX - 1970) * 12.0 + 12 - 1;

This needs a comment like the one in the change log explaining why this is the right value to use. Also, the 12 there should just be 12 rather than 12.0.

> +        if (!isfinite(doubleValue))
> +            return false;
> +        doubleValue = fabs(doubleValue - stepBase());
> +        if (isinf(doubleValue))
> +            return false;
> +        ASSERT(round(doubleValue) == doubleValue);
> +        ASSERT(round(step) == step);
> +        return fmod(doubleValue, step);

There is double error checking here. I suggest you just do a single !isfinite check once just before the assertions. The math can be done on nan and infinity and they will stay nan and infinity.

> +        parsed = round(parsed);
> +        if (parsed < 1.0)
> +            parsed = 1.0;

I think it's easier to read code like this if you use max instead of an if statement. You have this code twice in a row and could do that in both case.

> +    if (!fmod(step, 60000)) {

This should be a named constant.

> +    if (!fmod(step, 1000)) {

This should be a named constant.

You keep making local constants named nan to make your code readable, and this makes me think we should define either WTF::nan or WebCore::nan so you won't have to keep doing that.

[Kent Tamura]

(In reply to comment #2)

Fixed debug('<br/>foo') in tests pointed out in another bug.

> (From update of attachment 48483 [details])
> > +static const double monthDefaultMaximum = (INT_MAX - 1970) * 12.0 + 12 - 1;
> 
> This needs a comment like the one in the change log explaining why this is the
> right value to use. Also, the 12 there should just be 12 rather than 12.0.

Added a comment. 12.0 is required because (INT_MAX-1970)*12 can't be represented by an integer constant.

> > +        if (!isfinite(doubleValue))
> > +            return false;
> > +        doubleValue = fabs(doubleValue - stepBase());
> > +        if (isinf(doubleValue))
> > +            return false;
> > +        ASSERT(round(doubleValue) == doubleValue);
> > +        ASSERT(round(step) == step);
> > +        return fmod(doubleValue, step);
> 
> There is double error checking here. I suggest you just do a single !isfinite
> check once just before the assertions. The math can be done on nan and infinity
> and they will stay nan and infinity.

Fixed.

> > +        parsed = round(parsed);
> > +        if (parsed < 1.0)
> > +            parsed = 1.0;
> 
> I think it's easier to read code like this if you use max instead of an if
> statement. You have this code twice in a row and could do that in both case.

ok, I changed two instances to max().

> > +    if (!fmod(step, 60000)) {
> This should be a named constant.
> > +    if (!fmod(step, 1000)) {
> This should be a named constant.

Done.

> You keep making local constants named nan to make your code readable, and this
> makes me think we should define either WTF::nan or WebCore::nan so you won't
> have to keep doing that.

I didn't address it in this patch.
I feel the name 'nan' is too short for a large namespace.  Maybe 'notANumber'?


Landed as r54647