Closed Bug 1312059 Opened 8 years ago Closed 8 years ago

Reduce coupling between APZC and CompositorBridgeParent

Categories

(Core :: Panning and Zooming, defect, P3)

52 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(3 files)

The APZC code holds a refptr to CompositorBridgeParent which it uses for a few things. We should make this coupling looser by introducing some narrower interfaces that can be implemented by things other than CompositorBridgeParent. We may need this for Webrender integration.
WIP so far is the top two patches at https://github.com/staktrace/gecko-dev/commits/c9f41a5ce6e5281cb446b9f8bacc1e395085a589. All trees are closed so I can't push it to try, but I'll do that once stuff reopens.

I also want to move the coupling boundary back further, so that APZCTreeManager doesn't necessarily rely on stuff from CompositorBridgeParent.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb1dcf60bfbd053c71a76317928cf732b1b3a348 is the try push that's green. I had to make some changes from the previous version to get all the refcounting goop to work out. If you have suggestions on how to improve it I'm all ears; it would be nice to get rid of the two sets of AddRef/Release I had to add into CompositorBridgeParent.h while maintaining the class inheritance structure.
Comment on attachment 8803858 [details]
Bug 1312059 - Extract a CompositorController interface for the APZ code to request composites and do other compositor-related things.

https://reviewboard.mozilla.org/r/88062/#review87082

::: gfx/layers/apz/public/CompositorController.h:10
(Diff revision 1)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_layers_CompositorController_h
> +#define mozilla_layers_CompositorController_h
> +
> +#include "mozilla/RefCountType.h"

#include "nscore.h" for NS_IMETHOD_

::: gfx/layers/ipc/CompositorBridgeParent.cpp:1013
(Diff revision 1)
>  }
>  
> +void
> +CompositorBridgeParent::ScheduleComposite()
> +{
> +  ScheduleRenderOnCompositorThread();

Can we just call the interface method ScheduleRenderOnCompositorThread()?
Attachment #8803858 - Flags: review?(botond) → review+
Comment on attachment 8803859 [details]
Bug 1312059 - Extract a MetricsSharingController interface for the APZC code to use when dealing with shared frame metrics.

https://reviewboard.mozilla.org/r/88064/#review87106
Attachment #8803859 - Flags: review?(botond) → review+
Comment on attachment 8803860 [details]
Bug 1312059 - Stop passing the CompositorBridgeParent into the APZCTreeManager; instead the necessary subinterfaces can be obtained via the LayerTreeState.

https://reviewboard.mozilla.org/r/88066/#review87104

::: gfx/layers/apz/src/APZCTreeManager.cpp:70
(Diff revision 1)
>      , mPaintLogger(aTestData, aPaintSequence)
>    {
>    }
>  
>    // State that doesn't change as we recurse in the tree building
> -  CompositorBridgeParent* const mCompositor;
> +  const uint64_t mRootLayerTreeId;

Can we store a pointer to the root LayerTreeState here instead, instead of looking it up every time we make a new APZC?
Attachment #8803860 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #6)
> > +
> > +#include "mozilla/RefCountType.h"
> 
> #include "nscore.h" for NS_IMETHOD_

Done. Also included it in MetricsSharingController.h for the same reason.

> ::: gfx/layers/ipc/CompositorBridgeParent.cpp:1013
> > +CompositorBridgeParent::ScheduleComposite()
> > +{
> > +  ScheduleRenderOnCompositorThread();
> 
> Can we just call the interface method ScheduleRenderOnCompositorThread()?

In general I don't like naming interface methods based on implementation detail but I guess in this case the two names convey the same idea so it doesn't matter much. I've updated it as you requested.

(In reply to Botond Ballo [:botond] from comment #8)
> > -  CompositorBridgeParent* const mCompositor;
> > +  const uint64_t mRootLayerTreeId;
> 
> Can we store a pointer to the root LayerTreeState here instead, instead of
> looking it up every time we make a new APZC?

Makes sense, done.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a7aa9f51657
Extract a CompositorController interface for the APZ code to request composites and do other compositor-related things. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5ddc662700d
Extract a MetricsSharingController interface for the APZC code to use when dealing with shared frame metrics. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/b534adeedaca
Stop passing the CompositorBridgeParent into the APZCTreeManager; instead the necessary subinterfaces can be obtained via the LayerTreeState. r=botond
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9)
> > ::: gfx/layers/ipc/CompositorBridgeParent.cpp:1013
> > > +CompositorBridgeParent::ScheduleComposite()
> > > +{
> > > +  ScheduleRenderOnCompositorThread();
> > 
> > Can we just call the interface method ScheduleRenderOnCompositorThread()?
> 
> In general I don't like naming interface methods based on implementation
> detail but I guess in this case the two names convey the same idea so it
> doesn't matter much. I've updated it as you requested.

(My motivation was to have two similar-sounding methods in CompositorBridgeParent that do the same thing.)
(In reply to Botond Ballo [:botond] from comment #11)
> (My motivation was to have two similar-sounding methods in
> CompositorBridgeParent that do the same thing.)

er, s/have/avoid
> -  CompositorBridgeParent* const mCompositor;
> +  const CompositorBridgeParent::LayerTreeState* mLayerTreeState;

Tiny nit: given that all other fields in this section, including the mCompositor we're replacing, were toplevel-const, this should technically be:

  const CompositorBridgeParent::LayerTreeState* const mLayerTreeState;

(Feel free to land this change as part of another patch if it's convenient.)
(In reply to Botond Ballo [:botond] from comment #13)
> (Feel free to land this change as part of another patch if it's convenient.)

Doesn't look like I need to touch this code again in the near future, so I'll just land a follow-up to fix this.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0817d8421d3e
Followup to fix const-correctness. rs=botond
You need to log in before you can comment on or make changes to this bug.