Comment on attachment 39136[details]
Proposed patch
This is a big patch to review. :( Do you know about CaseInsentiveHash? Might be the right thing to use instead of:
336 else if (equalIgnoringCase(t, "date"))
337 newType = DATE;

(In reply to comment #2)
> This is a big patch to review. :( Do you know about CaseInsentiveHash? Might
> be the right thing to use instead of:
I haven't known it. Thanks.
Maybe I should move DateTime class to separated .h and .cpp. The class is going to have more methods in the near future.

Comment on attachment 39342[details]
Patch part 1: recognize date&time types as text fields
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.

Comment on attachment 39343[details]
Patch part2: ISO 8601 parser
You wrote the parser by yourself I assume? It's no code from any other license?
Also, we need tests for this parser.
I think we should consider splitting it off into its own file which DateMath can use. ISO8601DateParser or some other nicer name?
r- for the lack of tests.

Comment on attachment 39857[details]
Patch part 3: ValidityState.typeMismatch and layout tests (rev.3)
OK, here are the tests. or at least some tests. So my r- above was perhaps premature. I guess I'll still leave it r- awaiting your response to the other questions raised. I've not looked at your parser close enough to see if I had security or performance concerns, but I can look again.

Comment on attachment 39857[details]
Patch part 3: ValidityState.typeMismatch and layout tests (rev.3)
I think I would have used two separate methods:
shoudlBeInvalid
shoudlBeValid
instead of check true/false.
But it looks fine as is.

(In reply to comment #11)
> (From update of attachment 39343[details])
> You wrote the parser by yourself I assume? It's no code from any other
> license?
Yes, it's my code written for WebKit.
> I think we should consider splitting it off into its own file which DateMath
> can use. ISO8601DateParser or some other nicer name?
Do you think the new file should be in JavaScriptCore/wtf/ ?
I assume so. The parser might be used by JSON.parse(), not only html/HTMLInputElement.cpp

(In reply to comment #15)
> Do you think the new file should be in JavaScriptCore/wtf/ ?
> I assume so. The parser might be used by JSON.parse(), not only
> html/HTMLInputElement.cpp
I'm OK w/ either location.

Comment on attachment 40488[details]
Patch part 1: recognize date&time types as text fields (rev.4)
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!

Comment on attachment 41478[details]
Patch part 1: recognize date&time types as text fields (rev.5)
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.

Created attachment 42918[details]
Patch part 1: recognize date&time types as text fields (rev.6)
(In reply to comment #31)
> 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.
Done.
> 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.
Done.
> 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.
I updated that comment. I hope it get clearer.
If we write "return formControlTypes->get(inputType());", gcc in Xcode warns it:
WebKit/WebCore/html/HTMLInputElement.cpp:547: warning: returning reference to temporary
> This comment isn't clear:
> 509 // Needs to be lowercase according to DOM spec
Improved.

Created attachment 43216[details]
Recognize date&time types as text fields (rev.8)
Thank you for reviewing!
(In reply to comment #38)
> > + if (!t.isNull() && typeMap->contains(t))
> > + newType = typeMap->get(t);
> > else
> > newType = TEXT;
>
> It's not a good idiom for performance to do a contains followed by a separate
> get. Those are two completely separate hash table lookups. There are two
> alternatives:
>
> 1) Just call get without first checking contains. The result will be the
> default value, 0, which is TEXT
Done.
> I also think the comment is a little strange because there are tons of call
> sites for this and I don't think we want to add a comment like this at every
> one.
Removed the comment.
> > + types->add(HTMLInputElement::URL, "url");
> > + types->add(HTMLInputElement::WEEK, "week");
>
> These values are enum values, consecutive integers. This should be done with an
> array, not a HashMap.
Changed it to AtomicString[].