Closed Bug 860613 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Graphics: Layers, defect)

23 Branch
All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(3 files, 3 obsolete files)

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.
Attachment #736133 - Flags: review?(bgirard)
Attachment #736133 - Flags: feedback?(romaxa)
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.
Summary: Kill off CompositorParent::GetDefaultPanZoomController → Add a way to bind fennec's APZC to the primary scrollable layer
Comment on attachment 736133 [details] [diff] [review]
Kill off GetDefaultPanZoomController

yeah, haven't found time to push this patch, thanks kats
Attachment #736133 - Flags: feedback?(romaxa) → feedback+
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.
Attachment #736133 - Flags: review?(bgirard) → review+
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.
Updated to just remove the GetDefaultPanZoomController as per comments; carrying r+
Attachment #736133 - Attachment is obsolete: true
Attachment #736504 - Flags: review+
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.
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 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?
Attachment #736505 - Flags: review?(bgirard) → review+
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.
Attachment #736507 - Flags: review?(bgirard) → review+
(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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13)
> Thoughts?
> 

We discussed this on IRC and decided to leave it as-is.
You need to log in before you can comment on or make changes to this bug.