Closed Bug 1466611 Opened 6 years ago Closed 6 years ago

Render layout viewport on APZ minimap

Categories

(Core :: Panning and Zooming, enhancement, P3)

Product:
Core
Component:
Panning and Zooming
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: u608768, Assigned: u608768)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 1 obsolete file)

Bug 1466611 - Render layout viewport on APZ minimap.
59 bytes, text/x-review-board-request
botond
: review+
Details 
Until now, there was no clear distinction between the layout viewport and the visual viewport (except on overflow:hidden pages), so the APZ minimap only displayed the visual viewport [1] (and scrollable area, displayport, etc., but no layout viewport).

To assist with work on bug 1423011, we should update the minimap to also render the layout viewport. This would allow us to visualize changes in the layout viewport's offset while async-scrolling a page that's been pinched-zoomed (on pages that haven't been pinch-zoomed, both viewports should be the same).

[1] https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/gfx/layers/composite/ContainerLayerComposite.cpp#377-379
Comment on attachment 8983122 [details]
Bug 1466611 - Render layout viewport on APZ minimap.

https://reviewboard.mozilla.org/r/248960/#review255170

Thanks, this looks pretty good! A few comments:

::: gfx/layers/apz/public/APZSampler.h:88
(Diff revision 1)
>  
>    ParentLayerPoint GetCurrentAsyncScrollOffset(const LayerMetricsWrapper& aLayer);
>    AsyncTransform GetCurrentAsyncTransform(const LayerMetricsWrapper& aLayer);
>    AsyncTransformComponentMatrix GetOverscrollTransform(const LayerMetricsWrapper& aLayer);
>    AsyncTransformComponentMatrix GetCurrentAsyncTransformWithOverscroll(const LayerMetricsWrapper& aLayer);
> +  CSSRect GetCurrentAsyncViewport(const LayerMetricsWrapper& aLayer);

The naming of FrameMetrics::mViewport predates (our appreciation of) the distinction between the visual viewport and layout viewport.

In new code, let's use "visual viewport" or "layout viewport" instead of just "viewport". So, let's call this (and the corresponding method of AsyncPanZoomController) GetCurrentAsyncLayoutViewport().

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:3825
(Diff revision 1)
>    return mFrameMetrics.GetZoom();
>  }
>  
> +CSSRect
> +AsyncPanZoomController::GetCurrentAsyncViewport(AsyncTransformConsumer aMode) const {
> +  RecursiveMutexAutoLock lock(mRecursiveMutex);

Since the concept of a layout viewport only applies to the root scroll frame, let's only allow this to be called for the root content APZC, by adding:

MOZ_ASSERT(mFrameMetrics.IsRootContent(), "Only the root content APZC has a layout viewport");

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:3829
(Diff revision 1)
> +AsyncPanZoomController::GetCurrentAsyncViewport(AsyncTransformConsumer aMode) const {
> +  RecursiveMutexAutoLock lock(mRecursiveMutex);
> +  if (aMode == eForCompositing && mScrollMetadata.IsApzForceDisabled()) {
> +    return mLastContentPaintMetrics.GetViewport();
> +  }
> +  return mFrameMetrics.GetViewport();

There is one complication here that we need to address: we have a feature called "APZ frame delay" (added in bug 1375949), where changes to the async scroll offset don't take effect right away on the next composite, but one composite later. We need to apply the same treatment to the  layout viewport as well.

This would involve:

* A new field mCompositedLayoutViewport similar to mCompositedScrollOffset
* Updates to NotifyLayersUpdated() and SampleCompositedAsyncTransform() to update mCompositedLayoutViewport in the same cases when mCompositedScrollOffset is updated
* A new method GetEffectiveLayoutViewport() similar to GetEffectiveScrollOffset()
* Calling GetEffectiveLayoutViewport() here, similar to how GetCurrentAsyncScrollOffset() calls GetEffectiveScrollOffset()

::: gfx/layers/composite/ContainerLayerComposite.cpp:380
(Diff revision 1)
>    if (cdp) {
>      r = transform.TransformBounds(cdp->ToUnknownRect());
>      compositor->SlowDrawRect(r, criticalDisplayPortColor, clipRect, aContainer->GetEffectiveTransform());
>    }
>  
> -  // Render the viewport.
> +  // Render the layout viewport.

Let's only render the layout viewport for the root content APZC (fm.IsRootContent()).

(Note, if we add the MOZ_ASSERT I suggested above, we'll need to be careful not to call GetCurrentAsyncLayoutViewport() here at all if it's not the root content APZC.)
Attachment #8983122 - Flags: review?(botond)
Comment on attachment 8983122 [details]
Bug 1466611 - Render layout viewport on APZ minimap.

https://reviewboard.mozilla.org/r/248960/#review255254

::: gfx/layers/composite/ContainerLayerComposite.cpp:380
(Diff revision 1)
>    if (cdp) {
>      r = transform.TransformBounds(cdp->ToUnknownRect());
>      compositor->SlowDrawRect(r, criticalDisplayPortColor, clipRect, aContainer->GetEffectiveTransform());
>    }
>  
> -  // Render the viewport.
> +  // Render the layout viewport.

Took a slightly different approach than using MOZ_ASSERT for this, let me know if that's still preferred.
Attachment #8983122 - Flags: review?(kmadan)
Comment on attachment 8983232 [details]
Bug 1466611 - Use cached layout viewport when appropriate, only draw for RCD-RSF.

https://reviewboard.mozilla.org/r/249096/#review255280

Thanks for the update. Could you combine the changes into the original patch please? You can do this using "hg histedit" (and in the future, you can avoid that step by committing the changes with "hg commit --amend").
Attachment #8983232 - Attachment is obsolete: true
Attachment #8983232 - Flags: review?(botond)
(In reply to Botond Ballo [:botond] from comment #6)
> Comment on attachment 8983232 [details]
> Bug 1466611 - Use cached layout viewport when appropriate, only draw for
> RCD-RSF.
> 
> https://reviewboard.mozilla.org/r/249096/#review255280
> 
> Thanks for the update. Could you combine the changes into the original patch
> please? You can do this using "hg histedit" (and in the future, you can
> avoid that step by committing the changes with "hg commit --amend").

Done!
Comment on attachment 8983122 [details]
Bug 1466611 - Render layout viewport on APZ minimap.

https://reviewboard.mozilla.org/r/248960/#review255288

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:3738
(Diff revisions 2 - 3)
>  
> +CSSRect
> +AsyncPanZoomController::GetCurrentAsyncLayoutViewport(AsyncTransformConsumer aMode) const {
> +  RecursiveMutexAutoLock lock(mRecursiveMutex);
> +  if (!mFrameMetrics.IsRootContent()) {
> +    return CSSRect();

The intention behind adding a MOZ_ASSERT is to catch logic errors where some calling code calls GetCurrentAsyncLayoutViewport() on something other than the root content, not realizing that it's not getting a meaningful value.

A MOZ_ASSERT catches such situations more effectively (since it crashes the browser in debug builds) than returning an empty rect (whose consequences could go unnoticed for a long time).

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:4194
(Diff revisions 2 - 3)
>        // going to be ignored by layout, and so mExpectedGeckoMetrics
>        // becomes incorrect for the purposes of calculating the LD transform. To
>        // correct this we need to update mExpectedGeckoMetrics to be the
>        // last thing we know was painted by Gecko.
>        mFrameMetrics.CopyScrollInfoFrom(aLayerMetrics);
>        mCompositedScrollOffset = mFrameMetrics.GetScrollOffset();

We'll need to update mCompositedLayoutViewport here as well.
Attachment #8983122 - Flags: review?(botond)
Comment on attachment 8983122 [details]
Bug 1466611 - Render layout viewport on APZ minimap.

https://reviewboard.mozilla.org/r/248960/#review255292

Alternatively, you could have GetCurrentAsyncLayoutViewport() return a Maybe<CSSRect>, thereby forcing the caller to check for and handle the empty (in the sense of "empty Maybe", not "empty rect") case.
(In reply to Botond Ballo [:botond] from comment #10)
> Comment on attachment 8983122 [details]
> Bug 1466611 - Render layout viewport on APZ minimap.
> 
> https://reviewboard.mozilla.org/r/248960/#review255292
> 
> Alternatively, you could have GetCurrentAsyncLayoutViewport() return a
> Maybe<CSSRect>, thereby forcing the caller to check for and handle the empty
> (in the sense of "empty Maybe", not "empty rect") case.

Updated - opted to go the MOZ_ASSERT route, felt that it handled the case more effectively. Not sure the correct placement of the assertion: should it happen before or after we acquire the lock? Does it matter?
(In reply to Kashav Madan [:kashav] from comment #12)
> Not sure the correct placement of the assertion: should it
> happen before or after we acquire the lock? Does it matter?

It does matter: the lock needs to be held before accessing mFrameMetrics, as documented here [1] (which is what the patch does).

[1] https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/gfx/layers/apz/src/AsyncPanZoomController.h#873
Comment on attachment 8983122 [details]
Bug 1466611 - Render layout viewport on APZ minimap.

https://reviewboard.mozilla.org/r/248960/#review255368

Looks good, thanks!

Could you please do a Try push for the patch prior to landing? You can do this, now that you have Level 1 commit access, via "Automation -> Trigger a Try Build" in the MozReview UI.
Attachment #8983122 - Flags: review?(botond) → review+
Assignee: nobody → kmadan
Priority: -- → P3
Whiteboard: [gfx-noted]
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b7ddd726b2f9
Render layout viewport on APZ minimap. r=botond
https://hg.mozilla.org/mozilla-central/rev/b7ddd726b2f9
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Is manual testing required for this fix? 
If yes, could you please provide some clear steps on how we can check this?

Thank you.
Flags: qe-verify?
Flags: needinfo?(kmadan)
The APZ minimap is an internal debugging tool that is not meant for end-users, so I would not spend QA resources on this. Thanks for asking though!
Flags: needinfo?(kmadan)
Thank you for the reply. Updating the flag.
Flags: qe-verify? → qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: