[webkit-reviews] review denied: [Bug 71541] Unnecessary scroll bar after changing the dimensions of a DIV : [Attachment 134874] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied SravanKumar S(:sravan)
<ssandela at innominds.com>'s request for review:
Bug 71541: Unnecessary scroll bar after changing the dimensions of a DIV
https://bugs.webkit.org/show_bug.cgi?id=71541

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

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


> Source/WebCore/rendering/RenderLayer.cpp:2322
> +    if (box->hasAutoHorizontalScrollbar())
> +	   *needHBar = box->pixelSnappedWidth() < m_scrollSize.width();
> +    if (box->hasAutoVerticalScrollbar())
> +	   *needVBar = box->pixelSnappedHeight() < m_scrollSize.height();

needHBar and needVBar can be 0; this code needs a null check like the code
above it

It’s not good to do two different needHBar and needVBar computations. These
should be combined with the others above in "if/else" constructions.

There is no good reason to use m_scrollSize.width() when the existing code uses
scrollWidth(). That kind of unnecessary difference can easily cause confusion.
Similarly, there is no reason to reverse the order of the comparison, putting
the scroll width on the right side instead of the left side above.

Finally, this is the kind of code that needs a comment. What is it about
automatic scrollbars that causes you to need to use “width” rather than “client
width”? This is the kind of “why” issue that needs to be covered by comments.