Open Bug 865033 Opened 7 years ago Updated 6 years ago

CompositorParent should reference a Compositor not a LayerManager

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

People

(Reporter: nrc, Assigned: nrc)

References

Details

(Whiteboard: [leave open])

Attachments

(2 files)

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.
Yeah it's kinda silly to have all these methods of the form:

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

What's AsyncCompositionManager ?
> What's AsyncCompositionManager ?

oh nevermind, it's in bug 861727
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.
(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.
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
(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.
Whiteboard: [leave open]
Attachment #8344386 - Flags: review?(matt.woodrow)
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).
Assignee: nobody → ncameron
Attachment #8344387 - Flags: review?(matt.woodrow)
Attachment #8344386 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8344387 - Flags: review?(matt.woodrow) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/45193a6d7994 for extreme displeasure on the part of reftest-ipc
Depends on: 948555
You need to log in before you can comment on or make changes to this bug.