[webkit-reviews] review granted: [Bug 27454] Hook maxlength attribute to ValidityState tooLong() method : [Attachment 40214] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 27454: Hook maxlength attribute to ValidityState tooLong() method
https://bugs.webkit.org/show_bug.cgi?id=27454

Attachment 40214: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=40214&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    switch (inputType()) {
> +    case EMAIL:
> +    case PASSWORD:
> +    case SEARCH:
> +    case TELEPHONE:
> +    case TEXT:
> +    case URL: {
> +	   bool userEditted = !m_data.value().isNull(); // tooLong must be
false for the default value.
> +	   if (userEditted && hasAttribute(maxlengthAttr)) {
> +	       // It's safe to cast maxLength() to unsigned because maxLength()
can never be negative.
> +	       return value().length() > static_cast<unsigned>(maxLength());
> +	   }
> +	   break;

It's spelled "edited", not "editted".

I think the comment is hard to read. You say "tooLong must be false for the
default value" but what I think a better comment would be:

    // Return false for the default value even if it is longer than maxLength.

Also, better to have the comment on the line before. It's harder to read it on
the end of the line.

Should do return false here instead of break, so we can ASSERT_NOT_REACHED
outside the switch.

We normally prefer early-return style, so the return should be for the case
where there is no length check.

> +    }
> +    case BUTTON:
> +    case CHECKBOX:
> +    case COLOR:
> +    case FILE:
> +    case HIDDEN:
> +    case IMAGE:
> +    case ISINDEX:
> +    case NUMBER:
> +    case RADIO:
> +    case RANGE:
> +    case RESET:
> +    case SUBMIT:
> +	   break;

Should do return false here instead of break, so we can ASSERT_NOT_REACHED
outside the switch.

> +    return false;

Should ASSERT_NOT_REACHED here.

> +    const_cast<HTMLTextAreaElement*>(this)->updateValue();

As a rule, we should not use const_cast. Instead we should use mutable.

> +bool HTMLTextAreaElement::tooLong() const
> +{
> +    if (m_isDirty) {
> +	   bool ok;
> +	   unsigned maxLength =
getAttribute(maxlengthAttr).string().toUInt(&ok);
> +	   if (ok)
> +	       return value().length() > maxLength;
> +    }
> +    return false;
> +}

Our normal approach is early return. So we'd do:

    if (!m_isDirty)
	return false;

    if (!ok)
	return false;

Instead of the way the logic is structured above.

r=me as is, but pleaseconsider my comments.