865033 - CompositorParent should reference a Compositor not a LayerManager

Status: NEW

(Core :: Graphics: Layers, defect)

Description

[Nick Cameron [:nrc]]

Pretty much everything CompositorParent does on its LayerManager is actually Compositor stuff, we could cut out a whole bunch of redirect methods by having CompositorParent hold a reference to the Compositor instead of the LayerManager and let AsyncCompositionManager keep the LayerManager reference.

Comment 1

[Nicolas Silva [:nical]]

Yeah it's kinda silly to have all these methods of the form:

Bar Foo() { return mCompositor->Foo() }

What's AsyncCompositionManager ?

Comment 2

[Nicolas Silva [:nical]]

> What's AsyncCompositionManager ?

oh nevermind, it's in bug 861727

Comment 3

[Nicolas Silva [:nical]]

It would be nice if LayerManager was just a LayerTree, (just a data structure) and the all of the logic would be in AsyncCompositionManager. That would be a step in the direction of reusing  code on both the compositor and content sides.

Comment 4

[Nick Cameron [:nrc]]

(In reply to Nicolas Silva [:nical] from comment #3)
> It would be nice if LayerManager was just a LayerTree, (just a data
> structure) and the all of the logic would be in AsyncCompositionManager.
> That would be a step in the direction of reusing  code on both the
> compositor and content sides.

I think there are a lot of directions we could go, and some are mutually exclusive - like if we end up with just one layer type (or two) on the compositor, I'm not sure how much we could share with content.

I kind of wanted to move AsyncCompositionManager into LayerManager, but maybe moving stuff out of LayerManager into AsyncCompositionManager is better, I don't know.

Comment 5

[Nicolas Silva [:nical]]

Well, I generally like when classes don't do too many things, and also like to separate logic from data.
But After giving some thought about the LayerManager case, it probably doesn't have much to do as a data structure since the interesting stuff for that is in how ContainerLayer work. So it probably doesn't deserve it's own class for this aspect after all.

(In reply to Nick Cameron [:nrc] from comment #4)
> I think there are a lot of directions we could go, and some are mutually
> exclusive - like if we end up with just one layer type (or two) on the
> compositor, I'm not sure how much we could share with content.

I guess it depends on how much logic we move from layers to components like compositables

Comment 6

[Nick Cameron [:nrc]]

(Mostly this is notes to self)

Plan:

1. Figure out what to do about AndroidCompositorParent
2. Move mRenderBounds and its uses from LayerManagerComposite to Compositor
3. Split ShadowLayersUpdated and move most of it to AsyncCompositionManager (see also NotifyShadowTreeTransaction)
4. Fix initialisation (AllocPLayerTransaction)
5. Composite, CanComposite, and ComposeToTarget - not sure what to do here
6. Change uses of mLayerManager->GetCompositor(), change fields, tidy up remaining uses.

Comment 7

[Nick Cameron [:nrc]]

Attachment: Part 1 remove GetLayerManager

Comment 8

[Nick Cameron [:nrc]]

Attachment: Part 2 add mCompositor

This is a bit yuck to have both a layer manager and a compositor, but eventually layer manager should go. This patch blocks bug 946958, which in turn makes the rest of this bug easier to resolve (although I haven't started that yet).

Comment 9

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8344387 [details] [diff] [review]
Part 2 add mCompositor

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +728,4 @@
>  #endif
>      }
>  
> +    if (!compositor) {

We could only hit this if we got passed an invalid layers backend type, we should just assert that this doesn't happen.

Comment 10

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41839e4026bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d1a4d83eccf

Comment 11

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/45193a6d7994 for extreme displeasure on the part of reftest-ipc

Comment 12

[Nick Cameron [:nrc]]

try push: https://tbpl.mozilla.org/?tree=Try&rev=d08bbd03b851

Comment 13

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/96fd6d7d2f8c
https://hg.mozilla.org/integration/mozilla-inbound/rev/8927d5297df1

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/96fd6d7d2f8c
https://hg.mozilla.org/mozilla-central/rev/8927d5297df1