There are two separate if-else satements where setTargetRenderSurface logic is used. This is unnecessarily nuanced and error-prone logic. The desired semantics are actually straightforward: - if the layer has a renderSurface (including the rootLayer), then the layer's targetRenderSurface should be its own renderSurface. - if the layer doesn't have a renderSurface, then the layer's targetRenderSurface should be the nearest ancestor surface (which is also the parent's targetRenderSurface). This patch (coming in a moment) merges this scattered logic. Two details to note: (1) It was also convenient to merge two other if-statements in this patch: if (layer->parent()) and if (layer != rootLayer) are equivalent. (2) There is still a bit of unnecessary cruft because of the rootLayer special case, but cleaning that up will be in a future patch.
Created attachment 127658 [details] Patch tested on linux layout tests and unit tests, no apparent reggressions.
Comment on attachment 127658 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127658&action=review Just one comment, otherwise looks good. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:336 > + layer->setTargetRenderSurface(layer->renderSurface()); Maybe move this to CCLayerTreeHostImpl::calculateRenderPasses() where we create the RS for the root layer since that's a special case and the RS only gets created once.
Created attachment 127917 [details] Patch This patch pulls out the setTargetRenderSurface for the rootLayer special case. However, it caused an explosion of places (in unit tests) where we have to call setTargetRenderSurface. Because I will attempt to address the rootLayer special case in the next refactoring patch, I would prefer to stick with the previous patch on this bug. Vangelis, whatever you and other reviewers decide is OK with me. I tested this patch on linux, no apparent regressions.
Comment on attachment 127917 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127917&action=review > Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:89 > root->createRenderSurface(); > + root->setTargetRenderSurface(root->renderSurface()); is there ever a case where we want to call LayerType::createRenderSurface() but not set the layer's target to the newly created surface? Can we just do it in the create() call? It would seem to save a lot of LOC in this patch.
(In reply to comment #4) > (From update of attachment 127917 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=127917&action=review > > > Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:89 > > root->createRenderSurface(); > > + root->setTargetRenderSurface(root->renderSurface()); > > is there ever a case where we want to call LayerType::createRenderSurface() but not set the layer's target to the newly created surface? Can we just do it in the create() call? It would seem to save a lot of LOC in this patch. I believe this should work (will test it). But, I feel we should rename createRenderSurface() to reflect that, so it doesn't seem like a side-effect. How about setupNewRenderSurface()? or createAndSetupRenderSurface() ?
Created attachment 128059 [details] Patch
Comment on attachment 128059 [details] Patch LGTM! Slowly but surely this function is getting cleaned up. :)
Comment on attachment 128059 [details] Patch Ooh, that's nice.
Comment on attachment 128059 [details] Patch Cool, thanks for the reviews
Comment on attachment 128059 [details] Patch Clearing flags on attachment: 128059 Committed r108426: <http://trac.webkit.org/changeset/108426>
All reviewed patches have been landed. Closing bug.