Bug 78936 – [chromium] Refactor CCLayerTreeHostCommon: merge scattered setTargetRenderSurface logic

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Shawn Singh]

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.

[Shawn Singh]

Created attachment 127658 [details]
Patch

tested on linux layout tests and unit tests, no apparent reggressions.

[Vangelis Kokkevis]

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.

[Shawn Singh]

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.

[James Robinson]

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.

[Shawn Singh]

(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() ?

[Shawn Singh]

Created attachment 128059 [details]
Patch

[Adrienne Walker]

Comment on attachment 128059 [details]
Patch

LGTM! Slowly but surely this function is getting cleaned up.  :)

[James Robinson]

Comment on attachment 128059 [details]
Patch

Ooh, that's nice.

[Shawn Singh]

Comment on attachment 128059 [details]
Patch

Cool, thanks for the reviews

[WebKit Review Bot]

Comment on attachment 128059 [details]
Patch

Clearing flags on attachment: 128059

Committed r108426: <http://trac.webkit.org/changeset/108426>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.