[webkit-reviews] review denied: [Bug 70634] Mark GraphicsLayers as opaque when possible : [Attachment 114066] move the RenderObject walk

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Dana Jansens
<danakj at chromium.org>'s request for review:
Bug 70634: Mark GraphicsLayers as opaque when possible
https://bugs.webkit.org/show_bug.cgi?id=70634

Attachment 114066: move the RenderObject walk
https://bugs.webkit.org/attachment.cgi?id=114066&action=review

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


I don't like that this patch is getting more and more complex. I think you
should start by getting the easy cases working first (e.g. the layer's
RenderObject is known to be opaque), and then progress to doing RenderObject
walks in later patches. The IntRectEdge code is a large amount of new code. It
should be tested on all platforms, not just Chromium, but I'd prefer that it
just not be included in the initial patch.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2693
> +    const LayoutRect& bounds = borderBoundingBox();
> +
> +    // We do not consider overflow bounds here, and if we get a query rect
on the overflow area for the
> +    // render object (i.e. outside the border bounds), then we just return
false. This does not mean the area
> +    // in the rect is neccessarily non-opaque, and may be a false negative.
> +    // NOTE: Because of this, we don't need to look for
style()->boxShadow(), since that is always outside of the border
> +    // bounds.
> +    if (!bounds.contains(rect))
> +	   return false;
> +
> +    // FIXME: Check border-image. With 'fill' it can be drawn behind the
contents area and could be opaque.
> +    // Currently we check background color only, but border-image could make
the rect opaque even though the
> +    // background color is not.
> +
> +    const Color color =
style()->visitedDependentColor(CSSPropertyBackgroundColor);
> +    if (!color.isValid() || color.hasAlpha())
> +	   return false;
> +
> +    return true;

This is contrary to the direction I expressed a preference for. I think the
methods should default to returning false, and return true only in cases they
know they can give the correct answer for.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2740
> +    return true;

Ditto.

> Source/WebCore/rendering/RenderImage.cpp:432
> +    return true;

Ditto.

> Source/WebCore/rendering/RenderLayer.h:794
> +    // This is a mapping from RenderObject* to IntRect, which is the opaque
rect for each RenderObject
> +    // belonging to this RenderLayer.
> +    HashMap<const RenderObject*, IntRect> m_opaqueRectForRenderObject;
> +
> +    // This list contains bounding box rectangles of opaque RenderObjects in
the layer.
> +    OwnPtr<Vector<IntRectEdge> > m_opaqueRectsList;

It seems wrong to be adding these members to RenderLayer, when there's no
contract about who updates them. A caller has no way to know if opaqueRects()
will be return a stale list. There's just an implicit contract that
RenderLayerBacking will update the rects at the right time.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:474
> +    LayoutPoint ancestorRelOffset;
> +    layer->convertToLayerCoords(ancestorLayer, ancestorRelOffset);

Have you tested composited layers that contain 2d-transformed layers? It seems
like this convertToLayerCoords() could cross a transform boundary, which is
bad.

> Source/WebCore/rendering/RenderObject.cpp:2703
> +void RenderObject::updateOpaqueRect()

I don't know why this has to be on RenderObject, since it's a layer thing.

> Source/WebCore/rendering/RenderObject.h:1004
> +	   updateOpaqueRect();

This is going to be way too expensive. This is a hot function (hence the
inline).