[webkit-reviews] review denied: [Bug 29004] [HTML5][Forms] simple implementation of date&time types of INPUT element : [Attachment 39342] Patch part 1: recognize date&time types as text fields
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Sep 21 10:50:29 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 39342: Patch part 1: recognize date&time types as text fields
https://bugs.webkit.org/attachment.cgi?id=39342&action=review
------- Additional Comments from Eric Seidel <eric at webkit.org>
Explicit String construction isn't needed:
String("button")
the compiler will find String(char*) anyway.
Why two lookups?
333 if (!t.isNull() && typeMap->contains(t)) // contains() with a null
string makes crash.
334 newType = typeMap->get(t);
just do one get instead of two lookups.
Lets make a hash for these too:
@@ const AtomicString& HTMLInputElement::formControlType() const
Then you don't have a zillion DEFINE_STATIC_LOCAL calls, because the Hash can
be TYPE -> AtomicString
(That doesn't have to be part of this patch, but if you're in there cleaning
stuff up, you should consider it. I think it will be much less code. Heck, it
could even be a Vector instead of a Hash if you like.)
Default is bad with switches over enums:
default:
1048 return false;
it's breaks the compiler's ability to tell you when you're missing cases. In
this function 1028 bool HTMLInputElement::isTextField() const it seems we
don't even want a switch. We just want a single if, or? Or at least we want
an explicit list of the false cases if you're going to use a switch.
More information about the webkit-reviews
mailing list