[webkit-reviews] review denied: [Bug 29004] [HTML5][Forms] simple implementation of date&time types of INPUT element : [Attachment 41478] Patch part 1: recognize date&time types as text fields (rev.5)

[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 41478: Patch part 1: recognize date&time types as text fields
(rev.5)
https://bugs.webkit.org/attachment.cgi?id=41478&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
It seems your test case only needs to test the casing once.  once it's
confirmed that button and BUTTON both map to BUTTON, you don't really need to
check CHECKBOX, COLOR, EMail, etc.  Just the lowercase versions should be
enough and might make the test less confusing.

Cleanup of these mthods could have been done as a separate first patch, but
this is OK too.

I suspect the dom spec says nothing about the implementation, but it does say
that input.type has to be lowercase, right?  Would be more clear to say
something like:
// input.type must return a lower case value according to DOM3.


I dont understand this comment:
  // We don't use formControlTypes->get(inputType()) because this function
 542	 // tries to return a reference to a temporary AtomicString object in
that
 543	 // case.

This comment isn't clear:
 509	 // Needs to be lowercase according to DOM spec

Excellent cleanup!
 794	 if (inputType() == HIDDEN)
 795	     return false;
 796	 return HTMLFormControlElementWithState::rendererIsNeeded(style);

This patch looks fine.	The comments could/should be adjusted before landing.

I'm torn.  I would like to r+ this, but I would also like to understand the
formControlTypes->get(inputType())  issue before I do.	I guess we should go
one more round.  Ping me as soon as you post a new patch or respond to the
formControlTypes->get(inputType())  issue and I'll be happy to r+ this.