[Webkit-unassigned] [Bug 70634] Mark GraphicsLayers as opaque when possible

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=70634


Simon Fraser (smfr) <simon.fraser at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #114066|review?                     |review-
               Flag|                            |




--- Comment #43 from Simon Fraser (smfr) <simon.fraser at apple.com>  2011-11-09 11:22:24 PST ---
(From update of attachment 114066)
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).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.