[webkit-reviews] review denied: [Bug 19562] ValidityState object stub : [Attachment 21716] ValidityState stub class and related stuff

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Michelangelo De Simone
<m.des at mac.com>'s request for review:
Bug 19562: ValidityState object stub
https://bugs.webkit.org/show_bug.cgi?id=19562

Attachment 21716: ValidityState stub class and related stuff
https://bugs.webkit.org/attachment.cgi?id=21716&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
In general, this looks fine.

I'm sad that %baseTypeHash still exists (but that's not your fault).  That
information needs to move into .in or .idl files.

This needs your copyright instead of Apple's :)

+    // ValidityState objects are always bound to their controls at creation
time
isn't really needed.  The ASSERT says the same thing.

So is there any way a ValidityState could outlive its associated form control? 
If so, its going to end up with a bad pointer to a deleted form control.

This would be clearer:
+    return (!(typeMismatch() || stepMismatch() || rangeUnderflow() ||
rangeOverflow() ||
+	     tooLong() || patternMismatch() || valueMissing() ||
customError()));
Written in two parts:

+    bool formHasError = typeMismatch() || stepMismatch() || rangeUnderflow()
|| rangeOverflow() ||
+	     tooLong() || patternMismatch() || valueMissing() || customError();

+   return !formHasError;

The use of the extra variable name makes the long boolean expression much
clearer to read.

+	 static PassRefPtr<ValidityState> create(HTMLFormControlElement* owner
= 0)

Should not have =0 if the form control is required.  That makes it a compile
time error to call HTMLFormControlElement::create() w/o arguments instead of a
runtime error.

r- for the possible crasher (and for the =0 in the create call).

How about adding a minimal js test case which tests for the existence of this
interface?  Hum...I guess there is no way to get at it yet (you haven't added a
ValidityState accessor onto form controls.