[webkit-reviews] review granted: [Bug 11355] Overflow: auto; in 2 cascaded boxes, outer having overflow: auto; , inner having overflow: auto; height: 100%; width: 200%; - Renders scrollbars on cascaded block elements in wrong way : [Attachment 152062] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Pravin D
<pravind.2k4 at gmail.com>'s request for review:
Bug 11355: Overflow: auto; in 2 cascaded boxes, outer having overflow: auto;,
inner having overflow: auto; height: 100%; width: 200%; - Renders scrollbars on
cascaded block elements in wrong way
https://bugs.webkit.org/show_bug.cgi?id=11355

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

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


> Source/WebCore/ChangeLog:3
> +	   Overflow: auto; in 2 cascaded boxes, outer having overflow: auto;,
inner having overflow: auto; height: 100%; width: 200%; - Renders scrollbars on
cascaded block elements in wrong way

Could you change the name to something that underlines the bug a bit more here?


> Source/WebCore/rendering/RenderBox.cpp:2145
>	   result =
cb->computeContentBoxLogicalHeight(cbstyle->logicalHeight().value());

Nit: Probably more readable to use a temporary variable here to avoid re-using
|result| 3 times.

> Source/WebCore/rendering/RenderBox.cpp:2146
> +	   result = max<LayoutUnit>(0, result -
LayoutUnit(cb->scrollbarLogicalHeight()));

No need for the conversion to LayoutUnit here.

> Source/WebCore/rendering/RenderBox.cpp:2285
> +		   availableHeight = max<LayoutUnit>(0, availableHeight -
LayoutUnit(toRenderBox(cb)->scrollbarLogicalHeight()));

Unneeded conversion to LayoutUnit here too.

> LayoutTests/ChangeLog:8
> +	   Additional information of the change such as approach, rationale.
Please add per-function descriptions below (OOPS!).

This should be removed.

>
LayoutTests/fast/overflow/child-100percent-height-inside-fixed-container-with-o
verflow-auto-expected.txt:1
> +The container element should not contain a vertical scrollbar. The height of
the inner box should be 100% of the containers height minus the height of
horizontal scrollbar

I like to include the bug number and description in the tests. You can either
append it to the description() or use debug() here to achieve that.