Open
Bug 1270389
Opened 8 years ago
Updated 1 year ago
Add Visibility::IN_VIEWPORT to the frame visibility API
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
NEW
People
(Reporter: seth, Unassigned)
References
(Depends on 2 open bugs, Blocks 1 open bug)
Details
Attachments
(5 files, 5 obsolete files)
7.58 KB,
patch
|
Details | Diff | Splinter Review | |
24.11 KB,
patch
|
Details | Diff | Splinter Review | |
4.54 KB,
patch
|
Details | Diff | Splinter Review | |
9.64 KB,
patch
|
Details | Diff | Splinter Review | |
6.62 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•8 years ago
|
||
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.)
Attachment #8749060 -
Flags: review?(mstange)
Reporter | ||
Comment 2•8 years ago
|
||
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.
Attachment #8749064 -
Flags: review?(mstange)
Attachment #8749064 -
Flags: review?(botond)
Reporter | ||
Comment 3•8 years ago
|
||
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!
Attachment #8749065 -
Flags: review?(mstange)
Attachment #8749065 -
Flags: review?(botond)
Reporter | ||
Comment 4•8 years ago
|
||
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.
Attachment #8749066 -
Flags: review?(mstange)
Reporter | ||
Comment 5•8 years ago
|
||
Finally, we want to visualize IN_VIEWPORT visibility on the APZ minimap visibility visualization. (Say that three times fast!)
Attachment #8749067 -
Flags: review?(botond)
Updated•8 years ago
|
Attachment #8749060 -
Flags: review?(mstange) → review+
Comment 6•8 years ago
|
||
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.
Reporter | ||
Comment 7•8 years ago
|
||
(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•8 years ago
|
||
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•8 years ago
|
||
(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.
Depends on: 1259281
Comment 10•8 years ago
|
||
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).
Attachment #8749064 -
Flags: review?(botond) → feedback+
Updated•8 years ago
|
Attachment #8749065 -
Flags: review?(botond) → review+
Comment 11•8 years ago
|
||
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?
Attachment #8749067 -
Flags: review?(botond) → review+
Updated•8 years ago
|
Attachment #8749066 -
Flags: review?(mstange) → review+
Comment 12•8 years ago
|
||
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.
Attachment #8749064 -
Flags: review?(mstange) → review+
Comment 13•8 years ago
|
||
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.
Attachment #8749065 -
Flags: review?(mstange)
Attachment #8749065 -
Flags: review?(botond)
Attachment #8749065 -
Flags: review+
Comment 14•8 years ago
|
||
Sorry, bugzilla broke botond's r+ there.
Comment 15•8 years ago
|
||
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.
Attachment #8749065 -
Flags: review?(botond) → review+
Reporter | ||
Comment 16•8 years ago
|
||
(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.
Reporter | ||
Comment 17•8 years ago
|
||
Thanks for the quick reviews, folks! This is a rebased version of part 1a.
Reporter | ||
Updated•8 years ago
|
Attachment #8749060 -
Attachment is obsolete: true
Reporter | ||
Comment 18•8 years ago
|
||
This version of part 1b addresses all of the review comments.
Reporter | ||
Updated•8 years ago
|
Attachment #8749064 -
Attachment is obsolete: true
Reporter | ||
Comment 19•8 years ago
|
||
Rebased.
Reporter | ||
Updated•8 years ago
|
Attachment #8749065 -
Attachment is obsolete: true
Reporter | ||
Comment 20•8 years ago
|
||
Rebased.
Reporter | ||
Updated•8 years ago
|
Attachment #8749066 -
Attachment is obsolete: true
Reporter | ||
Comment 21•8 years ago
|
||
Rebased and renumbered to part 1e per review comments.
Reporter | ||
Updated•8 years ago
|
Attachment #8749067 -
Attachment is obsolete: true
Reporter | ||
Comment 22•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6413e5415ed1
Reporter | ||
Comment 23•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed38eb707406
Reporter | ||
Comment 24•8 years ago
|
||
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•2 years ago
|
||
The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.
Assignee: seth.bugzilla → nobody
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•