867474 - Split BasicShadowLayerManager into a separate ClientLayerManager

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: Split ClientLayerManager

This simplifies a lot of code, and should (hopefully) improve performance since we no longer pretend to composite the layers into a clipped-out destination.

Comment 1

[Nick Cameron [:nrc]]

Comment on attachment 743983 [details] [diff] [review]
Split ClientLayerManager

Review of attachment 743983 [details] [diff] [review]:
-----------------------------------------------------------------

Comments on everything except Client and Basic layers, which Splinter is getting its knickers in a twist about. Manual comments coming soon.

Please make ToClientLayer an instance method on Layer rather than global.

::: gfx/layers/Makefile.in
@@ +42,5 @@
> +		ClientTiledThebesLayer.cpp \
> +		ClientThebesLayer.cpp \
> +		ClientCanvasLayer.cpp \
> +		ClientColorLayer.cpp \
> +		ClientImageLayer.cpp \

spaces, not tabs, please

::: gfx/layers/basic/BasicTiledThebesLayer.cpp
@@ +112,3 @@
>  {
> +  LayerManager::DrawThebesLayerCallback callback = 
> +    ClientManager()->GetThebesLayerCallback(); 

trailing whitespace x2

::: widget/gonk/nsWindow.cpp
@@ +559,5 @@
>                                                     ScreenRotation(EffectiveScreenRotation()));
> +        } else if (mLayerManager->GetBackendType() == LAYERS_CLIENT) {
> +            ClientLayerManager* manager =
> +                static_cast<ClientLayerManager*>(mLayerManager.get());
> +            manager->SetDefaultTargetConfiguration(mozilla::layers::BUFFER_NONE, 

trailing whitespace

@@ +560,5 @@
> +        } else if (mLayerManager->GetBackendType() == LAYERS_CLIENT) {
> +            ClientLayerManager* manager =
> +                static_cast<ClientLayerManager*>(mLayerManager.get());
> +            manager->SetDefaultTargetConfiguration(mozilla::layers::BUFFER_NONE, 
> +                                                   ScreenRotation(EffectiveScreenRotation()));

this is kind of gross, should we lift SetDefaultTargetConfiguration to a super class or is it worth swallowing the gross-ness?

::: widget/gtk2/nsWindow.cpp
@@ -2130,5 @@
> -#endif
> -        nsBaseWidget::AutoLayerManagerSetup
> -          setupLayerManager(this, ctx, mozilla::layers::BUFFER_NONE);
> -
> -        listener->PaintWindow(this, region, 0);

we don't need this any more? It doesn't seem related to the split.

::: widget/xpwidgets/PuppetWidget.cpp
@@ +569,5 @@
>  #endif
>  
>      if (mozilla::layers::LAYERS_D3D10 == mLayerManager->GetBackendType()) {
>        mAttachedWidgetListener->PaintWindow(this, region, 0);
> +    } else if (mozilla::layers::LAYERS_CLIENT == mLayerManager->GetBackendType()) {

nit: Yoda programming, please do not have

Comment 2

[Matt Woodrow (:mattwoodrow)]

(In reply to Nick Cameron [:nrc] [PTO until 20th May] from comment #1)
> @@ +560,5 @@
> > +        } else if (mLayerManager->GetBackendType() == LAYERS_CLIENT) {
> > +            ClientLayerManager* manager =
> > +                static_cast<ClientLayerManager*>(mLayerManager.get());
> > +            manager->SetDefaultTargetConfiguration(mozilla::layers::BUFFER_NONE, 
> > +                                                   ScreenRotation(EffectiveScreenRotation()));
> 
> this is kind of gross, should we lift SetDefaultTargetConfiguration to a
> super class or is it worth swallowing the gross-ness?

I *think* the BasicLayerManager path can actually go away, since we always have remote layers with b2g. I won't do that right now though, trying to minimize chances of regressions.

> 
> ::: widget/gtk2/nsWindow.cpp
> @@ -2130,5 @@
> > -#endif
> > -        nsBaseWidget::AutoLayerManagerSetup
> > -          setupLayerManager(this, ctx, mozilla::layers::BUFFER_NONE);
> > -
> > -        listener->PaintWindow(this, region, 0);
> 
> we don't need this any more? It doesn't seem related to the split.

Yeah, this is the same as the other widget changes, doing nothing when we get there with a ClientLayerManager.

> 
> ::: widget/xpwidgets/PuppetWidget.cpp
> @@ +569,5 @@
> >  #endif
> >  
> >      if (mozilla::layers::LAYERS_D3D10 == mLayerManager->GetBackendType()) {
> >        mAttachedWidgetListener->PaintWindow(this, region, 0);
> > +    } else if (mozilla::layers::LAYERS_CLIENT == mLayerManager->GetBackendType()) {
> 
> nit: Yoda programming, please do not have

I might just leave this, as it's following local style.

Comment 3

[Nick Cameron [:nrc]]

Manual review comments:

Maybe CopyableCanvasLayer should be in a backend neutral file rather than BasicCanvasLayer? Also, please add a comment.

+++ b/gfx/layers/client/ClientContainerLayer.h
@@ -121,128 +112,167 @@ ContainerRepositionChild(Layer* aChild,
+  for (Layer* l = aLayer->GetParent(); l; l = l->GetParent()) {
+    if (l->GetContentFlags() & Layer::CONTENT_OPAQUE)
+      return true;
+    }
+ return false;

indent x2

Why is it OK to get rid of ContainerComputeEffectiveTransforms?

You've removed all the HasShadow() checks, AIUI, this not only checks that we're a shadowable layer, but that our shadow counterpart has been created. Do we still need such a check to check that the CompositeLayer has been created?

Can we merge ShadowableLayer and ClientLayer? Maybe as a follow up.

Comment 4

[Matt Woodrow (:mattwoodrow)]

(In reply to Nick Cameron [:nrc] [PTO until 20th May] from comment #3)
> Why is it OK to get rid of ContainerComputeEffectiveTransforms?

This is only used for non-shadow layers, when we're shadowing we want to use DefaultComputeEffectiveTransform so that our decisions match what the compositor does.


> 
> You've removed all the HasShadow() checks, AIUI, this not only checks that
> we're a shadowable layer, but that our shadow counterpart has been created.
> Do we still need such a check to check that the CompositeLayer has been
> created?

No, this was to check if we were shadowing at all. MaybeCreateShadowFor (which I've just renamed to CreateShadowFor since it's unconditional with ClientLayer) has an NS_ABORT_IF_FALSE if the shadow creation fails.

Previously we would often create a BasicShadowLayerManager (for the widget) but not actually do any shadowing. This was the case on all platforms not using OMTC. In this case all the layers created were BasicShadowable***Layers, but didn't have actual Shadows.

With ClientLayerManager, we are always shadowing, so we don't need to check.

> 
> Can we merge ShadowableLayer and ClientLayer? Maybe as a follow up.

Maybe? It doesn't seem unreasonable to keep them separate as one is an interface, the other is the implementation.

Comment 5

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af9e5dba68e8

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/af9e5dba68e8