1270389 - Add Visibility::IN_VIEWPORT to the frame visibility API

Status: NEW

(Core :: Layout, defect)

Description

[Seth Fowler [:seth] [:s2h]]

We want to get notified when frames enter or leave the viewport. This would allow us to do things like pause image animation when the image is not visible in the viewport, or throttle requestAnimationFrame in offscreen iframes which aren't visible. Currently we do both of those things, but we consider the displayport (or in the case of images, an even larger area) rather than the viewport. Constraining the work we do to things which are actually visible will help us reduce our energy and memory usage.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1a) - Add Visibility::IN_VIEWPORT to the frame visibility API.

OK, let's get this thing moving. A lot of this is quite similar to the
IN_DISPLAYPORT patch that landed quite recently, so I'll be a little bit less
verbose with my explanations. This patch adds IN_VIEWPORT to the Visibility enum
and exposes it on nsIFrame's frame visibility API.

(It's worth noting by the way that parts 1a-1d will be folded together before landing.)

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1b) - Add support for Visibility::IN_VIEWPORT to the pres shell and refresh driver code.

This patch makes a lot of boilerplate changes to the pres shell code to support
IN_VIEWPORT visibility, but there's also some meat here.
UpdateInViewportFrameVisibilitySync() is the code that gets called from the
refresh driver when there's an APZ scroll event, but we don't paint. Just like
IN_DISPLAYPORT visibility, if we paint we get IN_VIEWPORT visibility more or
less for free - you'll see in an upcoming patch - so we don't need to call
UpdateInViewportFrameVisibilitySync() in that case. The basic ideas for
UpdateInViewportFrameVisibilitySync() are:

1. We only need to consider frames that are within the displayport, since we're
   only considering APZ scroll events that don't trigger a paint, and hence
   cannot have moved the displayport.

2. To tell if a frame is within the viewport, we just need to walk up all its
   scroll frame ancestors to the root of the frame tree and make sure that the
   intersection of all their scrollports with the frame's rect is nonempty.

3. To include <iframe>'s and the like in these calculations, we need to walk
   down the view tree and do this update in child pres shells as well.

All of that is implemented in this patch.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1c) - Implement Visibility::IN_VIEWPORT.

This part actually implements Visibility::IN_VIEWPORT. It's simple once we have
the infrastructure from the previous parts in place. During painting, if a frame
is in the displayport we check if it's also in the viewport by checking the
cached ancestor scrollport information that we started tracking in bug 1263349.
When we get an APZ scroll event that doesn't trigger a paint, we tell the
refresh driver to call UpdateInViewportFrameVisibilitySync() on the next tick.
That's really it!

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1d) - Update various frame classes to use IN_VIEWPORT visibility.

This patch updates the various frame classes to use IN_VIEWPORT visibility. In
all but a few cases, this just boils down to using IN_VIEWPORT visibility
instead of IN_DISPLAYPORT visibile when we force a frame to be visible, because
when we do that we always want to use the "most visible" type of visibility.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add visualization of IN_VIEWPORT visibility to the APZ minimap.

Finally, we want to visualize IN_VIEWPORT visibility on the APZ minimap
visibility visualization. (Say that three times fast!)

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8749064 [details] [diff] [review]
(Part 1b) - Add support for Visibility::IN_VIEWPORT to the pres shell and refresh driver code.

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

This looks good except for the part that walks the presshell tree. I'd really prefer that to use EnumerateSubDocuments instead. And then you won't need the comment about the view tree structure - different documents are guaranteed to have different presshells.

Comment 7

[Seth Fowler [:seth] [:s2h]]

(In reply to Markus Stange [:mstange] from comment #6)
> This looks good except for the part that walks the presshell tree. I'd
> really prefer that to use EnumerateSubDocuments instead. And then you won't
> need the comment about the view tree structure - different documents are
> guaranteed to have different presshells.

Doh! I didn't know about EnumerateSubDocuments. That does seem perfect. I'll update the patch.

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8749064 [details] [diff] [review]
(Part 1b) - Add support for Visibility::IN_VIEWPORT to the pres shell and refresh driver code.

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

::: layout/base/nsPresShell.h
@@ +821,4 @@
>      bool mSuppressingVisibility;
>    };
>  
>    VisibleFramesContainer mVisibleFrames;

Does this patch have a code dependency that's not on m-c and isn't marked as a dependency of this bug? I can't find the change that added this variable, for example.

Comment 9

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #8)
> Does this patch have a code dependency that's not on m-c and isn't marked as
> a dependency of this bug? I can't find the change that added this variable,
> for example.

Looks like the missing dependency is bug 1259281 (and the things it depends on in turn); marking it.

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8749064 [details] [diff] [review]
(Part 1b) - Add support for Visibility::IN_VIEWPORT to the pres shell and refresh driver code.

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

I'm not really familiar enough with this code to r+ this patch, but I've looked it over and it looks sensible to me, so I'll f+ it.

