[webkit-reviews] review denied: [Bug 27455] Support custom validity error message : [Attachment 34177] Proposed patch, rev. 2

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 5 16:32:10 PDT 2009

Darin Adler <darin at apple.com> has denied Michelangelo De Simone
<micdesim at gmail.com>'s request for review:
Bug 27455: Support custom validity error message

Attachment 34177: Proposed patch, rev. 2

------- Additional Comments from Darin Adler <darin at apple.com>
>  #include "HTMLElement.h"
> +//#include "ValidityState.h"

I think you forgot to remove this.

>  ValidityState::ValidityState(HTMLFormControlElement* parent)
>      : m_control(parent)
> +    , m_customError("")
>  {
>      ASSERT(parent);
>  }
> +void ValidityState::setCustomValidity(const String& error)
> +{
> +    if (error.isNull()) {
> +	   m_customError = "";
> +	   return;
> +    }
> +
> +    m_customError = error;
> +}

Why is it important to store an empty string instead of a null string? It would
be nice to just let m_customError get initialized to null and not special case
null in setCustomValidity. Does it cause some sort of problem?

> +	   void setCustomValidity(const String&);

> -	   bool customError() { return false; }
> +	   bool customError() { return !m_customError.isEmpty(); }

> +	   String m_customError;

I assume that customError and setCustomValidity are both names from the HTML 5
specification, so I won't complain to you about them too much, since I guess
it's OK to have the ValidityState function names match the DOM function names.

But I must say both names are confusing. Set the custom validity? No, set a
custom validity error message. So it should be setCustomErrorMessage on
ValidityState. "Custom error"? No, "has custom error message", so the name
should be hasCustomErrorMessage, not customError.

Since m_customError is our own name, it can be as clear as we like. I suggest

I'm going to say review-, just so you can fix these minor issues.

More information about the webkit-reviews mailing list