Closed
Bug 1312059
Opened 8 years ago
Closed 8 years ago
Reduce coupling between APZC and CompositorBridgeParent
Categories
(Core :: Panning and Zooming, defect, P3)
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.
Assignee | ||
Comment 1•8 years ago
|
||
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.
Assignee | ||
Comment 2•8 years ago
|
||
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•8 years 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•8 years 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•8 years 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+
Assignee | ||
Comment 9•8 years ago
|
||
(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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee | ||
Comment 15•8 years ago
|
||
(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•8 years ago
|
||
Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0817d8421d3e Followup to fix const-correctness. rs=botond
Comment 17•8 years 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.
Description
•