Closed Bug 861727 Opened 7 years ago Closed 7 years ago

Move layers stuff from CompositorParent to LayerManagerComposite

Categories

(Core :: Graphics: Layers, defect)

21 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: nrc, Assigned: nrc)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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.
Depends on: 842518
Blocks: 842518
No longer depends on: 842518
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?
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.
Attachment #739328 - Flags: review?(bgirard)
rebased and reorganised a little
Attachment #739328 - Attachment is obsolete: true
Attachment #739328 - Flags: review?(bgirard)
Attachment #740629 - Flags: review?(bgirard)
Blocks: 865033
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.
Attachment #740629 - Attachment is obsolete: true
Attachment #740629 - Flags: review?(bgirard)
Attachment #741167 - Flags: review?(bgirard)
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.
Attachment #741167 - Flags: review?(chrislord.net)
Attachment #741167 - Flags: review?(bgirard)
Attachment #741167 - Flags: review+
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/)
Attachment #741167 - Flags: review?(chrislord.net)
(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.
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.
Attachment #741167 - Attachment is obsolete: true
Attachment #742779 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/fad7e0541873
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
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
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> I have tried 

that should be "haven't"
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Depends on: 867420
You need to log in before you can comment on or make changes to this bug.