::: layout/base/nsPresShell.cpp
@@ +4628,5 @@
>                            mVisibleRegions->mInDisplayPort,
>                            VisibilityCounter::IN_DISPLAYPORT,
>                            layersId, presShellId);
> +
> +  SendUpdateVisibleRegion(compositorChild,

Really nit-picking here, but in a state where the Part 1 patches are applied but the Part 2 patch isn't, this would overwrite the compositor's IN_DISPLAYPORT regions with the IN_VIEWPORT regions, so technically, the Part 2 patch should be folded in with these as well.

@@ +5920,5 @@
>            mOldInDisplayPortFrames.emplace();
>            mPresShell->mVisibleFrames.mInDisplayPort.SwapElements(*mOldInDisplayPortFrames);
>            break;
> +
> +        case VisibilityCounter::IN_VIEWPORT:

Do you need to add an |mPresShell->mVisibleFrames.mInViewport.Clear()| line to the IsVisibilitySuppressed() block above?

::: layout/base/nsRefreshDriver.cpp
@@ +1928,5 @@
>  #ifndef ANDROID  /* bug 1142079 */
>    mozilla::Telemetry::AccumulateTimeDelta(mozilla::Telemetry::REFRESH_DRIVER_TICK, mTickStart);
>  #endif
>  
> +  if (mNeedToRecomputeInViewportFrameVisibility && !mThrottled &&

The comment above ScheduleInViewportFrameVisibilityUpdate() says "Unlike approximate frame visibility updates, these are not throttled". Does that imply something about whether we should be checking the "mThrottled" flag here? (In particular, I notice we are checking it for mNeedToRecomputeApproxFrameVisibility, so I'm not sure how the two are different).

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8749067 [details] [diff] [review]
(Part 2) - Add visualization of IN_VIEWPORT visibility to the APZ minimap.

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

::: gfx/layers/composite/LayerManagerComposite.h
@@ +417,5 @@
> +    {
> +      case VisibilityCounter::MAY_BECOME_VISIBLE: return mApproximatelyVisibleRegions;
> +      case VisibilityCounter::IN_DISPLAYPORT:     return mInDisplayPortVisibleRegions;
> +      case VisibilityCounter::IN_VIEWPORT:        return mInViewportVisibleRegions;
> +    }

Should we MOZ_ASSERT or something in the default case?

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8749064 [details] [diff] [review]
(Part 1b) - Add support for Visibility::IN_VIEWPORT to the pres shell and refresh driver code.

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

r+ assuming you use EnumerateSubDocuments.

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 8749065 [details] [diff] [review]
(Part 1c) - Implement Visibility::IN_VIEWPORT.

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

::: layout/generic/nsFrame.cpp
@@ +2774,5 @@
>        nsIPresShell* shell = child->PresContext()->PresShell();
>        shell->MarkFrameVisible(child, VisibilityCounter::IN_DISPLAYPORT);
> +
> +      // If the frame is in the critical displayport, it may also be in the
> +      // viewport. A frame is considered to be IN_VIEWPORT if it's within the

Fun fact: Sometimes you can have a display port that doesn't completely include the viewport. But that should only be the case during fast scrolling and will be corrected as soon as scrolling stops.

Comment 14

[Markus Stange [:mstange]]

Sorry, bugzilla broke botond's r+ there.

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 8749065 [details] [diff] [review]
(Part 1c) - Implement Visibility::IN_VIEWPORT.

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

(In reply to Markus Stange [:mstange] from comment #14)
> Sorry, bugzilla broke botond's r+ there.

Restoring.

Comment 16

[Seth Fowler [:seth] [:s2h]]

(In reply to Botond Ballo [:botond] from comment #10)
> The comment above ScheduleInViewportFrameVisibilityUpdate() says "Unlike
> approximate frame visibility updates, these are not throttled". Does that
> imply something about whether we should be checking the "mThrottled" flag
> here? (In particular, I notice we are checking it for
> mNeedToRecomputeApproxFrameVisibility, so I'm not sure how the two are
> different).

The terminology is bad here. =( There are two notions of throttling. One is the fact that the overall refresh driver can be throttled, for example if the document is in a background tab. The other notion (which is being referred to in that comment) is that we will only update approximately frame visibility every N milliseconds, regardless of whether updates are requested at a higher frequency. (There are exceptions to that but I'm planning on removing them.) Search for mMinRecomputeVisibilityInterval in nsRefreshDriver.cpp to see how that's implemented.

I'll see if I can reword the comment a bit.

Comment 17

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1a) - Add Visibility::IN_VIEWPORT to the frame visibility API.

Thanks for the quick reviews, folks!

This is a rebased version of part 1a.

Comment 18

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1b) - Add support for Visibility::IN_VIEWPORT to the pres shell and refresh driver code.

This version of part 1b addresses all of the review comments.

Comment 19

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1c) - Implement Visibility::IN_VIEWPORT.

Rebased.

Comment 20

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1d) - Update various frame classes to use IN_VIEWPORT visibility.

Rebased.

Comment 21

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1e) - Add visualization of IN_VIEWPORT visibility to the APZ minimap.

Rebased and renumbered to part 1e per review comments.

Comment 22

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6413e5415ed1

Comment 23

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed38eb707406

Comment 24

[Seth Fowler [:seth] [:s2h]]

OK, now that bug 1263349 is ready to land (and indeed has already been pushed), this one *should* be ready to go, but it's worth one more try job to make sure.

Comment 25

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.