[Webkit-unassigned] [Bug 29292] [HTML5][Forms] Support for <textarea maxlength=N>

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=29292


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #39679|review?                     |review-
               Flag|                            |




--- Comment #2 from Darin Adler <darin at apple.com>  2009-09-17 12:37:34 PDT ---
(From update of attachment 39679)
> +    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()->selection().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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.