1053992 - Provide a visual indicator of when we fall back to sync scrolling

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

This should be pretty easy to do, and provide some value with respect to testing bug 967844. I think the easiest way to do this is where we composite layers - if we find a layer that is a ContainerLayer with no children but it does have a non-empty async transform, then it means we are doing sync scrolling because the contents of the layer are somewhere else in the tree. We can have a pref that flips on and off drawing a border on the layer when we run into this scenario.

Comment 1

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

Attachment: WIP

Generally seems to work, but the patch needs to be cleaned up

Comment 2

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

Attachment: Patch

Comment 3

[Benoit Girard (:BenWa)]

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

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

It looks good but I don't think it should run part of layers border since it's too crowded. You could have it run off FPS counter and it would add to my visual warning feature, or use a unique pref.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Off the FPS counter would be nice as we probably have a lot of people already setting that pref...

Comment 5

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

Attachment: Alternate patch

This adds a red square in the top-right corner when we have a sync-scrolling layer, and the FPS pref is enabled. I would have put it just after the three numbers but it didn't look easy to compute the width of that in the code and I didn't want to hard-code it. Verified that it works as expected.

Comment 6

[Benoit Girard (:BenWa)]

Comment on attachment 8481642 [details] [diff] [review]
Alternate patch

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

Sorry for the slow review

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +391,5 @@
> +      // If we have an unused APZ transform on this composite, draw a 20x20 red box
> +      // in the top-right corner
> +      EffectChain effects;
> +      effects.mPrimaryEffect = new EffectSolidColor(gfx::Color(1, 0, 0, 1));
> +      mCompositor->DrawQuad(gfx::Rect(aBounds.width - 20, 0, aBounds.width, 20),

Not the best indicator but this is probably ok. We can think of better ways to represent if we're using it a lot.

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Kats, can you attach to this bug an image/screengrab of what it looks like when we're "showing sync scrolling"?

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8481642 [details] [diff] [review]
Alternate patch

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

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +465,5 @@
> +          break;
> +        }
> +      }
> +    }
> +  }

This will happen all the time, even if we're not going to use the value because we're not drawing FPS? If that's the case, we're not worried about performance?

::: gfx/layers/composite/LayerManagerComposite.h
@@ +295,5 @@
>                                 float aContrastEffect);
>  
>    float mWarningLevel;
>    mozilla::TimeStamp mWarnTime;
> +  bool mUnusedApzTransformWarning;

This is one of those magical classes that doesn't need explicit initialization, so the absence of mUnusedApzTransformWarning(false) should not worry me?

Comment 9

[Benoit Girard (:BenWa)]

I agree with Milan, we should check if FPS is enabled and we should initialize mUnusedApzTransformWarning.

Comment 10

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

Made the changes as requested. I also added an initializer for mWarningLevel which was missing.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1285c2f4408

I'll attach a screenshot of what it looks like later today.

Comment 11

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

(Also try push https://tbpl.mozilla.org/?tree=Try&rev=2f47230f374c includes this patch)

Comment 12

[No-Jun Park [:njpark]]

Is this enabled by setting some pref?  Or would it be in Developer options?

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

Just show the FPS and you'd see this.

Comment 14

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

Attachment: Screenshot of visual indicator

The red square in the top-right corner is the indicator. As Milan said it only comes on if you have the FPS display enabled.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c1285c2f4408

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

No-Jun, see comment 14.

Comment 17

[No-Jun Park [:njpark]]

(In reply to Milan Sreckovic [:milan] from comment #16)
> No-Jun, see comment 14.

Thanks, when I open https://bug1011639.bugzilla.mozilla.org/attachment.cgi?id=8424102 on the phone, i see red dot possibly because of Bug 1063158, so I'm waiting it until it gets resolved.

Comment 18

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

Comment on attachment 8481642 [details] [diff] [review]
Alternate patch

Approval Request Comment
[Feature/regressing bug #]: part of the bug 1026271 feature targeted at 2.1
[User impact if declined]: no direct impact, but this patch makes it easier to detect scenarios where the feature doesn't work.
[Describe test coverage new/current, TBPL]: tested on m-c
[Risks and why]: low-risk as the code is pretty simple and is only enabled when the FPS counter diagnostic tool is enabled (i.e. non-default configuration)
[String/UUID change made/needed]: none

Comment 19

[No-Jun Park [:njpark]]

[Blocking Requested - why for this release]:
This feature is required to test the 2.1 graphics feature, bug 1026271

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9d4b52f66242