[webkit-reviews] review granted: [Bug 42814] maxLength should not be applied to non-text types : [Attachment 62283] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Kent Tamura <tkent at chromium.org>'s
request for review:
Bug 42814: maxLength should not be applied to non-text types
https://bugs.webkit.org/show_bug.cgi?id=42814

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +var input = document.createElement('input');
> +input.maxLength = 2;
> +input.type = 'number';
> +document.body.appendChild(input);
> +input.focus();
> +document.execCommand('insertText', false, '1234');
> +shouldBe('input.value', '"1234"');

Once you've set up the basics like this, it would be better to have the test
cover many cases. It's good to go past the minimum and make sure you've covered
all the code. Ideally you have a test that will fail if you change anything in
your patch, rather than a test that covers just the most important thing you've
changed.

It's unfortunate that HTMLInputElement itself calls a virtual function for
supportsMaxLength. It's true that it needs to be virtual so that InputElement
can call it, but when HTMLInputElement is calling itself the extra cost is not
justified. There is a programming technique that eliminates this without any
special care at call sites. See Node::localName, Node::virtualLocalName,
Element::localName and Element::virtualLocalName, for an example of the
technique. Using the technique would make our compiled code smaller and faster,
but would make the code a bit more ugly.

Something from the existing code, not new: It seems a little bit unfortunate
that when invoking the regular expression engine you can't tell it that you
want to match against the entire string. It ends up doing work you don't need.
We could consider adding a new operation to RegularExpression.

Otherwise things look good.