If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Reduce coupling between APZC and CompositorBridgeParent

RESOLVED FIXED in Firefox 52

Status

()

Core
Panning and Zooming
P3
normal
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: kats, Assigned: kats)

Tracking

(Blocks: 1 bug)

52 Branch
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [gfx-noted])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

11 months ago
mozreview-review
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 7

11 months ago
mozreview-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 8

11 months ago
mozreview-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.

Comment 10

11 months ago
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

Comment 11

11 months ago
(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.)

Comment 12

11 months ago
(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

11 months ago
> -  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

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3a7aa9f51657
https://hg.mozilla.org/mozilla-central/rev/f5ddc662700d
https://hg.mozilla.org/mozilla-central/rev/b534adeedaca
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(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

11 months ago
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0817d8421d3e
Followup to fix const-correctness. rs=botond

Comment 17

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0817d8421d3e
You need to log in before you can comment on or make changes to this bug.