[webkit-reviews] review denied: [Bug 25552] Support for HTML5 Forms "pattern" attribute : [Attachment 33122] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Michelangelo De Simone
<micdesim at gmail.com>'s request for review:
Bug 25552: Support for HTML5 Forms "pattern" attribute
https://bugs.webkit.org/show_bug.cgi?id=25552

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +	       RegularExpression patternRegExp(pattern(), TextCaseSensitive);
> +	       return patternRegExp.match(value()) != 0;

If you are trying to determine if the regular expression matches the entire
string you need to do more work. You need to create a test where there is a
regular expression that matches the beginning of a string but there are extra
characters in the value. I believe that test would fail.

This code also recompiles the regular expression pattern every time you do the
match. I suppose that's intentional. If you remove pattern() as I request
below, then you'll instead do getAttribute(patternAttr) and put it into a local
variable of type const AtomicString& at the top of the function.

> +String HTMLInputElement::pattern() const
> +{
> +    return getAttribute(patternAttr);
> +}

For new code, we try to avoid having this kind of function. Instead we put
[Reflect] in the .idl file and let the function be automatically generated. If
you were going to write it by hand there are some issues about using String vs.
AtomicString.

> +void HTMLInputElement::setPattern(const String& p)
> +{
> +    if (p.isEmpty()) {
> +	   int exccode;
> +	   removeAttribute(patternAttr, exccode);
> +    } else
> +	   setAttribute(patternAttr, p);
> +}

Are you sure this does the right thing for the empty string? I recommend just
using [Reflect] in the .idl file and not writing custom code for this. I think
it's unlikely this has a rule different from all other attributes.

I'd like to see some tests about how this works with all the special characters
for regular expressions, especially ones like "$" and "^".

review- mainly because of the "matches the entire string" issue.