[webkit-reviews] review denied: [Bug 124042] [CSS Regions] Fix positioning composited layers when the region has overflow:hidden : [Attachment 216398] patch

[bugzilla-daemon at webkit.org]

Alexandru Chiculita <achicu at adobe.com> has denied Mihai Maerean
<mmaerean at adobe.com>'s request for review:
Bug 124042: [CSS Regions] Fix positioning composited layers when the region has
overflow:hidden
https://bugs.webkit.org/show_bug.cgi?id=124042

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

------- Additional Comments from Alexandru Chiculita <achicu at adobe.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=216398&action=review


Interesting case. I have a few comments below.

>
LayoutTests/compositing/regions/position-layer-inside-region-overflow-hidden-ex
pected.html:3
> +		<title>Bug #124042: [CSS Regions] Fix positioning composited
layers when the region has overflow:hidden</title>

nit: Use 4 spaces instead of tabs.

>
LayoutTests/compositing/regions/position-layer-inside-region-overflow-hidden-ex
pected.html:6
> +				-webkit-font-smoothing: none;

nit: Add a comment to explain why you need this.

>
LayoutTests/compositing/regions/position-layer-inside-region-overflow-hidden-ex
pected.html:18
> +				/*overflow: hidden;*/

nit: Remove commented code.

>
LayoutTests/compositing/regions/position-layer-inside-region-overflow-hidden-ex
pected.html:22
> +			* {

Avoid the * and just inline the class names that actually require this.

>
LayoutTests/compositing/regions/position-layer-inside-region-overflow-hidden.ht
ml:23
> +				border:solid 1px #888;

I guess this will overwrite your border-width in .region. Is the border-width
needed?

The HTML comments apply to both test and expected files.

> Source/WebCore/rendering/RenderLayerBacking.cpp:877
> +		   RenderLayerBacking* layerOwnerBacking =
layerOwner->layer()->backing();

nit: Put this line after the comment.

> Source/WebCore/rendering/RenderLayerBacking.cpp:887
> +		   if (layerOwnerBacking->clippingLayer())

Can you make a test for the case when the clipping layer is not the region
itself? Example: Nested composited layers inside the region where the parent
one is clipping. I think this change is only needed when the clippingLayer is
the region. In any case, I would rather use the RenderLayers instead of the
GraphicsLayers to compute the correct location.