[Webkit-unassigned] [Bug 21271] Maxlength limits value

[bugzilla-daemon at webkit.org]

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





--- Comment #16 from TAMURA, Kent <tkent at chromium.org>  2009-09-15 20:24:22 PDT ---
(In reply to comment #13)
> 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.

I renamed constrainValue() to sanitizeUserInputValue().

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

Done.

> > -    // 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?

Yeah, we should have assertion for sanitizeValue(), and
RenderTextControlSingleLine.subtreeHasChanged() should call sanitizeValue().

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

Right.  This is redundant for now.  I added it because we need to implement
sanitizing rules for non-text types.

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