[webkit-reviews] review denied: [Bug 76486] RenderLayer ClipRect not accounting for transforms : [Attachment 126895] ConvertToLayerCoords fixed for general transforms

[bugzilla-daemon at webkit.org]

Simon Fraser (on vacation until 2-14) <simon.fraser at apple.com> has denied Shawn
Singh <shawnsingh at chromium.org>'s request for review:
Bug 76486: RenderLayer ClipRect not accounting for transforms
https://bugs.webkit.org/show_bug.cgi?id=76486

Attachment 126895: ConvertToLayerCoords fixed for general transforms
https://bugs.webkit.org/attachment.cgi?id=126895&action=review

------- Additional Comments from Simon Fraser (on vacation until 2-14)
<simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=126895&action=review


> Source/WebCore/rendering/RenderLayer.cpp:1335
> +    if (useTransforms && m_transform)
> +	   location = m_transform->mapPoint(location);

This doesn't do the right thing with 3D transform, because it will flatten the
point at every layer. The code would need to use TransformState to get this
right (or map points via RenderObjects).

I still think the right approach is to avoid calling convertToLayerCoords
across transform boundaries (maybe just 3D boundaries)? I'd have to wrap my
head around what that implies for clip rects though.

> Source/WebCore/rendering/RenderLayer.cpp:1354
> +    FloatPoint p1 = FloatPoint(rect.minXMinYCorner());
> +    FloatPoint p2 = FloatPoint(rect.maxXMinYCorner());
> +    FloatPoint p3 = FloatPoint(rect.minXMaxYCorner());
> +    FloatPoint p4 = FloatPoint(rect.maxXMaxYCorner());
> +
> +    convertToLayerCoordsInternal(ancestorLayer, p1, true);
> +    convertToLayerCoordsInternal(ancestorLayer, p2, true);
> +    convertToLayerCoordsInternal(ancestorLayer, p3, true);
> +    convertToLayerCoordsInternal(ancestorLayer, p4, true);
> +
> +    // Create a LayoutRect from the four converted corners.
> +    float xmin = min(min(p1.x(), p2.x()), min(p3.x(), p4.x()));
> +    float ymin = min(min(p1.y(), p2.y()), min(p3.y(), p4.y()));
> +    float xmax = max(max(p1.x(), p2.x()), max(p3.x(), p4.x()));
> +    float ymax = max(max(p1.y(), p2.y()), max(p3.y(), p4.y()));

I think it would be better to write methods that take FloatQuads to avoid
calling things 4 times.

> Source/WebCore/rendering/RenderLayer.h:429
> +    void convertToLayerCoords(const RenderLayer* ancestorLayer, LayoutPoint&
location, bool useTransforms = false) const;

The boolean argument makes the calling code less readable. It would be better
to use an enum or have two separate methods.