1269931 - Send visible region updates for pres shells associated with nested views

Status: REOPENED

(Core :: Layout, defect)

Description

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

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.

Comment 1

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

Attachment: Send visible region updates for pres shells associated with nested views.

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.

Comment 2

[Botond Ballo [: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.)

Comment 3

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

(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.

Comment 4

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

Attachment: Send visible region updates for pres shells associated with nested views.

Walk up the view tree instead of down, using a handy helper method. <smacks forehead>

Comment 5

[Botond Ballo [: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!

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9df21facccf419f73123fd675b9dc9823a854d2
Bug 1269931 - Send visible region updates for pres shells associated with nested views. r=botond

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e9df21facccf

Comment 8

[Timothy Nikkel (:tnikkel)]

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.