[webkit-reviews] review granted: [Bug 20445] HTML listbox is not resized horizontally when zooming : [Attachment 213087] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Renata Hodovan <reni at webkit.org>'s
request for review:
Bug 20445: HTML listbox is not resized horizontally when zooming
https://bugs.webkit.org/show_bug.cgi?id=20445

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

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


I’m going to say review+ but I have some serious reservations about this patch.


> Source/WebCore/ChangeLog:9
> +	   If any style changes happens on a HTMLSelectElement, we need to set
the m_optionsChanged property
> +	   of its renderer (RenderListBox) unless its size won't follow the
changed content.

The word "unless" needs to be replaced by the word "otherwise".

> Source/WebCore/html/HTMLSelectElement.cpp:91
> +    setOptionsChangedOnRenderer();

Is it really true that *any* style change needs to do this? I think this is not
correct. While the change works, it seems likely it is needlessly inefficient.

This code is non obvious, because the function name
"setOptionsChangedOnRenderer" is telling a lie. We are saying that the options
changed, but the options did not change. It’s worth considering changing names
to make this clear. Or at the very least we need a comment that says something
like:

    // Even though the options didn’t necessarily change, we will call
setOptionsChangedOnRenderer for its side effect
    // of recomputing the width of the element. We need to do that if the style
change included a change in zoom level.

I’m not sure if the comment I wrote is accurate. We should make a comment in
that style that correctly explains the reason for this line of code.

> Source/WebCore/html/HTMLSelectElement.h:110
> +    virtual void didRecalcStyle(Style::Change);

Should be marked OVERRIDE and FINAL, and should be private rather than
protected. Unless there’s a reason that one of those is incorrect.