[webkit-reviews] review granted: [Bug 30847] step attribute support for date&time types : [Attachment 48483] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 30847: step attribute support for date&time types
https://bugs.webkit.org/show_bug.cgi?id=30847

Attachment 48483: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=48483&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +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.