[webkit-reviews] review denied: [Bug 44886] Reflected attribute input.size wraps on negative values (Chrome), or returns them (Safari) : [Attachment 108576] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Antaryami Pandia
<xqb748 at motorola.com>'s request for review:
Bug 44886: Reflected attribute input.size wraps on negative values (Chrome), or
returns them (Safari)
https://bugs.webkit.org/show_bug.cgi?id=44886

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=108576&action=review


Patch is generally good, but not quite right.

> Source/WebCore/html/HTMLInputElement.cpp:1017
>  int HTMLInputElement::size() const
>  {
> -    return m_size;
> +    if (m_size > 0)
> +	   return m_size;
> +
> +    return defaultSize;
>  }

It’s not so great to have an m_size that is wrong rather than actually
containing the size. This logic should instead go into
HTMLInputElement::parseMappedAttribute in the code that sets m_size. That code
could be further improved by only calling setNeedsLayoutAndPrefWidthsRecalc if
m_size changes.

The alternative would be to remove m_size entirely and put all the parsing
logic into HTMLInputElement::size. That would be good because it would make the
object smaller. But it would make the optimization to call
setNeedsLayoutAndPrefWidthsRecalc less often difficult.