1442627 - Reduce usage of APZCTreeManager class in the codebase

Status: RESOLVED FIXED

(Core :: Panning and Zooming, enhancement)

Description

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

In order to prepare for the APZ sampler thread getting mapped to the render backend thread when WR is enabled, we should make sure that all the things that are supposed to happen on the sampler thread are clearly identified. I plan to do this by making sure all of those things go through the APZSampler interface class introduced in bug 1441916. And therefore less code should be using APZCTreeManager directly.

Comment 1

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=afcf0028bfd2f4eef88bf59d5c7fbc6d3fd32055

Try push is pretty green except for Android build failures. I fixed those locally, will put up patches for review.

Comment 2

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

Attachment: Bug 1442627 - Don't use GetAPZCTreeManager in CrossProcessCompositorBridgeParent.

Although CrossProcessCompositorBridgeParent still needs to create a
dummy APZCTreeManager of its own in place, we can at least stop it from
grabbing the "real" APZCTreeManager from CompositorBridgeParent, which
allows access to methods that might not be properly guarded with respect
to thread safety.

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

Comment 3

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

Attachment: Bug 1442627 - Switch some call sites in WebRenderBridgeParent to use APZSampler.

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

Comment 4

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

Attachment: Bug 1442627 - Add new APZSampler APIs to set the test scroll offset and zoom.

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

Comment 5

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

Attachment: Bug 1442627 - Reduce exposure of the APZCTreeManager.

Callers should be using one of the more specific subinterfaces like
IAPZCTreeManager (for controller-API methods) or APZSampler (for
sampler-API methods). There's also a bunch of android-specific
dynamic toolbar code that uses this function - I don't want to
deal with that right now, so instead of removing it entirely we can just
make it Android-only.

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

Comment 6

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

Attachment: Bug 1442627 - Move static helpers from APZCTreeManager to APZUtils.

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

Comment 7

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

Attachment: Bug 1442627 - Stop exporting APZCTreeManager.h in mozilla/layers/.

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

Comment 8

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

Attachment: Bug 1442627 - Stop exporting a few other apz/src headers.

These were mostly exported because APZCTreeManager included them and now
they don't need to be exported any more.

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

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8955649 [details]
Bug 1442627 - Don't use GetAPZCTreeManager in CrossProcessCompositorBridgeParent.

https://reviewboard.mozilla.org/r/224738/#review231060

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8955650 [details]
Bug 1442627 - Switch some call sites in WebRenderBridgeParent to use APZSampler.

https://reviewboard.mozilla.org/r/224740/#review231062

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8955651 [details]
Bug 1442627 - Add new APZSampler APIs to set the test scroll offset and zoom.

https://reviewboard.mozilla.org/r/224742/#review231064

::: gfx/layers/apz/src/APZSampler.cpp:108
(Diff revision 1)
> +  MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread());
> +  RefPtr<AsyncPanZoomController> apzc = mApz->GetTargetAPZC(aLayersId, aScrollId);
> +  if (apzc) {
> +    apzc->SetTestAsyncScrollOffset(aOffset);
> +  } else {
> +    NS_WARNING("Unable to find APZ in SetTestAsyncScrollOffset");

s/APZ/APZC

::: gfx/layers/apz/src/APZSampler.cpp:122
(Diff revision 1)
> +  MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread());
> +  RefPtr<AsyncPanZoomController> apzc = mApz->GetTargetAPZC(aLayersId, aScrollId);
> +  if (apzc) {
> +    apzc->SetTestAsyncZoom(aZoom);
> +  } else {
> +    NS_WARNING("Unable to find APZ in SetTestAsyncZoom");

likewise

::: gfx/layers/ipc/LayerTransactionParent.cpp:755
(Diff revision 1)
>    }
>    return IPC_OK();
>  }
>  
> -static AsyncPanZoomController*
> -GetAPZCForViewID(Layer* aLayer, FrameMetrics::ViewID aScrollID)
> +static RefPtr<CompositorBridgeParent>
> +GetRootCompositorBridgeParent(const uint64_t& aLayersId)

Can we unify this with CompositorBridgeParent::GetCompositorBridgeParentFromLayersId()?

Comment 12

[Botond Ballo [:botond]]

Comment on attachment 8955651 [details]
Bug 1442627 - Add new APZSampler APIs to set the test scroll offset and zoom.

https://reviewboard.mozilla.org/r/224742/#review231076

