[webkit-reviews] review denied: [Bug 124887] [CSS Regions] Fix painting when the composited region has overflow:hidden : [Attachment 218410] 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 124887: [CSS Regions] Fix painting when the composited region has
overflow:hidden
https://bugs.webkit.org/show_bug.cgi?id=124887

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

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


>
LayoutTests/compositing/regions/paint-inside-composited-region-overflow-hidden-
versus-div-expected.html:30
> +			<div class="content">a b c 1. a b c 2. a b c 3. a b c
4. a b c 5. a b c 6. a b c 7. a b c 8. a b c 9. a b c A. a b c B. a b c C. a b
c D. a b c E. a b c F. a b c 0. a b c 1. a b c 2. a b c 3. </div>

Is there any reason for using the "a b c" sequences? I think it makes it hard
to follow. Just use plain text.

> Source/WebCore/rendering/RenderLayer.cpp:6768
> +	   regionClipRect = backgroundClipRect(clipRectsContext); // Region
coordinates.

I think we need to be more explicit about explaining the difference between
region / fragment / fragment container.

> Source/WebCore/rendering/RenderLayer.cpp:6778
>	   regionClipRect = regionContentBox;

I think that this whole function needs some rework. For example, this extra
step will overwrite this line from above "regionClipRect = dirtyRect".

> Source/WebCore/rendering/RenderLayer.cpp:6782
> +	   // When the layer of the region is composited, the region receives a
GraphicsLayer of its own
> +	   // so the clipping coordinates (caused by overflow:hidden) must be
relative to the
> +	   // GraphicsLayer coordinates in which the region gets painted.
> +	   if (!backing()) {

The comment is confusing, especially because you have a !backing() just after
it. I think the term "region" is now confusing as you have a
"fragmentContainer" and a "fragment". Which one are you refering to as a
"region".

Also, there is a phase where all the layers are flatten even though they have a
backing. It's used to draw the "drag" image when you drag a portion of the
page. That's why I don't think it's a good idea to check just for !backing().

> Source/WebCore/rendering/RenderLayer.cpp:6785
> +	       regionClipRect.moveBy(regionOffsetFromRoot); // Screen
coordinates.

I don't understand how this could be "screen" coordinates.