1312059 - Reduce coupling between APZC and CompositorBridgeParent

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

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

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.

Comment 2

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

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 3

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

Attachment: Bug 1312059 - Extract a CompositorController interface for the APZ code to request composites and do other compositor-related things.

Review commit: https://reviewboard.mozilla.org/r/88062/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/88062/

Comment 4

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

Attachment: Bug 1312059 - Extract a MetricsSharingController interface for the APZC code to use when dealing with shared frame metrics.

Review commit: https://reviewboard.mozilla.org/r/88064/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/88064/

Comment 5

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

Attachment: Bug 1312059 - Stop passing the CompositorBridgeParent into the APZCTreeManager; instead the necessary subinterfaces can be obtained via the LayerTreeState.

Review commit: https://reviewboard.mozilla.org/r/88066/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/88066/

Comment 6

[Botond Ballo [:botond]]

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()?

Comment 7

[Botond Ballo [:botond]]

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

Comment 8

[Botond Ballo [:botond]]

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?

Comment 9

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

(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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.)

Comment 12

[Botond Ballo [:botond]]

(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

Comment 13

[Botond Ballo [:botond]]

> -  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.)

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3a7aa9f51657
https://hg.mozilla.org/mozilla-central/rev/f5ddc662700d
https://hg.mozilla.org/mozilla-central/rev/b534adeedaca

Comment 15

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

(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.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0817d8421d3e
Followup to fix const-correctness. rs=botond

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0817d8421d3e