[webkit-reviews] review granted: [Bug 21271] Maxlength limits value : [Attachment 39594] Another 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 21271: Maxlength limits value
https://bugs.webkit.org/show_bug.cgi?id=21271

Attachment 39594: Another proposed patch
https://bugs.webkit.org/attachment.cgi?id=39594&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
I don't think the names sanitizeValue and constrainValue are expressive enough,
especially without comments. These names do not make it clear that
constrainValue does everything sanitizeValue does, but also enforces a maximum
length based on the maxlength property.

Better names would be ones that make it clear what the difference is between
these two closely related functions. One is for user input and another is for
values that don't come from user input. Perhaps sanitizeValue and
sanitizeUserInputValue? Or constrainValue and constrainUserInputValue? These
are not as elegant as your names, but they are clearer, and I think they would
make the code easier to understand.

> -// large. However, due to http://bugs.webkit.org/show_bugs.cgi?id=14536
things
> +// large. However, due to http://bugs.webkit.org/show_bug.cgi?id=14536
things

If you're going to fix the URL I also suggest making it https since http just
redirects.

>  void InputElement::setValueFromRenderer(InputElementData& data,
InputElement* inputElement, Element* element, const String& value)
>  {
> -    // Renderer and our event handler are responsible for constraining
values.
> -    ASSERT(value == inputElement->constrainValue(value) ||
inputElement->constrainValue(value).isEmpty());

I'm a bit concerned about eliminating this assertion. Your change log doesn't
give any explanation for why it's correct to remove it. Should we change the
assertion to use sanitizeValue instead of constrainValue? Also, under what
circumstances do we get a string that violates maxlength from the renderer?

> +String HTMLInputElement::sanitizeValue(const String& proposedValue) const
> +{
> +    if (isTextField())
> +	   return InputElement::sanitizeValue(this, proposedValue);
> +    return proposedValue;
> +}

Since InputElement::constrainValue already checks isTextField and does nothing
if it is false, I do not think we need an additional check here. Lets keep this
consistent with the existing constrainValue function, which does not add an
isTextField check.

> -    input->setValueFromRenderer(input->constrainValue(text()));
> +    // We don't need to apply constrainValue() or sanitizeValue() here.
> +    // InputElement::handleBeforeTextInsertedEvent() does enough job.
> +    input->setValueFromRenderer(text());

The phrase "does enough job" is not grammatical and not specific enough. I
would write:

    // We don't need to call InputElement's constrainValue or sanitizeValue
    // function here because InputElement::handleBeforeTextInsertedEvent has
    // already called constrainValue.

I'd also like to understand if that's really true. Is there no code path where
that is violated?

I'm going to say review+ because I do think this is OK as-is, but I do have a
few concerns. Feel free to clear the review flag if you plan to do a new patch.