Summary: | [chromium] Refactor CCLayerTreeHostCommon: merge scattered setTargetRenderSurface logic | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Shawn Singh <shawnsingh> | ||||||||
Component: | Layout and Rendering | Assignee: | Shawn Singh <shawnsingh> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | enne, jamesr, vangelis, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Shawn Singh
2012-02-17 14:15:56 PST
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. |