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

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> 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 134962: patch
https://bugs.webkit.org/attachment.cgi?id=134962&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=134962&action=review


> Source/WebCore/rendering/RenderLayer.cpp:2323
> +    // In overflow: auto case if resizing of box takes place such that
> +    // box width/height is equal or greater than content width/height, then
existing scroll bars if any should be destroyed.
> +    // Ideally clientWidth/Height should be used to compare to
scrollWidth/Height to decide overflow, but in this case 
> +    // clientWidth/Height computed already has scrollbar's height/width
subtracted from it, hence box width/height after
> +    // accounting for borders is taken to decide overflow.

This comment is way too verbose and doesn't go to the point. Only overflow:
scroll scrollbars actually do need to check the client box as we always remove
the scrollbars' size from the content. For other values, it either doesn't
matter (overflow: hidden / visible) or the padding box should be used as the
scrollbar depends on the actual content.

> Source/WebCore/rendering/RenderLayer.cpp:2324
> +    if (needHBar && box->hasAutoHorizontalScrollbar())

This should be merged into the previous ifs. You could pick the right box by
looking at the overflow property:

// Overflow: scroll always uses the client box as we always display the
scrollbar and we disable it based on the available area.
// Other value of overflow uses the padding box.
int widthForHorizontalScrollbar = box->style()->overflow() == OSCROLL ?
box->pixelSnappedClientWidth() : box->pixelSnappedPaddingBoxWidth();
*needHBar = scrollWidth() > widthForHorizontalScrollbar;

The whole logic is really made more complicated that it should by the fact that
we mix together 2 code paths that have slightly different expectations
(overflow scroll vs the rest).

> Source/WebCore/rendering/RenderLayer.cpp:2327
> +	   *needHBar = scrollWidth() > box->pixelSnappedWidth() -
box->borderLeft() - box->borderRight();
> +    if (needVBar && box->hasAutoVerticalScrollbar())
> +	   *needVBar = scrollHeight() > box->pixelSnappedHeight() -
box->borderTop() - box->borderBottom();

Looks like you need to expose your snapped padding box's width and height in
RenderBox.

>
LayoutTests/fast/overflow/overflow-auto-destroy-scroll-after-resizing-expected.
html:8
> +	   <button onclick=changeDim()>Change Red DIV width and Height to 100
and 100</button>

This button is unneeded and harmful as changeDim() doesn't exist.

> LayoutTests/fast/overflow/overflow-auto-destroy-scroll-after-resizing.html:8
> +		   setTimeout(increaseOuterDiv, 0);

Why do we need a setTimeout here? It doesn't strike me as needed.