[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]

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!