Open Bug 1269931 Opened 4 years ago Updated 3 years ago

Send visible region updates for pres shells associated with nested views

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

REOPENED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(1 file, 1 obsolete file)

Right now the APZ minimap visibility visualization does not visualize anything for pres shells in nested views - for example, in iframes. We should fix this to make debugging easier; iframes are pretty come these days.
Here's the patch. Nothing too complicated; we just walk the view tree to find
nested views and send visible regions updates for those as well.
Attachment #8748426 - Flags: review?(botond)
Comment on attachment 8748426 [details] [diff] [review]
Send visible region updates for pres shells associated with nested views.

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

At least some of the call sites of PresShell::NotifyCompositorOfVisibleRegionsChange() occur in the context of a recursive walk that already descends into subdocuments.

For example, one of the callers of NotifyCompositorOfVisibleRegionsChange() is PresShell::RebuildApproximateFrameVisibility(), called in turn by MarkFramesInSubtreeApproximatelyVisible(). These latter two functions co-recurse, descending into subdocuments. Is this descent into subdocuments not redundant with the descent done during the view tree walk added in this patch?

::: layout/base/nsPresShell.cpp
@@ +4584,5 @@
> +                        uint64_t aLayersId,
> +                        uint32_t aPresShellId)
> +{
> +  for (auto iter = aRegions.ConstIter(); !iter.Done(); iter.Next()) {
> +    const ViewID viewId = iter.Key();

(Aside: ViewID is a terrible type name, isn't it, using as it does a completely different meaning of the word "view" than nsView? We should rename it to ScrollID or something at some point.)
(In reply to Botond Ballo [:botond] from comment #2)
> Is this descent into subdocuments
> not redundant with the descent done during the view tree walk added in this
> patch?

The issue that made this patch necessary was that no layer manager is available for view managers without a widget, so the existing recursive invocations of NotifyCompositorOfVisibleRegionsChange() generally failed instantly. Hence the idea of passing the CompositorBridgeChild down the view tree. However, now that I look at the patch again, it's pretty apparent to me that this can be drastically simplified by walking *up* the view tree instead of down, which will avoid the redundant recursive walks. Not sure why I went with the other approach in this patch; coding late at night may be to blame. =)

At any rate, thanks for calling me on that. A revised and much simpler patch is incoming.
Walk up the view tree instead of down, using a handy helper method. <smacks forehead>
Attachment #8748981 - Flags: review?(botond)
Attachment #8748426 - Attachment is obsolete: true
Attachment #8748426 - Flags: review?(botond)
Comment on attachment 8748981 [details] [diff] [review]
Send visible region updates for pres shells associated with nested views.

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

Nice!
Attachment #8748981 - Flags: review?(botond) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9df21facccf419f73123fd675b9dc9823a854d2
Bug 1269931 - Send visible region updates for pres shells associated with nested views. r=botond
https://hg.mozilla.org/mozilla-central/rev/e9df21facccf
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1272357
No longer depends on: 1272357
I backed this bug out on inbound, I expect the backout to get merged to mozilla-central. I plan to also request uplift to backout on aurora (so this bug would not be landed in any version of Firefox).

Bug 1284350 tracks the back out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
You need to log in before you can comment on or make changes to this bug.