860613 - Add a way to bind fennec's APZC to the primary scrollable layer

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

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

Attachment: Kill off GetDefaultPanZoomController

The CompositorParent class contains a GetDefaultPanZoomController method that romaxa added a while ago. It doesn't look like this is used anywhere, and would probably break things if used as-is, because the same APZC would be used for all layers. We should get rid of this; in fact on IRC romaxa pointed to https://github.com/tmeshkova/mozilla-central/commit/16d92951887c9326a9ae1a0ce03ff1999b7f65d4 which does get rid of it (along with doing some other stuff). I pulled out the relevant bits of that commit and created the attached patch for it.

In my patch queue to hook up APZC to Java, I do something similar to the EmbedLiteCompositorParent::ShadowLayersUpdated function that can be (partly) seen in the github commit above, where I override CompositorParent::ShadowLayersUpdated and use that to set the APZC as the user data of the layer. If that is not an appropriate way to accomplish the layer <-> APZC binding then speak now or forever hold your peace.

Comment 1

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

Attachment: Override CompositorParent::ShadowLayersUpdated in android to bind the APZC

I guess I might as well put this patch on this bug as well, since it immediately follows the other one in my queue. Not requesting review on it just yet since I still need to verify a couple of things but it should help provide context for the first patch.

Comment 2

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

Attachment: Override CompositorParent::ShadowLayersUpdated in android to bind the APZC

Whoops, fixed a rebase error

Comment 3

[Oleg Romashin (:romaxa)]

Comment on attachment 736133 [details] [diff] [review]
Kill off GetDefaultPanZoomController

yeah, haven't found time to push this patch, thanks kats

Comment 4

[Benoit Girard (:BenWa)]

Comment on attachment 736133 [details] [diff] [review]
Kill off GetDefaultPanZoomController

Review of attachment 736133 [details] [diff] [review]:
-----------------------------------------------------------------

r+ if you remove SetPanUserData.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +813,5 @@
>    return activeAnimations;
>  }
>  
> +void
> +CompositorParent::SetPanUserData(Layer *aLayer, AsyncPanZoomController* aController)

Per IRC discussion this method has no business being part of CompositorParent.

Comment 5

[Benoit Girard (:BenWa)]

Comment on attachment 736136 [details] [diff] [review]
Override CompositorParent::ShadowLayersUpdated in android to bind the APZC

