Bug 33911 – HTMLInputElement::valueAsDate setter support for type=date.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Kent Tamura]

HTMLInputElement::valueAsDate setter support for type=date.

[Kent Tamura]

Created attachment 47046 [details]
Patch

[Darin Adler]

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.

[Kent Tamura]

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.

[WebKit Commit Bot]

Comment on attachment 47046 [details]
Patch

Clearing flags on attachment: 47046

Committed r53598: <http://trac.webkit.org/changeset/53598>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(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.

[Darin Adler]

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.