Bug 60524 – [chromium] Clean up setLayerRenderer() calls in LayerRendererChromium

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Clean up setLayerRenderer() calls in LayerRendererChromium

[James Robinson]

Created attachment 92985 [details]
Patch

[James Robinson]

Cleanup time!  This is not the only cleanup we need to do around setLayerRenderer, but it's one of them and feels safe to land in isolation.

[Kenneth Russell]

Comment on attachment 92985 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92985&action=review

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:375
> +            }

Is this the total depth of the hierarchy for each layer (i.e., optional mask / replica / replica + mask sub-layers)? If so, I think this should be factored out into a separate method and called from both here and setLayerRendererRecursive.

Also, it seems to me that conceptually these updates don't belong in a method called paintLayerContents(), but if this is the best or most efficient place to put them it's fine.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:506
> +        setLayerRendererRecursive(layer->replicaLayer());

Are these recursive calls for the mask and replica layers really necessary? Or is the depth of the hierarchies of those layers bounded? If the latter, then this should re-use the logic that's being added to paintLayerContents.

[James Robinson]

Comment on attachment 92985 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92985&action=review

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:375
>> +            }
> 
> Is this the total depth of the hierarchy for each layer (i.e., optional mask / replica / replica + mask sub-layers)? If so, I think this should be factored out into a separate method and called from both here and setLayerRendererRecursive.
> 
> Also, it seems to me that conceptually these updates don't belong in a method called paintLayerContents(), but if this is the best or most efficient place to put them it's fine.

This function does not iterate through every layer in the tree, only those that will actually be painted so it can't share the iteration logic with setLayerRendererRecursive()

It's true that this call is fairly out of place here, but the paint implementation on ContentLayerChromium depends on the LayerRendererChromium pointer being up to date.  That will get cleaned up in the course of fixing https://bugs.webkit.org/show_bug.cgi?id=58833

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:506
>> +        setLayerRendererRecursive(layer->replicaLayer());
> 
> Are these recursive calls for the mask and replica layers really necessary? Or is the depth of the hierarchies of those layers bounded? If the latter, then this should re-use the logic that's being added to paintLayerContents.

Currently mask layers have no children, mask layers, or replica layers.  Replica layers have no children or replica layers, but may have a mask layer.  The difference between this and paintLayerContents is that paintLayerContents does not recurse its children.

It's possible to write this by calling setLayerRenderer() on the mask, replica, and replica mask layers if they exist rather than recursing, but that's a little more error prone and less flexible if the tree structure gets more general in the future.

[Kenneth Russell]

Comment on attachment 92985 [details]
Patch

OK. You might want to ask Vangelis to take a look before committing.

[Vangelis Kokkevis]

Comment on attachment 92985 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92985&action=review

One comment, otherwise looks good.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:834
>      if (layer->bounds().isEmpty())

Do we still want to have this early return?  I'm not sure why isEmpty() is different than !drawsContent() .  In the second case we'll call pushPropertiesTo()  but not in the first.

[James Robinson]

(In reply to comment #6)
> (From update of attachment 92985 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92985&action=review
> 
> One comment, otherwise looks good.
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:834
> >      if (layer->bounds().isEmpty())
> 
> Do we still want to have this early return?  I'm not sure why isEmpty() is different than !drawsContent() .  In the second case we'll call pushPropertiesTo()  but not in the first.

We need the early return for content layers currently so we avoid calling updateCompositorResources() on 'empty' layers since for empty layers the m_tiler member is null (because of http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/LayerTilerChromium.cpp#L47) and updateCompositorResources() doesn't null check it.  In general it seems a little better to avoid calling updateCompositorResources() for layers that are empty since we skip them in the paint step (http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp#L365) and the draw step (http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp#L903).

[James Robinson]

Committed r86208: <http://trac.webkit.org/changeset/86208>

[James Robinson]

This seems to have changed the rendering of media/video-transformed.html, but I think the new rendering is slightly better.  I don't have time/access to do anything about it right now but if someone wants to either suppress the diff and file a bug on me or revert I would not be too mad!