Bug 78936

Summary: [chromium] Refactor CCLayerTreeHostCommon: merge scattered setTargetRenderSurface logic
Product: WebKit Reporter: Shawn Singh <shawnsingh>
Component: Layout and RenderingAssignee: 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 Flags
Patch
none
Patch
none
Patch none

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 6 Shawn Singh 2012-02-21 15:33:52 PST
Created attachment 128059 [details]
Patch
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.