Closed Bug 1168629 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: tnikkel, Assigned: tnikkel)

References

Details

Attachments

(2 files)

No description provided.
Attached patch patchSplinter Review
We'll need to do something similar for fennec in the subdocument items BuildLayer function I think. This should work for b2g.
Attachment #8610871 - Flags: review?(bugmail.mozilla)
Attachment #8610871 - Flags: review?(botond)
Attachment #8610871 - Flags: review?(bugmail.mozilla) → review+
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?
Attachment #8610871 - Flags: review?(botond) → review+
(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.
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?
(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.
Nothing clears them on the content side. I think it's only the root layer that will have this problem.
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.
Attachment #8613273 - Flags: review?(mstange)
Attachment #8613273 - Flags: review?(bugmail.mozilla)
Attachment #8613273 - Flags: review?(botond)
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?
Attachment #8613273 - Flags: review?(mstange) → review+
(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.
Sounds reasonable. I think I was thinking of the code that reuses PaintedLayers, which isn't relevant here.
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.
Attachment #8613273 - Flags: review?(bugmail.mozilla)
Attachment #8613273 - Flags: review?(botond)
https://hg.mozilla.org/mozilla-central/rev/09fdfa4efd09
https://hg.mozilla.org/mozilla-central/rev/8426ae483df7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1180899
You need to log in before you can comment on or make changes to this bug.