|Summary:||[chromium] Refactor CCLayerTreeHostCommon: merge scattered setTargetRenderSurface logic|
|Product:||WebKit||Reporter:||Shawn Singh <shawnsingh>|
|Component:||Layout and Rendering||Assignee:||Shawn Singh <shawnsingh>|
|Severity:||Normal||CC:||enne, jamesr, vangelis, webkit.review.bot|
|Version:||528+ (Nightly build)|
Description Shawn Singh 2012-02-17 14:15:56 PST
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.
Comment 1 Shawn Singh 2012-02-17 14:19:26 PST
Created attachment 127658 [details] Patch tested on linux layout tests and unit tests, no apparent reggressions.
Comment 2 Vangelis Kokkevis 2012-02-20 18:28:47 PST
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.
Comment 3 Shawn Singh 2012-02-20 22:13:23 PST
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 4 James Robinson 2012-02-21 10:08:14 PST
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.
Comment 5 Shawn Singh 2012-02-21 10:32:57 PST
(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() ?
Comment 7 Adrienne Walker 2012-02-21 15:43:26 PST
Comment on attachment 128059 [details] Patch LGTM! Slowly but surely this function is getting cleaned up. :)
Comment 8 James Robinson 2012-02-21 16:03:32 PST
Comment on attachment 128059 [details] Patch Ooh, that's nice.
Comment 9 Shawn Singh 2012-02-21 16:47:11 PST
Comment on attachment 128059 [details] Patch Cool, thanks for the reviews
Comment 10 WebKit Review Bot 2012-02-21 17:49:23 PST
Comment on attachment 128059 [details] Patch Clearing flags on attachment: 128059 Committed r108426: <http://trac.webkit.org/changeset/108426>
Comment 11 WebKit Review Bot 2012-02-21 17:49:28 PST
All reviewed patches have been landed. Closing bug.