Bug 60524 - [chromium] Clean up setLayerRenderer() calls in LayerRendererChromium
Summary: [chromium] Clean up setLayerRenderer() calls in LayerRendererChromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-09 18:11 PDT by James Robinson
Modified: 2011-05-10 20:01 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.94 KB, patch)
2011-05-10 11:02 PDT, James Robinson
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2011-05-09 18:11:47 PDT
[chromium] Clean up setLayerRenderer() calls in LayerRendererChromium
Comment 1 James Robinson 2011-05-10 11:02:46 PDT
Created attachment 92985 [details]
Patch
Comment 2 James Robinson 2011-05-10 11:03:25 PDT
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.
Comment 3 Kenneth Russell 2011-05-10 11:38:10 PDT
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.
Comment 4 James Robinson 2011-05-10 12:00:31 PDT
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.
Comment 5 Kenneth Russell 2011-05-10 12:41:09 PDT
Comment on attachment 92985 [details]
Patch

OK. You might want to ask Vangelis to take a look before committing.
Comment 6 Vangelis Kokkevis 2011-05-10 18:18:53 PDT
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.
Comment 7 James Robinson 2011-05-10 19:23:50 PDT
(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).
Comment 8 James Robinson 2011-05-10 19:29:05 PDT
Committed r86208: <http://trac.webkit.org/changeset/86208>
Comment 9 James Robinson 2011-05-10 20:01:22 PDT
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!