861727 - Move layers stuff from CompositorParent to LayerManagerComposite

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nick Cameron [:nrc]]

CompositorParent has turned into a bit of a dumping ground for anything async. It should just be responsible for dealing with IPC stuff for the PCompositor protocol. In particular it does a whole bunch of layers manipulation stuff that would be better off in LayerManagerComposite. I propose moving it all.

Things to move:

SetPanZoomControllerForLayerTree
ShadowLayersUpdated
TransformFixedLayers
TransformShadowTree
SyncViewportInfo
mXScale, mYScale
mScrollOffset
mContentRect

And anything else to do with APZC, OMTA, layers, etc.

What do you think about this idea?

I guess the downside is that LayerManagerComposite will get bigger, but that seems more appropriate than CompositorParent.

Comment 1

[Chris Lord [:cwiiis]]

I like the idea of having things in the right place, but I also like that CompositorParent is a pretty small file that holds all of the async stuff in an easily findable place - I wonder if we could encapsulate these things in a class of its own, rather than bundling it with another large class? AsyncCompositionManager or something?

Comment 2

[Nick Cameron [:nrc]]

Attachment: Move layers stuff from CompositorParent to AsyncCompositionManager

I'm still not 100% convinced that doing this is better than moving to LayerManagerComposite, but it is definitely better than the current situation. I hope this opens the door for more refactoring of this ugly duckling so it can become the beautiful swan it secretly hopes to be.

Comment 3

[Nick Cameron [:nrc]]

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

Comment 4

[Nick Cameron [:nrc]]

Attachment: Move layers stuff from CompositorParent to AsyncCompositionManager

rebased and reorganised a little

Comment 5

[Nick Cameron [:nrc]]

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

Comment 6

[Nick Cameron [:nrc]]

Attachment: Move layers stuff from CompositorParent to AsyncCompositionManager

OK, third time lucky. This one should actually work and I think is actually slightly nicer.

These classes are still a bit crufty, I think they still need tidying up (see e.g., bug 865033), but I would rather do that as a follow up and just do a split here.

Comment 7

[Nick Cameron [:nrc]]

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

Comment 8

[Benoit Girard (:BenWa)]

Comment on attachment 741167 [details] [diff] [review]
Move layers stuff from CompositorParent to AsyncCompositionManager

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

AFAIK this is for the most part preserve the original behavior so it's fine by me.

Cwiiis you had a stronger opinion with the code moves then I did so I'm leaving you the option to review this, or to pass if you don't have time.

::: gfx/layers/ipc/CompositorParent.h
@@ +3,4 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#ifndef GFX_AsyncCompositionManager_H

uppercase

@@ +61,5 @@
> +    , mLayerManager(aManager)
> +    , mIsFirstPaint(false)
> +    , mLayersUpdated(false)
> +    , mReadyForCompose(true)
> +  {}

MOZ_COUNT_CTOR

@@ +76,5 @@
>    // Sample transforms for layer trees.  Return true to request
>    // another animation frame.
>    bool TransformShadowTree(TimeStamp aCurrentFrame);
> +
> +  // Calucluates the correct rotation and applies the transform to

typo

@@ +78,5 @@
>    bool TransformShadowTree(TimeStamp aCurrentFrame);
> +
> +  // Calucluates the correct rotation and applies the transform to
> +  // our layer manager
> +  void SetRotation();

This isn't a setter. Maybe ComputeRotation? Couldn't this be called from Updated()?

@@ +88,5 @@
> +    mLayersUpdated = true;
> +    mTargetConfig = aTargetConfig;
> +  }
> +
> +  // Returns true if our current orientation does not match aOrientation.

This comment isn't informative.

@@ +94,5 @@
> +  {
> +    return mTargetConfig.orientation() != aOrientation;
> +  }
> +
> +  bool ReadyForCompose() { return mReadyForCompose; }

What does ReadyForCompose tell you about AsyncCompositionManager?

@@ +153,5 @@
>    float mYScale;
>    nsIntPoint mScrollOffset;
>    nsIntRect mContentRect;
>  
> +  nsRefPtr<LayerManagerComposite> mLayerManager;

This is owned by CompositorParent so I think we're ok with cycles? I asked for MOZ_COUNT_CTOR just in case.

Comment 9

[Chris Lord [:cwiiis]]

Comment on attachment 741167 [details] [diff] [review]
Move layers stuff from CompositorParent to AsyncCompositionManager

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

I don't have anything useful to add to this, the code moves look sane to me. Will remove my review rather than r+ as I've not given this a thorough look-over, but I broadly approve :)

I would request that you build an android apk and play around to make sure there are no regressions before pushing this - especially with fixed position content (I have a load of test pages at http://chrislord.net/files/mozilla/)

Comment 10

[Nick Cameron [:nrc]]

(In reply to Chris Lord [:cwiiis] from comment #9)
> Comment on attachment 741167 [details] [diff] [review]
> Move layers stuff from CompositorParent to AsyncCompositionManager
> 
> Review of attachment 741167 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't have anything useful to add to this, the code moves look sane to me.
> Will remove my review rather than r+ as I've not given this a thorough
> look-over, but I broadly approve :)
> 
> I would request that you build an android apk and play around to make sure
> there are no regressions before pushing this - especially with fixed
> position content (I have a load of test pages at
> http://chrislord.net/files/mozilla/)

SO, I'm not entirely sure what some of those tests are meant to do, some look buggy to me, but I checked that I get exactly the same behaviour with and without the patch. I played around on a bunch of websites and I think everything is looking right.

Comment 11

[Nick Cameron [:nrc]]

Attachment: Move layers stuff from CompositorParent to AsyncCompositionManager

Renased this about a million times, it has been utter hell. I think I picked a really bad time to do this. Ho-hum. Anyway, carrying r=BenWa and I'm going to try and finally land this stupid thing.

Comment 12

[Nick Cameron [:nrc]]

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

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 14

[Kartikaya Gupta (email:[e-mail])]

I have tried updating to include this patch yet, but looking at the diff I believe it breaks the SyncFrameMetrics call path I added in [1]. The part that I think is broken specifically is that in my patch I override CompositorParent::SyncFrameMetrics in android/widget/nsWindow.cpp (which has an AndroidCompositorParent subclass of CompositorParent). I guess MOZ_OVERRIDE is a completely useless decoration because it doesn't break compiles if the superclass changes?

[1] https://hg.mozilla.org/mozilla-central/rev/4b2b6c39adf9

Comment 15

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #14)
> I have tried 

that should be "haven't"

Comment 16

[Chris Peterson [:cpeterson]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #14)
> an AndroidCompositorParent subclass of CompositorParent). I guess
> MOZ_OVERRIDE is a completely useless decoration because it doesn't break
> compiles if the superclass changes?

gcc 4.7 is the first version of gcc to check MOZ_OVERRIDE. Our default mozconfigs use Android's gcc 4.6.3.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

I filed bug 867269 for this.