[Webkit-unassigned] [Bug 30847] step attribute support for date&time types

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=30847





--- Comment #3 from TAMURA, Kent <tkent at chromium.org>  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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.