Review of attachment 736136 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/xpwidgets/nsBaseWidget.cpp
@@ -882,5 @@
>  void nsBaseWidget::CreateCompositor(int aWidth, int aHeight)
>  {
>    bool renderToEGLSurface = false;
>  #ifdef MOZ_ANDROID_OMTC
>    renderToEGLSurface = true;

renderToEGLSurface is an android thing only. We can remove that bool from NewCompositorParent and have it as true in the android widget implementation.

Comment 6

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

Attachment: Kill off GetDefaultPanZoomController (v2)

Updated to just remove the GetDefaultPanZoomController as per comments; carrying r+

Comment 7

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

Attachment: Add APIs on Layer to get/set the APZC

Comment 8

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

Attachment: Override CompositorParent::ShadowLayersUpdated in android to bind the APZC

Comment 9

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 736507 [details] [diff] [review]
Override CompositorParent::ShadowLayersUpdated in android to bind the APZC

Review of attachment 736507 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/android/nsWindow.cpp
@@ +2332,5 @@
> +    virtual void ShadowLayersUpdated(mozilla::layers::ShadowLayersParent* aLayerTree,
> +                                     const mozilla::layers::TargetConfig& aTargetConfig,
> +                                     bool isFirstPaint) MOZ_OVERRIDE
> +    {
> +        CompositorParent::ShadowLayersUpdated(aLayerTree, aTargetConfig, isFirstPaint);

Perhaps it would be cleaner to register a listener to ShadowLayersUpdated rather than making ShadowLayersUpdated a virtual function.

Comment 10

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

I'm not opposed to that. However I see other potential cleanups that are enabled by having an Android-specific subclass of CompositorParent, so there's a bit of a "two birds with one stone" thing happening here. But either approach is fine by me.

Comment 11

[Benoit Girard (:BenWa)]

Comment on attachment 736505 [details] [diff] [review]
Add APIs on Layer to get/set the APZC

Review of attachment 736505 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Layers.cpp
@@ +405,5 @@
> +  { }
> +
> +  // We don't keep a strong ref here because PanZoomUserData is only
> +  // set transiently, and APZC is thread-safe refcounted so
> +  // AddRef/Release is expensive.

Sounds a bit scary. Can we debug assert that the refcount is > 1 then when removing/getting it then?

Comment 12

[Benoit Girard (:BenWa)]

Comment on attachment 736507 [details] [diff] [review]
Override CompositorParent::ShadowLayersUpdated in android to bind the APZC

Review of attachment 736507 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with last comment fixed.

::: widget/android/nsWindow.cpp
@@ +2319,5 @@
>  }
>  
> +class AndroidCompositorParent : public mozilla::layers::CompositorParent {
> +public:
> +    AndroidCompositorParent(nsIWidget* aWidget, bool aRenderToEGLSurface,

enum > bool but this is propagating an existing param so it's ok for this.

@@ +2332,5 @@
> +    virtual void ShadowLayersUpdated(mozilla::layers::ShadowLayersParent* aLayerTree,
> +                                     const mozilla::layers::TargetConfig& aTargetConfig,
> +                                     bool isFirstPaint) MOZ_OVERRIDE
> +    {
> +        CompositorParent::ShadowLayersUpdated(aLayerTree, aTargetConfig, isFirstPaint);

FWIT I prefer this but this is a matter of opinion. This should make it clearer how the compositor differ across platform since they will because we're making different design choices to fit better with the platform.

@@ +2351,5 @@
> +
> +void
> +nsWindow::SetPanZoomController(mozilla::layers::AsyncPanZoomController* apzc)
> +{
> +    if (apzc) {

This code behave different if (sApzc != null && apzc != null) vs (sApzc != null && apzc == null) i.e. sApzc_orig->SetCompositorParent(nullptr) never gets called. Is this intentional. Even if it doesn't happen this seems very fragile.

Comment 13

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

(In reply to Benoit Girard (:BenWa) from comment #11)
> Sounds a bit scary. Can we debug assert that the refcount is > 1 then when
> removing/getting it then?

Shouldn't that be "refcount > 0"? The refcount being 1 here should be fine, right? Also, in order to implement this I'm going to have to add something to AsyncPanZoomController.h to expose the ref count, since currently it's defined as protected by the NS_INLINE_DECL_THREADSAFE_REFCOUNTING macro.

And finally, if there *are* problems with this code, and we're left with a dangling reference to a free'd APZC, then trying to access the mRefCnt field on it could be a bad idea. (Particularly since it'll be a function call rather than a field access).

Thoughts?

(In reply to Benoit Girard (:BenWa) from comment #12)
> This code behave different if (sApzc != null && apzc != null) vs (sApzc !=
> null && apzc == null) i.e. sApzc_orig->SetCompositorParent(nullptr) never
> gets called. Is this intentional. Even if it doesn't happen this seems very
> fragile.

Yeah, this was not intentional. I don't know why I wrote it that way, I'll fix it up.

Comment 14

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #13)
> Thoughts?
> 

We discussed this on IRC and decided to leave it as-is.

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/e351053c0ef8
https://hg.mozilla.org/integration/mozilla-inbound/rev/24e3e435129e
https://hg.mozilla.org/integration/mozilla-inbound/rev/1338721bd243

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e351053c0ef8
https://hg.mozilla.org/mozilla-central/rev/24e3e435129e
https://hg.mozilla.org/mozilla-central/rev/1338721bd243