Closed Bug 1053992 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35
blocking-b2g 2.1+
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: kats, Assigned: kats)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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.
Attached patch WIP (obsolete) — Splinter Review
Generally seems to work, but the patch needs to be cleaned up
Attachment #8474696 - Attachment is patch: true
Assignee: nobody → bugmail.mozilla
Attached patch Patch (obsolete) — Splinter Review
Attachment #8474696 - Attachment is obsolete: true
Attachment #8481556 - Flags: review?(bgirard)
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.
Attachment #8481556 - Flags: review?(bgirard) → review+
Off the FPS counter would be nice as we probably have a lot of people already setting that pref...
Attached patch Alternate patchSplinter Review
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.
Attachment #8481642 - Flags: review?(bgirard)
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.
Attachment #8481642 - Flags: review?(bgirard) → review+
Kats, can you attach to this bug an image/screengrab of what it looks like when we're "showing sync scrolling"?
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?
I agree with Milan, we should check if FPS is enabled and we should initialize mUnusedApzTransformWarning.
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.
Is this enabled by setting some pref?  Or would it be in Developer options?
Just show the FPS and you'd see this.
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.
Attachment #8481556 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/c1285c2f4408
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
No-Jun, see comment 14.
Flags: needinfo?(npark)
(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.
Flags: needinfo?(npark)
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
Attachment #8481642 - Flags: approval-mozilla-aurora?
[Blocking Requested - why for this release]:
This feature is required to test the 2.1 graphics feature, bug 1026271
blocking-b2g: --- → 2.1?
blocking-b2g: 2.1? → 2.1+
Attachment #8481642 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.