Bug 30847

Created attachment 48483 [details] Proposed patch

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.

(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