Bug 88953 – [chromium] Refactor CCLayerTreeHostCommon: remove root layer special case initialization

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

[Shawn Singh]

In CCLayerTreeHostCommonTest some of the test cases don't actually initialize renderSurfaceLayerList the same way that the code actually does.  This means the tested scenarios are ever-so-subtly different than practical scenarios.  Furthermore, its been a sore point against code elegance of CCLTH and CCLTHi that the root layer has to initialize junk outside of calcDrawTransforms.

It would be very nice to remove this special casing, so that we can blindly call calculateDrawTransforms for a root layer that may or may not have an existing renderSurface, and receives an empty renderSurfaceLayerList instead of it already being populated with one awkward item.

[Dana Jansens]

/upvote

[Adrienne Walker]

I think that CCLTH(i) has this special root layer code because only CCLTH(i) knows about the viewport size.

Were you thinking about just making a different calcDraw entry point where you could pass in the viewport and then abstracting all that surface initialization into CCLTHCommon?

Cleaning up this code would be great.  :)

[Shawn Singh]

(In reply to comment #2)
> I think that CCLTH(i) has this special root layer code because only CCLTH(i) knows about the viewport size.
> 
> Were you thinking about just making a different calcDraw entry point where you could pass in the viewport and then abstracting all that surface initialization into CCLTHCommon?
> 
> Cleaning up this code would be great.  :)

As a first pass, yeah thats what I had in mind.  In my own words, it would be moving the special case stuff into CCLayerTreeHostCommon.

In the best case, it eventually becomes clear how to generalize without any "if layer==rootLayer" clauses, but that's probably not possible.

[Shawn Singh]

Created attachment 154166 [details]
Patch

unit tests and manual tests seem OK. I will do more thorough tests of debug mode and release mode after 91417 lands and this patch gets rebased to it.

[Shawn Singh]

Please note that there might be a part 2 for this, if it works out, which will try to reduce the root layer special-ness from inside of calcDrawTransforms.   This current patch just removes the special-ness from the outside perspective.

[Adrienne Walker]

Comment on attachment 154166 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154166&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:470
> +        CCLayerTreeHostCommon::calculateDrawTransforms(rootLayer, deviceViewportSize(), m_deviceScaleFactor, updateList, layerRendererCapabilities().maxTextureSize);

style nit: output parameters last, please.

> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:956
> +    // FIXME: creating a renderSurface on renderSurface1 might not have the intended effect here. do we mean to setForceRenderSurface(true) ??
> +    //        (same FIXME exists in 1 more location.)

Can you investigate both of these tests?

[Shawn Singh]

Created attachment 154403 [details]
Patch

[Shawn Singh]

(In reply to comment #6)
> (From update of attachment 154166 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154166&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:470
> > +        CCLayerTreeHostCommon::calculateDrawTransforms(rootLayer, deviceViewportSize(), m_deviceScaleFactor, updateList, layerRendererCapabilities().maxTextureSize);
> 
> style nit: output parameters last, please.

done

> 
> > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:956
> > +    // FIXME: creating a renderSurface on renderSurface1 might not have the intended effect here. do we mean to setForceRenderSurface(true) ??
> > +    //        (same FIXME exists in 1 more location.)
> 
> Can you investigate both of these tests?

done!

[Adrienne Walker]

Comment on attachment 154403 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154403&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:394
> +// Should be called just before the recursive calculateDrawTransformsInternal().
> +template<typename LayerType, typename LayerList>
> +void setupRootLayerAndSurfaceForRecursion(LayerType* rootLayer, LayerList& renderSurfaceLayerList, const IntSize& deviceViewportSize)

I had one last thought.  Rather than commenting that this needs to be called at a certain time, why don't you just make it call calculateDrawTransformsInternal directly.  Then, it could also do all the setup that you duplicate in the calculateDrawTransforms functions.

[Shawn Singh]

(In reply to comment #9)
> (From update of attachment 154403 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154403&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:394
> > +// Should be called just before the recursive calculateDrawTransformsInternal().
> > +template<typename LayerType, typename LayerList>
> > +void setupRootLayerAndSurfaceForRecursion(LayerType* rootLayer, LayerList& renderSurfaceLayerList, const IntSize& deviceViewportSize)
> 
> I had one last thought.  Rather than commenting that this needs to be called at a certain time, why don't you just make it call calculateDrawTransformsInternal directly.  Then, it could also do all the setup that you duplicate in the calculateDrawTransforms functions.

In part 2 of this, I'm expecting this function to go away.  So, would it be OK to keep it as-is for this patch?  If it turns out not to be possible to totally remove the root layer setup, then I'll change it at that time to this idea you're proposing - are you OK with that?

[Adrienne Walker]

Comment on attachment 154403 [details]
Patch

R=me.  Ok, sounds good.

[Shawn Singh]

Committed r123654: <http://trac.webkit.org/changeset/123654>