[Webkit-unassigned] [Bug 59951] Implement Date and Time Input Value Sanitization

[bugzilla-daemon at webkit.org]

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





--- Comment #5 from Joseph Pecoraro <joepeck at webkit.org>  2011-05-02 14:10:58 PST ---
(In reply to comment #4)
> (From update of attachment 91951 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91951&action=review
> 
> > Source/WebCore/html/BaseDateAndTimeInputType.cpp:221
> > +    double parsedValue = parseLocalizedDate(proposedValue, dateType());
> > +    if (isfinite(parsedValue))
> 
> I don’t think you need the local variable, nor do I think the local variable’s name
> adds much clarity. We can put this all on one line.

Done. This was the style of the rest of the file. But I agree, in this case it doesn't matter.


> > Source/WebCore/html/BaseDateAndTimeInputType.cpp:227
> > +    if (!parseToDateComponents(proposedValue, 0))
> > +        return String();
> > +
> > +    return proposedValue;
> 
> I know that often we do early return, but this function seems to return early in each
> case where it decides the value is good, except for this last case, where it does the
> opposite. I suggest reversing the sense to make the function overall read better.

Done. I literally just did that 30 seconds ago, heh =)


> However, this pointed out to me that we can still get type
> mismatches if a user types into the date field, because it
> is a text field. I should update the tests to make sure that
> if, instead of elem.value = '...', the user types '...' that there
> is or is not a type mismatch.

This turned out not to be true. Because the JS elem.value access
will still get the sanitized value, even if the user has typed "foo"
into the text field. So this was a false alarm. The only wacky case
is an accepted localized is not sanitized but would cause a
type mismatch.

Anyone have any thoughts on this? I think I should remove
the localized string check in sanitize. It is already an unlikely
scenario (no port currently implements LocalizedDate
convertFromVisibleValue). Also it means a non-standard
string could be assigned to "value".


> Also, this may be a performance issue, since validity state
> checking seems to run sanitization on each value change,
> causing this to be run 7 times, which could be costly.
> I'll post a follow-up.

Even though Darin r+'d this, I'd like to take a look at this
issue before landing to see if it can be improved. I'll probably
end up opening a separate bug on this.

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