Stop exposing APZCTreeManager to the android dynamic toolbar code

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: kats, Assigned: kats)

Tracking

unspecified
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(4 attachments)

There's a bunch of code ifdef'd to MOZ_WIDGET_ANDROID that uses the GetAPZCTreeManager function on CompositorBridgeParent to get a handle to the APZCTM and then get a handle to the DynamicToolbarAnimator class. As part of bug 1442627 we're reducing access to APZCTreeManager and this is one of the things that I punted on because it was android-only. This bug is to clean that up at some point. We'll definitely need it cleaned up before we can enable WR on android.
This actually wasn't hard to do, so I went ahead and did it.
Assignee: nobody → bugmail
Comment hidden (mozreview-request)
Priority: -- → P3
Whiteboard: [gfx-noted]

Comment 8

Last year
mozreview-review
Comment on attachment 8956567 [details]
Bug 1443301 - Remove unused function.

https://reviewboard.mozilla.org/r/225472/#review231786
Attachment #8956567 - Flags: review?(botond) → review+

Comment 9

Last year
mozreview-review
Comment on attachment 8956568 [details]
Bug 1443301 - Give the dynamic toolbar class a non-owning ref to the APZCTreeManager.

https://reviewboard.mozilla.org/r/225474/#review231788

::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.h:171
(Diff revision 1)
>    bool ToolbarInTransition();
>    void QueueMessage(int32_t aMessage);
>  
>    // Read only Compositor and Controller threads after Initialize()
>    uint64_t mRootLayerTreeId;
> +  MOZ_NON_OWNING_REF APZCTreeManager* mApz;

How can we be sure the Animator does not outlive the TreeManager? In addition to the TreeManager, the UiCompositorControllerParent stores a RefPtr to the Animator.

Comment 10

Last year
mozreview-review
Comment on attachment 8956569 [details]
Bug 1443301 - Update dynamic toolbar init codepath to not need an APZCTreeManager.

https://reviewboard.mozilla.org/r/225476/#review231796

::: gfx/layers/ipc/UiCompositorControllerParent.cpp:283
(Diff revision 1)
>    LayerTreeState* state = CompositorBridgeParent::GetIndirectShadowTree(mRootLayerTreeId);
>    MOZ_ASSERT(state);
>    MOZ_ASSERT(state->mParent);
>    state->mUiControllerParent = this;
>  #if defined(MOZ_WIDGET_ANDROID)
> -  RefPtr<APZCTreeManager> manager = state->mParent->GetAPZCTreeManager();
> +  AndroidDynamicToolbarAnimator* animator = state->mParent->GetAndroidDynamicToolbarAnimator();

Why not use |mAnimator|?

Comment 11

Last year
mozreview-review
Comment on attachment 8956570 [details]
Bug 1443301 - Stop exposing mApzcTreeManager from CompositorBridgeParent.

https://reviewboard.mozilla.org/r/225478/#review231800
Attachment #8956570 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #9)
> How can we be sure the Animator does not outlive the TreeManager? In
> addition to the TreeManager, the UiCompositorControllerParent stores a
> RefPtr to the Animator.

Oh, good catch! I'll update the patch to have APZCTreeManager::Destroy tell the animator to null out it's mApz pointer, and update the mApz usages to guard for nullptr, in the places it's not already checking for if (parent) {...}.

(In reply to Botond Ballo [:botond] from comment #10)
> >  #if defined(MOZ_WIDGET_ANDROID)
> > -  RefPtr<APZCTreeManager> manager = state->mParent->GetAPZCTreeManager();
> > +  AndroidDynamicToolbarAnimator* animator = state->mParent->GetAndroidDynamicToolbarAnimator();
> 
> Why not use |mAnimator|?

It hasn't been set yet. The call to animator->Initialize is the thing that calls back into this class to set mAnimator. These functions do extra stuff so I didn't want to mess with that part.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

Last year
mozreview-review
Comment on attachment 8956568 [details]
Bug 1443301 - Give the dynamic toolbar class a non-owning ref to the APZCTreeManager.

https://reviewboard.mozilla.org/r/225474/#review231850

::: gfx/layers/apz/src/AndroidDynamicToolbarAnimator.cpp:645
(Diff revision 2)
>      }
>  
>      uint32_t timeDelta = aTimeStamp - mControllerLastEventTimeStamp;
>      if (mControllerLastEventTimeStamp && timeDelta && aDelta) {
>        float speed = -(float)aDelta / (float)timeDelta;
> -      CompositorBridgeParent* parent = CompositorBridgeParent::GetCompositorBridgeParentFromLayersId(mRootLayerTreeId);
> +      if (mApz) {

Continuing to play devil's advocate: this function runs on the controller thread. What's to stop the compositor thread from getting a timeslice and nulling out mApz in between the check and the access here?
That's what bug 1443436 is for, is basically a pre-existing issue.

Comment 20

Last year
mozreview-review
Comment on attachment 8956568 [details]
Bug 1443301 - Give the dynamic toolbar class a non-owning ref to the APZCTreeManager.

https://reviewboard.mozilla.org/r/225474/#review231868
Attachment #8956568 - Flags: review?(botond) → review+

Comment 21

Last year
mozreview-review
Comment on attachment 8956569 [details]
Bug 1443301 - Update dynamic toolbar init codepath to not need an APZCTreeManager.

https://reviewboard.mozilla.org/r/225476/#review231870
Attachment #8956569 - Flags: review?(botond) → review+

Comment 22

Last year
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/81873f9d6821
Remove unused function. r=botond
https://hg.mozilla.org/integration/autoland/rev/3034411c797e
Give the dynamic toolbar class a non-owning ref to the APZCTreeManager. r=botond
https://hg.mozilla.org/integration/autoland/rev/7fdb52108da9
Update dynamic toolbar init codepath to not need an APZCTreeManager. r=botond
https://hg.mozilla.org/integration/autoland/rev/5729b8d91b48
Stop exposing mApzcTreeManager from CompositorBridgeParent. r=botond
You need to log in before you can comment on or make changes to this bug.