Bug 31342 – Type constraint check for date&time types

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

Add support for ValidaityState.typeMismatch, and layout tests covering Bug#31340 too.

[Kent Tamura]

Created attachment 42945 [details]
ValidityState.typeMismatch and layout tests (rev.6)

Moved from Bug#29004 without changes.

[Kent Tamura]

Comment on attachment 42945 [details]
ValidityState.typeMismatch and layout tests (rev.6)

I need to update the patch due to changes of depending patch.

[Kent Tamura]

Created attachment 43345 [details]
ValidityState.typeMismatch and layout tests (rev.7)

- Update for ISODateTime interface change.

[Darin Adler]

Comment on attachment 43345 [details]
ValidityState.typeMismatch and layout tests (rev.7)

> +bool HTMLInputElement::formStringToISODateTime(InputType type, const String& src, ISODateTime* out)

I'd name it "formString" or "string" rather than "src", since the name of the function says "formString".

> +    ISODateTime result;
> +    if (!out)
> +        out = &result;

I don't think "result" is a great name for this local buffer used when the result is not wanted. Maybe "ignoredResult" would be better. Or even "localBuffer" or something like that.

> +    const UChar* uchars = src.characters();

Personally, I'd just name this "characters".

> +    default:
> +        ASSERT_NOT_REACHED();
> +    }
> +    return false;

I'd put the return in with the default, or put the ASSERT out with the return. Normally I also put in all the other cases, because I like the way gcc will warn you if you add a new enum and don't have a case for it, but that would be a long list of cases.

r=me as is

[Kent Tamura]

I'll commit manually with the following changes.

(In reply to comment #4)
> (From update of attachment 43345 [details])
> > +bool HTMLInputElement::formStringToISODateTime(InputType type, const String& src, ISODateTime* out)
> 
> I'd name it "formString" or "string" rather than "src", since the name of the
> function says "formString".

Renamed to "formString."

> > +    ISODateTime result;
> > +    if (!out)
> > +        out = &result;
> 
> I don't think "result" is a great name for this local buffer used when the
> result is not wanted. Maybe "ignoredResult" would be better. Or even
> "localBuffer" or something like that.

Renamed to "ignoredResult."

> > +    const UChar* uchars = src.characters();
> 
> Personally, I'd just name this "characters".

Done.

> > +    default:
> > +        ASSERT_NOT_REACHED();
> > +    }
> > +    return false;
> 
> I'd put the return in with the default, or put the ASSERT out with the return.
> Normally I also put in all the other cases, because I like the way gcc will
> warn you if you add a new enum and don't have a case for it, but that would be
> a long list of cases.

Moved "return" to the default section.


Thanks!

[Kent Tamura]

Comment on attachment 43345 [details]
ValidityState.typeMismatch and layout tests (rev.7)

Landed as r51163
http://trac.webkit.org/changeset/51163