[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 #113198|                            |review-
               Flag|                            |




--- Comment #34 from Simon Fraser (smfr) <simon.fraser at apple.com>  2011-11-01 12:08:48 PST ---
(From update of attachment 113198)
View in context: https://bugs.webkit.org/attachment.cgi?id=113198&action=review

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2678
> +    // 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.
> +
> +    const Color color = style()->visitedDependentColor(CSSPropertyBackgroundColor);
> +    if (!color.isValid() || color.hasAlpha())
> +        return false;

I think these new methods need to be written conservatively; they should err towards returning 'false' unless known to be opaque. Then the FIXME becomes harmless.

> Source/WebCore/rendering/RenderLayerBacking.cpp:244
> +    bool opaque = false;
> +    for (size_t i = 0; i < opaqueRegions.size(); ++i)
> +        if (opaqueRegions[i].contains(layerBounds)) {
> +            opaque = true;
> +            break;
> +        }
> +    m_graphicsLayer->setContentsOpaque(opaque);

Rather than collect rects like this, why not just have calculateCompositedBounds() have an out param that returns whether the layers are known to be fully opaque in the given bounds? Then you could avoid extra work as soon as you find out that it is not.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:452
> +void recursiveCollectOpaqueRenderObjects(const RenderLayer* layer, const RenderObject* renderer, const RenderBoxModelObject* container, Vector<LayoutRect>& opaqueRegions, LayoutPoint totalRelOffset)

We prefer static methods to using anonymous namespaces.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:455
> +        const RenderBoxModelObject* boxmodel = static_cast<const RenderBoxModelObject*>(renderer);

Don't like boxmodel as a variable name. It's a boxModelObject.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:458
> +        if (boxmodel->layer() && boxmodel->layer() != layer)
> +            return; // We've left the render layer.

This is wrong. RenderBoxModelObject::layer() only returns a RL if the RO has one; it never returns the layer for some ancestor RO.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:468
> +        if (boxmodel->isOpaqueInRect(area)) {
> +            area.move(containerOffset);
> +            opaqueRegions.append(area);
> +        }

The approach taken here is doing an exhaustive walk of all the RO even when one is found to be non-opaque. This could get very expensive. You need to be able to bail early if you find non-opaque content. Ideally we'd do this testing at the same time as some other tree-walk (maybe you cache it during painting).

> Source/WebCore/rendering/RenderLayerCompositor.cpp:472
> +        // Containers are not always parents, so in general it is not okay to just assume we are the container for our
> +        // child because we are its parent. However, if we are not its container (for e.g. it is positioned absolute and has
> +        // a different container), then it will also be composited independently, and have its own RenderLayer/GraphicsLayer.

The comment (and maybe the code) is confused. Not every RL has a GL. A GL can render a subtree of RLs.

> Source/WebCore/rendering/RenderObject.h:749
> +    // Returns true if the foreground within the query rect will be filled completely opaque by the RenderObject.
> +    // This checks only within the contents area of the RenderObject, so if you want to test the entire surface
> +    // area, you need to test boxDecorationsAreOpaqueInRect() as well.
> +    virtual bool foregroundIsOpaqueInRect(const IntRect&) const { return false; }
> +
> +    // Returns true if the background within the query rect will be filled completely opaque by the RenderObject.
> +    // This checks only within the contents area of the RenderObject, so if you want to test the entire surface
> +    // area, you need to test boxDecorationsAreOpaqueInRect() as well.
> +    virtual bool backgroundIsOpaqueInRect(const IntRect&) const { return false; }
> +
> +    // Returns true if the background within the query rect would be filled completely opaque (assuming the content area was opaque).
> +    // This returns true if the query rect is entirely in the contents area of the render object, as the opaque result depends
> +    // only on the foregroundIsOpaqueInRect() or backgroundIsOpaqueInRect() result. A true result from this function should
> +    // *never* be used without also consulting at least one of the previously mentioned functions as well.
> +    virtual bool boxDecorationsAreOpaqueInRect(const IntRect&) const { return false; }

The verbosity of the comments indicates that there's something wrong with the code factoring and/or the method names.

> LayoutTests/compositing/opaque/borders.html:67
> +      layoutTestController.waitUntilDone();
> +    }
> +
> +    function doTest()
> +    {
> +      if (window.layoutTestController) {
> +        document.getElementById('layers').innerText = layoutTestController.layerTreeAsText();
> +        layoutTestController.notifyDone();
> +      }
> +    }
> +    
> +    window.addEventListener('load', doTest, false);

You don't need waitUntilDone if your test finishes inside the load event.

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