Closed Bug 1064840 Opened 6 years ago Closed 6 years ago

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

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 --- unaffected
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.0 --- unaffected
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
[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
blocking-b2g: --- → 2.1?
Attached patch Patch (obsolete) — Splinter Review
Verified that this works by flipping the layout.async-containerless-scroll.enabled pref to false and testing on the case in bug 1011639.
Attachment #8486455 - Flags: review?(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().
Attached patch Patch v2Splinter Review
Good call! Updated
Attachment #8486455 - Attachment is obsolete: true
Attachment #8486455 - Flags: review?(botond)
Attachment #8486501 - Flags: review?(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.
Attachment #8486501 - Flags: review?(botond) → review+
https://hg.mozilla.org/mozilla-central/rev/825737c8c1d1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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
Attachment #8486501 - Flags: approval-mozilla-aurora?
blocking-b2g: 2.1? → 2.1+
Attachment #8486501 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.