1168629 - always make sure that there is at least one layer with the metrics for the root scroll frame/root element

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

We'll need to do something similar for fennec in the subdocument items BuildLayer function I think. This should work for b2g.

Comment 2

[Botond Ballo [:botond]]

Comment on attachment 8610871 [details] [diff] [review]
patch

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

::: layout/base/nsDisplayList.cpp
@@ +1521,5 @@
> +      (XRE_IsParentProcess() && !presShell->GetRootScrollFrame());
> +
> +  // Add metrics if there are none in the layer tree with the id (create an id
> +  // if there isn't one already) of the root scroll frame/root content.
> +  bool addMetricsIfNoneInTreeWithId =

ensureMetricsForRootId?

@@ +1522,5 @@
> +
> +  // Add metrics if there are none in the layer tree with the id (create an id
> +  // if there isn't one already) of the root scroll frame/root content.
> +  bool addMetricsIfNoneInTreeWithId =
> +    nsLayoutUtils::UsesAsyncScrolling() &&

I think gfxPrefs::AsyncPanZoomEnabled() would be more appropriate here, as we don't want this for non-APZ Fennec. ('content' will be nullptr anyways below.)

@@ +1525,5 @@
> +  bool addMetricsIfNoneInTreeWithId =
> +    nsLayoutUtils::UsesAsyncScrolling() &&
> +    !gfxPrefs::LayoutUseContainersForRootFrames() &&
> +    aBuilder->IsPaintingToWindow() &&
> +    !presContext->GetParentPresContext();

Is there ever a parent pres context in PaintRoot?

Comment 3

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #2)
> Is there ever a parent pres context in PaintRoot?

In general there can be (painting for drawWindow calls, or -moz-element), but there shouldn't be if we are painting to the window.

Comment 4

[Timothy Nikkel (:tnikkel)]

When a page first loads, before it has content to draw this patch makes us attach metrics to the root (where before there would be no metrics in the layer tree). Then when content loads we build a container layer for the root again and we look for the old layer and reuse it. It still has the metrics on it, and nothing clears them. We'll need to clear metrics on layers that we reuse, but probably not all. Maybe need to be careful about generating more IPC traffic?

Comment 5

[Kartikaya Gupta (email:[e-mail])]

(In reply to Timothy Nikkel (:tn) from comment #4)
> It still has the metrics
> on it, and nothing clears them.

Do you mean nothing clears them on the content side? Or that you remove the metrics on the content side but that removal doesn't get forwarded properly over to the compositor side? If the latter then it's a bug in the layers forwarder code.

Comment 6

[Timothy Nikkel (:tnikkel)]

Nothing clears them on the content side. I think it's only the root layer that will have this problem.

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: Clear metrics if we don't set them

BuildContainerLayerFor looks to see if there was a layer for the same frame with the same display item type (null display item is considered a type) and reuses that layer if it finds one. That means that if we set metrics we need to clear them if we ever want the layer to not have metrics.

I think this should be happening in all other cases except nsDisplaySubdocument. We can fix it in another bug, it's not as important since we likely don't run into this situation for nsDisplaySubdocument yet.

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8613273 [details] [diff] [review]
Clear metrics if we don't set them

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

Why don't you want to do this in the place where we recycle layers, where we reset the other layer properties? Because it would cause unnecessary IPC traffic?

Comment 9

[Timothy Nikkel (:tnikkel)]

(In reply to Markus Stange [:mstange] from comment #8)
> Why don't you want to do this in the place where we recycle layers, where we
> reset the other layer properties? Because it would cause unnecessary IPC
> traffic?

SetupScrollingMetadata is the function that clears framemetrics in general for containerless scrolling. So I was following the pattern where the code that sets the framemetrics is responsible for clearing (or setting them to their new value) on the next paint.

Comment 10

[Markus Stange [:mstange]]

Sounds reasonable. I think I was thinking of the code that reuses PaintedLayers, which isn't relevant here.

Comment 11

[Timothy Nikkel (:tnikkel)]

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

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09fdfa4efd09
https://hg.mozilla.org/integration/mozilla-inbound/rev/8426ae483df7

Comment 13

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8610871 [details] [diff] [review]
patch

Before landing I changed nsLayoutUtils::ContainsMetricsWithId to look at all frame metrics on a layer instead of just the last one since the metrics with the scrollid we are looking for will generate a hittesting node no matter where it is in the frame metrics array.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09fdfa4efd09
https://hg.mozilla.org/mozilla-central/rev/8426ae483df7