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

[bugzilla-daemon at webkit.org]

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





--- Comment #35 from Dana Jansens <danakj at chromium.org>  2011-11-01 13:51:54 PST ---
(From update of attachment 113198)
View in context: https://bugs.webkit.org/attachment.cgi?id=113198&action=review

Thanks for the comments. The process for determining if a GraphicsLayer is opaque is still very simplified in this patch (Does some RenderObject fully occlude the GraphicsLayer?) but this is far from sufficient.  The goal is to determine if the union of opaque ROs fully occlude the GL. More comments inline.

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:2678
>> +        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.

I do agree, and I thought that was what I had done.  Currently this function returns true only if the query rect is entirely over the background color, and the background color is opaque.

It would be possible to do this with early exit returning true instead (though I think there would have to be only a single early exit then, since it is basically an AND of all the conditions to get to the true statement.

if (contains && colorvalid && !colorhasalpha)
    return true;
return false;

Would that be preferable to you?  Or I could include a comment indicating this intent also.

>> Source/WebCore/rendering/RenderLayerBacking.cpp:244
>> +    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.

The reason we collect rects here rather than computing as soon as we find a new rect, is that in order to determine if the union of the rects cover the GraphicsLayer in some reasonable (polynomial) amount of time, we will need to do something like sorting the rects.  For this reason I collect them into a Vector which can be easily sorted and then tested for covering.

It doesn't look needed right now, since the test is simply checking for a single RO that covers the GraphicsLayer, but that's not the final intent, as that misses a lot (A composited div with two divs in it, etc).

>> 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.

Fixing.

>> 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.

Fixing.

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:458
>> +            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.

Can it return a decendant's RenderLayer?  The intent here was to prevent looking at any RO that are inside another RL (since we are walking the RLs already in calculateCompositedBounds.  If RL->renderer() points to a subtree of RO that all belong to the single RL, then we can remove this check. Otherwise what should we do instead to  not look at a RO twice.

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:468
>> +        }
> 
> 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).

Yes, because the intent is to determine if the union of opaque ROs cover the entire GraphicsLayer. To do so, we need to find them all since they can be in arbitrary positions within the GraphicsLayer, unless there is some underlying structure here that I can take advantage of and am not right now?  As I understood, a non-opaque RO could still be above/below some (set of) opaque RO belonging to the current or other RenderLayers in the GraphicsLayer.

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:472
>> +        // 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.

The comment meant to say that such a RO will be composited and thus be in a different GraphicsLayer, which implies a different RenderLayer. I will fix that.

>> Source/WebCore/rendering/RenderObject.h:749
>> +    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.

Will improve for clarity by refering to the content rect in the names, and shorten the comments.

foregroundContentsAreOpaqueInRect()
backgroundContentsAreIsOpaqueInRect()

>> LayoutTests/compositing/opaque/borders.html:67
>> +    window.addEventListener('load', doTest, false);
> 
> You don't need waitUntilDone if your test finishes inside the load event.

Thanks.

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