[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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #48483|review?                     |review+
               Flag|                            |




--- Comment #2 from Darin Adler <darin at apple.com>  2010-02-10 12:46:39 PST ---
(From update of attachment 48483)
> +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.

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