Bug 30847

Summary: step attribute support for date&time types
Product: WebKit Reporter: Kent Tamura <tkent>
Component: FormsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: cshu, mike
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://www.whatwg.org/specs/web-apps/current-work/multipage/common-input-element-attributes.html#attr-input-step
Bug Depends on: 27451, 29004, 29070, 32696    
Bug Blocks: 29359    
Attachments:
Description Flags
Proposed patch darin: review+

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