[webkit-reviews] review granted: [Bug 18703] changing the 'size' property on a text input does not affect its length : [Attachment 24640] Improved patch for bug 18703

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Glenn Wilson <gwilson at google.com>'s
request for review:
Bug 18703: changing the 'size' property on a text input does not affect its
length
https://bugs.webkit.org/show_bug.cgi?id=18703

Attachment 24640: Improved patch for bug 18703
https://bugs.webkit.org/attachment.cgi?id=24640&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
I think the basics of this patch are great. It can land as-is.

There are two ways I think it could be improved.

The real architecture for something like this is to make the DOM element call
updateFromElement() on the renderer at the right time. This is considered
better than making a specific call to the renderer, because it's up to the
renderer to know what decisions it makes about the appearance based on what
attributes. So instead of a call to setNeedsLayoutAndPrefWidthsRecalc, we'd
have a call to updateFromElement here. Then
RenderTextControl::updateFromElement would get logic to notice changes in the
size attribute (for HTMLInputElement) and the cols attribute (for
HTMLTextAreaElement) to determine whether that will change the size. This is
better encapsulation. We want the invalidation based on size to go into
RenderText control because RenderTextControl::calcPrefWidths is the function
that knows the algorithm for how it affects the size. Unfortunately,
HTMLTextAreaElement already uses the same design you're using here, so I don't
think this change makes things any worse.

So:

    1) Use updateFromElement and put the logic about exactly what the effect of
this attribute it into RenderTextControl. And change HTMLTextAreaElement to
call updateFromElement instead.

We want to make sure to to do recalc that's unnecessary; it's common for sites
to have bugs where they repeatedly set an attribute, and we'd like to minimize
the work we do in cases like that.

    2) Don't do any update if the size isn't changing. This can be done most
elegantly inside a RenderTextControl::updateFromElement function by remembering
the old size factor value from calcPrefWidths, but it can even be done in
HTMLInputElement::parseMappedAttribute by checking the old m_size value and not
calling anything if it's not changing.

The test would be even better if the regression test said what it was testing
in the output.

Anyway, I would have loved to see the improvements above done, but the change
seems fine exactly as-is.

r=me