[webkit-reviews] review granted: [Bug 20445] HTML listbox is not resized horizontally when zooming : [Attachment 213087] Proposed patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Oct 1 09:50:47 PDT 2013
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.
More information about the webkit-reviews
mailing list