Summary: | step attribute support for date&time types | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kent Tamura <tkent> | ||||
Component: | Forms | Assignee: | 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
Kent Tamura
2009-10-28 03:35:47 PDT
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 |