Bug 33911

Summary: HTMLInputElement::valueAsDate setter support for type=date.
Product: WebKit Reporter: Kent Tamura <tkent>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 32697    
Attachments:
Description Flags
Patch none

Description Kent Tamura 2010-01-20 10:23:18 PST
HTMLInputElement::valueAsDate setter support for type=date.
Comment 1 Kent Tamura 2010-01-20 10:26:20 PST
Created attachment 47046 [details]
Patch
Comment 2 Darin Adler 2010-01-20 16:21:16 PST
Comment on attachment 47046 [details]
Patch

> +    if (beforeGregorianStartDate(m_year, m_month, m_monthDay))
> +        return false;

It seems bad that in this case the function returns false, but has already had a side effect and changed the state of the ISODateTime object. Ideally functions would either succeed or leave the object untouched. In this patch there seems no problem with the behavior, but it is surprising to me.
Comment 3 Kent Tamura 2010-01-20 18:22:13 PST
Thank you for reviewing.

(In reply to comment #2)
> (From update of attachment 47046 [details])
> > +    if (beforeGregorianStartDate(m_year, m_month, m_monthDay))
> > +        return false;
> 
> It seems bad that in this case the function returns false, but has already had
> a side effect and changed the state of the ISODateTime object. Ideally
> functions would either succeed or leave the object untouched. In this patch
> there seems no problem with the behavior, but it is surprising to me.

Yeah, I understand it is not good in general.
ISODateTime has some such fail-but-update code in parse*() methods too. I thought this behavior was acceptable because I had no idea of scenario to use 1 ISODateTime instance for multiple purpose, and we need complicated code to avoid such update in some cases.

I'll commit this change as is, and will add comments to ISODateTime.h in the next patch.
Comment 4 WebKit Commit Bot 2010-01-20 20:39:12 PST
Comment on attachment 47046 [details]
Patch

Clearing flags on attachment: 47046

Committed r53598: <http://trac.webkit.org/changeset/53598>
Comment 5 WebKit Commit Bot 2010-01-20 20:39:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Darin Adler 2010-01-21 09:46:09 PST
(In reply to comment #3)
> we need complicated code to avoid such update in some cases.

One simple way to do it is to make a temporary copy of the entire object, and discard it unless you're about to return true.

Another is to have all the logic work on local variables and not modify the actual ISODateTime object itself at all, and then copy those values into the object when you're about to return true.

Both of these should be simple refactoring, probably not as complicated as you are fearing.
Comment 7 Darin Adler 2010-01-21 09:47:48 PST
But another option is to simply say that when false is returned, the date time object is in an "undefined" state in the documentation for the class. We could even have a debugging flag to enforce that rule.

Another way is to eliminate the return value and instead have the ISODateTime get into a "bad", "undefined", "failed", or "empty" state to indicate the error. A persistent state rather than a return value from the setter function. Could clean up the interface a bit, but does create more risk someone might forget to check the success or failure.