[webkit-reviews] review denied: [Bug 29292] [HTML5][Forms] Support for <textarea maxlength=N> : [Attachment 39679] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 29292: [HTML5][Forms] Support for <textarea maxlength=N>
https://bugs.webkit.org/show_bug.cgi?id=29292

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +    if (!hasAttribute(maxlengthAttr))
> +	   return;
> +    bool ok;
> +    unsigned maxLength = getAttribute(maxlengthAttr).string().toUInt(&ok);
> +    if (!ok)
> +	   return;

It's a shame to do two hash table lookups here for the maxlength attribute. And
there's no reason for that check. toUInt will give false for "ok" when
maxlengthAttr is null, so I believe the hasAttribute check is unnecessary extra
work.

> +    int currentLength =
InputElement::numGraphemeClusters(toRenderTextControl(renderer())->text().impl(
));
> +    int selectionLength =
InputElement::numGraphemeClusters(plainText(document()->frame()->selection()->s
election().toNormalizedRange().get()).impl());

I think these functions should just be public functions, perhaps in
PlatformString.h, rather than members of the InputElement class. It also seems
inconvenient to have to pass them StringImpl* rather than just a String. As we
make these functions more public it makes sense to me to make them more normal
functions and less special-purpose.

> +    if (newLength < static_cast<int>(proposedValue.length())) {
> +	   String string = proposedValue;
> +	   return string.left(newLength);
> +    }
> +    return proposedValue;

This can just be written:

    return proposedValue.left(newLength);

The left function already has optimization for the case where the length is >=
the length of the string, and you don't need to redo it.

> +    bool ok;
> +    unsigned maxLength = getAttribute(maxlengthAttr).string().toUInt(&ok);
> +    return ok ? maxLength : 0;

toUInt already guarantees it will return 0 if the string can't be parsed, so
you can just write:

    return getAttribute(maxlengthAttr).string().toUInt();

> +    static String sanitizeUserInputValue(const String&, int);

It's not good to leave out the argument name here for the int. It's clear what
this int would be. So it needs a name.

I'm going to say review- because I think at least some of my suggestions above
really ought to be done.