::: gfx/layers/ipc/LayerTransactionParent.cpp:762
(Diff revision 1)
> -  AsyncPanZoomController* resultApzc = nullptr;
> -  ForEachNode<ForwardIterator>(
> -      aLayer,
> -      [aScrollID, &resultApzc] (Layer* layer)
> -      {
> -        for (uint32_t i = 0; i < layer->GetScrollMetadataCount(); i++) {
> +  MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread());
> +
> +  CompositorBridgeParent::LayerTreeState* lts =
> +      CompositorBridgeParent::GetIndirectShadowTree(aLayersId);
> +  if (!lts) {
> +    return nullptr;

Oh, and +1 for removing ad-hoc code for iterating over the layer tree to find an APZC :)

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8955652 [details]
Bug 1442627 - Reduce exposure of the APZCTreeManager.

https://reviewboard.mozilla.org/r/224744/#review231078

Would be good to have a follow-up bug on file for dealing with this.

Comment 14

[Botond Ballo [:botond]]

Comment on attachment 8955653 [details]
Bug 1442627 - Move static helpers from APZCTreeManager to APZUtils.

https://reviewboard.mozilla.org/r/224746/#review231084

::: gfx/layers/apz/src/APZUtils.h:97
(Diff revision 1)
>  
>    bool mTargetConfirmed : 1;
>    bool mRequiresTargetConfirmation : 1;
>  };
>  
> +namespace apz {

Having come across namespace apz, someone might wonder why things like APZSampler are not in it.

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 8955654 [details]
Bug 1442627 - Stop exporting APZCTreeManager.h in mozilla/layers/.

https://reviewboard.mozilla.org/r/224748/#review231088

::: gfx/layers/ipc/CompositorBridgeParent.cpp:14
(Diff revision 1)
>  #include <stdio.h>                      // for fprintf, stdout
>  #include <stdint.h>                     // for uint64_t
>  #include <map>                          // for _Rb_tree_iterator, etc
>  #include <utility>                      // for pair
> +
> +#include "apz/src/APZCTreeManager.h"  // for APZCTreeManager

Is there a plan for eventually axing the "apz/src" include from files outside of gfx/layers/apz (like this one)?

Comment 16

[Botond Ballo [:botond]]

Comment on attachment 8955655 [details]
Bug 1442627 - Stop exporting a few other apz/src headers.

https://reviewboard.mozilla.org/r/224750/#review231090

Comment 17

[Botond Ballo [:botond]]

Comment on attachment 8955654 [details]
Bug 1442627 - Stop exporting APZCTreeManager.h in mozilla/layers/.

https://reviewboard.mozilla.org/r/224748/#review231092

Comment 18

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

(In reply to Botond Ballo [:botond] from comment #11)
> s/APZ/APZC

Fixed, in both places

> > -static AsyncPanZoomController*
> > -GetAPZCForViewID(Layer* aLayer, FrameMetrics::ViewID aScrollID)
> > +static RefPtr<CompositorBridgeParent>
> > +GetRootCompositorBridgeParent(const uint64_t& aLayersId)
> 
> Can we unify this with
> CompositorBridgeParent::GetCompositorBridgeParentFromLayersId()?

Good catch, done. I dropped the function in LayerTransactionParent entirely, and replaced the bottom half of the one in WebRenderBridgeParent with a call to GetCompositorBridgeParentFromLayersId.

(In reply to Botond Ballo [:botond] from comment #13)
> 
> Would be good to have a follow-up bug on file for dealing with this.

Filed bug 1443301.

(In reply to Botond Ballo [:botond] from comment #14)
> > +namespace apz {
> 
> Having come across namespace apz, someone might wonder why things like
> APZSampler are not in it.

Maybe they should be :) It just seemed like these utility functions didn't belong in any particular class but were sufficiently apz-specific that I didn't want them sitting in the layers namespace either. So I decided to do this for now but I'm not opposed to moving more things into an apz namespace, or to move these functions out of the apz namespace and use some other naming approach.

(In reply to Botond Ballo [:botond] from comment #15)
> > +#include "apz/src/APZCTreeManager.h"  // for APZCTreeManager
> 
> Is there a plan for eventually axing the "apz/src" include from files
> outside of gfx/layers/apz (like this one)?

Not a concrete plan, other than "I would like it to happen". Presumably if we really wanted we could add wrapper methods on APZUtils or some other "interface" type class that does the actual construction of APZCTreeManager and we might be able to get rid of this include that way. I'm not really sure, and I didn't want to go too far down that road since it would be a distraction from the main threading changes I'm trying to make.

Comment 19

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8680aef63128a105094d1f2fa1c0ba41c327632e

Comment 20

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

Now with unified build fixes for Android: https://treeherder.mozilla.org/#/jobs?repo=try&revision=35ae7396d27dabd6f0c72c97c79ecdb358a79b70

Seems good so far.

Comment 21

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

Comment on attachment 8955649 [details]
Bug 1442627 - Don't use GetAPZCTreeManager in CrossProcessCompositorBridgeParent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224738/diff/1-2/

Comment 22

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

Comment on attachment 8955650 [details]
Bug 1442627 - Switch some call sites in WebRenderBridgeParent to use APZSampler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224740/diff/1-2/

Comment 23

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

Comment on attachment 8955651 [details]
Bug 1442627 - Add new APZSampler APIs to set the test scroll offset and zoom.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224742/diff/1-2/

Comment 24

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

Comment on attachment 8955652 [details]
Bug 1442627 - Reduce exposure of the APZCTreeManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224744/diff/1-2/

Comment 25

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

Comment on attachment 8955653 [details]
Bug 1442627 - Move static helpers from APZCTreeManager to APZUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224746/diff/1-2/

Comment 26

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

Comment on attachment 8955654 [details]
Bug 1442627 - Stop exporting APZCTreeManager.h in mozilla/layers/.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224748/diff/1-2/

Comment 27

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

Comment on attachment 8955655 [details]
Bug 1442627 - Stop exporting a few other apz/src headers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224750/diff/1-2/

Comment 28

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

Try push in comment 20 had failures on android because the SetTestAsyncScrollOffset and SetTestAsyncZoom functions weren't working. The problem, as always, was the special handling of the case when the LayerTransactionParent's id is 0. After trying a few things I think the best solution is to just conform to the existing pattern of adding a virtual method on CompositorBridgeParentBase and implementing it in the CompositorBridgeParent and CrossProcessCompositorBridgeParent subclasses. That pattern can use some cleanup because it has a bunch of duplicated code but I'll punt on doing that in this bug.

Comment 29

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0f5cfbc5423f1c7df97fa12649a826f16641790

Comment 30

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

Comment on attachment 8955649 [details]
Bug 1442627 - Don't use GetAPZCTreeManager in CrossProcessCompositorBridgeParent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224738/diff/2-3/

Comment 31

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

Comment on attachment 8955650 [details]
Bug 1442627 - Switch some call sites in WebRenderBridgeParent to use APZSampler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224740/diff/2-3/

Comment 32

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

Comment on attachment 8955651 [details]
Bug 1442627 - Add new APZSampler APIs to set the test scroll offset and zoom.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224742/diff/2-3/

Comment 33

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

Comment on attachment 8955652 [details]
Bug 1442627 - Reduce exposure of the APZCTreeManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224744/diff/2-3/

Comment 34

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

Comment on attachment 8955653 [details]
Bug 1442627 - Move static helpers from APZCTreeManager to APZUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224746/diff/2-3/

Comment 35

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

Comment on attachment 8955654 [details]
Bug 1442627 - Stop exporting APZCTreeManager.h in mozilla/layers/.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224748/diff/2-3/

Comment 36

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

Comment on attachment 8955655 [details]
Bug 1442627 - Stop exporting a few other apz/src headers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224750/diff/2-3/

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4fa35b9daa65
Don't use GetAPZCTreeManager in CrossProcessCompositorBridgeParent. r=botond
https://hg.mozilla.org/integration/autoland/rev/322301ce2f82
Switch some call sites in WebRenderBridgeParent to use APZSampler. r=botond
https://hg.mozilla.org/integration/autoland/rev/54e61d972192
Add new APZSampler APIs to set the test scroll offset and zoom. r=botond
https://hg.mozilla.org/integration/autoland/rev/af1eee8c58f7
Reduce exposure of the APZCTreeManager. r=botond
https://hg.mozilla.org/integration/autoland/rev/306f68c9845e
Move static helpers from APZCTreeManager to APZUtils. r=botond
https://hg.mozilla.org/integration/autoland/rev/34496aa46b31
Stop exporting APZCTreeManager.h in mozilla/layers/. r=botond
https://hg.mozilla.org/integration/autoland/rev/343c384f751c
Stop exporting a few other apz/src headers. r=botond

Comment 38

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/4fa35b9daa65
https://hg.mozilla.org/mozilla-central/rev/322301ce2f82
https://hg.mozilla.org/mozilla-central/rev/54e61d972192
https://hg.mozilla.org/mozilla-central/rev/af1eee8c58f7
https://hg.mozilla.org/mozilla-central/rev/306f68c9845e
https://hg.mozilla.org/mozilla-central/rev/34496aa46b31
https://hg.mozilla.org/mozilla-central/rev/343c384f751c