[webkit-reviews] review granted: [Bug 31342] Type constraint check for date&time types : [Attachment 43345] ValidityState.typeMismatch and layout tests (rev.7)
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Nov 18 15:26:51 PST 2009
Darin Adler <darin at apple.com> has granted TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 31342: Type constraint check for date&time types
https://bugs.webkit.org/show_bug.cgi?id=31342
Attachment 43345: ValidityState.typeMismatch and layout tests (rev.7)
https://bugs.webkit.org/attachment.cgi?id=43345&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
> +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
More information about the webkit-reviews
mailing list