1064840 - Don't show apz-failure visual indicator if there is a content layer with that APZ

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

From bug 1063158 comment 13:

> In bug 1053992 I think we should just suppress the visual marker if there
> are layers with content and FrameMetrics with the same ID as the
> FrameMetrics of the scrollinfo layer.

This bug is to implement this.

Comment 1

[No-Jun Park [:njpark]]

[Blocking Requested - why for this release]:
Since Bug 1063158 is marked as wontfix, and this bug is created to suppress the visual indicator warning, it is required to verify Bug 1026271

Comment 2

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

Attachment: Patch

Verified that this works by flipping the layout.async-containerless-scroll.enabled pref to false and testing on the case in bug 1011639.

Comment 3

[Botond Ballo [:botond]]

Comment on attachment 8486455 [details] [diff] [review]
Patch

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +575,5 @@
>                                                 &overscrollTransform);
>  
> +    Matrix4x4 asyncTransform = Matrix4x4(asyncTransformWithoutOverscroll) * overscrollTransform;
> +    if (!aLayer->IsScrollInfoLayer() || asyncTransform.IsIdentity()) {
> +      controller->MarkAsyncTransformAppliedToContent();

I'm not sure how I feel about an "async transform applied to content" flag whose interpretation seems to be "at least one layer associated with the APZ is a non-scrollinfo layer OR the transform is the identity transform".

I think it would make more sense to have this flag only mean the first thing ("at least one layer associated with the APZ is a non-scrollinfo layer") (and perhaps alter the name accordingly), and check the second condition separately.

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +434,5 @@
>      // on it down to the bottom using GetFirstChild and not worry about walking onto another
>      // underlying layer.
>      for (LayerMetricsWrapper i(aContainer); i; i = i.GetFirstChild()) {
>        if (AsyncPanZoomController* apzc = i.GetApzc()) {
> +        if (!apzc->GetAsyncTransformAppliedToContent()) {

That is, here we would check !apzc->GetAsyncTransformAppliedToContent() && !Matrix4x4(apzc->GetCurrentTransform()).IsIdentity().

Comment 4

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

Attachment: Patch v2

Good call! Updated

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8486501 [details] [diff] [review]
Patch v2

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +573,5 @@
>      controller->SampleContentTransformForFrame(&asyncTransformWithoutOverscroll,
>                                                 scrollOffset,
>                                                 &overscrollTransform);
>  
> +    Matrix4x4 asyncTransform = Matrix4x4(asyncTransformWithoutOverscroll) * overscrollTransform;

You no longer need this declared up here.

Comment 6

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

Good catch. Removed that bit and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/825737c8c1d1

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/825737c8c1d1

Comment 8

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

Comment on attachment 8486501 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/regressing bug #]: bug 1053992
[User impact if declined]: the visual indicator added in bug 1053992 is not particularly useful without this change, as it provides incorrect indications.
[Describe test coverage new/current, TBPL]: manual testing
[Risks and why]: low risk, code is pretty well understood. should only affect non-default configurations (i.e. when the FPS counter is enabled from the developer HUD or via prefs)
[String/UUID change made/needed]: none

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/630e8e47d348