[webkit-reviews] review denied: [Bug 29004] [HTML5][Forms] simple implementation of date&time types of INPUT element : [Attachment 40488] Patch part 1: recognize date&time types as text fields (rev.4)
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Oct 19 14:52:48 PDT 2009
Eric Seidel <eric at webkit.org> has denied TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 29004: [HTML5][Forms] simple implementation of date&time types of INPUT
element
https://bugs.webkit.org/show_bug.cgi?id=29004
Attachment 40488: Patch part 1: recognize date&time types as text fields
(rev.4)
https://bugs.webkit.org/attachment.cgi?id=40488&action=review
------- Additional Comments from Eric Seidel <eric at webkit.org>
366 // get() with unregistered names returns HTMLInputElement::TEXT.
Wild.
// get() with a null string makes crash.
// get() with a null string causes a crash.
would be better.
I would probably have written:
75 if (!t.isNull()) // get() with a null string makes crash.
376 newType = typeMap->get(t);
357377 else
358378 newType = TEXT;
as:
if (typeMap.contains(t))
newType = typeMap.get();
else
newType = TEXT;
Then you don't need any fancy emptyValue either.
You could do:
448 static const Vector<AtomicString>* createFormControlTypes()
as just an accessor and use a static Vector<AtomicString> w/o a pointer if you
like. It's OK as is, but:
452 (*types)[HTMLInputElement::BUTTON] = "button";
is slightly harder to read wiht a pointer. I think at this point, I wouldn't
bother changing it though.
43 TEXT = 0, // TEXT must be 0 because it's the default type.
is not needed if you change the way the hash lookup works, no?
68 static const int MAX_INPUT_TYPES = DATETIMELOCAL + 1;
Could be easier as a LAST_INPUT_TYPE a the end of the enum, no?
This looks great, but I think we shoudl go one more round. Thanks again!
More information about the webkit-reviews
mailing list