[webkit-reviews] review granted: [Bug 43537] Introduce isValidValue(const String&) of HTMLInputElement and HTMLTextAreaElement : [Attachment 63549] Patch 3

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Kent Tamura <tkent at chromium.org>'s
request for review:
Bug 43537: Introduce isValidValue(const String&) of HTMLInputElement and
HTMLTextAreaElement
https://bugs.webkit.org/show_bug.cgi?id=43537

Attachment 63549: Patch 3
https://bugs.webkit.org/attachment.cgi?id=63549&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    // Common flag for HTMLInputElement::tooLong() and
HTMLTextAreaElement::tooLong().
> +    enum NeedsToCheckDirtyFlag {
> +	   CheckDirtyFlag,
> +	   IgnoreDirtyFlag,
> +    };

I think this reads better on a single line than on four lines.

> +    Color color(value); // This accepts named colors such as "white".
> +    return color.isValid() && !color.hasAlpha();

This seems wrong. Why should "transparent" be an invalid color? Why is a color
valid only if it has alpha of 255? There is no need to check isValid if you are
also checking alpha, by the way, since invalid color is guaranteed to have an
alpha of 0. I don't think there are enough test cases for this.

> +    int matchLength;
> +    int matchOffset = regExp.match(address, 0, &matchLength);
> +
> +    return !matchOffset && matchLength == addressLength;

Matching and then checking if the offset is 0 is an inefficient way to do an
anchored regular expression match. We should probably add a complete string
match operation to RegularExpression so you don't have to do things this
roundabout way.

> +bool HTMLInputElement::isValidValue(const String& value) const
> +{
> +    if (inputType() == CHECKBOX || inputType() == FILE || inputType() ==
RADIO) {
> +	   ASSERT_NOT_REACHED();
> +	   return false;
> +    }

This probably needs a "why" comment.

> +    return !typeMismatch(value) && !stepMismatch(value) &&
!rangeUnderflow(value)
> +	   && !rangeOverflow(value) && !tooLong(value, IgnoreDirtyFlag)
> +	   && !patternMismatch(value) && !valueMissing(value);

I would format these with one check per line.

Since my comments are on code you are moving, not code you are adding, I am
going to say review+ but I'd like to see a lot more test cases here and that
Color thing sure does seem wrong.