[webkit-reviews] review denied: [Bug 117120] [CSS Regions] Positioned elements in regions get clipped if they fall outside the region : [Attachment 218512] Patch

Tue Dec 17 07:17:34 PST 2013

Mihnea Ovidenie <mihnea at adobe.com> has denied Radu Stavila
<stavila at adobe.com>'s request for review:
Bug 117120: [CSS Regions] Positioned elements in regions get clipped if they
fall outside the region
https://bugs.webkit.org/show_bug.cgi?id=117120

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

------- Additional Comments from Mihnea Ovidenie <mihnea at adobe.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=218512&action=review

r-
In general looks good but i have some comments.

> LayoutTests/ChangeLog:8
> +	   Added tests for the correct painting of the borders of differently
positioned elements overflowing regions.

Please add a link to the bug for all the tests and/or a description of what
each test is trying to achieve - you only describe the passing condition.

> LayoutTests/fast/regions/sticky-border-overflow.html:1
> +<!DOCTYPE html>

The other 2 tests were testing the behaviour of static/absolute positioned
elements inside the relative positioned element. This test is using a different
pattern for testing - is this what you intended?

> Source/WebCore/ChangeLog:8
> +	   Fixed the computing of the box decorations clip rect when having
statically positioned

I would like to see a description of the problem - what was wrong and how this
patch if fixing that problem.

> Source/WebCore/rendering/RenderFlowThread.cpp:1307
> +	   if (containerBlock->style().writingMode() !=
iterBox->style().writingMode())

Where is this code path tested within your proposed tests for this patch? I do
not see any writing modes declarations.

> Source/WebCore/rendering/RenderFlowThread.cpp:1316
> +    // point in restricting the X clipping rect (this also saves us the
trouble

Do you mean " restricting the clipping rect dimensions perpendicular to the
fragmentation direction" ?  From what i can tell you also adjust the width, not
only the x.

> Source/WebCore/rendering/RenderFlowThread.cpp:1317
> +    // of handling percent-based widths and margins).

I do not understand this comment about handling percent-based widths and
margins and i do not see any related tests on this.