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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=27455

Attachment 34177: Proposed patch, rev. 2
https://bugs.webkit.org/attachment.cgi?id=34177&action=review

------- 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
m_customErrorMessage.

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