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

[bugzilla-daemon at webkit.org]

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

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


I would like to see a revised version but the change looks great now. The style
bot looked like it was sick when you uploaded your patch.

> Source/WebCore/ChangeLog:8
> +	   overflow: auto was being determined based on clientWidth and
clientHeight, which during re-styling

We prefer to talk about style change or style application / selection and not
re-styling.

> Source/WebCore/ChangeLog:9
> +	   scenarios will blindly exclude previous scrollbars while computing
overflow after re-styling.

Technically, this makes sense for overflow: scroll as it is used to determine
if we should enable / disable our scrollbars. For other values of overflow,
this is wrong as it wrongly reduce the available area. It would be nice to
speak about this difference (overflow: scroll vs auto / overlay).

> Source/WebCore/rendering/RenderBox.cpp:516
> +LayoutUnit RenderBox::paddingBoxWidth() const
> +{
> +    return width() - borderLeft() - borderRight();
> +}
> +
> +LayoutUnit RenderBox::paddingBoxHeight() const
> +{
> +    return height() - borderTop() - borderBottom();
> +}
> +
> +int RenderBox::pixelSnappedPaddingBoxWidth() const
> +{
> +    return snapSizeToPixel(paddingBoxWidth(), paddingBoxLeft());
> +}
> +
> +int RenderBox::pixelSnappedPaddingBoxHeight() const
> +{
> +    return snapSizeToPixel(paddingBoxHeight(), paddingBoxTop());
> +}

I think those should be inlined into the header as much as possible.

> Source/WebCore/rendering/RenderBox.h:216
> +    LayoutUnit paddingBoxLeft() const { return paddingLeft(); }
> +    LayoutUnit paddingBoxTop() const { return paddingTop(); }

I don't think you need to expose those 2.