Bug 30847 - step attribute support for date&time types
Summary: step attribute support for date&time types
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL: http://www.whatwg.org/specs/web-apps/...
Keywords:
Depends on: 27451 29004 29070 32696
Blocks: 29359
  Show dependency treegraph
 
Reported: 2009-10-28 03:35 PDT by Kent Tamura
Modified: 2010-02-11 01:14 PST (History)
2 users (show)

See Also:


Attachments
Proposed patch (63.36 KB, patch)
2010-02-10 04:02 PST, Kent Tamura
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2009-10-28 03:35:47 PDT
Add support for step attribute of INPUT element with type=date, time, datetime, datetime-local, month, and week.
Comment 1 Kent Tamura 2010-02-10 04:02:02 PST
Created attachment 48483 [details]
Proposed patch
Comment 2 Darin Adler 2010-02-10 12:46:39 PST
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.
Comment 3 Kent Tamura 2010-02-11 01:14:18 PST
(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